[ENH] HTTP API routes overhaul

[drewkim]

Improvements & Bug fixes

• Overhauls the HTTP API routes to follow a more RESTful approach. Now routes follow the patten of /tenants/{tenant}/databases/{database}/collections/{collection_id}.
• Configures a "handshake" where the client calls the server auth implementation to possible resolve the tenant/database based on the provided API key.
• Updates clients to work with new route definitions.

Test plan

How are these changes tested?

• Tests pass locally with pytest for python, yarn test for js, cargo test for rust

Documentation Changes

Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs repository?

[drewkim]

• [ENH] HTTP API routes overhaul #2954 : 3 dependent PRs (#2960 , #2967 , #2968 ) 👈
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @drewkim and the rest of your teammates on Graphite

[github-actions]

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

• Can you think of any use case in which the code does not behave as intended? Have they been tested?
• Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
• If appropriate, are there adequate property based tests?
• If appropriate, are there adequate unit tests?
• Should any logging, debugging, tracing information be added or removed?
• Are error messages user-friendly?
• Have all documentation changes needed been made?
• Have all non-obvious changes been commented?

System Compatibility

• Are there any potential impacts on other parts of the system or backward compatibility?
• Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

• Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

[drewkim]

This is not optimal, as it causes us to make a call to the API every time. However, we can't call async code from the constructor so the alternative would be causing a breaking change for anyone using our JS client by forcing them to call a follow-on method upon initialization that sets the tenant/database. I propose we go about this way for now and implement the more efficient method when we apply more sweeping breaking client changes.

[HammadB]

Nice fix

[HammadB]

Code cleanliness nit - can we somehow bifurcate the v1 code into a sep file for easier readying?

[drewkim]

It's not the most straightforward to do so and includes module-ing FastAPI. I think it should be somewhat more readable now with the split into functions. If it's still tough to read, lmk and I'll go down that route but felt inefficient given that we're immediately deleting the v1 routes.

[HammadB]

Fair but, Isn't it just a matter of creating a wrapper class that extends this and moving the functions there?

[drewkim]

I don't believe it's that straightforward—wouldn't that create two running instances of the server?

[HammadB]

No, you only instantiate one.

Server() -> Say this is current class
ServerV2(Server) -> Adds v2 routes

or alternatively
Server -> Current class with v2 routes only
ServerV1(Server) -> Adds v1 routes for bw compat.

This is fine as is, but I think it should be simple

[drewkim]

Ah I see what you mean. Updated to wrap with class that has v1 routes

[drewkim]

Decided to revert this and keep all in one file because it will break hosted Chroma otherwise. This is how I implemented your suggestion:

class FastAPI(Server):
# Setup v2 routes

class FastAPIWithV1(FastAPI):
# Setup v1 routes
# Post route setup
self._app.include_router(self.router)
use_route_names_as_operation_ids(self._app)
... 

# In chromadb/app.py:
from chromadb.server.fastapi.v1 import FastAPIWithV1
server = FastAPIWithV1(settings)

There is some configuration that necessarily has to happen post adding routes, so we can only fire it in the wrapper class. We thus also have to update the app to use the wrapper class. This makes it such that hosted Chroma will fail once we deploy changes, since it initializes the server in python/chroma_hosted/chroma_hosted/app.py. It won't have access to the wrapper class, which has the necessary post route configuration.

There is a way around this, by doing something like:

class FastAPI(Server):
...
class FastAPI(FastAPIWithV1):
...

However, in this case we still have to split things up in an awkward way, like moving router initialization and it doesn't make for a clean delete of the old routes, especially if there's some time between and context is lost. Overall, I think it's cleaner to keep things all in one file.

[HammadB]

What happens if the user supplied tenant or database doesn't match?

[drewkim]

They are only overridden if they are (not provided or the default values) and we can definitely determine what they should be based on the API key (i.e. a specific tenant and only one specific database exists). Thus, if they don't match but the API key is correct, then the request will fail authorization since it's a mismatch of resources.

[HammadB]

I don't see where that would happen?

User does: chromadb.Client(tenant=FOO, database=BLAH)
Code does: self.get_user_identity() -> Returns UserIdentity(tenant=NOT_FOO, database=SOMETHING)
Code does: maybe_set_tenant_and_database will just set tenant and database to NOT_FOO, SOMETHING

I'm probably missing something.

[drewkim]

In maybe_set_tenant_and_database, the logic is:

if (not tenant or tenant == DEFAULT_TENANT) and new_tenant:
    tenant = new_tenant
if (not database or database == DEFAULT_DATABASE) and new_database:
    database = new_database

So the tenant/database only get set to the values pulled from auth if the provided ones don't exist or are the default valeus

[HammadB]

Right and I am saying should we make the client init fail, under a case like this, rather than deferring the failure to the first request the user will make?

[drewkim]

Ah, got it. Updated to throw an exception in this error case and moved resolution to the auth library under a new utils section (I thought in your other comments were suggesting to throw it into the server auth provider).

[HammadB]

Can we make this error more descriptive to users, they might not know what "resolved" means.

Also - does this expose us to enumeration attacks?

[drewkim]

I don't think it does any more than already—an attacker could do the same thing with collections currently, just receiving a 401 if their API doesn't match the resource. I can change this to be a more opaque 401 to match.

[HammadB]

I'd like to see:

• unit tests of the auth overriding logic
• v1 client bw compat tests with a v2 server
• @codetheweb or @jeffchuber to TAL at the JS changes.
• just a quick thought about enumeration attacks

Otherwise looks great - thank you!

[drewkim]

Reviewed with @codetheweb offline

[codetheweb]

nit: might be slightly cleaner to combine the two checks into the single this._initPromise
also should probably mark init() as private, not sure why it wasn't already

[drewkim]

I believe there're some tests relying on it being public