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

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

Open
viktorvsk opened this issue Apr 24, 2024 · 2 comments
Open

Comments

@viktorvsk
Copy link

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:

  • Python version: 3.12
  • Django version: 5.0.2
  • Django-Ninja version: 1.1.0
  • Pydantic version: 2.6.4
@vitalik
Copy link
Owner

vitalik commented Apr 24, 2024

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
Copy link
Author

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

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

No branches or pull requests

2 participants