vitalik / django-ninja

💨 Fast, Async-ready, Openapi, type hints based framework for building APIs
https://django-ninja.dev
MIT License
6.98k stars 420 forks source link

[BUG] Swagger uses wrong endpoints for dynamically generated routers #1140

Open viktorvsk opened 4 months ago

viktorvsk commented 4 months ago

Describe the bug

In case you want to create routers dynamically like the following (when you have a lot of similar resources for example):

api = NinjaAPI()
api.add_router("/users/", RouterFactory("User").get_router())
api.add_router("/posts/", RouterFactory("Post").get_router())

Where your implementation may look like the following:

from ninja import Router, Query

class Factory:
    def __init__(self, class_name: str):
        module = import_module(f"core.models.records.{class_name}")
        self.record = getattr(module, class_name)
        self.schema_out = getattr(module, f"{class_name}SchemaOut")
        self.list_filter_schema = getattr(module, f"{class_name}ListFilterSchema")

    def get_router(self):
        router = Router()
        @router.get("/", response=List[self.schema_out])
        def list_records(request, filters: self.list_filter_schema = Query(...)):
            return self.record.objects.all()

Currently everything will work correctly except for Swagger. Swagger will correctly display endpoints for users and posts with correct schemas but it will actually send requests only to /users/ (even when you click Try it Out on the posts section) in this case because in the example we've defined add_router("/users"/) first.

I assume its because somewhere under the hood Swagger uses function names to resolve endpoints. Next simple hack fixes this by changing the implementation to:

from ninja import Router, Query

class Factory:
    def __init__(self, class_name: str):
        module = import_module(f"core.models.records.{class_name}")
        self.record = getattr(module, class_name)
        self.schema_out = getattr(module, f"{class_name}SchemaOut")
        self.list_filter_schema = getattr(module, f"{class_name}ListFilterSchema")
        self.class_name = class_name

    def get_router(self):
        router = Router()
        @router.get("/", response=List[self.schema_out])
        def list_records(request, filters: self.list_filter_schema = Query(...)):
            return self.record.objects.all()
        list_records.__name__ = f"list_{self.class_name}_records"

Notice this line list_records.__name__ = f"list_{self.class_name}_records"

It seems like a dirty hack (it is). And it seems like its not a popular way in python to handle this. I wasn't able to google anything similar. But just in case someone has the same problem, maybe it would be helpful to keep it here.

P.S. If I'm doing something wrong with this approach entirely would be great to get feedback because I'm not yet much experienced in python/django so appreciate any advice :)

Versions:

vitalik commented 4 months ago

Hi @viktorvsk

interesting trick..

in general django-ninja generates operation id as string like "<module-name>_<function_name>"- and because you have all in one module that probably gives an issue

you can try to use operation_id parameter:

    def get_router(self):
        router = Router()
        @router.get("/", response=List[self.schema_out], operation_id=f"list_{self.class_name}_records")
        def list_records(request, filters: self.list_filter_schema = Query(...)):
            return self.record.objects.all()

OR overwrite get_openapi_operation_id globally - see docs and example here https://django-ninja.dev/reference/operations-parameters/#operation_id

viktorvsk commented 4 months ago

Hey @viktorvsk thanks, thats definitely a much cleaner way to handle this. And given its already documented probably this issue could be closed