Azure / autorest.python

Extension for AutoRest (https://github.com/Azure/autorest) that generates Python code
MIT License
79 stars 57 forks source link

deserialization for `Union` about DPG model #2009

Closed msyyc closed 10 months ago

msyyc commented 1 year ago

using TypeSpec.Http;

@doc("Illustrates inheritance and polymorphic model.") @scenarioService("/type/model/inheritance") namespace Test.Union;

@doc("Cat") model Cat { eatFish: boolean; }

@doc("Dog") model Dog { eatBone: boolean; }

@doc("Animal") model Animals { content: Cat[] | Dog[]; }

@route("/test/union") interface Discriminated { @scenario @scenarioDoc("test") @get getAnimal(): Animals; }


- Genereated SDK: https://github.com/msyyc/autorest.python/tree/test-union-backup/packages/typespec-python/alpha/generated_code

- [Scenario that error happened](https://github.com/msyyc/autorest.python/blob/test-union-backup/packages/typespec-python/alpha/generated_code/test.py):
![image](https://github.com/Azure/autorest.python/assets/70930885/d9ca72a2-5e5d-469d-8602-bc6fd22d83fb)

When a model property has Union type, Python doesn't report error when deserialization. But when users use returned model like `A.B`, python SDK meet error like up.

- root cause
DPG model will try each of union types to do deserialization until one succeeds. However, DPG model also support additional properties, so it always succeeds for first type.
msyyc commented 1 year ago

I think union is convenient for typespec definition but not good feature for SDK users. Reason: (1) It is very hard to deserialization when runtime for SDK. (2) Even if SDK can deserialize for Union type, SDK users still need to check whether specific property exists so that they can confirm the property type of returned model

lirenhe commented 1 year ago

Here is the real case that is used by OpenAI: https://github.com/Azure/azure-rest-api-specs/blob/main/specification/cognitiveservices/OpenAI.Inference/models/images.tsp#L85

iscai-msft commented 10 months ago

closing because this will be solved during tcgc adoption #2266