Model.resolved mutates shared discriminator field across sibling models via dict.update reference copy #646
Open
gravithex wants to merge 1 commit intopython-restx:masterfrom
Conversation
…iminator mutation
This file contains hidden or 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
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.
Describe the bug
When two or more models inherit from a common parent that defines a discriminator field, all sibling models end up sharing the same String field instance for the discriminator. Each model mutates that shared instance when its
.resolvedis computed, so whichever sibling is resolved last overwrites the default for all others.This is distinct from (but related to) #314, which describes a stale value on the second call. This issue causes the discriminator to be wrong on subsequent calls, once both sibling models have had their
.resolvedcomputedTo reproduce
Expected behavior
class should be "LegalEntity" when marshalling a LegalEntity instance.
Root cause
The bug is in
RawModel.resolved(model.py):dict.updatecopies values by reference. Sinceparent.resolvedis a@cached_property(always the same object), every child's resolved dict holds a reference to the sameString(discriminator=True)instance from the parent.Then
candidates[0].default = self.namemutates that shared instance. The last sibling to call.resolvedsets the default for all of them.Proposed fix
Deep copy the parent fields before merging, so each child model owns a private copy before mutating it:
This is more targeted than the approach in PR #359 (replacing
@cached_propertywith@property), which fixes the symptom by disabling caching entirely at the cost of recomputing a deepcopy on every marshal call. The proposed fix here preserves the@cached_propertyperformance benefit while eliminating the shared mutation at its source.Environment