Allow Unset values to avoid being send during serialization #10

bowenwr · 2020-10-26T19:33:59Z

Introduces a new sentinel value, UNSET which becomes the new default for nullable fields which aren't required. If a field is set to this value, it will not be sent as part of the payload during serialization.

Since we still want to treat endpoint arguments and query params as they were originally without Unset, this logic has moved to to_query_string on Property.

Given a str type, a nullable property which isn't required might look something like:

my_property: Union[str, None, Unset] = UNSET

bowenwr · 2020-10-26T19:59:50Z

FYI @benchling/ce-team and @rswang since Daniel is out

packyg · 2020-10-27T00:19:37Z

openapi_python_client/templates/types.py

+Unset = NewType("Unset", object)
+UNSET: Unset = object()


Shouldn't this be

Suggested change

Unset = NewType("Unset", object)

UNSET: Unset = object()

Unset = NewType("Unset", object)

UNSET: Unset = Unset(object())

(not that it matters)

I guess another alternative would be to keep it as UNSET = object() and use Literal[UNSET] as the type? (Also doesn't seem like it matters)

When I tried that previously, PyCharm complained:

'Literal' may be parameterized with literal ints, byte and unicode strings, bools, Enum values, None, other literal types, or type aliases to other literal types

packyg · 2020-10-27T00:19:44Z

openapi_python_client/parser/properties.py

        else:
            return f"{self.python_name}: {self.get_type_string()}"

+    def to_query_string(self) -> str:


This threw me for a while - I thought get_query_string was building literal query strings, e,g. ?property=value which this is... not

I think you should rename this to something like

Suggested change

def to_query_string(self) -> str:

def to_query_method_arg(self) -> str:

or

Suggested change

def to_query_string(self) -> str:

def to_query_parameter(self) -> str:

(and get_query_string()/query_string() to match)

Thanks, I renamed it. It's unclear to me whether the "new" query behavior should be the default the this response body/model body should be the new method, but it seems to me like it's used in more places.

I also had already changed it and then had to put back the query parsing....

packyg · 2020-10-27T00:21:11Z

openapi_python_client/templates/model.pyi

+
+        {% for property in model.required_properties + model.optional_properties %}
+        {% if not property.required %}
+        if {{property.python_name}} != UNSET:


You should use instance equality with a sentinel object

Suggested change

if {{property.python_name}} != UNSET:

if {{property.python_name}} is not UNSET:

packyg

LGTM, just the overlooked rename

packyg · 2020-10-27T15:36:25Z

openapi_python_client/templates/endpoint_macros.pyi

 {% endfor %}
 {% for parameter in endpoint.header_parameters %}
-{{ parameter.to_string() }},
+{{ parameter.to_query_string() }},


I think you forgot to update this one

Suggested change

{{ parameter.to_query_string() }},

{{ parameter.to_query_method_arg() }},

bowenwr · 2020-10-27T17:08:02Z

FYI @mzhu22

bowenwr · 2020-10-28T15:11:45Z

One of the major drawbacks to this approach is that it applies to both request and response bodies. When really we only want it to apply to request bodies. I don't think there's a way to really control this via the spec without misusing it.

Do we think it would be better to leave the original typings as is and only change the defaults to Unset and ignore typing violations?

packyg · 2020-10-28T16:31:22Z

Do we think it would be better to leave the original typings as is and only change the defaults to Unset and ignore typing violations?

@bowenwr good point. Maybe it would be better to leave Unset off the type. You could then lie to mypy with just cast(<Target Type>, UNSET) and mypy should behave as expected otherwise.

The response objects still have None for those values, overriding the default UNSET val, right?

forest-benchling

👏 yay! Are you going to also try to submit this PR upstream? Seems like a useful addition for others as well.

forest-benchling · 2020-10-28T19:17:15Z

tests/test_openapi_parser/test_properties.py

@@ -24,13 +24,23 @@ def test_get_type_string(self):
        p = Property(name="test", required=True, default=None, nullable=False)


Is there any way we can have a more end-to-end test that unset values are not sent over to the API endpoint?

I'm a bit nervous since I'm very unfamiliar with this codebase.

forest-benchling · 2020-10-28T19:18:20Z

openapi_python_client/templates/property_templates/union_property.pyi

@@ -36,7 +38,7 @@ else:
    {% endif %}
 {% if inner_property.template %}
 {% from "property_templates/" + inner_property.template import transform %}
-    {{ transform(inner_property, source, destination) | indent(8) }}
+    {{ transform(inner_property, source, destination) | indent(4) }}


What caused this indentation level to change?

forest-benchling · 2020-10-28T19:21:31Z

openapi_python_client/templates/model.pyi

+        if {{property.python_name}} is not UNSET:
+            properties["{{ property.name }}"] = {{ property.python_name }}
+        {% else %}
+        properties["{{ property.name }}"] = {{ property.python_name }}


Is it possible that a property is both required AND unset, and if so is it desired that it would be exposed in the dictionary directly (instead of, say, erroring)?

If a property is required, we shouldn't allow unset - we simply shouldn't allow a default. It could still be nullable and required, in which case we should allow it to be None.

forest-benchling · 2020-10-28T19:24:18Z

openapi_python_client/parser/properties.py

-        if no_optional or (self.required and not self.nullable):
-            return self._type_string
-        return f"Optional[{self._type_string}]"
+        return type_string(no_optional, self, self._type_string)


Could this be a method on Property instead of a function that takes in self? (Same for query_method_arg)

forest-benchling · 2020-10-28T19:25:51Z

openapi_python_client/parser/properties.py

@@ -63,24 +70,42 @@ def get_imports(self, *, prefix: str) -> Set[str]:
            prefix: A prefix to put before any relative (local) module names. This should be the number of . to get
            back to the root of the generated client.
        """
-        if self.nullable or not self.required:
+        if self.required and self.nullable:
            return {"from typing import Optional"}


If self.required is true, why do we need to import Optional?

forest-benchling · 2020-10-28T19:32:01Z

openapi_python_client/parser/properties.py

+
+
+def query_method_arg(no_optional: bool, prop: Property, inner_type_string: str, is_outer_union: bool = False) -> str:
+    if no_optional or (prop.required and not prop.nullable):


Potentially slightly more readable as:

type_string = inner_type_string if is_outer_union: type_string = f"Union[{type_string}]" if not no_optional and (not prop.required or prop.nullable): type_string = f"Optional[{type_string}]" return type_string

Similarly for type_string above. Don't feel too strongly though, will leave it to you.

forest-benchling · 2020-10-28T19:33:09Z

openapi_python_client/parser/properties.py

+        if is_outer_union:
+            return f"Optional[Union[{inner_type_string}]]"
+        return f"Optional[{inner_type_string}]"
+    elif not prop.nullable and not prop.required:


Should this be elif prop.nullable and not prop.required, given that we're unioning it with None in L632?

I dropped this change in the latest commit where I preserve the original typing and just set the default.

forest-benchling · 2020-10-28T19:35:43Z

openapi_python_client/templates/property_templates/dict_property.pyi

@@ -12,6 +12,9 @@ if {{ source }} is not None:
 {% if property.required %}
 {{ destination }} = {{ source }}
 {% else %}
-{{ destination }} = {{ source }} if {{ source }} else None
+if {{ source }} is UNSET:


For my context, where is the macro transform actually called?

It's called frequently in the various .pyi templates - you'll find it easily if you search and make sure you aren't filtering to only .py files.

bowenwr · 2020-10-29T04:23:56Z

Do we think it would be better to leave the original typings as is and only change the defaults to Unset and ignore typing violations?

@bowenwr good point. Maybe it would be better to leave Unset off the type. You could then lie to mypy with just cast(<Target Type>, UNSET) and mypy should behave as expected otherwise.

The response objects still have None for those values, overriding the default UNSET val, right?

I liked this suggestion, I implemented it and tested it out. It seems to pass our integration tests. I still needed to_query_method_arg to preserve the original defaults but otherwise I like this change as a lot less invasive.

bowenwr · 2020-10-29T04:28:31Z

👏 yay! Are you going to also try to submit this PR upstream? Seems like a useful addition for others as well.

I'd like to upstream this at some point though it would probably need considerably more polish and E2E tests. I'll probably incubate it a bit internally too to make sure I haven't missed any cases as we expand our spec coverage.

bowenwr force-pushed the allow-unset-values branch from 4bdadd6 to e9a3ee5 Compare October 26, 2020 19:42

bowenwr marked this pull request as ready for review October 26, 2020 19:58

bowenwr requested review from packyg and forest-benchling October 26, 2020 19:59

packyg suggested changes Oct 27, 2020

View reviewed changes

packyg reviewed Oct 27, 2020

View reviewed changes

bowenwr requested a review from packyg October 27, 2020 02:22

packyg approved these changes Oct 27, 2020

View reviewed changes

forest-benchling suggested changes Oct 28, 2020

View reviewed changes

bowenwr requested a review from forest-benchling October 29, 2020 04:26

forest-benchling approved these changes Oct 29, 2020

View reviewed changes

Allow Unset values to avoid being send during serialization

d83e554

bowenwr force-pushed the allow-unset-values branch from 9dcce0c to d83e554 Compare October 29, 2020 19:13

bowenwr merged commit 89c10b3 into main Oct 29, 2020

emann mentioned this pull request Nov 2, 2020

Better compatibility for "required" vs. "nullable" openapi-generators/openapi-python-client#230

Merged

eli-bl deleted the allow-unset-values branch November 26, 2024 22:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Unset values to avoid being send during serialization #10

Allow Unset values to avoid being send during serialization #10

bowenwr commented Oct 26, 2020 •

edited

Loading

bowenwr commented Oct 26, 2020

packyg Oct 27, 2020 •

edited

Loading

forest-benchling Oct 28, 2020 •

edited

Loading

bowenwr Oct 29, 2020

packyg Oct 27, 2020

bowenwr Oct 27, 2020

packyg Oct 27, 2020

packyg left a comment

packyg Oct 27, 2020

bowenwr commented Oct 27, 2020

bowenwr commented Oct 28, 2020

packyg commented Oct 28, 2020 •

edited

Loading

forest-benchling left a comment

forest-benchling Oct 28, 2020

forest-benchling Oct 28, 2020

forest-benchling Oct 28, 2020

bowenwr Oct 29, 2020

forest-benchling Oct 28, 2020 •

edited

Loading

forest-benchling Oct 28, 2020

forest-benchling Oct 28, 2020

forest-benchling Oct 28, 2020

bowenwr Oct 29, 2020

forest-benchling Oct 28, 2020

bowenwr Oct 29, 2020

bowenwr commented Oct 29, 2020

bowenwr commented Oct 29, 2020

	def to_query_string(self) -> str:
	def to_query_method_arg(self) -> str:

	def to_query_string(self) -> str:
	def to_query_parameter(self) -> str:

	if {{property.python_name}} != UNSET:
	if {{property.python_name}} is not UNSET:

	{{ parameter.to_query_string() }},
	{{ parameter.to_query_method_arg() }},

		@@ -24,13 +24,23 @@ def test_get_type_string(self):
		p = Property(name="test", required=True, default=None, nullable=False)



		def query_method_arg(no_optional: bool, prop: Property, inner_type_string: str, is_outer_union: bool = False) -> str:
		if no_optional or (prop.required and not prop.nullable):

Allow Unset values to avoid being send during serialization #10

Allow Unset values to avoid being send during serialization #10

Conversation

bowenwr commented Oct 26, 2020 • edited Loading

bowenwr commented Oct 26, 2020

packyg Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

forest-benchling Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

packyg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowenwr commented Oct 27, 2020

bowenwr commented Oct 28, 2020

packyg commented Oct 28, 2020 • edited Loading

forest-benchling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

forest-benchling Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowenwr commented Oct 29, 2020

bowenwr commented Oct 29, 2020

bowenwr commented Oct 26, 2020 •

edited

Loading

packyg Oct 27, 2020 •

edited

Loading

forest-benchling Oct 28, 2020 •

edited

Loading

packyg commented Oct 28, 2020 •

edited

Loading

forest-benchling Oct 28, 2020 •

edited

Loading