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

Create catch-all/fallback route for UI app #932

Merged
merged 16 commits into from
Nov 19, 2024
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 27 additions & 22 deletions conda-store-server/conda_store_server/_internal/server/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -288,15 +288,32 @@ async def exception_handler(request, exc):
# docker registry api specification does not support a url_prefix
app.include_router(views.router_registry)

if self.enable_ui:
if self.enable_metrics:
app.include_router(
views.router_ui,
prefix=trim_slash(self.url_prefix) + "/admin",
views.router_metrics,
prefix=trim_slash(self.url_prefix),
)

if self.additional_routes:
for path, method, func in self.additional_routes:
getattr(app, method)(path, name=func.__name__)(func)

if isinstance(self.conda_store.storage, storage.LocalStorage):
self.conda_store.storage.storage_url = (
f"{trim_slash(self.url_prefix)}/storage"
)
app.mount(
self.conda_store.storage.storage_url,
StaticFiles(directory=self.conda_store.storage.storage_path),
name="static-storage",
)

# This needs to come at the end because if the UI is enabled,
# it becomes the catch all route
if self.enable_ui:
peytondmurray marked this conversation as resolved.
Show resolved Hide resolved
app.include_router(
views.router_conda_store_ui,
prefix=trim_slash(self.url_prefix),
views.router_ui,
prefix=trim_slash(self.url_prefix) + "/admin",
)

# serving static files
Expand All @@ -321,7 +338,9 @@ async def exception_handler(request, exc):
def redirect_home(request: Request):
return RedirectResponse(request.url_for("get_conda_store_ui"))

@app.get("/favicon.ico", include_in_schema=False)
@app.get(
trim_slash(self.url_prefix) + "/favicon.ico", include_in_schema=False
)
async def favicon():
return FileResponse(
os.path.join(
Expand All @@ -331,26 +350,12 @@ async def favicon():
)
)

if self.enable_metrics:
# Put this at the very end
app.include_router(
views.router_metrics,
views.router_conda_store_ui,
prefix=trim_slash(self.url_prefix),
)

if self.additional_routes:
for path, method, func in self.additional_routes:
getattr(app, method)(path, name=func.__name__)(func)

if isinstance(self.conda_store.storage, storage.LocalStorage):
self.conda_store.storage.storage_url = (
f"{trim_slash(self.url_prefix)}/storage"
)
app.mount(
self.conda_store.storage.storage_url,
StaticFiles(directory=self.conda_store.storage.storage_path),
name="static-storage",
)

return app

def _check_worker(self, delay=5):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,7 @@ async def get_entity(request: Request, auth=Depends(get_auth)):

async def get_templates(request: Request):
return request.state.templates


async def get_url_prefix(request: Request, server=Depends(get_server)):
return server.url_prefix
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,12 @@
REACT_APP_API_URL: "{{ url_for('get_conda_store_ui') }}",
REACT_APP_LOGIN_PAGE_URL: "{{ url_for('get_login_method') }}?next=",
REACT_APP_LOGOUT_PAGE_URL: "{{ url_for('post_logout_method') }}?next=/",
REACT_APP_URL_BASENAME: new URL("{{ url_for('get_conda_store_ui') }}").pathname
REACT_APP_URL_BASENAME: "{{ url_prefix }}"
};
</script>
<script defer src="static/conda-store-ui/main.js"></script>
<link href="static/conda-store-ui/main.css" rel="stylesheet">
<script defer src="{{ url_prefix.rstrip("/") }}/static/conda-store-ui/main.js"></script>
<link href="{{ url_prefix.rstrip("/") }}/static/conda-store-ui/main.css" rel="stylesheet">
<link href="{{ url_prefix.rstrip("/") }}/favicon.ico" rel="icon">
</head>
<body>
</body>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,34 @@
# license that can be found in the LICENSE file.

from fastapi import APIRouter, Depends, Request
from fastapi.responses import HTMLResponse

from conda_store_server._internal.server import dependencies

router_conda_store_ui = APIRouter(tags=["conda-store-ui"])
router_conda_store_ui = APIRouter(
tags=["conda-store-ui"],
# Provide the response class so that it can show up in the autogenerated API
# docs. The docs show the endpoints as returning HTML rather than JSON.
default_response_class=HTMLResponse,
)


# Yes, "{path:path}"" makes "/" redundant. But if we don't define "/", then we
# always have to pass `path` to the `url_for` function like so:
# url_for("get_conda_store_ui", path="")
@router_conda_store_ui.get("/")
@router_conda_store_ui.get("{path:path}")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@peytondmurray I'm not entirely sure why, but this seems to be the line that is causing at least some of the CI checks to fail.

If you remove this line and go to localhost:8080/api/v1/namespace, you'll see that it redirects to same URL but with a forward slash at the end. If you then put this line back in and go to the same URL, you'll notice that it tries to load the client app. You get an HTML page back that tries to load the client app at that URL, but the client renders an error since it doesn't recognize that route (api/v1/namespace).

Any idea how to fix it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Recap

With line 22 removed:

  • localhost:8080/api/v1/namespace -> localhost:8080/api/v1/namespace/
  • localhost:8080/api/v1/namespace/ -> JSON containing list of namespaces

