-
Notifications
You must be signed in to change notification settings - Fork 75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ser deser for discriminated union and polymorphic base #2169
Merged
qiaozha
merged 34 commits into
Azure:main
from
qiaozha:ser-deser-for-discriminated-union-and-polymorphic-base
Feb 1, 2024
Merged
Ser deser for discriminated union and polymorphic base #2169
qiaozha
merged 34 commits into
Azure:main
from
qiaozha:ser-deser-for-discriminated-union-and-polymorphic-base
Feb 1, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
qiaozha
requested review from
sarangan12,
joheredi and
MaryGao
as code owners
December 19, 2023 07:59
qiaozha
commented
Dec 20, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initial reviews
packages/typespec-test/test/openai_modular/generated/typespec-ts/src/utils/serializeUtil.ts
Show resolved
Hide resolved
packages/typespec-test/test/openai_modular/generated/typespec-ts/src/api/operations.ts
Outdated
Show resolved
Hide resolved
packages/typespec-test/test/openai_modular/generated/typespec-ts/src/utils/serializeUtil.ts
Outdated
Show resolved
Hide resolved
packages/typespec-test/test/openai_modular/generated/typespec-ts/src/utils/serializeUtil.ts
Show resolved
Hide resolved
packages/typespec-test/test/openai_generic/generated/typespec-ts/review/openai-generic.api.md
Outdated
Show resolved
Hide resolved
packages/typespec-test/test/openai_modular/generated/typespec-ts/src/rest/responses.ts
Show resolved
Hide resolved
packages/typespec-test/test/openai_modular/generated/typespec-ts/src/models/models.ts
Outdated
Show resolved
Hide resolved
qiaozha
commented
Dec 28, 2023
qiaozha
commented
Jan 2, 2024
joheredi
reviewed
Jan 2, 2024
...ages/typespec-test/test/loadtesting_modular/generated/typespec-ts/review/load-testing.api.md
Outdated
Show resolved
Hide resolved
joheredi
reviewed
Jan 2, 2024
joheredi
reviewed
Jan 2, 2024
joheredi
reviewed
Jan 2, 2024
joheredi
reviewed
Jan 2, 2024
joheredi
reviewed
Jan 2, 2024
packages/typespec-test/test/openai_modular/generated/typespec-ts/review/openai_modular.api.md
Show resolved
Hide resolved
qiaozha
commented
Jan 3, 2024
packages/typespec-test/test/openai_modular/generated/typespec-ts/review/openai_modular.api.md
Show resolved
Hide resolved
qiaozha
commented
Jan 22, 2024
joheredi
approved these changes
Jan 29, 2024
lirenhe
reviewed
Feb 1, 2024
lirenhe
approved these changes
Feb 1, 2024
I will merge this PR and use #2253 to track other non-problematic union serialize and deserialize work. Let me know if anyone has any concerns. |
qiaozha
deleted the
ser-deser-for-discriminated-union-and-polymorphic-base
branch
February 1, 2024 15:16
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #2139
fixes #1982
Summary
In this PR, we will support serialize/deserialize for discriminated union and polymorphic base, as we previously agree that we should not handle other unions because it's hard to predict the type as of the additional properties.
As such we will not generate predict function for unions, but we still need to distinguish which union variants are special, we only generate special handling logic for those special variants.
We also address the circular reference issue in this PR, as circular reference in inheritance and in combination could be handled differently
Special Union Variants / Subtypes
In general, we have two kinds of basic special union variants.
For model type, we have the following basic three types of special union variants.
3. model with property of datetime type.
4. model with property of binary array type.
5. model that has different property name between rest layer and modular layer.
To extend the combination, we get:
6. nested model i.e. model with property that is a model with one of the above 4-6 conditions.
7. model with property of special union.
8. An array type, with all the above 7 types as the element types.
9. A record type, with all the above 7 types as the value type. (TBD...)
Example for Discriminated Union
The typespec would be like
In this case, WidgetData0 is not a special union, but WidgetData1 is, we will not generate special serialize or deserialize logic for it. Our deserialize util would just be like
Example for Polymorphic Base
For simplicity, in OpenAI's case, we have typespec like
In this case, ChatMessageTextContentItem is not a special union variant, but ChatMessageImageContentItem is, our serialize util would be like
Circular Reference Issue
This is for typespec like
In this case, Gold has a property which is type of his ancestors, but the overall model doesn't have anything special subtype, we will not generate the serialize and deserialize utils, if Gold has a birthday property of utcDateTime type, then we should generate the serialize utils for it.
It is possible that a discriminated union can also have circular reference, the logic should be the same, but ut TBD...
This is for typespec like
In this case, Foo has a property point to Bar and Bar also has a property point to Foo, as this is out of the scope of discriminated union and polymorphic base, the current logic is, we will just detect if these models has anything special, if it's not, we will just give whatever we get to the Modular layer if it's for deserialize and to the Rest layer if it's for serialize. otherwse, we will generate as any. a case in point is ChatMessage in OpenAI's case.
LRO and Paging dependency issue
While integrate the latest OpenAI's typespec, I found they have a LRO operation that is defined in their typespec, but is removed in their typespec, which cause us need to generate LRO helper in RLC, but doesn't need to generate the LRO in Modular, as we currently don't really split RLC from Modular, we need to take both into consideration about whether we have a core-lro or core-paging dependency.