Change optional Open API query parameters to allow `None` #297

forest-benchling · 2021-01-13T13:37:48Z

Fixes #285.

Instead of methods like

def sync_detailed(
    *,
    client: Client,
    size: Union[Unset, int] = UNSET,
) -> Response[Thing]:

we will now generate

def sync_detailed(
    *,
    client: Client,
    size: Union[Unset, None, int] = None,
) -> Response[Thing]:

and treat None the same as Unset.

This is a bit different from the original proposal in the linked issue, but if you scroll down you can see that it's what was agreed upon in the end IIUC (though I wasn't sure if we agreed on what the default value should be). In addition, it has the benefit of preserving back-compat.

As part of this issue, I also tried to collapse nested Optional and Union types, so that what was formerly Union[Unset, Optional[int]] or Optional[Union[Unset, int]] is now Union[Unset, None, int].

codecov · 2021-01-13T14:09:24Z

Codecov Report

Merging #297 (4b8fc70) into main (d76bae8) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #297   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           47        47           
  Lines         1385      1392    +7     
=========================================
+ Hits          1385      1392    +7

Impacted Files	Coverage Δ
...penapi_python_client/parser/properties/__init__.py	`100.00% <100.00%> (ø)`
...i_python_client/parser/properties/enum_property.py	`100.00% <100.00%> (ø)`
..._python_client/parser/properties/model_property.py	`100.00% <100.00%> (ø)`
...penapi_python_client/parser/properties/property.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d76bae8...05d284e. Read the comment docs.

forest-benchling · 2021-01-13T14:41:24Z

cc @dbanty @emann LMK if you have any feedback! @packyg @bowenwr note that the implementation of get_type_string is a bit more complicated than what I originally showed you, since I tried to get rid of silly types like Union[Unset, None, Optional[int]].

bowenwr · 2021-01-15T23:01:26Z

@dbanty Any thoughts on this?

dbanty · 2021-01-15T23:04:18Z

@dbanty Any thoughts on this?

I am planning on reviewing this weekend (which is starting soon!). So I should have some feedback or an approval for you by Tuesday 😊

dbanty

Awesome work! I really love cleaning up all those type string functions to centralize the nullable/required logic more. My comments are mostly split between some if-logic consolidation and the big question of whether we need more logic in the templates to ensure that nullable is always the same as not-required for query params.

Also, you'll need to pull main into your branch because I fixed that py thing as well while I was doing something else 😦. The reason it fails on the local task but not in CI is because it's required only by a dev dependency, and CI intentionally leaves those out since they don't matter to the end product. Probably worth updating the task to do the same at some point 🧐.

Thanks again! Looking forward to your response / follow-up changes!

dbanty · 2021-01-16T18:26:21Z

openapi_python_client/parser/properties/__init__.py

+        if self.required:
+            if self.nullable:
+                return f"Union[None, {self._get_inner_prop_string()}]"
+            else:
+                return type_string
+        else:
+            if self.nullable:
+                return f"Union[Unset, None, {self._get_inner_prop_string()}]"
+            else:
+                if query_parameter:
+                    # For query parameters, None has the same meaning as Unset
+                    return f"Union[Unset, None, {self._get_inner_prop_string()}]"
+                else:
+                    return f"Union[Unset, {self._get_inner_prop_string()}]"


I think this cleans up the logic a little bit (one case per output option) and covers the case of required and nullable and query_parameter correctly. Spacing may be wrong because I'm editing in GitHub >.>.

Suggested change

if self.required:

if self.nullable:

return f"Union[None, {self._get_inner_prop_string()}]"

else:

return type_string

else:

if self.nullable:

return f"Union[Unset, None, {self._get_inner_prop_string()}]"

else:

if query_parameter:

# For query parameters, None has the same meaning as Unset

return f"Union[Unset, None, {self._get_inner_prop_string()}]"

else:

return f"Union[Unset, {self._get_inner_prop_string()}]"

if self.required and not self.nullable:

return type_string

if query_parameter or (self.nullable and not self.required):

# For query parameters, nullable and required are the same

return f"Union[Unset, None, {self._get_inner_prop_string()}]"

