Mark that a dependency should never be a query parameter. about litestar HOT 22 CLOSED

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 😁

from litestar.

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 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!

from litestar.

I am 💯 onboard

from litestar.

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.

from litestar.

What about a user-land configuration that is similar to the RESERVED_KWARGS constant, and if a field name appears in there, it is ignored for doc-generation purposes?

def create_parameters(...) -> List[Parameter]:
    ...
    for f_name, field in handler_fields.items():
        ...
        if f_name not in RESERVED_KWARGS and f_name not in <collection of names declared by user to be excluded from docs>:
            ...

The collection of names could be configurable at each level of the app hierarchy and resolved for the handler similar to dependencies.

I'm not sure what such a configuration should be named, which is why I've used that awkward placeholder in the example above.

from litestar.

Hm this indeed seems like a bit of a tougher issue to resolve. It effectively comes down to telling apart an unsatisfied dependency from an optional query/header/cookie parameter.
As it is I don't think we can, an optional type with a None value is the same between those two cases, so maybe an approach here would be to explicitly mark dependencies that can be unsatisfied/optional, like you suggest above.

Imho keeping track of dependencies in an extra dict is a bit cumbersome and not that intuitive, so I was kind of thinking about marking these occurences in some other way such that most of the overhead could be transferred away from the developer (who has to manually maintain these dicts) and to Starlite, which can then figure things out automatically.

To this end I'd propose a solution based on type hints, in which we introduce a dummy-type Dependency (name TBD) with which we mark dependencies where needed. This type information can then be used to make decisions on when to include/exclude parameters in specific circumstances.

Example app:

from starlite import Starlite, get, Provide, Dependency
from starlite.app import DEFAULT_OPENAPI_CONFIG


class LimitOffset:
    def __init__(
        self,
        limit: str,
        offset: str,
    ):
        ...


class Repo:
    def __init__(
        self,
        limit_offset: Dependency[LimitOffset] | None,
    ) -> None:
        ...


# Repo has unsatisfied dependencies: Transitive dependency *should not* show up in OpenAPI
@get("/test1", dependencies={"a": Provide(Repo)})
def handler1(a: int) -> str:
    return "OK"


# Repo has satisfied dependencies: Transitive dependency *should* show up in OpenAPI
@get(
    "/test2",
    dependencies={
        "a": Provide(Repo),
        "limit_offset": Provide(LimitOffset),
    },
)
def handler2(a: int) -> str:
    return "OK"


app = Starlite(
    route_handlers=[
        handler1,
        handler2,
    ],
    openapi_config=DEFAULT_OPENAPI_CONFIG,
)

This app has two handlers which exhibit the problem you laid out above, as far as I understood. test1 has the issue, test2 is a "valid" configuration.

By marking limit_offset: Dependency[LimitOffset] | None, in the Repo dependency we are then able to tweak the create_parameters method by adding a check for this at the start of the loop over the handler fields:

for f_name, field in handler_fields.items():
    if field.type_ == Dependency and f_name not in dependencies:
        if field.allow_none:
            continue
        else:
            raise Exception(
                f"{f_name} dependency not satisfied and not optional in handlers {', '.join(route_handler.paths)}"
            )
    # rest of the loop as before

The Dependency type is incredibly simple:

class Dependency(Generic[T]):
    def __class_getitem__(cls, item):
        return cls

This will accomplish two things at once where, if a parameter representing a dependency anywhere in a dependency chain is...

1. optional and the dependency is not available, it is skipped and will not appear in the schema.
2. not optional and the dependency is not available, an exception is raised because the dependency can not be satisfied, which is probably an error.

The second part can of course be omitted if it's too strict. I'm not 100% certain how Starlite currently handles unsatisfied dependencies in other contexts, so it'd probably be best to emulate whatever it does there.

Let me know your thoughts on this. I'm relatively new to this so I'm bound to simply not know about a couple edge cases, I'm sure, but to me this seems like a somewhat workable approach at first glance.

from litestar.

That's a good clarification of the issue, thanks.

LMK if I'm off the mark anywhere.

Dependencies have a few properties that are interesting to the problem:

• dependencies are either allowed to be parameters or not (will be marked at injection point with type annotation)
• dependencies are either required to be provided via the injection mechanism, or not (marked at injection point with a default value or Optional type, or both)
• dependencies are either provided somewhere in the handler's resolvable path, or not.

We will use the Dependency type annotation via model field in create_parameters() as a marker to differentiate between the "allowed to be parameter or not" property of the dependency when generating docs. This lets us bail early if the dependency has a default provided, and should not be a documented parameter.

Then also that there is an error state that occurs when:

