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: query parameters can also be picked up from constructors of injected classes #2470

Open
1 of 4 tasks
filipvh opened this issue Oct 18, 2023 · 4 comments
Open
1 of 4 tasks
Labels
Dependency Injection This is related to our DI feature Question This is a question and further information is requested

Comments

@filipvh
Copy link

filipvh commented Oct 18, 2023

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

  • Linux
  • Mac
  • Windows
  • Other (Please specify in the description above)

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
@filipvh filipvh added Bug 🐛 This is something that is not working as expected Triage Required 🏥 This requires triage labels Oct 18, 2023
@provinzkraut
Copy link
Member

provinzkraut commented Oct 18, 2023

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.

@provinzkraut provinzkraut added Question This is a question and further information is requested and removed Bug 🐛 This is something that is not working as expected Triage Required 🏥 This requires triage labels Oct 18, 2023
@filipvh
Copy link
Author

filipvh commented Oct 18, 2023

😱 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
Copy link
Member

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

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).

@JacobCoffee JacobCoffee added the Dependency Injection This is related to our DI feature label Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dependency Injection This is related to our DI feature Question This is a question and further information is requested
Projects
Status: Backlog
Development

No branches or pull requests

4 participants