yuval9313 / FastApi-RESTful

Quicker way to develop FastApi
MIT License
180 stars 25 forks source link

[BUG] Tags are duplicated when using cbv #192

Open jgentil opened 1 year ago

jgentil commented 1 year ago

Describe the bug When using a cbv with an APIRouter that defines tags, those tags are duplicated.

To Reproduce

from fastapi import APIRouter
from fastapi_restful.cbv import cbv

testrouter = APIRouter(prefix='/testing', tags=['test'])

@cbv(testrouter)
class TestService:
    @testrouter.get("/test")
    def test_stuff(self) -> list[dict]:
        return [{}]

Expected behavior Each route to have a single 'test' tag. This fixes a bug where ReDoc-style documentation lists two entries per endpoint when using cbv. See: https://github.com/tiangolo/fastapi/discussions/6165

Additional context The problem seems to be that, during the processing in the _register_endpoints function in https://github.com/yuval9313/FastApi-RESTful/blob/1201235c3c27d5bf1c712b7bc76b316b6c4c12b3/fastapi_restful/cbv.py#L112-L117 Here the route is removed from the router that is passed in and then added to a new router. When a route is added to a router, the router's tags are assigned to the route's tags attribute. So when the route is removed from the previous router and added to cbv_router, and then cbv_router is included back into the original router, include_router will go through and add router's tags to each route, but that was already done once, so it ends up with two identical tags.

I don't actually understand this code very well. Why can't the routes be modified inline? I feel like this was maybe more necessary when using an InferringRouter? I tested it out just preliminarily and it seems fine, but the test suite does not pass so clearly there's more to it than this.

For now I'll submit a PR that simply removes the tags from the route that are added by that router. It works well enough and passes tests. :)

jgentil commented 1 year ago

Really the entire _register_endpoints is a bit of a mess. router.routes is iterated no less than three times, and these lines seem really contradictory:

https://github.com/yuval9313/FastApi-RESTful/blob/1201235c3c27d5bf1c712b7bc76b316b6c4c12b3/fastapi_restful/cbv.py#L95 https://github.com/yuval9313/FastApi-RESTful/blob/1201235c3c27d5bf1c712b7bc76b316b6c4c12b3/fastapi_restful/cbv.py#L109

Should they be Route or APIRoute? Why is WebsocketRoute valid in the second check, but APIWebsocketRoute isn't in the first? I only ask because mypy is mad since WebsocketRoute has no tags attribute, and I wasn't sure if a WebsocketRoute would be valid here since the first check ensures that it cannot be.