search

Chavithra / degiro-connector

This is yet another library to access Degiro's API.
BSD 3-Clause "New" or "Revised" License
212 stars 47 forks source link

Is there a way to access fault data on API requests, instead of returning `None`? #54

Open funnel20 opened 2 years ago

funnel20 commented 2 years ago

For example the Trading API might return a negative response while creating or updating orders, see for example the error response in https://github.com/Chavithra/degiro-connector/pull/50#issuecomment-997392518

{"errors": [{"text": "Le prix renseigné (300.0500) ne respecte pas la tick size (écart minimal entre les décimales). Le prix doit être un multiple de (0.1000)."}]}

However, this is only observed in the CRITICAL Logging. The functions check_order(), confirm_order() and update_order() will return just None in case of an error.

It's important to know what the cause of the error is. Is it for example an error with the order (such as the example above), which is refused by DeGiro? Or is it caused by a server error (HTTP Status codes) or even no internet connection (HTTP Connection error)?

This data seems already present in the underlying code of the Connector, as can be seen in the Logging messages. Would it be possible to use this in the response of the functions?

For example the standard output of these functions can be a dict with 3 parameters. Here the response of a successful request:

{
   "http_status" : 200
   "result" : True
   "error" : None
}

where result can also contain a dict, for example with the current response from check_order()

In case of a DeGiro error, it will look like:

{
   "http_status" : 200
   "result" : False
   "error" : 'Le prix renseigné (300.0500) ne respecte pas la tick size (écart minimal entre les décimales). Le prix doit être un multiple de (0.1000).'
}

In case of a Network error, it will look like:

{
   "http_status" : 500
   "result" : False
   "error" : 'The server has an internal error.'
}

This information is useful for our script, to decide what fault action to take. E.g.

Please let me know if this is possible.

Chavithra commented 2 years ago

Hi, did you try with the latest release and setting raw=True ?

funnel20 commented 2 years ago

@Chavithra Thanks for the tip, will try later this week.

funnel20 commented 2 years ago

@Chavithra Best wishes for the New Year.

I've tried the confirm_order() with raw==True of version 2.0.13 but I'm a bit puzzled. When forcing the error that the limit price is way too low compared to the actual price, I see the following logging traces in the Terminal:

400
{"errors":[{"text":"Uw opgegeven prijs 0.8900 is te laag ten opzichte van de laatste koers 4.4900. De minimum limietprijs is 3.4900."}]}
{'errors': [{'text': 'Uw opgegeven prijs 0.8900 is te laag ten opzichte van de laatste koers 4.4900. De minimum limietprijs is 3.4900.'}]}

These seem to match with the following code section: https://github.com/Chavithra/degiro-connector/blob/e950396191d85c28a0ee304dc531684458dc6218/degiro_connector/trading/actions/action_confirm_order.py#L140-L148

It consists of 3 calls to logger.fatal(), where:

Also the received dict from confirm_order() is the text without the status code:

2022-01-02 19:41:37,609 INFO     BRKR: DeGiro confirm_order() response:
{'errors': [{'text': 'Uw opgegeven prijs 0.8900 is te laag ten opzichte van de laatste koers 4.4900. De minimum limietprijs is 3.4900.'}]}

Update: I now understand that .json() in L147 returns the json-encoded content of a response (i.e. property text (L142) of Response object).

It's far more easy to perform fault handling on error codes (such as different HTTP status codes), than on (localised) error messages. Would it be an objection for you to add the status code to the raw response? In this case L146 can be changed into:

response_dict = {"status_code": status_code, "text": response_raw.json()}

To contain both properties of the Response object.

2022-01-02T21:00:19.567 INFO     BRKR: DeGiro confirm_order() response:
{'status_code': 400, 'text': {'errors': [{'text': 'Uw opgegeven prijs 0.8900 is te laag ten opzichte van de laatste koers 4.4900. De minimum limietprijs is 3.4900.'}]}}

Another benefit of this would be that instead of polluting the Terminal with 3 times logger.fatal(), a single will be sufficient.

I'm happy to assist you and also update the docu.

Chavithra commented 2 years ago

Hi @funnel20,

Thanks

Happy New Year to you too !

How about returning one of the following ?

I think the Tuple is too implicit.

Also I am wondering :

funnel20 commented 2 years ago

@Chavithra To start with your most important question: we're discussing changes to the output of existing functions. So this means breaking changes that impact all users. So it's a major update to version 3.0 of the connector.

