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

fix: LowerCaseQueryStringMiddleware should not truncate query parameters #677

Merged
merged 3 commits into from
Aug 21, 2023

Conversation

jthetzel
Copy link
Contributor

@jthetzel jthetzel commented Jul 27, 2023

Small fix where LowerCaseQueryStringMiddleware causes query parameter error with pre-signed urls. Co-authoring with @jackharrhy . Closes #678 .

@vincentsarago
Copy link
Member

@jthetzel thanks for the PR.

Is there a possible test we could add?

@jackharrhy
Copy link
Contributor

@vincentsarago tests now exist

before:

>       assert response.json() == {"url": [url]}
E       AssertionError: assert {'url': ['htt...=geospatial']} == {'url': ['htt...anet=better']}
E         Differing items:
E         {'url': ['https://developmentseed.org?solutions=geospatial']} != {'url': ['https://developmentseed.org?solutions=geospatial&planet=better']}
E         Use -v to get more diff

after:
✔️ test pass, the second query param is not trimmed

@jthetzel jthetzel changed the title wip: LowerCaseQueryStringMiddleware with pre-signed url causes gdal "does not exist" error fix: LowerCaseQueryStringMiddleware with pre-signed url causes gdal "does not exist" error Jul 31, 2023
@jthetzel jthetzel changed the title fix: LowerCaseQueryStringMiddleware with pre-signed url causes gdal "does not exist" error fix: LowerCaseQueryStringMiddleware should not truncate query parameters Jul 31, 2023
@jthetzel
Copy link
Contributor Author

jthetzel commented Aug 5, 2023

This is ready for review. Thanks @vincentsarago !

@vincentsarago
Copy link
Member

Thanks @jackharrhy, @jthetzel
could one of you add a line in the changelog 🙏

@jackharrhy
Copy link
Contributor

added a line in the changelog @vincentsarago :)

@vincentsarago vincentsarago merged commit 5023044 into developmentseed:main Aug 21, 2023
8 of 9 checks passed
@jthetzel jthetzel deleted the fix-string-middleware branch August 21, 2023 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

included LowerCaseQueryStringMiddleware middleware affects query strings
3 participants