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

add paging links tests #195

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

vincentsarago
Copy link
Member

trying to replicate issue from #194

cc @Kirill888

@Kirill888
Copy link

@vincentsarago thanks for looking into this, it could very well be an interaction with Magnum wrapper then

this is roughly how my current setup looks

prefix = "/stac"
_app = FastAPI(
        root_path=prefix,
        lifespan=lifespan,
)
# ... more setup
_api = StacApi(
        title=title,
        description=description,
        settings=settings,
        extensions=application_extensions,
        client=CoreCrudClient(pgstac_search_model=post_request_model),
        items_get_request_model=items_get_request_model,
        search_get_request_model=get_request_model,
        search_post_request_model=post_request_model,
        collections_get_request_model=collections_get_request_model,
        middlewares=middlewares,
        app=_app,
        **kw,
)

# AWS Lambda wrapper
handler = Mangum(
        app,
        api_gateway_base_path=app.root_path,
        lifespan="off",
        text_mime_types=text_mime_types,
 )

In the debugger I see that _api.app.router.prefix = '', should I expect it to be /stac instead?

@vincentsarago
Copy link
Member Author

@Kirill888

In the debugger I see that _api.app.router.prefix = '', should I expect it to be /stac instead?

I wonder if this is because we don't set prefix when registering the router in https://github.com/stac-utils/stac-fastapi/blob/main/stac_fastapi/api/stac_fastapi/api/app.py#L440C9-L440C32

🤔

@Kirill888
Copy link

Kirill888 commented Feb 3, 2025

@vincentsarago I'm not too familiar with FastAPI or web programming in Python in general, but my understanding is this

  • FastAPI.root_path is about telling the app sitting behind a reverse proxy what URL the user is using to access it. And this URL can have extra components on the left side that is used by the proxy for routing but is removed by the proxy when forwarding to the APP, e.g. root_path=/internal/api/v1.2. See Proxy with a stripped path prefix FastAPI docs.
  • While Router.prefix is about gluing together different parts of the API within a single app, e.g. prefix=/collections

So I think in the test you should NOT set prefix= on the Router at all and invoke these methods with URLs without root_path= component present, yet the output should contain root_path prefix in all the URLs back to self.

@vincentsarago
Copy link
Member Author

So I think in the test you should NOT set prefix= on the Router at all and invoke these methods with URLs without root_path= component present, yet the output should contain root_path prefix in all the URLs back to self.

The test added in this PR test endpoint with/without root-path or prefix (2x2 matrix)

if we look at https://github.com/encode/starlette/blob/a7d0b14c7378aa2e95b0b13583fe0dadace363be/tests/test_routing.py#L568-L575, it seems that we have to call /{root-path}/{router-prefix}/{endpoint-path} within the TestClient 🤷

@Kirill888
Copy link

@vincentsarago see #201

With that change to the test, I DO get test failure for link generation on my fork:

https://github.com/Kirill888/stac-fastapi-pgstac/actions/runs/13361242617/job/37311202071?pr=1#step:7:1068

=========================== short test summary info ============================
FAILED tests/api/test_links.py::tests_app_links[/api/v1] - AssertionError: assert 'http://stac.io/search' == 'http://stac.io/api/v1/search'
  
  - http://stac.io/api/v1/search
  ?                -------
  + http://stac.io/search
====== 1 failed, 888 passed, 25 skipped, 8 warnings in 359.23s (0:05:59) =======
Error: Process completed with exit code 1.

Comment on lines +12 to +13
endpoint_prefix = root_path + prefix
url_prefix = "http://stac.io" + endpoint_prefix

Choose a reason for hiding this comment

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

see #201 , but this should also work:

Suggested change
endpoint_prefix = root_path + prefix
url_prefix = "http://stac.io" + endpoint_prefix
endpoint_prefix = prefix
url_prefix = "http://stac.io" + root_path + endpoint_prefix

we want to model situation when root_path is dropped by the reverse proxy, but needs to be present in the output links as this is the only way to reach the API.

Copy link
Member Author

Choose a reason for hiding this comment

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

🤔 I'm not sure to get this to be honest

In all cases, the client should be called {root-path}/{prefix}/endpoint as shown in https://github.com/encode/starlette/blob/a7d0b14c7378aa2e95b0b13583fe0dadace363be/tests/test_routing.py#L573-L575

Choose a reason for hiding this comment

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

🤔 I'm not sure to get this to be honest

In all cases, the client should be called {root-path}/{prefix}/endpoint as shown in https://github.com/encode/starlette/blob/a7d0b14c7378aa2e95b0b13583fe0dadace363be/tests/test_routing.py#L573-L575

I don't understand how you come to this conclusion from that one test alone. I'm just trying to replicate the scenario I see in the debugger of a real system. Omitting the root path in the call to the test client still routes to the endpoint in question, but this time with the request object behaving the same way as in the real system behind a reverse proxy that removes routing prefix.

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