Create catch-all/fallback route for UI app

[gabalafou]

Fixes #899.

Pull request checklist

• Did you test this change locally?
• n/a Did you update the documentation (if required)?
• Did you add/update relevant tests for this change (if required)?

How to test

Before starting, create an environment named "test" under the "default" namespace.

Set url_prefix to "/":

• localhost:8080/ -> 200, should load app
• localhost:8080/foo -> 200, should see 404 page (but HTTP status code should be 200)
• localhost:8080/foo/bar -> 200, UI loads with sidebar and blank main panel (this needs to be handled better by the client, the client thinks this is a route for namespace/environment, tries to request namespace=foo, environment=bar, but currently doesn't handle the error case, so it's basically a silent error)
• localhost:8080/default/test -> 200, should load the app page for the default/test environment
• localhost:8080/not-found -> 404 with page generated from React app, not the server. How can you tell? The server returns JSON.

Set url_prefix to "/conda-store", go to the following URLs, behavior should be exactly the same as above:

• localhost:8080/conda-store
• localhost:8080/conda-store/foo
• localhost:8080/conda-store/foo/bar
• localhost:8080/conda-store/default/test
• localhost:8080/conda-store/not-found

[netlify]

✅ Deploy Preview for conda-store canceled.

Name	Link
🔨 Latest commit	3745e2c
🔍 Latest deploy log	https://app.netlify.com/sites/conda-store/deploys/6734c4a5354ea5000824fea3

[gabalafou]

self review

[gabalafou]

Adding url_for('get_conda_store_ui') was the only way I could figure out how to put the url_prefix here.

Why is this change needed? Previously, the server would only serve the UI app at url_prefix (e.g., either at "/" or "/conda-store", depending on configuration).

However, this PR allows the UI app to be loaded at /conda-store/default/test, for example. At that URL, this page would try to fetch the JavaScript and CSS at:

• /conda-store/default/static/conda-store-ui/main.js
• /conda-store/default/static/main.css

These routes end up matching the catch-all, which means the server serves this template page for those requests instead of the desired JavaScript and CSS.

[gabalafou]

It would be great to turn the manual test plan I put in the PR description into a set of automated tests

[soapy1]

It would be great to turn the manual test plan I put in the PR description into a set of automated tests

+100 that would be awesome! I would make a new test file in https://github.com/conda-incubator/conda-store/tree/main/conda-store-server/tests/_internal/server/views called test_conda_store_ui.py. There are a bunch of fixutres defined in conftest that you can use to setup tests.

[peytondmurray]

Tested and the routes/html codes are as expected. Thanks!

[peytondmurray]

Hmm, looks like something here is making the integration tests fail.

[gabalafou]

@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?

[gabalafou]

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

[gabalafou]

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(

[peytondmurray]

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?

[marcelovilla]

@gabalafou @peytondmurray is there any chance this PR will make the next conda-store release?

We're trying to upgrade conda-store in Nebari and would like to include this fix if possible.

[peytondmurray]

@marcelovilla We're in the midst of exploring options here - yesterday I proposed to @gabalafou the idea of simply overriding FastAPI's default HTTPException handler with something like

from fastapi import FastAPI, HTTPException
from fastapi.responses import RedirectResponse

app = FastAPI()

@app.exception_handler(HTTPException):
async def handle_bad_route(request, exc):
    return RedirectResponse("/")

Will get back to you on whether this can get in the next release!

[marcelovilla]

@peytondmurray good to know, thanks for the prompt reply!

[gabalafou]

I also considered that idea, but I wasn't sure it would work because some of the API endpoints raise a 404 if they cannot find the resource in the database. For example, the /api/v1/namespace/{namespace} endpoint raises 404 if it cannot find the namespace in the database:

conda-store/conda-store-server/conda_store_server/_internal/server/views/api.py

Lines 282 to 299 in feac99d

	@router_api.get(
	"/namespace/{namespace}/",
	response_model=schema.APIGetNamespace,
	)
	async def api_get_namespace(
	namespace: str,
	request: Request,
	auth=Depends(dependencies.get_auth),
	conda_store=Depends(dependencies.get_conda_store),
	):
	with conda_store.get_db() as db:
	auth.authorize_request(
	request, namespace, {Permissions.NAMESPACE_READ}, require=True
	)
	namespace = api.get_namespace(db, namespace, show_soft_deleted=False)
	if namespace is None:
	raise HTTPException(status_code=404, detail="namespace does not exist")

My suspicion was that if we add a custom exception handler for 404s to serve the React app, then if someone makes a request to /api/v1/namespace/nonexistent_namespace_example/, they would get a 200 with an HTML page for the React app, but they should get a 404 with a JSON error payload instead.

I was imagining something like this:

@app.exception_handler(404):
async def get_conda_store_ui(
    request: Request,
    templates=Depends(dependencies.get_templates),
    url_prefix=Depends(dependencies.get_url_prefix),
):
    context = {
        "request": request,
        "url_prefix": url_prefix,
        "ui_prefix": router_conda_store_ui.prefix,
    }
   return templates.TemplateResponse("conda-store-ui.html", context)

I created a minimal FastAPI app to double check my suspicion:

# main.py

from fastapi import FastAPI, APIRouter, HTTPException
from fastapi.responses import HTMLResponse, RedirectResponse


app = FastAPI()

router_api = APIRouter(
    tags=["api"],
    prefix="/api/v1",
)


@router_api.get("/namespace/{namespace}")
async def get_namespace(namespace):
    if (namespace == "foobar"):
        raise HTTPException(status_code=404, detail="namespace does not exist")
    else:
        return {"message": f"namespace {namespace} found"}


@app.exception_handler(404)
async def handle_bad_route(request, exc):
    html_content = """
    <html>
        <head>
            <title>Conda Store</title>
        </head>
        <body>
            <h1>Pretend I'm a React app</h1>
        </body>
    </html>
    """
    return HTMLResponse(content=html_content, status_code=200)


app.include_router(router_api)

Result:

• 🟢 localhost:8000 => 200 HTML "Pretend I'm a React app"
• 🟢 localhost:8000/some/other/random/route => 200 HTML "Pretend I'm a React app"
• ❌ localhost:8000/api/v1/namespace/raise404 => 200 HTML "Pretend I'm a React app". (This should be a 404 w/JSON.)
• 🟢 localhost:8000/api/v1/namespace/foobar => 200 JSON {"message":"namespace foobar found"}

The only way I see around this is by subclassing or somehow annotating exceptions raised in our code so that we can re-raise them in the exception handler.

---

Out of curiosity, do you have any concerns about the approach I've taken in my most recent commits, which is to put the app at /ui?

[peytondmurray]

No, I don't have a problem with setting the default to /ui/. I tested locally

URL Entered	Result
/	Redirected to /conda-store/ui/
/conda-store/ui	(no redirect)
/conda-store/ui/	(no redirect)
/conda-store/ui/username/foo (existing environment)	(no redirect)
/conda-store/ui/username/foo2 (nonexistent)	(no redirect, but the /ui/ interface appeared)

Should we update window.location to be /conda-store/ui/ for nonexistent routes as well on the client side?

[gabalafou]

Great, thanks @peytondmurray!

1. This is good, should redirect
2. Should show the UI in the "no environment selected" state
3. Ditto
4. Should show the UI in the "view environment" state (for the "foo" environment)
5. I'm not exactly sure what the UI should show given the current state of the codebase; however, we'll have to add some client-side code that shows the user a 404 page when they try to view an environment that doesn't exist or that they don't have access to

[gabalafou]

Cross-reference:

• update test_conda_store_server_url_prefix #993