fix(http-client-python): do not precompute Model class as XML deserializer#10891
fix(http-client-python): do not precompute Model class as XML deserializer#10891l0lawrence wants to merge 1 commit into
Conversation
…lizer A downstream customization that subclassed a generated Model and re-annotated a nested Model field (e.g. `azure.storage.fileshare._models.Metrics` re-annotating `retention_policy: RetentionPolicy` with a custom `RetentionPolicy` subclass) caused the shared `_RestField._type` on the parent (generated) class to be populated from the custom annotation. `_build_xml_field_plan` then precomputed the custom Model class as the deserializer for the parent class too, and `_init_from_xml` invoked `CustomModel(<ET.Element>)` directly — raising if the customization's `__init__` enforces a strict signature. Stop precomputing Model and Optional[Model] fields in the XML field plan; defer to the existing `_deserialize` fallback, which tolerates the polluted `rf._type` via `_deserialize_default`'s exception swallowing. Scalar deserializer precomputation (the main perf win of microsoft#10698) is preserved. Includes a regression test that reproduces the azure-sdk-for-python pattern from issue trace. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
commit: |
|
All changed packages have been documented.
Show changes
|
|
You can try these changes here
|
iscai-msft
left a comment
There was a problem hiding this comment.
what's the perf regression by no longer precomputing the model class?
|
Hi @@l0lawrence. Your PR has had no update for 14 days and it is marked as a stale PR. If it is not updated within 14 additional days, the PR will automatically be closed. If you want to refresh the PR, please remove the |
Regression from #10698.
When the customized class is instantiated first,
Model.__new__walks the MRO and setsrf._typefrom the last-wins annotation (the custom class). The_RestFieldis shared with the generated parent, so the parent'srf._typeends up pointing atpartial(_deserialize_model, CustomRetentionPolicy)._build_xml_field_planextracts that and storesCustomRetentionPolicyas the precomputeddeserfor the generated parent's field plan._init_from_xmlthen callsCustomRetentionPolicy(<ET.Element>)directly, the element is bound positionally toenabled(truthy),daysis None, and you get theValueError.Before #10698 the same path went through
_deserialize->_deserialize_default, whoseexcept Exception: passswallowed the ValueError and returned the rawET.Elementfor the consumer's_from_generatedshim to handle.Fix: stop precomputing Model and
Optional[Model]fields in_build_xml_field_plan. Scalarrest_field(deserializer=...)callables are still precomputed. Model fields fall back to_deserialize(rf._type, item)like before. Also removed the now-unused_extract_xml_model_typehelper.Added a regression test that reproduces the pattern. It fails on current code and passes with the fix. Two existing tests that were also broken by the same precomputation (
test_polymorphic_deserialization,test_enumeration_results_blobs_unwrapped) start passing as a side effect. All 375 unit tests pass after regen.