You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Okay, I'd like to answer this #186 (comment) here, as that's the more appropriate place and any effort to fix this issue probably needs a new PR.
But correct encoding of Edm.Binary as parameters in URL should be covered as well, right?
Of course it should be correct in all components of the URL. According to URI Conventions there could be two place in the URI with value content: The Key Value in the Resource Path (3.1.) and Filter System Query Option (4.5.). Both cases refer to the so called Literal, that is specific to the Primitive Data Type.
The "binary encoding" of URL is really something to handle in the service.py module and the ODataHttpRequest, since URI encoding is not part of the odata metadata and odata model per se. I see it as basic separation of responsibility. Consider hypothetical possibility of supporting the same odata metadata, but with query the server with different than HTTP protocol (it is not yet existing something like this and probably never be, but let's say Odata on something like Gopher or IPFS protocol)
Full agreement on the separation of concerns, thanks for pointing that out. With that in mind, maybe it wasn't precise enough to call this an URL generation issue. Actually it's an issue with the generation of the so called Literal.
And I'm afraid this can't be fixed in the ODataHttpRequest class because its methods only ever get the URL parts as string components. They'd have to look up the data types of the contained entities again to know, how they need to be manipulated.
So for the key value the path component is fetched from
So if anybody is ever going to specifiy OData V2 over another fancy protocol, there'd still be an adressing scheme, that would probably also refer to the type depended literals. And that makes this issue touch the model domain.
Or put in other words, if the literal shouldn't be inside the model domain, then why are the TypTraits classes and their to_literal methods.
OK, "OData V2 over another fancy protocol" will probably never happen. I was trying to illustrate a point about separation, but not stating a valid, although hypothetical requirement. My bad. So let's consider to stick OData V2 only. And with that it mind, even the only encoding/decoding strictly base64-base16 will suffice for now.
You are also correct with all the comments about service.py:218 and service.py:990, the ODataHttpRequest is not that great place after all.
So I would probably continue with #186 as currently is, by adding/modifying tests and call it a day :)
Option 2 defined there ("Values are stored in their Python native representation") is more dangerous. The backward compatibility break is a reason enough not to do it. But also (maybe I am again missing something), in the end, there would need to be basically same difference in binary encoding for the reading of the value (e.g. generating POST body) and for the to_literal() method (generating URL key/params), as in Option 1; so in the end, option 1 with extra steps. Am I Right?
I was trying to illustrate a point about separation, but not stating a valid, although hypothetical requirement. My bad.
That's allright. I got the point.
in the end, there would need to be basically same difference in binary encoding for the reading of the value (e.g. generating POST body) and for the to_literal() method (generating URL key/params)
Agree, so for Option 1 that means: to/from_json does nothing and to/from_literal does base64 to base16 and vice versa.
Activity
phanak-sap commentedon Jan 1, 2022
Thanks, that's good.
Even better would be with metadata file, as something that can be actually run locally, for example like https://github.com/phanak-sap/pyodata-issue-files/tree/master/%23142. Feel free to add PR to that repository, with new folder "#187" so its easy to link with this particular issue
inxonic commentedon Jan 3, 2022
Okay, I'd like to answer this #186 (comment) here, as that's the more appropriate place and any effort to fix this issue probably needs a new PR.
Of course it should be correct in all components of the URL. According to URI Conventions there could be two place in the URI with value content: The Key Value in the Resource Path (3.1.) and Filter System Query Option (4.5.). Both cases refer to the so called Literal, that is specific to the Primitive Data Type.
Full agreement on the separation of concerns, thanks for pointing that out. With that in mind, maybe it wasn't precise enough to call this an URL generation issue. Actually it's an issue with the generation of the so called Literal.
And I'm afraid this can't be fixed in the
ODataHttpRequestclass because its methods only ever get the URL parts as string components. They'd have to look up the data types of the contained entities again to know, how they need to be manipulated.So for the key value the path component is fetched from
python-pyodata/pyodata/v2/service.py
Lines 371 to 372 in 21a99b5
to_literalin theEntityKeyclasspython-pyodata/pyodata/v2/service.py
Line 218 in 21a99b5
The same is true for the filter value in the query component, that calls
to_literalin theGetEntitySetFilterpython-pyodata/pyodata/v2/service.py
Line 990 in 21a99b5
So if anybody is ever going to specifiy OData V2 over another fancy protocol, there'd still be an adressing scheme, that would probably also refer to the type depended literals. And that makes this issue touch the model domain.
Or put in other words, if the literal shouldn't be inside the model domain, then why are the TypTraits classes and their
to_literalmethods.phanak-sap commentedon Jan 4, 2022
OK, "OData V2 over another fancy protocol" will probably never happen. I was trying to illustrate a point about separation, but not stating a valid, although hypothetical requirement. My bad. So let's consider to stick OData V2 only. And with that it mind, even the only encoding/decoding strictly base64-base16 will suffice for now.
You are also correct with all the comments about service.py:218 and service.py:990, the
ODataHttpRequestis not that great place after all.So I would probably continue with #186 as currently is, by adding/modifying tests and call it a day :)
Option 2 defined there ("Values are stored in their Python native representation") is more dangerous. The backward compatibility break is a reason enough not to do it. But also (maybe I am again missing something), in the end, there would need to be basically same difference in binary encoding for the reading of the value (e.g. generating POST body) and for the to_literal() method (generating URL key/params), as in Option 1; so in the end, option 1 with extra steps. Am I Right?
inxonic commentedon Jan 5, 2022
That's allright. I got the point.
Agree, so for Option 1 that means: to/from_json does nothing and to/from_literal does base64 to base16 and vice versa.
Okay, I will dig into the tests.
Fix Edm.Binary literal representation