With line 22 in place:

  • localhost:8080/api/v1/namespace -> index.html for client app
  • localhost:8080/api/v1/namespace/ -> JSON containing list of namespaces

Copy link
Contributor Author

@gabalafou gabalafou Nov 6, 2024

Choose a reason for hiding this comment

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

This seems to suggest to me if FastAPI tries all the routes and cannot find a match, then it will add a trailing slash as a last-ditch attempt to not return a 404.

I see there is an option redirect_slashes for the FastAPI class whose default value is True.

I will have to look at the FastAPI source code to see how and when it uses that option.

I wonder if the solution is to explicitly map both routes to the same handler. The following code would allow the user to get the internal admin UI via /admin or via /admin/

@router_ui.get("")
@router_ui.get("/")
async def ui_list_environments(

Copy link
Contributor

@peytondmurray peytondmurray Nov 6, 2024

Choose a reason for hiding this comment

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

I don't have context on this, but it seems like this is intended behavior on the FastAPI side of things. Others have wondered about how to prevent this and about the impacts of redirection on http[s].

Looking at the integration tests, the error shows that the URL is malformed:

aiohttp.client_exceptions.ContentTypeError: 200, message='Attempt to decode JSON with unexpected mimetype: text/html; charset=utf-8', url='http://localhost:8080/conda-store/api/v1/environment?status=COMPLETED&artifact=CONDA_PACK&packages=python&packages=ipykernel/'

It seems like this should be http://localhost:8080/conda-store/api/v1/environment/?status=COMPLETED&artifact=CONDA_PACK&packages=python&packages=ipykernel, no?

async def get_conda_store_ui(
request: Request,
path: str,
templates=Depends(dependencies.get_templates),
url_prefix=Depends(dependencies.get_url_prefix),
):
context = {
"request": request,
"url_prefix": url_prefix,
}
return templates.TemplateResponse("conda-store-ui.html", context)
response = templates.TemplateResponse("conda-store-ui.html", context)
if path.endswith("not-found"):
response.status_code = 404
gabalafou marked this conversation as resolved.
Show resolved Hide resolved
return response
105 changes: 105 additions & 0 deletions conda-store-server/tests/_internal/server/views/test_conda_store_ui.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
# Copyright (c) conda-store development team. All rights reserved.
# Use of this source code is governed by a BSD-style
# license that can be found in the LICENSE file.

import pytest
from fastapi.testclient import TestClient


def assert_client_app(response):
"""A few checks that should all pass if the response is ok and contains
the client app
"""
assert response.is_success
assert "text/html" in response.headers["content-type"]
assert "condaStoreConfig" in response.text


class TestUIRoutes:
url_prefix = "/"

def full_route(self, route):
if self.url_prefix == "/":
return route
else:
return self.url_prefix + route

@pytest.fixture
def testclient(self, conda_store_server):
conda_store_server.enable_ui = True
conda_store_server.url_prefix = self.url_prefix
return TestClient(conda_store_server.init_fastapi_app())

def test_base_route(self, testclient):
"""The server should return the client app"""
response = testclient.get(self.url_prefix)
assert_client_app(response)

def test_unknown_routes(self, testclient):
"""Rather than return a 404, the server should return the client app
and let it handle unknown routes
"""
response = testclient.get(self.full_route("/foo"))
assert_client_app(response)

response = testclient.get(self.full_route("/foo/bar"))
assert_client_app(response)

def test_not_found_route(self, testclient):
"""The /not-found route should also return the client app
but with a 404 status code
"""
response = testclient.get(self.full_route("/not-found"))
assert response.status_code == 404
assert "text/html" in response.headers["content-type"]
assert "condaStoreConfig" in response.text

def test_route_outside_ui_app(self, testclient):
"""The server should not return the client app for a server-side
route
"""
# Trailing slash needed on next line. Is this a problem?
response = testclient.get(self.full_route("/admin/"))
assert response.is_success
assert "condaStoreConfig" not in response.text

response = testclient.get(self.full_route("/favicon.ico"))
assert response.is_success
assert response.headers["content-type"].startswith("image/")


def assert_not_found_not_client_app(response):
assert response.status_code == 404
assert "condaStoreConfig" not in response.text


class TestUIRoutesCustomPrefix(TestUIRoutes):
url_prefix = "/conda-store"

def test_unknown_route_outside_prefix(self, testclient):
"""The server should return a 404 for an unknown route outside
the url prefix and should not return the client app
"""
response = testclient.get("/foo/bar")
assert_not_found_not_client_app(response)


def test_ui_disabled(conda_store_server):
"""When the UI is disabled, the server should return 404s for
all UI routes and should not return the client app
"""
conda_store_server.enable_ui = False
conda_store_server.url_prefix = "/"
testclient = TestClient(conda_store_server.init_fastapi_app())

response = testclient.get("/")
assert_not_found_not_client_app(response)

response = testclient.get("/not-found")
assert_not_found_not_client_app(response)

response = testclient.get("/admin/")
assert_not_found_not_client_app(response)

response = testclient.get("/favicon.ico")
assert_not_found_not_client_app(response)
Loading