litestar-org / litestar

Production-ready, Light, Flexible and Extensible ASGI API framework | Effortlessly Build Performant APIs
https://litestar.dev/
MIT License
5.49k stars 374 forks source link

Bug: query parameters can also be picked up from constructors of injected classes #2470

Open filipvh opened 1 year ago

filipvh commented 1 year ago

Description

class HealthController(Controller):
    path = "health"

    @get(path="", cache=False, tags=["health"])
    async def get_health_status(self, health_service: HealthService) -> Response[HealthStatusDto]:
        result = await health_service.get_health_status()
        return Response(result, status_code=HTTP_200_OK if result.all_ok else HTTP_400_BAD_REQUEST)
class HealthService:
    def __init__(self,
                 ldap_connection_pool: LdapConnectionPool
                 ):
        self._ldap_connection_pool: LdapConnectionPool = ldap_connection_pool

       # rest of class
class LdapConnectionPool:
    def __init__(self, config_service: ConfigService):
        self._config_service = config_service

       # rest of class
class ConfigService:

    def __init__(self, config: Config | None = None):
        if config:
            self._internal_config = config
        else:
            self._internal_config = None
            self.read_config()

       # rest of class
class Config(BaseModel):
    # Allow none for testing
    a_config: AConfig | None
    b_config: BConfig | None

after this our swagger showed:

image

I would assume, qParams (and such) would only be picked u in the signature of the method in the controller?

URL to code causing the issue

No response

MCVE

No response

Steps to reproduce

No response

Screenshots

No response

Logs

No response

Litestar Version

2.2.1

Platform


[!NOTE]
While we are open for sponsoring on GitHub Sponsors and OpenCollective, we also utilize Polar.sh to engage in pledge-based sponsorship.

Check out all issues funded or available for funding on our Polar.sh Litestar dashboard

  • If you would like to see an issue prioritized, make a pledge towards it!
  • We receive the pledge once the issue is completed & verified
  • This, along with engagement in the community, helps us know which features are a priority to our users.

Fund with Polar

provinzkraut commented 1 year ago

This is intended behaviour and generally how the dependency injection works, it's not unique to classes. Any callable can request parameters.

If this were not the case, you couldn't really use any parameters in dependencies, as you'd have to explicitly request them in the handler and pass them to a dependency callable, greatly reducing their usefulness.

I'd suggest that you keep your dependency chain / other utility functions separated to avoid cases like these.

filipvh commented 1 year ago

:scream: so you can have qParams, hidden away in dependencies? This seems like a dangerous thing. I would expect parameters, qParams and expected bodies to be only definable on the controller. This way, you have a clean picture of what is exposed.

provinzkraut commented 1 year ago

I think it actually makes sense if you think of query parameters as implicitly defined dependencies. You can request dependencies within dependencies without explicitly declaring them, so query parameters shouldn't be an exception to that.

That being said, we have previously talked about getting rid of the implicit requests for anything other than dependencies in favour of a more explicit approach. This might look something like this then:


def some_dependency(another_query_param: Query[str]) -> str:
  return another_query_param

@get("/{path_param:str}", dependencies={"some": some_dependency})
async def handler(
    path_param: str, 
    some: str,
    data: Body[dict[str, str]], 
    query_param: Query[int], 
    header_param: Header[str],
) -> str:
   ...

where everything that's not directly injected into the handler has to be explicitly declared.

Maybe it's worth bringing this up again @litestar-org/maintainers? If we were to introduce this, we could also make it optional / hide it in the experimental features for now and maybe then make it the default for 3.0.

peterschutt commented 1 year ago

I like the idea, and it would allow us to get rid of reserved kwargs, e.g., users could name "data" whatever they want (recent relevant discord discussion: https://discord.com/channels/919193495116337154/1164442955357110292).