[python] Add option to return None instead of raising exception when accessing unset attribute

[mks-m]

See #7781

Add option to return None instead of raising exception when accessing unset attribute.

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.
• If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
• Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. 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.
• File the PR against the correct branch: master
• Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

cc @spacether

[auto-labeler]

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

[spacether]

@keymone can this be implemented with a get method? That way we do not need to add a new generator additionalProperty or python Configuration property.
This would behave like dict.get
So one can have:

pet_kwargs = {"name": "Hannah", "color": "black"}
pet = Pet(**pet_kwargs)
animal_type = pet.get("animal_type") # None is the default_value automatically
# animal_type == None
animal_type = pet.get("animal_type", "dog")
# animal_type == "dog"

Though we will have a name collision with get when one has a property named get, one can do:
model.get("get")
We are fundamentally representing dictionary data

The reasons that I prefer a get method are:

• this is consistent with how dictionaries implement this feature
• this is simpler than adding a new codegen constant
• this will not break existing functionality

[mks-m]

@spacether I understand the motivation behind .get() approach and we can have that as well without any conflict with this PR.

But why not have both? Compared to current implementation both .get and not throwing exceptions are preferable and lead to better code quality.

Yes, model instances can be treated as dictionaries and dictionaries throw a KeyError when non-existant key is being retrieved, however I don't think retrieving non-existent key is the same as retrieving existing but unset attribute. So maybe I should add throwing exception when non-existant attribute is being retrieved?

Also, dictionaries don't pretend to be objects and don't expose direct model.attr interface. Why have this interface, which is appealing for it's conciseness and simplicity but burden users with having to deal with exceptions or explicitly verify presence of an attribute all the time? Let's have an option for a more relaxed interface.

[spacether]

Yes, model instances can be treated as dictionaries and dictionaries throw a KeyError when non-existant key is being retrieved, however I don't think retrieving non-existent key is the same as retrieving existing but unset attribute. So maybe I should add throwing exception when non-existant attribute is being retrieved?

This gets into very murky territory
We either should raise key errors or we shouldn't. We shouldn't have normal cases where we don't raise errors all the time then have a corner case where we always do raise them. That's why I think that providing an interface that raises an exception (dot access model.property) clearly describes what is happening and adding get, that allows you to control the default provides clear predictable behavior here.

But why not have both? Compared to current implementation both .get and not throwing exceptions are preferable and lead to better code quality.

Because it is confusing to have multiple ways to do the same thing. Which is the preferred method? Do they both work the same way?

How do you feel about chatting about this over Slack to get on the same page? We both definitely want to make using these models easier for users and to allow them to get back a default value rather than a key error. I am in our channel at https://join.slack.com/t/openapi-generator/shared_invite/enQtNzAyNDMyOTU0OTE1LTY5ZDBiNDI5NzI5ZjQ1Y2E5OWVjMjZkYzY1ZGM2MWQ4YWFjMzcyNDY5MGI4NjQxNDBiMTlmZTc5NjY2ZTQ5MGM

[spacether]

How about updating the docstring here?

we use this to set values for instance.field_name = val and instance["field_name"] = val

[spacether]

How about just using name here? We know that name was not found.

• this same comment applies on line 70 too

[mks-m]

i just compacted the previous functionality to find path_to_item, if you think just name is enough instead of the old path code, i'm fine with that

[spacether]

How about unset_attribute_value = object
Then we always point to a singleton and don't need to instantiate additional empty objects

[mks-m]

well, somebody could concievably set an attribute to object value, __unset_attribute_value__ will generate unique object only once per class, so i'm not worried about performance here

[spacether]

This looks great, thanks you @keymone!

Can you add a python test in one of our manual tests that accesses a type:object's model properties with:

• when a property is stored in a model instance
  • access instance.prop and assert that the result is what you expect
  • access instance["prop"] and assert that the result is what you expect
  • access instance.get("prop") and assert that the result is what you expect
• when a property is not stored in a model instance
  • access instance.prop and assert that the result is what you expect or the expected exception is raised
  • access instance["prop"] and assert that the result is what you expect or the expected exception is raised
  • access instance.get("prop") and assert that the result is what you expect or the expected exception is raised
  • access instance.get("prop", None) and assert that the result is None

That will verify that this is working and throw a test error if this feature is accidentally changed/broken in the future.

[mks-m]

@spacether i think all those cases are covered except .get with existing and absent attribute value. i've added those but test broke for unrelated reason.

[spacether]

Thanks for adding those tests. Your CI error is unrelated so I will merge this.
Thank you for the your PR!