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/forwarding headers #415

Merged
merged 9 commits into from
Aug 1, 2022
Merged

Fix/forwarding headers #415

merged 9 commits into from
Aug 1, 2022

Conversation

captaincoordinates
Copy link
Contributor

Related Issue(s):

Description:
Support Forwarded and X-Forwarded-* headers so that stac-fastapi responses contain links that accommodate load balancer or proxy configurations.

Note: there is duplication of both code and concepts across the pgstac and sqlalchemy applications. Each application has its own method of generating links (I don't know why this logic wasn't shared) and therefore each has a copy of the get_base_url_from_request function here. This explains why this PR has duplicate code and does not follow a more sensible DRY strategy - the repo really isn't set up for it and I am not looking to make large-scale refactoring changes here.

PR Checklist:

  • Code is formatted and linted (run pre-commit run --all-files)
  • Tests pass (run make test)
  • Documentation has been updated to reflect changes, if applicable, and docs build successfully (run make docs)
  • Changes are added to the CHANGELOG.

@gnosys-ccoupal
Copy link

Looks great. I will give this fix a try.. One thing I noted.. base_url is used in a few times in stac_fastapi/types/stac_fastapi/types/core.py as part of landing page. Should these be addressed as well?

@captaincoordinates
Copy link
Contributor Author

base_url is used in a few times in stac_fastapi/types/stac_fastapi/types/core.py as part of landing page. Should these be addressed as well?

@gnosys-ccoupal yes, I didn't see that, and it complicates things. Currently I have an implementation in both the pgstac and sqlalchemy application, but stac_fastapi/types/stac_fastapi/types/core.py is not part of either application and therefore doesn't have access to the header logic.

Perhaps the better solution is to introduce a middleware in stac_fastapi/api/middleware.py that modifies request.base_url, so that no additional logic is required outside the middleware.

@captaincoordinates captaincoordinates marked this pull request as draft July 5, 2022 23:59
@captaincoordinates
Copy link
Contributor Author

Converted to Draft PR while working on a suitable middleware approach.

@captaincoordinates captaincoordinates marked this pull request as ready for review July 6, 2022 16:46
@captaincoordinates
Copy link
Contributor Author

No longer draft status - ready to review with new middleware approach that covers all response links

Copy link
Collaborator

@geospatial-jeff geospatial-jeff left a comment

Choose a reason for hiding this comment

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

This LGTM! Thanks for writing solid tests. Because this feature is talked about a lot I'm going to write some unittests covering the middleware itself and then get this merged.

@geospatial-jeff geospatial-jeff self-requested a review August 1, 2022 18:12
@geospatial-jeff geospatial-jeff merged commit b7580fe into stac-utils:master Aug 1, 2022
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.

3 participants