[python-nextgen] use __fields_set__ to determine if the field is needed in to_dict

[wing328]

• use __fields_set__ to determine if the nullable field is needed (set explicitly to None) in to_dict
• add tests
• to fix [REQ][python-nextgen] Differentiate between null and unset values #15056

PR checklist

• Read the contribution guidelines.
• Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
• Run the following to build the project and update samples:

  ./mvnw clean package 
  ./bin/generate-samples.sh
  ./bin/utils/export_docs_generators.sh
  

  Commit all changed files.
  This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
  These must match the expectations made by your contribution.
  You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
  For Windows users, please run the script in Git BASH.
• In case you are adding a new generator, run the following additional script :

  ./bin/utils/ensure-up-to-date.sh
  

  Commit all changed files.
• File the PR against the correct branch: master (6.3.0) (minor release - breaking changes with fallbacks), 7.0.x (breaking changes without fallbacks)
• If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

FYI @taxpon (2017/07) @frol (2017/07) @mbohlool (2017/07) @cbornet (2017/09) @kenjones-cisco (2017/11) @tomplus (2018/10) @arun-nalla (2019/11) @spacether (2019/11) @krjakbrjak (2023/02)

[robertschweizer]

Thanks a lot for proposing a fix!

I'm a bit worried that this does not provide a "clean" way of checking whether a field is "null" or unset in response content. That can that only be done by inspecting the to_dict() return value or __fields_set__, right?

[wing328]

@robertschweizer have you reviewed the new tests: https://github.com/OpenAPITools/openapi-generator/pull/15086/files#diff-638b6ec3d1b35e0b1694a2c1ba3c3d24fb00291092e5abe63d8ec3a86b4bdb6d ?

I think this is also what the pydantic users get used to: https://docs.pydantic.dev/usage/models/#creating-models-without-validation

[robertschweizer]

Yes, I have reviewed the new test. It inspects whether fields are set with __fields_set__ and to_json().

I think there's a trade-off: Using __fields_set__, models can be used like this:

# Check if field is set
"field" in instance.to_dict()
"field" in instance.__fields_set__

# Unset field
instance.__fields_set__.remove("field")

Using an Unset type like I proposed in #15056 (which would mean a breaking change of the generated clients):

# Check if field is set
instance.field is Unset
"field" in instance.to_dict()

# Unset field
instance.field = Unset

I find the Unset would be more explicit, but that's probably not worth the very breaking change of using it for all unset values instead of None in the model classes.

This is not a request to change this MR or so, I just wanted to clarify the trade-off coming with this solution.

[wing328]

I find the Unset would be more explicit,

i'm not against this solution or any other solutions.

My approach is simply to go with what pydantic offers at the moment as pydantic seems to be pretty mature in handling many use cases.

I wonder if you can make a suggestion via https://github.com/pydantic/pydantic to create a new type called Unset as Samuel (author of pydantic) is working on 2.0 of Pydantic and he may be open to other approaches.

[robertschweizer]

Thanks for the suggestion! Looking more closely at pydantic, I think version 2's Required vs. Nullable Cleanup offers everything needed for correct representation of JSON schemas. The only thing missing in version 1 is actually the validation of non-required non-nullable fields like here:

from pydantic import BaseModel


class ModelClass(BaseModel):
    not_required_not_nullable: int = None

inst = ModelClass(not_required_not_nullable=None)  # Fails in pydantic-2 but not pydantic-1

Only missing is a pretty way of checking for membership and modifying instances after creation. I find it unlikely that they will redo things only for such usability improvement so shortly before the v2 release.

To use what pydantic offers as much as possible,
could the OpenAPI generated clients safely use pydantic's exclude_unset=True instead of exclude_none=True and later setting set attributes that are None?

[wing328]

could the OpenAPI generated clients safely use pydantic's exclude_unset=True instead of exclude_none=True and later setting set attributes that are None?

By doing so, the nullable attribute will be completely ignored, right?

[robertschweizer]

pydantic's exclude_unset=True would take this into account correctly for the following type annotations:

from pydantic import BaseModel

class ModelClass(BaseModel):
    not_required_and_nullable: Optional[int] = None
    not_required_not_nullable: int = None
    required_but_nullable: Optional[int]
    required_not_nullable: int

But the int = None fails in mypy and is actually incorrect (the value can be None when accessed).

So there is no way to use pydantic's exclude_unset for this and this MR is needed as is.

Thanks for your replies! I did end up opening pydantic/pydantic#5326 to ask pydantic maintainers' opinion about an Unset type.

[wing328]

@robertschweizer can you please PM me via Slack for a quick chat (IM)?

https://join.slack.com/t/openapi-generator/shared_invite/zt-12jxxd7p2-XUeQM~4pzsU9x~eGLQqX2g