Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix deserialization subtype #2430

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Fix deserialization subtype #2430

wants to merge 9 commits into from

Conversation

msyyc
Copy link
Member

@msyyc msyyc commented Mar 4, 2024 •
edited
Loading

fixes #2799

.pop logic is introduced in https://github.com/Azure/msrest-for-python/pull/12/files#diff-c75745c963d37c430993b85e7d95b87db2c0baa4b284389fc839d8fb7d3fdfe1 while I don't find any context why it is needed to remove the polymorphic key

@msyyc msyyc marked this pull request as ready for review March 4, 2024 03:59
@iscai-msft
Copy link
Contributor

@msyyc can you verify with @lmazuel? this code is changing msrest code that has been here forever. thanks

@msyyc
Copy link
Member Author

msyyc commented Mar 5, 2024

After discussion, we need more test for the fix to confirm it pass all scenarios

@msyyc msyyc changed the title Fix deserialization subtype Fix deserialization subtype (Hold on) Mar 5, 2024
@msyyc msyyc closed this Apr 26, 2024
@msyyc msyyc deleted the fix-deserialization-subtype branch August 30, 2024 01:12
@msyyc msyyc mentioned this pull request Aug 30, 2024
Open
@msyyc msyyc restored the fix-deserialization-subtype branch August 30, 2024 02:00
@msyyc msyyc reopened this Aug 30, 2024
msyyc
msyyc commented Sep 2, 2024
View reviewed changes
packages/autorest.python/test/unittests/test_serialization.py
self.assertEqual(animal.name, "Didier")

# deserialize must not change original data
self.assertEqual(message["dType"], "Animal")

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a complicated test which contains nested polymorphic however autorest.python can't handle that scenario and I already created #2804 to track. Now that no users report similar issue, we don't need to consider that complicated scenarios.
For simple scenarios, changing pop to get has no influence for deserialization so I think it is ok to review now.

@msyyc msyyc changed the title Fix deserialization subtype (Hold on) Fix deserialization subtype Sep 2, 2024
@iscai-msft
Copy link
Contributor

@msyyc have you been able to run this change through the pipeline that you created to test current sdks?

@msyyc
Copy link
Member Author

msyyc commented Sep 4, 2024 •
edited
Loading

@msyyc have you been able to run this change through the pipeline that you created to test current sdks?

SDK regeneration pipeline is mainly for typespec-python since it only regenerates for package which contains tsp-location.yaml while this feature is about msrest model deserialization which is for legacy package (most are mgmt SDK) generated with autorest.python. And I look through all the mgmt SDK that there is no complicated nested-polymorphic-discriminator scenarios.

@tadelesh
Copy link
Member

tadelesh commented Sep 4, 2024

@msyyc could you try with azure.search.documents from the original ask: Azure/azure-sdk-for-python#37024.

@msyyc
Copy link
Member Author

msyyc commented Sep 5, 2024

@msyyc could you try with azure.search.documents from the original ask: Azure/azure-sdk-for-python#37024.

Already added in test case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iscai-msft iscai-msft Awaiting requested review from iscai-msft iscai-msft is a code owner

@tadelesh tadelesh Awaiting requested review from tadelesh tadelesh is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Serialization from_dict modifies input dictionary
3 participants
@msyyc @iscai-msft @tadelesh