litestar: Mark that a dependency should never be a query parameter.

This is a side-effect of the change in 1.3.9 and the way that I’ve structured the consumption of dependencies in the example app.

The __init__() method of the repository looks like this:

    def __init__(
        self,
        limit_offset: LimitOffset | None = None,
        updated_filter: BeforeAfter | None = None,
    ) -> None:
        ...
        if limit_offset is not None:
            self.apply_limit_offset_pagination(limit_offset)
        if updated_filter is not None:
            self.filter_on_datetime_field(updated_filter)

If the repository is included as a dependency and if the the limit_offset and updated_filter dependencies are available those dependencies get consumed by the repository, and their dependencies end up documented as query params. However, for routes where the limit_offset and updated_filter dependencies aren’t available to the route, they themselves are shown as query params:

The reason I use this pattern is that without it, every list route, e.g., /users or /users/{user_id}/items has to do something like this:

    @get(
        dependencies={
            "limit_offset": Provide(limit_offset_pagination),
            "updated_filter": Provide(filter_for_updated),
        }
    )
    async def get(
        self,
        repository: UserRepository,
        is_active: bool = Parameter(query="is-active", default=True),
        limit_offset: LimitOffset,
        updated_filter: BeforeAfter,
    ) -> list[UserModel]:
        repository.apply_limit_offset_pagination(limit_offset)
        repository.filter_on_datetime_field(updated_filter)
        return await repository.get_many(is_active=is_active)

which reduces down to this if the repository can be left to optionally consume the dependency if it is available:

    @get(
        dependencies={
            "limit_offset": Provide(limit_offset_pagination),
            "updated_filter": Provide(filter_for_updated),
        }
    )
    async def get(
        self,
        repository: UserRepository,
        is_active: bool = Parameter(query="is-active", default=True),
    ) -> list[UserModel]:
        return await repository.get_many(is_active=is_active)

About this issue

Original URL
State: closed
Created 2 years ago
Comments: 22 (22 by maintainers)

Most upvoted comments

That does make sense to me, since the argument is effectively exactly the same as for the Parameter stuff that already exists in Starlite, so it’s probably the most sensible way forward since it’s the most similar approach we can likely come up with.

timwedde on Jun 7, 2022

I am 💯 onboard

Goldziher on Jun 7, 2022

OK I’m pretty sure I know where I sit on this now.

TL;DR

I propose adding a Dependency function that sits alongside Parameter and Body. I think it fits, and that Dependency deserves to be a front-line name in the starlite api, not buried inside the kwargs module with a narrow, mostly internal scope of usage.

My reasoning follows.

Thinking about the app at runtime from an information flow point of view. Any handler can use information provided by the client or developer.

Client info comes from parameters (wherever they originate) and payload/body/form data, and developer information can quite literally come from anywhere else, but we have a specific system for developers to provide information to handlers, which is dependency injection. Dependencies themselves can either source their information from the client, or from the developer, and so on recursively forever.

Default behavior is that we infer any function parameter in the tree of handler function parameters down through recursive dependency function parameters to be a developer provided dependency if a dependency is “provided” to the handler with the same key or name as the function parameter. Anything else, we assume comes from the client as a query parameter, unless we are told different via reserved kwargs, or the Parameter function.

Any function parameter in the dependency graph, be it a dependency or parameter can have a default value, and so not be required to be provided if sensible defaults can be deduced by the developer. That works for both dependencies and parameters with no change to plain typed python function declaration syntax, but breaks down for doc generation where dependencies have default values that do not need to be sourced from the parameter.

In the minimal case, we can do a very good job of inferring the source of function parameters, and generating minimal docs from that information, which allows keeping parameter bloat to a minimum. But at some point, we need more information from the developer about each of the information types to be able to offer a richer experience/documentation.

For client provided information, we have the Parameter and Body functions through which the developer can give more information to Starlite about how they should be documented and validated, but there is no analogy to that for the developer provided information.

This is where I think a Dependency function truly fits in. And, I actually think it should be called Dependency, not OptionalDependency like we’ve discussed earlier. I think the current kwargs.Dependency name, is too far away from the front-line user interface, and it’s usage too internal for us to worry about it crowding the developer interaction API. Whether that means we do a breaking change and change the name of kwargs.Dependency to something else, or let them coexist (perhaps through a deprecation period) is a decision we have to make. Having the name Dependency alongside Parameter and Body just seems “right”.

It would also allow us to make it an error condition where a function parameter is explicitly declared a developer provided value, has no default, and isn’t provided to the handler, and I think that should arise out of handlers.base like other ImproperlyConfiguredException cases.

If interested in how I see the implementation looking (it really is no different to Parameter and Body) see https://github.com/starlite-api/starlite/compare/main...peterschutt:exclude-by-value.

I’ll leave this for a day or two before I get started in case there is anything that needs to be looked at that I haven’t considered, or if anyone would really like to push back on my reasoning.

Cheers!

peterschutt on Jun 7, 2022

So respective of what you guys wrote above about options 1,2,3.

Option 3 is possible, it’s basically equivalent to the pydantic UnDefined type. Mind you, it’s going to be annoying to type and from my PoV it’s not actually more ergonomic - I dislike parameter bloat. Still, if you guys feel strongly about it, it’s fine by me - I don’t think there is right or wrong here.

Option 1 is simplest and to me easiest to grasp, and I in general follow the notion of “simple is better than complex”. You could have this parameter declared on any level, and the add logic to resolve it. But to me a degree of repetition is not so bad- DRY is not a religion 😜.

So in summary, you guys go ahead and decide how to proceed here.

Please make sure to update the documentation as well 😁

Goldziher on Jun 6, 2022