1. the dependency is not allowed to be a parameter (marked by using Dependency[T] at injection point).
2. the dependency is not optional (again marked at injection point)
3. dependency is not declared somewhere in the handler's resolvable path.

Have I got that right? I really like where you are going and definitely better to not force the developer to book-keep the solution themselves.

I have some points/clarifications:

• Does the way you use __class_getitem__(cls...): return cls mean that type checkers will assume the value of that parameter to be of the same type as the type parameter to the generic class? Are there any edge-cases to this we'd need to be wary of?
• starlite.kwargs.Dependency already exists, would it make sense to make that generic and use it as the type? Otherwise I think we should use a different name.
• for the case where the dep is not allowed to be a parameter, and is not "provided" should we be raising an error from the doc generation layer? I agree, there should be an exception for the case somewhere. At the moment if an injected dependency isn't provided we return a 400, e.g.,: "Missing required parameter user_repository for url http://localhost:8000/v1/users/c11f5e35-99d0-409b-bddd-7c136b81d306/items". I think your solution here is fine given that it is not already an error from the application but it is a change in runtime behavior, so I wonder how @Goldziher feels about that WRT semantic releases. We do have the option of just ignoring the case for doc generation and looking at the runtime validation of the dependencies as a separate issue and scheduling it into a more major release (even if the exception still ends up right here where you have it).
• Should we look at field.default instead of, or in combination with field.allow_none in create_parameters()? If we choose to just ignore the error case for now then this isn't an immediate issue.

Docs and tests will be important for this, too.

I'd prefer to rebase #129 on this work so I'd be happy to chip in with working on any part of it.

So I suppose we need to get a nod from @Goldziher that he'd be happy with the api design, then we can press forward with impl, tests and docs, or reassess if need be.

from litestar.

Hi guys, great to see this kind of process here finally 😀.

I see no issue with going for a minor version (1.4.0) rather than a patch version (1.3.10). The change here is also not a backwards breaking change really, although it might cause some exceptions raised in update. It's just important to ensure that the exception communicates the solution clearly, but that's easy to do.

from litestar.

Does the way you use class_getitem(cls...): return cls mean that type checkers will assume the value of that parameter to be of the same type as the type parameter to the generic class? Are there any edge-cases to this we'd need to be wary of?

To be honest, my knowledge of advanced type hinting is still somewhat lacking, so I'm not 100% sure on the ramifications of this. I'm sure there's a cleaner way to implement a dummy-type of sorts like this, I just don't know of it yet. Suggestions on how to handle this better a definitely welcome. The only requirement for this annotation is that we'll be able to tell it apart from the class it wraps at analysis time.

Edit: I was toying around with making a distinct optional type or something of the sort but thanks to duck typing it always collapses to a regular Optional, which is not ideal. The only way around this I could find that works is making a distinct None type and producing a Union[Dependency, DistinctNone]. Probably a bit too hacky to use as a solution though.

starlite.kwargs.Dependency already exists, would it make sense to make that generic and use it as the type? Otherwise I think we should use a different name.

That could make sense, since internally it's already creating a graph of these objects anyway, so using it as a type annotation as well could work. It'd certainly reduce the need to create another type to worry about. My one question here would be whether this would be mixing concerns too much (depending on what this class already does), so that should probably be figured out.

for the case where the dep is not allowed to be a parameter, and is not "provided" should we be raising an error from the doc generation layer?

I suppose it depends on what kind of error this is. Can the application run properly with an unsatisfied dependency or will this lead to undefined/unexpected behavior? If it's the latter, imho it should be an actual exception at startup to prevent a situation where things look like they're working, but in some cases they actually don't. If it's fine to have unsatisfied dependencies then I agree it should not be a hard failure but rather a warning/error in the docs or at runtime.

Should we look at field.default instead of, or in combination with field.allow_none in create_parameters()? If we choose to just ignore the error case for now then this isn't an immediate issue.

We can rely on self.required is False and default_value is None as well since those two together form the value for allow_none. This largely came from me digging through the pydantic source code for a useable property for this example, so I'm not terribly attached to it in any case. Originally I figured this field would contain additional info about the type hinting here but that turned out to be false, so it doesn't add any more info than just using those two fields instead.

from litestar.

So I just ran this little test:

from typing import Any, Generic, TypeVar

T = TypeVar("T")


class Dependency(Generic[T]):
    def __class_getitem__(cls, item):
        return cls


def fn(param: Dependency[int]) -> None:
    param += 3

and mypy complains:

src/app/__init__.py:12: error: Unsupported operand types for + ("Dependency[int]" and "int")  [operator]
src/app/__init__.py:12: error: Incompatible types in assignment (expression has type "int", variable has type "Dependency[int]")  [assignment]
Found 2 errors in 1 file (checked 1 source file)
ERROR: 1

