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

base_url usage on landing page #635

Merged
merged 3 commits into from
Apr 9, 2024

Conversation

zachcoleman
Copy link
Contributor

@zachcoleman zachcoleman commented Feb 17, 2024

Related Issue(s):

Description:

Adds the base_url usage for two links where it was missing.

PR Checklist:

  • pre-commit hooks pass locally
  • 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.

@vincentsarago
Copy link
Member

@zachcoleman thanks for your PR, I need to catch up with stac-fastapi but is there any tests for this or should we make one?

@zachcoleman
Copy link
Contributor Author

zachcoleman commented Mar 25, 2024

@zachcoleman thanks for your PR, I need to catch up with stac-fastapi but is there any tests for this or should we make one?

For this functionality, I think not. It just matches the usage around it. This seems like it'd just be adding tests for sake of adding tests. I think it will be more useful to probably test get_base_url (as it's not tested today).

@vincentsarago vincentsarago merged commit d18f2d6 into stac-utils:main Apr 9, 2024
6 checks passed
@vincentsarago
Copy link
Member

FYI: this ended up introducing a bug https://github.com/stac-utils/stac-fastapi-pgstac/pull/101/files#r1576403075

"href": urljoin(
str(request.base_url), request.app.openapi_url.lstrip("/")
),
"href": urljoin(base_url, request.app.openapi_url.lstrip("/")),
Copy link
Member

Choose a reason for hiding this comment

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

When a user adds a router_prefix to the API, the base_url as the prefix added in

def get_base_url(request: Request) -> str:
"""Get base URL with respect of APIRouter prefix."""
app = request.app
if not app.state.router_prefix:
return str(request.base_url)
else:
return "{}{}/".format(
str(request.base_url), app.state.router_prefix.lstrip("/")
)

The issue is that, when we create the fastapi app

app: FastAPI = attr.ib(
default=attr.Factory(
lambda self: FastAPI(
openapi_url=self.settings.openapi_url,
docs_url=self.settings.docs_url,
redoc_url=None,
),
takes_self=True,
),
converter=update_openapi,
)

we set the openapi_url to either /api or to what the user has set the openapi_url settings.

so by default we register the openapi to /api but here when we do urljoin(base_url, request.app.openapi_url.lstrip("/")),, we endup with /{base_url}/{router_prefix}/api

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.

2 participants