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

[funnel20]

For example the Trading API might return a negative response while creating or updating orders, see for example the error response in #50 (comment)

{"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.

• updating an order
• a retry
• an abort ("call the manager" 😃)

Please let me know if this is possible.

[Chavithra]

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

[funnel20]

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

[funnel20]

@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:

degiro-connector/degiro_connector/trading/actions/action_confirm_order.py

Lines 140 to 148 in e950396

	except requests.HTTPError as e:
	status_code = getattr(response_raw, "status_code", "No status_code found.")
	text = getattr(response_raw, "text", "No text found.")
	logger.fatal(status_code)
	logger.fatal(text)
	if raw is True:
	response_dict = response_raw.json()
	logger.fatal(response_dict)
	return response_dict

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

• L143 shows status code 400
• L144 shows text
• L147 shows basically the same as L144 and misses de HTTP status code (400 in this case), while still referring to response_raw that contains status_code as indicated in L141+143.

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]

Hi @funnel20,

Thanks

Happy New Year to you too !

How about returning one of the following ?

• the requests.Response object
• or a specific Error including the requests.Response object
• or a specific Error including : text + status_code

I think the Tuple is too implicit.

Also I am wondering :

• how the modification of the interface can impact the other users ?
• if it's possible to use the same error handling system for all the Actions ?

[funnel20]

@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:

• False
  The Connector parses the HTTP Response body and returns the desired properties with Python names as a dict. Sometimes a time stamp is added, which can be misleading, as it's not the timestamp of the DeGiro acceptance, but the current system time upon reception of the response.
• True
  The JSON body of the HTTP Response object is returned as a dict

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

• False
  Returns None in case of any requests.HTTPError, without sharing the details.
• True
  The JSON body of the HTTP Response contains no HTTP fault data, only the errors dict, which is a localised string from DeGiro. This does not report about any of the HTTP Connection faults.

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:

• False(default)
  The first argument response of the Tuple is exactly the same as the current; a dict on success, and None on error.
  Consider the original code from a user:
  

  degiro-connector/examples/trading/order.py

  Lines 60 to 62 in cb62896

  	confirmation_response = trading_api.confirm_order(
  	confirmation_id=confirmation_id, order=order
  	)

  
  The only change that should be done for migration is either add the second error argument for the unpacking:

  confirmation_response, error = trading_api.confirm_order(confirmation_id=confirmation_id, order=order)

  Or get the first argument by index:

  confirmation_response[0] = trading_api.confirm_order(confirmation_id=confirmation_id, order=order)

• True
  The current implementation returns the JSON body from the requests.Response object as dict
  

  degiro-connector/degiro_connector/trading/actions/action_confirm_order.py

  Line 139 in e950396

  response_dict = response_raw.json()

  
  The proposed new output contains the entire requests.Response object.
  So the migration would be as simple as applying the .json() on the function response:

  confirmation_response = trading_api.confirm_order(confirmation_id=confirmation_id, order=order, raw=True)
  confirmation_response = checking_response.json()

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]

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]

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]

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

[funnel20]

Awesome, thanks for teaching me something new this morning.

[funnel20]

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]

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]

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]

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]

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]

Here is my requirement proposal.

Currently

When an action fails we are :

• logging the error (HTTPError, JSONDecodeError,...)
• returning None

New behaviour

We will add a new parameter to an action.

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

• log the error
• raise an Exception

When this parameter is disabled, we will :

• log the error
• return None

The raised Exception needs to contain :

• raw http request
• raw http response

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

[funnel20]

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]

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]

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]

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():

• The existing behaviour is performed when raise_exception is False. The only new feature is the Tip in the terminal about this new input parameter.
• The new behaviour when raise_exception is True, is as simple as forwarding the existing 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:

• return True for HTTP 200
• return None and show logs in the terminal in case of an error. For example for HTTPError:

  ERROR:root:404
  ERROR:root:{"message":"Not Found","documentation_url":"https://docs.github.com/rest"}
  INFO:root:Tip: set property `raise_exception` to `True` when calling this function and catch these fault details as exception `requests.exceptions.HTTPError`.
  INFO:root:Received data from server:
  None
  

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:

• return True for HTTP 200
• return None and show the same logs as the existing code (except the tip) in the terminal in case of an error. For example for HTTPError, the first 2 log lines come from the API. The other 2 demonstrate the existence of the request and response objects in the returned exception:

  ERROR:root:404
  ERROR:root:{"message":"Not Found","documentation_url":"https://docs.github.com/rest"}
  ERROR:root:HTTPError: 404 Client Error: Not Found for url: https://api.github.com/users/google
  ERROR:root:Server responded with HTTP Status code 404 and <Response [404]> on <PreparedRequest [POST]> request.
  

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]

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

[Deliki]

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]

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]

My bad, will respect the process.

[Chavithra]

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]

@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 :

• edge cases
• possible improvements

Might also be able to add unittest to mock :

• exceptions raising
• api calls

Thanks

[funnel20]

@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]

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.