If we lose the actual type of the dependency where it is injected it is less than ideal.

I wonder if it could work if we follow the pattern that is already used by the Parameter function, e.g.:

class Repo:
    def __init__(
        self,
        limit_offset: LimitOffset | None = NonParameter(default=None),
    ) -> None:
        ...

(I'm not necessarily advocating for the name NonParameter but I think it needs to be different from Dependency to avoid the name collision with kwargs.Dependency.)

In Parameter, kwargs like header, parameter and cookie are written to FieldInfo.extra which is then inspected in the create_parameters() function to determine where the parameter should be documented. Maybe we could limit the kwargs that the NonParameter could accept to a subset that makes sense, have it create a FieldInfo object that represents the dependency and put a value like non_parameter=True into extra and then inspect for that in create_parameters().

It's late here now so I haven't tested that idea out at all. I'll try to put some time into it tomorrow morning.

from litestar.

Wouldn't it be simpler to add an exclude option to route handlers that allow excluding specific keys from the openapi docs?

From my PoV the fact that the Repo has these parameters declared and is in the dependency chain mean that the parsing is correct. We cannot know in advance where these will be required or not.

I would therefore propose something along these lines:

   @get(
        dependencies={
            "limit_offset": Provide(limit_offset_pagination),
            "updated_filter": Provide(filter_for_updated),
        },
        exclude_from_docs=["limit_offset", "updated_filter"]
    )
    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)

from litestar.

Would that work for the pattern where limit_offset may be defined at an arbitrary depth up the dependency tree above the handler method in your example? I.e. it might only be defined at the controller level. Should the exclude list affect things beyond its own scope? In such a case, the handler method would affect dependencies on the controller, at least for schema generation.
Imho the parsing is not quite working correctly when unsatisfied dependencies show up in the docs by default.

We cannot know in advance where these will be required or not.

To this point: With the stuff outlined above, we could, so it's definitely possible.

from litestar.

Would that work for the pattern where limit_offset may be defined at an arbitrary depth up the dependency tree above the handler method in your example? I.e. it might only be defined at the controller level. Should the exclude list affect things beyond its own scope? In such a case, the handler method would affect dependencies on the controller, at least for schema generation.
Imho the parsing is not quite working correctly when unsatisfied dependencies show up in the docs by default.

We cannot know in advance where these will be required or not.

To this point: With the stuff outlined above, we could, so it's definitely possible.

You'd use exclude only on route handlers, which are the "root" level as far as schema generation is concerned, so yes, I think it will resolve the issue. Or perhaps I don't understand it in full.

from litestar.

For this case:

   @get(
        dependencies={
            "limit_offset": Provide(limit_offset_pagination),
            "updated_filter": Provide(filter_for_updated),
        },
        exclude_from_docs=["limit_offset", "updated_filter"]
    )
    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)j

... it is redundant to exclude those dependencies at the handler level as they are provided for that route and so get resolved to their underlying parameters correctly. The issue is for routes where the dependencies are not provided. E.g.,

@post(exclude_from_docs=["limit_offset", "updated_filter"])
async def post(self, ...) -> UserModel:
    ...

Wouldn't it be simpler to add an exclude option to route handlers that allow excluding specific keys from the openapi docs?

That would have to be repeated for every other route that needs to exclude them.

However, My initial instinct was along these lines too - although I did agree with @timwedde that a solution that took the book-keeping away from the programmer would be nice, and that is what has sent us down this path of looking at alternatives.

@Goldziher Is there any reason this exclude_from_docs couldn't be declared at any level? E.g., in the case of limit_offset in the example app, I'd be tempted to define it at the application level so that it never shows in docs. We'd then resolve the exclude_from_docs for the handler at doc generation time pulling in exclusions from every node on the handlers path up to the app.

In summary, we have come up with 3 ways to go about this:

1. Exclude by name

@post(exclude_from_docs=["limit_offset", "updated_filter"])
async def post(self, ...) -> UserModel:
    ...

No doubt the simplest approach, and if can be done at any level of the app, would at least solve my use-case in example app.

2. Exclude by type

class Repo:
    def __init__(
        self,
        limit_offset: Dependency[LimitOffset] | None,
     ) -> None:
        ...

Has the benefit of being declared non-parameter at the point of injection (which is similar in utility to being able to define exclude_from_docs in higher up levels of the app structure - I.e., no repetition needed to always exclude). However (AFAICT), we lose the type of the underlying dependency in the function where it is injected.

3. Exclude by value

class Repo:
    def __init__(
        self,
        limit_offset: LimitOffset | None = NonParameter(default=None),
    ) -> None:
        ...