if self.nullable:

return f"Union[None, {self._get_inner_prop_string()}]"

return f"Union[Unset, {self._get_inner_prop_string()}]"

I think the case that differs is if query_parameter=True, required=True, nullable=True. In the original code, we get f"Union[None, {self._get_inner_prop_string()}]", whereas in the suggestion, we get f"Union[Unset, None, {self._get_inner_prop_string()}]". It seems like the original code would be correct in that case, unless it's the case that query parameters cannot exist with required=True, nullable=True?

Yeah, hmm, it seems like the semantics are weird for a query parameter with required=True, nullable=True...what would it mean for you to pass None?

This also impacts your other comment in endpoint_macros.pyi...if we decide that we aren't gonna allow passing None, then I think we also need {% if property.required and not property.nullable %} in L25.

This is so brain-twisting 🤔

@dbanty (cc @bowenwr) OK I think I figured out why I'm so confused. Seems like for query parameters, it's redundant to have BOTH required and nullable as properties, because there's no distinction between None and UNSET.

In the current implementation, we have:

However, it's confusing because in the last row, we specifically say nullable=False, but still it's allowing None.

Is it possible to ban the nullable attribute on query parameters? That would simplify things a lot. Though it would be a breaking change.

I think what we're going for here is to interpret both nullable and required to mean the same thing in the context of query parameters. Basically, there is no standard way to pass null through a query parameter, so rather than setting our own decision we're opting to not have a null value. In which case, null and not required become the same thing semantically when using this generator, and therefore should be the same signature.

Interestingly, in my response to #295 I encountered the "style" attribute which does seem to indicate that an "empty" value should be provided as empty, as the generated clients do today. It's not clear if empty is equivalent to null though...

I see. In that case @dbanty do you think it would be reasonable to stick to the original implementation, which converts query_parameter=True, required=True, nullable=True into Union[None, {self._get_inner_prop_string()}]?

dbanty · 2021-01-16T18:31:59Z

openapi_python_client/parser/properties/property.py

+        if self.required:
+            if self.nullable:
+                return f"Optional[{type_string}]"
+            else:
+                return type_string
+        else:
+            if self.nullable:
+                return f"Union[Unset, None, {type_string}]"
+            else:
+                if query_parameter:
+                    # For query parameters, None has the same meaning as Unset
+                    return f"Union[Unset, None, {type_string}]"
+                else:
+                    return f"Union[Unset, {type_string}]"


I can't include it in a suggestion because of the way GitHub split the lines, but you could make this first case the same if check as no_optional. This is basically the same change I suggested for UnionProperty.

Suggested change

if self.required:

if self.nullable:

return f"Optional[{type_string}]"

else:

return type_string

else:

if self.nullable:

return f"Union[Unset, None, {type_string}]"

else:

if query_parameter:

# For query parameters, None has the same meaning as Unset

return f"Union[Unset, None, {type_string}]"

else:

return f"Union[Unset, {type_string}]"

if self.required and not self.nullable:

return type_string

if query_parameter or (self.nullable and not self.required):

# For query parameters, nullable has the same meaning as required

return f"Union[Unset, None, {type_string}]"

if self.nullable:

return f"Optional[{type_string}]"

return f"Union[Unset, {type_string}]"

openapi_python_client/parser/properties/property.py

dbanty · 2021-01-16T18:36:10Z

openapi_python_client/templates/endpoint_macros.pyi

    {% endfor %}
 }
    {% for property in endpoint.query_parameters %}
        {% if not property.required %}


I think to truly make not-required the same as nullable for query params we want to add this.

Suggested change

{% if not property.required %}

{% if not property.required or property.nullable %}

dbanty · 2021-01-16T18:40:02Z

openapi_python_client/templates/property_templates/date_property.pyi


-{% macro transform(property, source, destination, declare_type=True) %}
+{% macro transform(property, source, destination, declare_type=True, query_parameter=False) %}
 {% if property.required %}


I think we also need this? I could be confusing myself now 🥸

Suggested change

{% if property.required %}

{% if property.required and (not query_parameter or not property.nullable) %}