Having that said, most users are probably making apps to automate their trading. Meaning they put their money at risk. Therefor it's inevitable to have proper fault detection and fault handling on the API calls. And this answers your last question: yes, we should provide it consistently on all API calls; TraderAPI and QuoteCastAPI.

I've thought about possible solutions to minimise the migration impact, which allow to supply a migration script, as you have done previously for breaking changes.

Current situation

First I'd like to have a proper understanding of the meaning of parameter raw. As it's currently implemented I conclude this:

This current implementation does not allow proper fault handling, since both options for raw lack fault data:

Not all API calls support parameter raw (e.g. trading_api.connect()).

Proposal

I'm new to Python, but am an iOS developer for 13 years. In iOS it's common that most API calls return 2 objects:

  1. The happy flow response object
  2. A non-happy flow error object

Only one of the 2 contains data depending of a successful or erroneous result, the other is always None. I've discovered that Python has a similar function output, a Tuple.

Based on this, my proposal would be the following, as indicated by this pseudo-code example:

def confirm_order(raw: bool = False) -> Tuple[Any, dict]:
    """
    When `raw` is set to `False` (default), a Tuple is returned with 2 arguments:
    `response`
    `error`

    On a successful call `response` contains the requested data, while `error` is `None`.
    When a fault occurs, `response` is `None` and `error` contains the details.

    When `raw` is set to `True`, the `requests.Response` object is returned as single output. 
    This also contains error data, so no need for a separate argument.
    """
    # Make HTTP request to DeGiro:
    requests = ...

    # Return result:
    if raw:
        # Return the entire `Response`` object as only argument:
        return requests.Response
    else:
        if requests.Response.status_code == 200:
            # When OK, parse `Response` and return as first dict, while setting error argument to None:
            return {"order_id" : "123456"}, None
        else:
            # Parse DeGiro fault data from body (or the `ConnectionError` in case there is even no response) and return as second dict. The response dict should be None:
            return None, {"status_code" : requests.Response.status_code, "error" : "Some error from DeGiro"}

Of course we can discuss the exact content of the new error output argument.

For the default raw is False, the Tuple can be unpacked and allows proper optional fault handling:

response, error = confirm_order()
if response:
    print(response)
    # Do the happy flow actions
else: 
    print(error)
    # Handle the error

For raw is True a single argument is returned and the developer can optionally do its custom fault handling on the requests.Response object, e.g. the HTTP Status Code:

response = confirm_order(raw=True)
status_code = getattr(response, "status_code")
if  status_code == 200:
    print(response.json())
    # Do the happy flow actions
else: 
    print(status_code)
    # Handle the error

Migration Guide

This proposed new output is close to the existing implementation for both values of raw:

Since these migration steps are straightforward, I expect that both can be applied in a migration script to help your users migrate automatically.

Note

When you agree upon expanding the default output for raw is False with error data, I personally don't need to use raw is True. And thus no changes required to its output. However, from a technical perspective it should be appropriate to supply as much of raw data as requested by raw is True.

e9henrik commented 2 years ago

The pythonic way to do error handling would be to throw an exception, and the exception object could contain the status code and error message.

An alternative to providing a migration script could be to have exceptions enabled by an option, and log a warning message when the option is not enabled (i.e. "Deprecation warning: error handling with exceptions not enabled, see [some documentation link]"). Then, in perhaps 2-3 months, make exceptions the default and remove the optional argument.

funnel20 commented 2 years ago

Thanks @e9henrik for your better suggestion. Like mentioned I'm new to Python, so if exceptions are the way to go; perfect. Out of curiosity; can an exception contain more than just an error message string, like a dict?

e9henrik commented 2 years ago

Sure. Exceptions are regular python objects. See for example https://www.programiz.com/python-programming/user-defined-exception

funnel20 commented 2 years ago

Awesome, thanks for teaching me something new this morning.

funnel20 commented 2 years ago

An alternative to providing a migration script could be to have exceptions enabled by an option, and log a warning message when the option is not enabled (i.e. "Deprecation warning: error handling with exceptions not enabled, see [some documentation link]"). Then, in perhaps 2-3 months, make exceptions the default and remove the optional argument.

Great idea. This would even prevent breaking changes. It can just be a boolean that defaults to False, similar to parameter raw.

funnel20 commented 2 years ago

