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

Case insensitive query parameters #321

Merged
merged 10 commits into from
Jun 11, 2021

Conversation

lorenzori
Copy link
Contributor

Hi, thank you for this awesome tool!

Some client applications require URL query parameters to be case-insensitive, so I have added a middleware to make all query parameters in the request lowercase following an example from an issue in FastAPI.

With this PR for example both https://host/cog/WMTSCapabilities.xml?url=path/to/cog/...tif and https://host/cog/WMTSCapabilities.xml?URL=path/to/cog/...tif
will work.

I am not 100% the WMTS standard says anything about this so happy with any feedback.

@vincentsarago
Copy link
Member

🙏 thanks for the PR @lorenzori

As I said in the comment, I'm not 💯 sure we want to add this middleware by default but I think it's fine to add it in the code so people can add it to their own application.

The titiler.application module aims to be a demo and we highly suggest that titiler users create their own application.

I opened another issue #322 because mixing lowercase/uppercase will break the tilejson and wmts endpoints.

@lorenzori
Copy link
Contributor Author

hey @vincentsarago thanks for the feedback! I have made the LowerCaseQueryStringMiddleware middleware optional as suggested using flag TITILER_API_LOWER_CASE_QUERY_PARAMETERS.

We have looked into making only the query keys lowercase instead of the entire query to avoid breaking the other endpoints and tests pass now locally, however is a bit of a hack as I didn't find an elegant solution with Starlette's Request.

@vincentsarago vincentsarago merged commit adbcd13 into developmentseed:master Jun 11, 2021
@vincentsarago
Copy link
Member

🙏 thanks @lorenzori, I added tests and fixed some format errors

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.

None yet

2 participants