Escape slashes in entity keys when they appear in the URL path

[frij-aws]

trafficstars

Pyodata does not correctly escape the path when an entity key contains a /. This minor correction quotes the entity key with safe="" to ensure slashes are quoted. The rest of the path is quoted with the default (`safe='/') so that path delimiters are not quoted.

Closes #282

[cla-assistant[bot]]

All committers have signed the CLA.

[phanak-sap]

HI @frij-aws

thank you for your work. Generic stuff I commented in the issue, here just what's relevant to the code changes. Nothing existing in the test suite is changed or broken, that's great.

If I may, I would like to ask you to enhance bit the test code.

1. up to consideration for you is whether it would make sense to you to update existing metadata.xml or create new metadata.xml for the test fixtures with these "interesting" entity names. But the new tests are good enough for covering this. If pytest fixtures are not what you work with frequently, ignore this point.
2. however I would like you to also add new test for this entity id case related to batch processing. I do not have service at hand with such $metadata I could test this PR against directly and I am not sure if this fix needs to be applied in the encode_multipart / decode_multipart functions as well. So pls check in the test_service_v2.py test_batch_request and test_batch_request_not_encoded_path and add new one that will cover the expected encoding for your FOO/BAR entity.
3. Is is possible that the FOO/BAR id case is also applicable to the function import names, which are also forming URLs?

[frij-aws]

Just an update on this stalled PR (got sidetracked with some other deadlines). Although this proposed change works for basic GET/PATCH/DELETE operations, it does not solve the issue for nav properties where the key contains a slash. Fixing that will require a significant refactor. But I've been able to work around it by simply escaping the key myself and then using encode_path=False to avoid double-escaping. So now I'm wondering if this whole PR can be scrapped and just leave the issue as a known bug with a known workaround. I will take some time to test some more scenarios to confirm if the workaround works in all cases; so far I've just tried it with nav properties.

[phanak-sap]

Just an update on this stalled PR (got sidetracked with some other deadlines). Although this proposed change works for basic GET/PATCH/DELETE operations, it does not solve the issue for nav properties where the key contains a slash. Fixing that will require a significant refactor. But I've been able to work around it by simply escaping the key myself and then using encode_path=False to avoid double-escaping. So now I'm wondering if this whole PR can be scrapped and just leave the issue as a known bug with a known workaround. I will take some time to test some more scenarios to confirm if the workaround works in all cases; so far I've just tried it with nav properties.

Hi @frij-aws

Same on my side. Priorities elsewhere, many deadlines, restructuralization changes..

At this moment it depends on whether you want to jump on the "significant refactor" you see or not. Obviously fix in code is better than note in docs and as you see, last year the library practically did not change.. so such refactoring would be safe in terms on no incoming merge conflicts and would be sole feature in isolated release and version number.

[phanak-sap]

Since you added additional PR #292 - do you have somewhere the intended significant refactor" you see ? Any results for the "some time to test some more scenarios to confirm if the workaround works in all cases" ?

What do you thing about the work from this pull request now?

[frij-aws]

Let's cancel this PR. It will be too hard to implement fully and the use of `encode_path=False solves the problem for me.