Hi @Chavithra , what do you think of the solution by @e9henrik with exceptions and an additional input parameter to prevent breaking changes (and a migration guide/script)?

Chavithra commented 2 years ago

Hi @Chavithra , what do you think of the solution by @e9henrik with exceptions and an additional input parameter to prevent breaking changes (and a migration guide/script)?

Hi @funnel20, I think its a good idea.

I won't have the time this month to implement it.

Feel free to join me on Discord if you want to talk about this.

Thanks

funnel20 commented 2 years ago

Hi @Chavithra , I don't have (or want) Discord. Since we're no longer talking about breaking changes, is it an idea that we can formulate the requirements here. Once we have mutual understanding and an agreement, I'm happy to code a PR for just 1 function, e.g. Trading API connect(). We can do some iterations, or accept it as a solution. Then it will probably be more or less copy/paste to the other functions, to have the exact same behaviour.

Chavithra commented 2 years ago

Hi @Chavithra , I don't have (or want) Discord. Since we're no longer talking about breaking changes, is it an idea that we can formulate the requirements here. Once we have mutual understanding and an agreement, I'm happy to code a PR for just 1 function, e.g. Trading API connect(). We can do some iterations, or accept it as a solution. Then it will probably be more or less copy/paste to the other functions, to have the exact same behaviour.