Wouldn't we be safe in that case by the {% if property.nullable %}if {{ source }} else None {%endif%} on L14?

Oh I see, I was thinking that in endpoint_macros we were only ignoring params that were UNSET, not also None.

Actually though I think I just spotted another issue but I can't comment on it. Line 25 in endpoint_macros will include all params if they are required, even if they are None. If we truly want to treat nullable and optional as the same thing we'll need to adjust that if and the one on line 35.

dbanty · 2021-01-16T18:41:44Z

openapi_python_client/templates/property_templates/datetime_property.pyi

+{% macro transform(property, source, destination, declare_type=True, query_parameter=False) %}
 {% if property.required %}
 {% if property.nullable %}
 {{ destination }} = {{ source }}.isoformat() if {{ source }} else None


There's probably something in here too. I guess we need some end to end tests with just nullable or just non-required to make sure we're covering those bases? We don't want to ever pass None to a query param. I'm going to stop writing this same note over and over now 😅

I added every permutation of nullable, required for the datetime query parameter to the E2E test.

However, I wasn't sure how far to take it--ideally it would cover all types, but that would be an exponential explosion 😬

pyproject.toml

dbanty · 2021-01-16T18:44:05Z

tests/test_parser/test_properties/test_init.py


        p = Property(name="test", required=False, default=None, nullable=True)
-        assert p.get_type_string() == f"Union[Unset, Optional[{base_type_string}]]"
+        assert p.get_type_string() == f"Union[Unset, None, {base_type_string}]"


