Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

frij-aws
Copy link

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

Copy link

cla-assistant bot commented Sep 19, 2024

CLA assistant check
All committers have signed the CLA.

CHANGELOG.md Outdated
@@ -5,7 +5,8 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/)
and this project adheres to [Semantic Versioning](http://semver.org/).

## [Unreleased]

### Fixed
- Escape slashes in entity keys when they appear in the URL path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: if you see the changelog entries, we add also author names for important changes. So feel free to add yourself to this line :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding my name for the fame and fortune!

@phanak-sap phanak-sap added the bug Something isn't working label Sep 20, 2024
@phanak-sap
Copy link
Contributor

phanak-sap commented Sep 20, 2024

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?

@phanak-sap phanak-sap added this to the 1.11.2 milestone Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect escaping of path
2 participants