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

OpenAPI construction #499

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

c-wygoda
Copy link
Contributor

Related Issue(s):

Description:

  • Use standard FastAPI OpenAPI builder and inject metadata through env settings
  • Remove stac_api.api.app:StacAPi.customize_openapi method as it unnecessarily wraps the FastApi.openapi call.

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.

@geospatial-jeff
Copy link
Collaborator

@c-wygoda Whats the reason for this change?

@c-wygoda
Copy link
Contributor Author

We want to be able to specify more of the OpenApi metadata fields and in the process I found the customize_openapi method to be a rather redundant wrapper around the OpenApi functionality, if the FastAPI params are used directly in the attribute definition of the app.

@c-wygoda
Copy link
Contributor Author

This would also tackle #454 if I'm not mistaken.

@geospatial-jeff
Copy link
Collaborator

geospatial-jeff commented Nov 25, 2022

I found the customize_openapi method to be a rather redundant wrapper around the OpenApi functionality,

I agree, this code is leftover from years ago before this project was open sourced when customization of OpenAPI was less flexible. Now customize_openapi is hurting more than it helps (ref #454).

We want to be able to specify more of the OpenApi metadata fields

There is precedence to define openapi related things in ApiSettings so I think this is a good approach 👍

Note to self that at some point in the future we should deprecate StacApi.title, api_Version, and description (obviously a breaking change).

@c-wygoda
Copy link
Contributor Author

Thanks. I'm happy to help as I'm responsible for an instance, so hopefully can contribute more. Direct feedback welcome as I slowly learn the ropes of this project...

@geospatial-jeff
Copy link
Collaborator

geospatial-jeff commented Nov 25, 2022

@c-wygoda The changes LGTM but I do want to play with this a little more before we merge to understand the difference between setting servers to self.app.servers compared to pulling from the environment within the context of #454 #489 #497

@c-wygoda
Copy link
Contributor Author

c-wygoda commented Dec 6, 2022

How can I help to move forward? More test coverage?

@gadomski
Copy link
Member

@geospatial-jeff ping

@gadomski gadomski self-requested a review January 31, 2023 17:34
@gadomski gadomski modified the milestones: 2.4.4, 3.0.0 Jan 31, 2023
@eseglem eseglem mentioned this pull request Feb 6, 2023
4 tasks
@gadomski gadomski removed their request for review March 13, 2023 16:21
@vincentsarago
Copy link
Member

@jonhealy1 I think we should use this instead of #639 (we would need to update the landing page)

@vincentsarago
Copy link
Member

I'm not 💯 what we should do.

This kinda breaking change so definitely something we should keep for 3.0, but at the same time #639 does almost the same thing without breaking anything.

Both PR fixes stuff but do not represent the best solution to the issue:

  • 499: fixes the OpenAPI but does not forward title, description,... to the landing page
  • 639: add info to both the OpenAPI and landing page but doesn't use the Config class and add some code duplication

IMO we should wait after 2.5 to be released and see if we can refactor a bit to accommodate both changes

cc @jonhealy1

@m-mohr
Copy link
Contributor

m-mohr commented Apr 10, 2024

This PR should add some documentation how to set the env variables...

@vincentsarago
Copy link
Member

in #639 we implemented part of the proposed solution, Let's wait after 2.5 release to finish this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants