Azure / autorest.python

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

Serialization `from_dict` modifies input dictionary #2799

Closed pvaneck closed 2 weeks ago

pvaneck commented 2 months ago

In the generated serialization.py file, if a user tries to call from_dict on a model that has _subtype_map values, those values are popped from the original dictionary. See here.

Here is a sample issue showcasing this behavior: https://github.com/Azure/azure-sdk-for-python/issues/37024

Here is also a simpler recreation:

from azure.search.documents.indexes._generated.models import VectorSearchAlgorithmConfiguration

data = {
    "name": "hnsw",
    "kind": "hnsw",
}
config = VectorSearchAlgorithmConfiguration.from_dict(algo)
print(data) # "kind" key will now be missing.

VectorSearchAlgorithmConfiguration has:

_subtype_map = {
    "kind": {"exhaustiveKnn": "ExhaustiveKnnAlgorithmConfiguration", "hnsw": "HnswAlgorithmConfiguration"}
}

I feel like the input dictionary should remain unmodified. Is mutating the input dictionary intentional?

iscai-msft commented 2 months ago

@pvaneck this might have been accidentally introduced bc we're optimizing for our new models, and with our new models we don't have anything like subtype_map. Let me take a look at this today

tadelesh commented 2 months ago

it is a same issue to this. but in that issue, we just fix the impact for lro operation, not fix the root problem. @msyyc do you remember why we need to pop the discriminator from the sub type map?

msyyc commented 2 months ago

We ever had discussion with Laurent https://github.com/Azure/autorest.python/pull/2430#issuecomment-1977701646 that it might bring risks if change the deserialization logic of msrest. Let me make up some complicated test to check it, if passed, I think we had better fix it directly like https://github.com/Azure/autorest.python/pull/2430/files

msyyc commented 1 month ago

@pvaneck We will fix it soon and to unblock your development, you could manually change .pop to .get of https://github.com/Azure/azure-sdk-for-python/blob/ee884a7cc7144b0cb1a67f1a2ff7761bbc9348e8/sdk/search/azure-search-documents/azure/search/documents/_generated/_serialization.py#L465 like https://github.com/Azure/autorest.python/pull/2430/files#diff-69fe784fcd0105d8481bda21d39c386910fee457412d6635db30af71de1ec642 if needed.

iscai-msft commented 2 weeks ago

@pvaneck this has been released, can you give it a try?