Repository navigation
Use Optional for Query Params Instead of Union[Unset, ...] #285
Description
Is your feature request related to a problem? Please describe.
Although I'd like to keep Unset for body params, it makes a lot less sense for query params. I'd like non-required query params to return to the use of Optional.
While it would be inconsistent with the way we handle body params, I like that it encapsulates Unset (which is something of a workaround) more tightly.
Describe the solution you'd like
Given:
'/things':
get:
tags:
- things
operationId: getThings
parameters:
- name: size
in: query
schema:
type: int
I'd like the signature to look like:
def sync_detailed(
*,
client: Client,
size: Optional[int] = None,
) -> Response[Thing]:
Instead of:
def sync_detailed(
*,
client: Client,
size: Union[Unset, int] = UNSET,
) -> Response[Thing]:
Describe alternatives you've considered
We have several endpoints with a large number of optional query params that default to None in the wrapping interface. Will probably just iterate the dict and assign Unset everywhere we have None now.
Additional context
I feel like I did this to myself having requested the original Unset feature. 😅
Activity
dbanty commentedon Jan 5, 2021
Oof. There's definitely not a clear answer here. The purpose of
Unsetwas to differentiate between "not required" and "nullable". I feel like going back to usingOptionalas "not required" in some locations but not others will just cause confusion. Even worse, you've made me realize that it's not obvious what we should do with a query param which hasnullable: Truein the OpenAPI document.The OpenAPI spec doesn't seem to draw any distinction between location (e.g. nullable is not forbidden in query parameters). However, there is also no obvious way to send nulls in query parameters because... well they're stings. So it seems to me there is ambiguity in the spec which means it's going to be very implementation-specific how nulls are dealt with (if at all).
If you pass a
Noneto httpx in theparams=argument, it will encode as an empty string (e.g.?a=) which is definitely not the same as null. However, requests (which is a much more familiar library to most Python devs) will completely omit query params with a value ofNone.bowenwr commentedon Jan 5, 2021
Yeah, I can totally appreciate that it's confusing to have different paths for different parameter types. I think of
None/nullas omission vs. empty literal. In general I'm trying to avoid exposingUnsetexternally (we are wrapping the client) so maybe I will acceptNoneand convert it toUnsetor just do something in our fork but not upstream it. Thanks for your thoughts on this!dbanty commentedon Jan 5, 2021
Maybe it would be useful to have a configurable
Nonebehavior for query params since there is no universal standard. One of:That wouldn't fix the typing problem, unless the document specified the param as nullable the generated client wouldn't accept
Nonestill, but if you're wrapping the client you couldcastor# type: ignore?We could also take the config a step further and if
Nonebehavior is omit then always acceptNonefor non-required params. Gets a little icky with special cases in the generated code though.Another option is to just say that in this project
nullableis the same asnot requiredfor query params and make the typeUnion[Unset, None, <whatever>]. I've never needed to actually passnullin a query param so I'm fine with that.bowenwr commentedon Jan 6, 2021
This is probably the easiest option that solves all our problems if we're treating
Noneas omit there, assuming no one objects or has a use case around sending NULL.dbanty commentedon Jan 6, 2021
Sounds like a plan then. And if anyone ends up having different requirements around
nullablewe can revisit having configurable behavior with this as the default.dbanty commentedon Feb 1, 2021
@bowenwr , @forest-benchling , and @emann I want to pull the discussion which has sort of spiraled out in #297 back to the issue here. In thinking about this more I want to take a different approach. I think we should not be changing type annotation based on location (query) since that's causing a lot of confusion and cascading code changes. Instead, I think we leave the function/type definitions as they are (
Unsetfornot required,Nonefornullable).Instead, I think the solution here is to omit any query param with the value
Nonein generated functions which make requests. This actually seems to be more in line with the spec which has anallowEmptyValueparam which defaults to false, indicating to me that not including values which were set to null is the correct default behavior anyway.This is sort of a half solution to your specific problem, but I think the best way forward. The types will show incorrectly still if you pass
Noneinto an endpoint param which is notnullable, but the behavior of passing thatNonewill be as expected.emann commentedon Feb 1, 2021
Definitely seems like the cleanest solution + gives us an easy way to support
allowEmptyValuein the future. I think keeping the semantics ofUnset/not requiredandNone/nullableintact is also a good idea if nothing else for our own sanity while developing.forest-benchling commentedon Feb 3, 2021
@dbanty @emann @bowenwr
To me, it makes it more confusing for the type annotation to not reflect the implementation. The type annotation would be saying
Noneis not accepted, but we're saying the implementation would allow it. Not only would this be confusing for developers of this repo, it would also mean we'd have to silence MyPy errors because MyPy would be confused why we are still checking forNone.I'm not sure the
allowEmptyValueparam is a reason to do this--firstly, in the spec it says it has been deprecated and is no longer recommended, and secondly, what I gathered from my skimming of OAI/OpenAPI-Specification#1573 is that it has more to do with empty strings than withNone(although I could be wrong as I didn't read the entire thread).What I'm leaning towards is keeping the OpenAPI spec, the type annotations, and the implementation all in alignment, for our own sanity--AKA, the status quo.
However, I do think that is a very reasonable use case to want to pass
Noneinstead ofUnsetas query parameters. For the SDK we're building, we don't wantUnsetto go spilling out into SDK and all our application code. Therefore, here is an idea: What if we were to allow a flag, either insync_detailedor in the client, to convertNonetoUnsetat run-time for query parameters?dbanty commentedon Feb 8, 2021
I think that for the sake of simplicity (and expedience, because I want to move this issue along) I'm going to go with my solution above.
Unsetwill continue to be used fornot requiredandNonefornullable. Then the only change to make us compliant with OpenAPI's defaults is to omit anyNoneinstead of passing it through as an empty string.While the original issue here is spawned from Benchling's need to surface different types for their SDK, any solution has to be generally applicable enough to not cause undue complexity (either for consumers or within the codebase itself). That being said, the unstable custom templates feature exists for just such reasons: folks needing customization that doesn't necessarily belong in this general purpose product. You could probably achieve your original ask (replacing
UnsetwithNone) by providing a custom template forendpoint_macros.py.jinjawhich is the same except in theargumentsmacro it does a string replace ofUnsetwithNonefor query params.If there is a way we can make the custom templates feature more useful to you for this, let me know. We could, for example, potentially make the templates a separate package which you could fork and maintain as your custom templates (to make upstream changes easier to pull in). It might also make sense for all the type annotation building stuff to live in templates instead of the core Python code.
Also if you think I'm missing something here and that my solution isn't the ideal one for general purpose clients, I'm always happy to reconsider.
fix!: Omit any `None` passed as a query param in generated clients. C…
fix!: Omit any `None` passed as a query param in generated clients. C…