Hello @funnel20, alright will try to formulate a specification tonight (or during the week if I can't tonight).

Chavithra commented 2 years ago

Here is my requirement proposal.

Currently

When an action fails we are :

New behaviour

We will add a new parameter to an action.

When this parameter is enabled, on action fail we will :

When this parameter is disabled, we will :

The raised Exception needs to contain :

This parameter must be disabled by default (for now) : to allow seamless update for projects using the current version of the library.

funnel20 commented 2 years ago

Hi @Chavithra , this looks good, especially since it will be a seamless update for exiting projects.

Two questions:

  1. For the returned Exception, do you want to forward the raised exception (HTTPError, JSONDecodeError,...), or make a custom one, or even custom ones?
  2. Is there a need / reason to return the http request in the Exception? I expect the http response is sufficient, as that contains the HTTP Status Code and the (JSON) response body.
Chavithra commented 2 years ago

Hi @funnel20,

  1. Whatever you think makes the most sense. Although I don't know if HTTPError (or others) can store details about the request/response.
  2. Better be safe than sorry : I think it will allow more possibilities for the user of this library.
funnel20 commented 2 years ago
  1. Good one. As also indicated by @e9henrik we can make a custom Exception to handle our dedicated set of return parameters. But when for example a ConnectionException occurs (e.g. no internet connection), there is no http response. So we can either populate that return parameter as None, but then the exception parser still has no idea about the root cause and what kind of action should take place. So probably it's better to return a dedicated custom Exception for each possible Exception that the API connection and parsing can raise.
  2. That's often not the best rational for a requirement. But if you insist
funnel20 commented 2 years ago

I've been prototyping today and discovered that the exceptions that are raised by the requests library already have the request and response objects as can be seen in the source code of the RequestException. All other exceptions, such as HTTPError, ConnectTimeout, ConnectionError and JSONDecodeError, inherit from RequestException. So no need for custom exception(s) 👍

Proposed changes

Based on the existing code of version 2.0.14 (for example for update_order()) I made a generic request handler for fictitious call get_data of class API:

class API:
    def get_data(self, raise_exception:bool=False) -> bool:
        try:
            # Make GET request to server and use JSON response body:
            response_raw = requests.request("GET", "https://api.github.com/users/google")
            response_raw.raise_for_status()
            response_dict = response_raw.json()
        except requests.exceptions.HTTPError as exception:
            self.handle_exception(exception, raise_exception, True)
            return None
        except Exception as exception:
            self.handle_exception(exception, raise_exception)
            return None

        # Do something with `response_dict`...

        return response_raw.status_code == 200

Notes

  1. The new optional input parameter raise_exception that defaults to False for a seamless migration of existing users.
  2. For the simplicity of this proposal I've left out the input parameter raw.
  3. The current code handles specific HTTPError (to print details to terminal) and the generic Exception.

These 2 exceptions are handled generically in method handle_exception():

    def handle_exception(self, exception:Exception, raise_exception:bool, is_http_error:bool=False):
        # Show details in terminal.
        # This is the current behaviour of version 2.0.14, see: https://github.com/b-aeby/degiro-connector/blob/c9a9a236eb89a50f0141a16603d028eaef3e8c58/degiro_connector/trading/actions/action_update_order.py#L126-L129
        if is_http_error:
            status_code = getattr(exception.response, "status_code", "No status_code found.")
            text = getattr(exception.response, "text", "No text found.")
            logging.error(status_code)
            logging.error(text)
        else:
            logging.error(exception)

        if raise_exception:
            # Forward standard exceptions, such as `HTTPError`, `ConnectTimeout`, `ConnectionError` and `JSONDecodeError`:
            raise(exception)
        else:
            # Migration idea: Teach user about new feature:
            logging.info("Tip: set property `raise_exception` to `True` when calling this function and catch these fault details as exception `{0}`.".format(ExceptionUtils.get_full_class_name(exception)))

Notes

  1. The line to show the tip contains a reference to ExceptionUtils.get_full_class_name(). It will return a string like "requests.exceptions.ConnectTimeout". The source code can be found in the executable example at the bottom of this post.

Existing usage

Existing users will still use the function call without argument:

    # Init API:
    api = API()

    # Make an API call:
    my_data = api.get_data()    
    logging.info("Received data from server:\n{0}".format(my_data))

This will either:

New usage

Add parameter raise_exception to the function call and set it to True. Embed the call in a try/except condition and handle whatever specific or generic exceptions:

    # Init API:
    api = API()

    # Make an API call:
    try:
        my_data = api.get_data(raise_exception=True)
        logging.info("Received data from server:\n{0}".format(my_data))
    except requests.exceptions.HTTPError as exception:
        logging.error("HTTPError: {0}".format(exception))
        logging.error("Server responded with HTTP Status code {0} and {1} on {2} request.".format(ExceptionUtils.http_status_code(exception), exception.response, exception.request))
    except requests.exceptions.ConnectTimeout as exception:
        logging.error("ConnectTimeout: {0}".format(exception))
    except requests.exceptions.ConnectionError as exception:
        logging.error("ConnectionError: {0}".format(exception))
    except Exception as exception:
        logging.error("Exception: Failed to connect to server with exception: {0}".format(exception))

This will either:

Notes

  1. The line to show the HTTPError details contains a reference to ExceptionUtils.http_status_code(). It's a convenient method to return the HTTP Status Code as integer (e.g. 404). The source code can be found in the executable example at the bottom of this post. Of course this is also possible by exception.response.status_code, but that might raise a AttributeError in case the exception has no response attribute.

Working example code

Here is the code for a working example. It shows the current output in the Terminal, and the new output when using raise_exception. Just copy/paste everything in a file error-handling.py and run. Note that the comments in get_data() contain instructions to force different exceptions.

import logging
import requests

class ExceptionUtils:
    def get_full_class_name(obj) -> str:
        """
        Returns the full class name of an object.

        Example
        -------
        Get the full class name of a specific `Exception`, for instance from the `requests` library:
    try:
        response = requests.request("GET", "https://wwz.google.com")
    except Exception as exception:
        print(get_full_class_name(exception))
    ```
    This will print `requests.exceptions.ConnectionError`.
    """
    # See: https://stackoverflow.com/a/58045927/2439941
    module = obj.__class__.__module__
    if module is None or module == str.__class__.__module__:
        return obj.__class__.__name__
    return module + '.' + obj.__class__.__name__

def http_status_code(exception:Exception) -> int:
    """
    Convenience method to return the HTTP reponse status code from an `Exception`.

    When there is no `response` object or it has no information about the status code, `0` is returned.

    For the description of the HTTP status codes, see: https://en.wikipedia.org/wiki/List_of_HTTP_status_codes.
    """
    if hasattr(exception, 'response') and hasattr(exception.response, 'status_code'):
        return exception.response.status_code
    else:
        return 0

class API: def get_data(self, raise_exception:bool=False) -> bool: try:

To force the following exceptions:

        # - HTTPError       : change method from "GET" to "POST"
        # - ConnectionError : change "github" in `url` to "git44hub"
        # - ConnectTimeout  : change `timeout` from 30.001 to 0.001
        # - JSONDecodeError : change `url` from "https://api.github.com/users/google" to "https://api.github.com/zen"

        # Make GET request to server and use JSON response body:
        response_raw = requests.request("GET", url="https://api.github.com/users/google", timeout=30.001)
        response_raw.raise_for_status()
        response_dict = response_raw.json()
    except requests.exceptions.HTTPError as exception:
        self.handle_exception(exception, raise_exception, True)
        return None
    except Exception as exception:
        self.handle_exception(exception, raise_exception)
        return None

    # Do something with `response_dict`...

    return response_raw.status_code == 200

def handle_exception(self, exception:Exception, raise_exception:bool, is_http_error:bool=False):
    # Show details in terminal.
    # This is the current behaviour of version 2.0.14, see: https://github.com/b-aeby/degiro-connector/blob/c9a9a236eb89a50f0141a16603d028eaef3e8c58/degiro_connector/trading/actions/action_update_order.py#L126-L129
    if is_http_error:
        status_code = getattr(exception.response, "status_code", "No status_code found.")
        text = getattr(exception.response, "text", "No text found.")
        logging.error(status_code)
        logging.error(text)
    else:
        logging.error(exception)

    if raise_exception:
        # Forward standard exceptions, such as `HTTPError`, `ConnectTimeout`, `ConnectionError` and `JSONDecodeError`:
        raise(exception)
    else:
        # Migration idea: Teach user about new feature:
        logging.info("Tip: set property `raise_exception` to `True` when calling this function and catch these fault details as exception `{0}`.".format(ExceptionUtils.get_full_class_name(exception)))

def main():

SETUP LOGGING LEVEL

logging.basicConfig(level=logging.INFO)

# Init API:
api = API()

# Make an API call:
# 1) Seamless migration for existing usage without the new optional `raise_exception`
my_data = api.get_data()    
logging.info("Received data from server:\n{0}".format(my_data))

# 2) New usage with the new optional `raise_exception` to parse all kind of faults:
logging.info("Use `raise_exception`")
try:
    my_data = api.get_data(raise_exception=True)
    logging.info("Received data from server:\n{0}".format(my_data))
except requests.exceptions.HTTPError as exception:
    logging.error("HTTPError: {0}".format(exception))
    logging.error("Server responded with HTTP Status code {0} and {1} on {2} request.".format(ExceptionUtils.http_status_code(exception), exception.response, exception.request))
except requests.exceptions.ConnectTimeout as exception:
    logging.error("ConnectTimeout: {0}".format(exception))
except requests.exceptions.ConnectionError as exception:
    logging.error("ConnectionError: {0}".format(exception))
except Exception as exception:
    logging.error("Exception: Failed to connect to server with exception: {0}".format(exception))

Run main

if name == "main": main()



@Chavithra Please let me know what you think about this.
Chavithra commented 2 years ago

That's a great work @funnel20 ! Will check that in detail the following days and give you my feedback.

Deliki commented 2 years ago

Is it possible to share more examples on the degiro api? How you ise it? I would love to integrate it with backtrader, any ideas how to do that? Regards

funnel20 commented 2 years ago

Is it possible to share more examples on the degiro api? How you ise it? I would love to integrate it with backtrader, any ideas how to do that? Regards

@samardzicd Please respect the GitHub rules by commenting on-topic to an issue. You ask a general question that has nothing to do with this particular issue. The docu for each function contains example code and often a reference to a sample script, for example how to login. How much more examples do you need? So either read the docu and play around with the examples, or open a new issue with a specific question.

Deliki commented 2 years ago

My bad, will respect the process.

Chavithra commented 2 years ago

My bad, will respect the process.

@funnel20 is right : opening another issue for theses points will help the tracking and make sure I won't forget about it

Will love to have this repo used in backtrader !

Let us know which examples you would like (in another issue).

Chavithra commented 2 years ago

@funnel20 seems good to me.

I think this should be implemented in an actual PR for instance for check_order. This way we can bot fetch it and check for :

Might also be able to add unittest to mock :

Thanks

funnel20 commented 2 years ago

@Chavithra Awesome. I'd love to create a PR. Few questions:

  1. What do you think of the suggested parameter name raise_exception?
  2. In my example I created the ExceptionUtils helper class. Do you think this is Pythonian correct? If so, any suggestion where in your project I can add this class? Or can I add the functions to an existing class?
funnel20 commented 2 years ago

That's a great work @funnel20 ! Will check that in detail the following days and give you my feedback.

@Chavithra Any comments on my proposal would be appreciated. And if you agree, please answer my questions on how to integrate this in the current project.