Honestly I like this style better overall. I think the Optional name gets confusing in some cases (the whole reason we're changing this behavior...).

dbanty · 2021-01-16T18:45:51Z

tests/test_parser/test_properties/test_init.py

+
+        p = Property(name="test", required=False, default=None, nullable=False)
+        assert p.get_type_string(query_parameter=True) == f"Union[Unset, None, {base_type_string}]"
+        assert p.get_type_string(no_optional=True, query_parameter=True) == base_type_string


This feels like maybe a good opportunity to use @pytest.mark.parametrize to make the different test cases and expected outcomes easier to read. Not at all required (clearly lots of other tests could use it but don't) but could be nice.

Co-authored-by: Dylan Anthony <43723790+dbanty@users.noreply.github.com>

forest-benchling · 2021-01-20T15:48:40Z

@dbanty PTAL at the new changeset. While addressing your comments, I realized that the existing code generation wasn't correctly handling the case where a query parameter is nullable but not required.

forest-benchling · 2021-01-21T14:30:42Z

@dbanty (cc @bowenwr) I made another change in the latest commit to pass a json kwarg to get_type_string, reflecting whether the type is the type after JSON serialization. This change cleans up the logic in the property templates, but along the way it also fixes a couple pre-existing bugs:

The types of union properties and model properties were declared incorrectly in to_dict
Enums were not correctly being serialized in to_dict

dbanty · 2021-01-26T16:02:25Z

I haven't forgotten about this I promise, just been a bit busy 😅. Thanks so much for putting all this work in, I'll review as soon as I can!

dbanty · 2021-01-31T23:40:20Z

openapi_python_client/parser/properties/property.py

+            return type_string
+        if self.required:
+            if self.nullable:
+                return f"Optional[{type_string}]"


I think we're still not in alignment on design here. nullable should be the same as not required semantically when considering a query parameter. So if required and nullable and query_parameter it should be returning f"Union[Unset, None, {type_string}]". I believe my previous suggestion to this (and the other, similar) code block makes this happen.

It's possible I'm missing or misunderstanding something about this, and if so please let me know. Otherwise I'll hold off additional review until you make that change.

Hmm...if required and nullable and query_parameter are all True, wouldn't we want the type to be Optional[{type_string}], not allowing Unset? Otherwise, if you allowed Unset, why would you make it "required"?

Or is it different for query parameters, where "required" would still allow Unset to pass through.

It's different for query parameters. Basically we're not allowing nullable query parameters and instead treating nullable like not required. Additionally, we're allowing None to function just like Unset, but either way we're not passing through the parameter. I think having a different result for nullable and required might infer to the user that we can/will actually pass through a None, which we will not.

@dbanty I started implementing this approach, but it causes a bunch of other errors because in the templates, we are assuming that if something is required and nullable, then we only need to check for None, whereas with this change we now also need to check for Unset.

I think this is getting pretty complicated and hard to reason about, and I worry about future maintainability. Maybe we should just make the type Optional if it's either not required or nullable, and throw away Unset in the case of query parameters? cc @bowenwr

In my original implementation of Unset I didn't apply it to query parameters. I'm personally in favor of that approach.

If we have to pick one or the other I'd prefer Unset over None because it better indicates what we're doing with that parameter to the user. It also happens to be the behavior of the released version for non-required query params. What's your opinion @emann?

dbanty · 2021-02-08T20:38:43Z

Closing in favor of #331 which, while not nearly as cool, brings us in line with the spec with a relatively simple change. Thank you so much for all your hard work on this @forest-benchling , sorry we couldn't come up with a better solution that would more cleanly solve Benchling's problem while appeasing my annoying sensibilities 😅. Hopefully #331 is good enough at least for now and we can talk about other options in the future if it's not.

forest-benchling added 6 commits January 13, 2021 08:36

Change signature of query parameters to allow None

e9ac07c

Run lint

3265a75

Fix type error

43d8bc4

Upgrade dependencies to fix py vulnerability

6d01c2b

Add task for unit tests

cbd8136

Fix remaining unit tests

aac9b3a

forest-benchling added 2 commits January 13, 2021 09:33

Collapse Optional and Union types

233f73e

Run black

0f2706d

forest-benchling marked this pull request as ready for review January 13, 2021 14:39

dbanty requested changes Jan 16, 2021

View reviewed changes

dbanty added this to the 0.8.0 milestone Jan 16, 2021

forest-benchling and others added 9 commits January 19, 2021 17:03

Merge main

1c78da0

Apply suggestions from code review

6a403e8

Co-authored-by: Dylan Anthony <43723790+dbanty@users.noreply.github.com>

Fix tests

d1b33b2

Add another check for nullable

75d250d

Change tests to use parametrize

74444ca

Add more e2e tests for datetime

e906b1a

Fix some tests

c11de57

Fix remaining tests

a045cf3

Fix unit tests

fa92f9a

forest-benchling requested a review from dbanty January 20, 2021 15:47

Refactor out logic for JSON types and fix enum value bug

1512225

forest-benchling mentioned this pull request Jan 21, 2021

fix: Fix optional model properties BNCH-20284 benchling/openapi-python-client#35

Merged

Refactor union base type string

4b8fc70

forest-benchling mentioned this pull request Jan 28, 2021

Fix deserialization of unions #319

Closed

dbanty reviewed Jan 31, 2021

View reviewed changes

forest-benchling added 2 commits February 1, 2021 13:21

Merge upstream/main

f98ea06

Regenerate tests

05d284e

dbanty mentioned this pull request Feb 1, 2021

Use Optional for Query Params Instead of Union[Unset, ...] #285

Closed

dbanty closed this Feb 8, 2021

eli-bl deleted the benchling-issue-285 branch November 26, 2024 22:44

	{% if not property.required %}
	{% if not property.required or property.nullable %}

	{% if property.required %}
	{% if property.required and (not query_parameter or not property.nullable) %}

Uh oh!

Change optional Open API query parameters to allow None #297

Change optional Open API query parameters to allow None #297

Uh oh!

Conversation

forest-benchling commented Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

forest-benchling commented Jan 13, 2021

Uh oh!

bowenwr commented Jan 15, 2021

Uh oh!

dbanty commented Jan 15, 2021

Uh oh!

dbanty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

forest-benchling Jan 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

forest-benchling commented Jan 20, 2021

Uh oh!

forest-benchling commented Jan 21, 2021

Uh oh!

dbanty commented Jan 26, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbanty commented Feb 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Change optional Open API query parameters to allow `None` #297

Change optional Open API query parameters to allow `None` #297

forest-benchling commented Jan 13, 2021 •

edited

Loading

codecov bot commented Jan 13, 2021 •

edited

Loading

forest-benchling Jan 19, 2021 •

edited

Loading