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.

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

Isolate Recipe defaults to prevent modification via instances #509

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gildegoma
Copy link

Describe the problem

If a recipe defines default values for list-based or dict-based fields (such as JSONField or ArrayField), the objects built from this recipe directly refer to these recipe defaults instead of holding their own copy of the data.

Therefore, any modifications on these fields affect the recipe "parent" attribute and all depending instances. In my opinion/experience, this behavior is unexpected and error prone.

Note: For other data types, the isolation between recipe and instances is already guaranteed, which is also an argument to apply the same behavior in all cases.

Describe the proposed change

With this change, every baked instance holds an isolated copy of the recipe default attributes of any Container class (e.g., dict or list). This prevents from affecting the parent recipe, and other (existing or future) instances, when such fields are modified on an instance.

Note: The intent of this PR is trying to be "as clear as possible", without claiming to have properly solved the problem itself. The resolving implementation proposed is my "best bet", but it might be quite naive, and could likely miss important elements. Please feel free to pick what you like and create a new PR that would supersede it (especially if you feel you can fix it quickly without much coordination/communication overhead here 😉).

Workaround

With the current situation, it is possible to use a Callable as workaround to provide an immutable default value for list-based or dict-based attributes:

def get_data():
   return {"one": 1}

my_recipe = Recipe(MyModel, data=get_data, ...)

This solution is but not convenient as it requires awareness of the recipe behavior (and additional code).

PR Checklist

  • Change is covered with tests
  • CHANGELOG.md is updated if needed -> to be discussed in PR review

With this change, every baked instance holds an isolated copy of the
recipe default attributes of any Container class (e.g., dict or list).
This prevents from affecting the parent recipe, and other (existing or
future) instances, when such fields are modified on an instance.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant