Feat/detect discriminator bnch 49688

[straz]

Capture the discriminator property and mappings when parsing the OpenAPI spec.

See more extensive notes in the PR for aurelia.

The timing of merging this PR needs to be synchronized with the merge of the aurelia PR.

Effect on generated code
This effect requires changes from two PRs working together

• (this PR) updates the parser to capture discriminator_property and discriminator_mappings extracted from openapi.yaml into a ModelProperty instance.
• PR for aurelia augments the codegen templates to render these properties to the corresponding discriminator-aware code.

Before example

  class EntityOrInaccessibleResource:
        ...
    @classmethod
    def from_dict(cls: Type[T], src_dict: Dict[str, Any]) -> T:
        d = src_dict.copy()
        entity_or_inaccessible_resource = cls()

        entity_or_inaccessible_resource.additional_properties = d
        return entity_or_inaccessible_resource

After example

  class EntityOrInaccessibleResource:
        ...
    @classmethod
    def from_dict(cls: Type[T], src_dict: Dict[str, Any]) -> T:
        d = src_dict.copy()
        discriminator = d["resourceType"]

        if discriminator == "entity":
            entity_or_inaccessible_resource = EntityWithResourceType.from_dict(d)
        elif discriminator == "inaccessible":
	    entity_or_inaccessible_resource = InaccessibleResource.from_dict(d)
	else:
            raise ValueError(f"Unexpected discriminator value: {discriminator}")

        entity_or_inaccessible_resource.additional_properties = d
        return entity_or_inaccessible_resource

[alexrejto]

Just a minor style change requested. I'll let @bowenwr be the blocking reviewer on this.

[alexrejto]

Can we do this since the above line checks data.discriminator is not None?

Suggested change

	for k, v in (data.discriminator.mapping if data.discriminator else {}).items():
	for k, v in data.discriminator.mapping.items():

[straz]

oh, good catch, thanks!

[straz]

oh, good catch, thanks!

Well, hmm. I think I had that there because data.discriminator might be {"mapping": None}.

[dtkav]

If {"mapping": None} then this will still return None since the check is on data.discriminator right?
May be worth a closer look.

[straz]

I have something like this in mind:

from openapi_python_client.schema import Discriminator, Schema

data = Schema(discriminator=Discriminator(propertyName="resourceType"))

>>> data.discriminator is None
False
>>> data.discriminator.mapping is None
True
>>> [(k,v) for k,v in data.discriminator.mapping.items()]
*** AttributeError: 'NoneType' object has no attribute 'items'

[dtkav]

Your intent makes sense.
To make your code match your intent I think you need to change the code from:

(data.discriminator.mapping if data.discriminator else {}).items():

to:

(data.discriminator.mapping if data.discriminator.mapping is not None else {}).items():

or:

(data.discriminator.mapping or {}).items():

alternatively you could replace both checks with EAFP

try:
    for k, v in data.discriminator.mapping.items():
        discriminator_mappings[k] = Reference.from_ref(v)
except AttributeError:
    # Either data.discriminator or data.discriminator.mapping was not set
    pass

[straz]

You are right indeed, thanks! hopefully fixed this.

[dtkav]

The timing of merging this PR needs to be synchronized with the merge of the aurelia PR.

It just needs to be landed first right? Or is there some other dependency I'm missing.

[bowenwr]

Could you please update this PR and/or the related aurelia PR with an example of before/after for code gen? It helps immensely to understand the change and its possible implications.

It might be in Slack already but there are some looooong threads in there and I'd rather centralize PRs are the source of truth.

[bowenwr]

Adding context from Data Interfaces Office hours today in case this doesn't land before I'm OOO.

The core requirement is that when a user accesses a discriminated class, they receive the concrete discrimated type (and not a composite type).

One suggested implementation is that union classes shouldn't exist as class but as Union types.

So ideally something like:

InaccessibleResource = Union[Entity, InaccessibleResource]
Entity = Union[AaSequence, CustomEntity, DnaSequence, ...]

When deserialized in the SDK, you would receive the concrete class.

So as a user I would never receive a InaccessibleResource, I would receive a CustomEntity or InacessibleResource

The main impediment is here is that it breaks compatibility, as some users have resorted to their own type detection for classes like Entity to try to make it work. Although we could say this is unsupported and are willing to "break" the workaround, there's hesitation to do so because of previous customer pushback on "broken" releases.

I suggested that the main priority is satisfying the requirement. We should time box a compatibility investigation to see if there's some small, reasonable thing we might be able to do could help here.

Finally, get sign off for dev advocacy on any "breaking" changes, even if the breakage affects workarounds.