service: add $format query option to requests which expects a response in the json format

[Masterchen09]

Although it seems to be part of the OData specification (https://www.odata.org/documentation/odata-version-2-0/operations/), it seems that there are OData APIs out there which ignore the Accept header which is sent by the library (like the OData API of the SAP Analytics Cloud).

As the library is relying on the responses in the JSON format (with one exception when the metadata is requested), it is not possible to use an OData API which ignores the Accept header without manually adding the $format query option to each request using the custom method.

This PR adds an optional config parameter (add_format_query_option) which makes it possible to automatically add the $format query option to all requests which expect a response in the JSON format (i.e. requests which are sending an Accept header).

By default the config parameter is set to false to not change the current behavior.

Alternatively I have (first) made a variant without a config parameter (see here), where the $format query option is always added to the request...however I think it is better to opt-in this behavior instead of enforcing it in general.

[codecov-commenter]

Codecov Report

Merging #227 (59456b0) into master (e18fc73) will increase coverage by 0.06%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #227      +/-   ##
==========================================
+ Coverage   92.90%   92.97%   +0.06%     
==========================================
  Files           6        6              
  Lines        2792     2817      +25     
==========================================
+ Hits         2594     2619      +25     
  Misses        198      198              

Impacted Files	Coverage Δ
pyodata/v2/model.py	93.36% <100.00%> (+0.01%)	:arrow_up:
pyodata/v2/service.py	91.56% <100.00%> (+0.19%)	:arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[Masterchen09]

Currently other formats than json are not supported by the library as each response is deserialized using the json method of the Response object. Also the Accept header is hardcoded with the value application/json and is added when the corresponding request expects the response in the JSON format.

But I am not sure if this is even an issue: OData supports the XML-based Atom format and the JSON format. The question is if the specification says that an OData compliant API must support both formats or only one of them. If only one of them must be supported, then yes, it is an issue if the supported format is not the JSON format. I think supporting other formats is a much bigger thing, than I have thought about here:

I thought only about a config parameter which adds the format query option additionally next to the Accept header: If in the future the actual format can be specified because the XML-based Atom format or other formats are supported by the library, then there might be another config parameter which influences the actual format which is sent in the Accept header and optionally is added to the URL in the format query option.

• but from previous block, it is obvious that the boolean for add_format_query_option: bool (default False) - If true, the $format query option is added with the value json to all requests is not a good type for the parameter, nor the config option behavior.

If in the future multiple formats are supported there might be another config parameter: A string for the actual format. But to optionally control whether the format query option should be added to the URL or not a boolean config parameter is needed.

• last but not least - don't forget that in your specific case, you can always append anything (even the $format=json) to every query generated by pyodata by using the custom() method. . Maybe it will be good enough workaround for now

That is exactly what I am doing right now, but of course adding the format query option explicitly might be something the library could handle a bit better than doing it manually and of course this is something which can only be done in case of an ODataHttpRequest (or a subclass of it), but this is not available for the get_proprty or get_value methods of the EntityProxy as these two methods are using the method http_get_odata of the Service class, where you do not have the possiblity to add some custom headers (but this is might also be something I have also not handled correctly using the format query option).

[phanak-sap]

Hi Masterchen09

I am still under impression, that this particular PR does not provide much additional value (in comparison to usage of .custom()), only binds us to particular config parameter that needs to be maintained. Yes, "if in the future multiple formats are supported there might be another config parameter: A string for the actual format." . But maybe this would be enough, right - if filled, then it will append to URL (since it must enforced this way).

On the contrary, having two parameters leads to situation, where string format parameter is set to 'json', but your add_format_query_option is set to 'false' - what behavior should this combination cover? I am unable to find use case for this combination - and this leads me that one, string parameter is enough to cover the atom/json formats in odata responses.

I am inclined in this case to close the pull request.

[Masterchen09]

I am not really sure if we are both talking about the same thing...😅

There are two possibilities to tell the requested API which format the client supports/wants: The Accept header and the $format query option. Both possiblities are mentioned in the OData specification, while the $format query option takes precedence before the Accept header. Currently the library only adds the Accept header with the value application/json to the requests which are expected to return a response in the JSON format.

This PR was intended to also add the $format query option to these requests (either optionally via a config parameter or to all requests which expects the response in the JSON format). Of course this is also possible using the custom method, but if the goal "is to hide all OData protocol implementation details" this is definitely something which could be done by the library.

On the contrary, having two parameters leads to situation, where string format parameter is set to 'json', but your add_format_query_option is set to 'false' - what behavior should this combination cover? I am unable to find use case for this combination - and this leads me that one, string parameter is enough to cover the atom/json formats in odata responses.

Assuming there is a parameter for the format and a parameter for the presence of the $format query option, the first parameter would control the value of the Accept header and the value of the $format query option, while the second parameter would only control whether the $format query option should be added to the requested URL or not.

If we are assuming that the library also supports the Atom format the first parameter could be set to atom and the second to false, then the Accept header would be set to application/atom+xml, but the $format query option would not be added to the requested URL. Of course if the value of the first parameter is set to json it would correspond to the current behavior (the Accept header is set to application/json and the $format query option is not added). If instead in both cases the second parameter is set to true, the $format query option would be added with the corresponding value of the first parameter (atom or json).

Basiclly as long the library does not supports other formats the first parameter is not needed and it is more or less only the question whether there should be a config parameter for controlling the precense of the $format query option or not (or whether the $format query option should be added by the library at all).

[phanak-sap]

Hi @Masterchen09,

after long time - I really thought about this probably more length of time I should, but I really thought this trough with cost/benefits of both merging and closing. I then decided not to merge this at the moment.

The idea is on basic level sound and you provided good code, especially when considered all previous fixes. I hope we will meet on PRs in the future as well. But this one is binding me to specific Config class structure, when the odata v2 support for Atom/XML is not present in pyodata (so would be better to have all config things with this such feature together, then up-front) and also odata V4 is the biggest feature requirement - so the Config may look very different, when maybe will be odata version depenedent (v4 does not have the json/atom split, so no $format anyway). I do not want to be forced for backward compatibility for this config option (maybe the 2.x version will be necessary, but I hope pyodata will be on trunk-based development as long as possible). Additionaly, all other config options are more of parser related, than service-url-generation related - so it kind of ingerently point that the config class should be maybe more granular anyway, or maybe config per module, I don't know... And it is doable with the custom() method anyway, so it is not even not that pressing edge case. All of that combined were forming my decision.

I will keep this idea in mind and again, thanks for all the effort and I hope I will see you next time on some code improvement pull request or issue/enhancement.