Same benefits as Exclude by Type but we retain the typing info. There's a precedent for the pattern with Parameter and this would also open up the use of default_factory for non-parameter dependency defaults. I'm not sold on NonParameter name though.

Personally, I give 1 vote to 1. for simplicity, and 1 vote to 3. for the ergonomics. Losing the typing info rules out 2. IMO.

Edit: wanted to phrase my thoughts better:)

from litestar.

Yeah, if there's no way to keep the type then 2 is indeed not really workable. I did toy around with distinct None type unions like I edited in my previous post but that's likely too hacky in any case.

The exclude by value (3) is getting somewhat close to FastAPI's way of resolving dependencies where you simply declare a Depends(method) every time you need to inject something as a dependency. This would resolve the problem in transitive dependencies and is similar to having to add a NonParameter(something), but the approach here would be simpler.
Personally I'd much prefer tackling the problem at the root by marking the optional dependency somehow versus patching the symptoms at the other end via an exclude dict, but I can see arguments for both.
In my case my vote would go towards 3 since I think it's a nicer pattern overall than 1 would be. As far as the name goes, it could maybe be something like OptionalDependency(something) instead of NonParameter since it's likely better to declare what something is versus what something is not.

In addition, forcing people to mark optional dependencies explicitly is probably better since it makes it very obvious that both states would need to be handled by the user's code.

from litestar.

Yeah, if there's no way to keep the type then 2 is indeed not really workable.

Just for interest's sake, I'd love to know if there is a solution for this type of thing, even if we don't use it. Perhaps it could be solved with a plugin to override the type within the scope of the function, but after reading the status update from SQLAlchemy on Mypy / Pep-484 Support for ORM Mappings, I'm not sure supporting a plugin is a can of worms I'm keen to open for myself. I think that part of the ecosystem needs time to develop - hopefully it gets to a point where there is a standard API for type checker plugins so that they worked for all the different type checkers, not just mypy.

I'd much prefer tackling the problem at the root by marking the optional dependency somehow versus patching the symptoms at the other end via an exclude dict

I'm coming down here too.

As far as the name goes, it could maybe be something like OptionalDependency(something)

Works for me. It is a dependency that doesn't need to be provided, so optional in that sense.

explicitly

Yes explicit declarative nature of 3 is a +.

I also like that we'd be making some of the functionality of Field available to these dependencies. E.g., runtime validation of the dependency value, the default factories, etc, that would otherwise only be available to explicit parameters with Parameter (or by using Field directly, I suppose that would work?).

But devil is always in the detail. I've just started work for the day so should have some time later tonight to put some work in to this. So I'd probably say I'm 0.5 for 1. and 1.5 for 3. with my uncertainty based on seeing how the implementation unfolds.

from litestar.

On the topic of where, if at all we should raise that exception we've discussed, if a dependency is duplicated it raises out of handlers.base.validate_dependency_is_unique():

/usr/local/lib/python3.10/site-packages/starlite/handlers/base.py:115: in validate_dependency_is_unique
    raise ImproperlyConfiguredException(
E   starlite.exceptions.ImproperlyConfiguredException: Provider for key entity_mappings_repository is already defined under the different key repository. If you wish to override a provider, it must have the same key.

from litestar.

I've done a quick and dirty implementation of each approach in my fork, and also an application of each approach in the example app.

Exclude by name approach

Starlite: main...peterschutt:exclude-by-name
Example app: litestar-org/litestar-pg-redis-docker@main...peterschutt:with-exclude-by-name

Exclude by value approach

Starlite: main...peterschutt:exclude-by-value
Example app: litestar-org/litestar-pg-redis-docker@main...peterschutt:with-exclude-by-value

Both implementations are only rough, especially for the first approach I'd tidy up where I've declared the parameters if we end up going that way.

Also, I've not worried about raising the exception. One thought on that, with OptionalDependency function, we could enforce that a default, or default factory is declared for the dependency so that the error state that we detailed earlier would not be possible to enter into at all (that is the non-parameter dependency, no default and dependency not provided state).

While the OptionalDependency() function approach might be slightly less intuitive than the exclude_from_docs approach, the diff to apply the change in the starlite source is much simpler.

I'm still leaning toward the Exclude by Value approach, but will let it simmer for a day or two.

from litestar.

I already looked at both. As I wrote earlier, both solutions are fine by me. If you feel this is the better solution, go ahead .

from litestar.

Yep cool, I know you were on board but I just want to be very clear about what I'm doing and why, especially given that you only threw me the keys a couple of days ago.

So especially, I just want to know you are on board with me taking the name "Dependency", and that there'll be another possible startup exception case, both of which are new to the conversation. So I'll now press ahead.

Thanks!

from litestar.

Thanks for feedback guys.

from litestar.

Closed in #137

from litestar.