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

_save_kwargs inside of a recipe definition overridden #308

Closed
jeremy-engel opened this issue Apr 6, 2022 · 6 comments
Closed

_save_kwargs inside of a recipe definition overridden #308

jeremy-engel opened this issue Apr 6, 2022 · 6 comments
Assignees

Comments

@jeremy-engel
Copy link

As a result of #292, when certain attributes are passed as arguments to Recipe, they are overridden by defaults before the recipe is used to create the object.

Expected behavior

When using a recipe such as my_recipe = Recipe("MyModel", _save_kwargs={"foo": "bar"}), by running baker.make_recipe(my_recipe), the kwargs for the model's save should include the key foo with value bar.

Actual behavior

The kwargs are empty using this recipe. This contrasts with my_recipe_2 = Recipe("MyModel"), where by running baker.make_recipe(my_recipe_2, _save_kwargs={"foo": "bar"}), the correct kwargs are used.

Reproduction Steps

Below are updates to the project test files demonstrating the issue.

# tests.generic.models

class ModelWithSaveKwargs(Dog):
    def save(self, *args, **kwargs):
        print(kwargs)
        self.breed = kwargs.pop("breed")
        return super(ModelWithSaveKwargs, self).save(*args, **kwargs)

# tests.generic.baker_recipes

with_save_kwargs = Recipe("generic.ModelWithSaveKwargs", _save_kwargs={"breed": "updated_breed"})

# tests.test_recipes.TestExecutingRecipes

    def test_pass_save_kwargs_in_recipe_definition(self):
        dog = baker.make_recipe("tests.generic.with_save_kwargs")
        assert dog.breed == "updated_breed"

And as a patch:

diff --git a/tests/generic/baker_recipes.py b/tests/generic/baker_recipes.py
index f4e27c7..a8aebc8 100644
--- a/tests/generic/baker_recipes.py
+++ b/tests/generic/baker_recipes.py
@@ -113,6 +113,8 @@ movie_with_cast = Recipe(
 
 overrided_save = Recipe("generic.ModelWithOverridedSave")
 
+with_save_kwargs = Recipe("generic.ModelWithSaveKwargs", _save_kwargs={"breed": "updated_breed"})
+
 ip_fields = Recipe(
     "generic.DummyGenericIPAddressFieldModel",
     ipv4_field=seq("127.0.0.", increment_by=2),
diff --git a/tests/generic/models.py b/tests/generic/models.py
index 0acf214..c3248cb 100755
--- a/tests/generic/models.py
+++ b/tests/generic/models.py
@@ -226,6 +226,12 @@ class ModelWithOverridedSave(Dog):
         return super(ModelWithOverridedSave, self).save(*args, **kwargs)
 
 
+class ModelWithSaveKwargs(Dog):
+    def save(self, *args, **kwargs):
+        self.breed = kwargs.pop("breed")
+        return super(ModelWithSaveKwargs, self).save(*args, **kwargs)
+
+
 class Classroom(models.Model):
     students = models.ManyToManyField(Person, null=True)
     active = models.BooleanField(null=True)
diff --git a/tests/test_recipes.py b/tests/test_recipes.py
index 45d8780..ab5163c 100644
--- a/tests/test_recipes.py
+++ b/tests/test_recipes.py
@@ -332,6 +332,10 @@ class TestExecutingRecipes:
         )
         assert owner == dog.owner
 
+    def test_pass_save_kwargs_in_recipe_definition(self):
+        dog = baker.make_recipe("tests.generic.with_save_kwargs")
+        assert dog.breed == "updated_breed"
+
     def test_ip_fields_with_start(self):
         first, second = baker.make_recipe("tests.generic.ip_fields", _quantity=2)

Versions

Python: 3.10.4
Django: 4.0.3
Model Bakery: 1.5.0

@The-Compiler
Copy link

I'm seeing the same with _create_files=True as well, bisected to #292 as well (to be more exact, this commit: Better type hints for model_bakery.recipe by @hoefling · Pull Request #292 · model-bakers/model_bakery).

With this recipe, I now get:

...
  File "/srv/www/studentenportal/apps/documents/models.py", line 159, in exists
    return os.path.exists(self.document.path)
  File "/usr/local/lib/python3.7/site-packages/django/db/models/fields/files.py", line 58, in path
    self._require_file()
  File "/usr/local/lib/python3.7/site-packages/django/db/models/fields/files.py", line 40, in _require_file
    raise ValueError("The '%s' attribute has no file associated with it." % self.field.name)
ValueError: The 'document' attribute has no file associated with it.

amureki added a commit that referenced this issue Jun 30, 2022
PR #292 introduced explicit defaults for `make` and `prepare`,
which were then overwriting custom values in certain cases (explained in #308).

This PR attempts to fix this issue by not having explicit defaults.
@amureki
Copy link
Collaborator

amureki commented Jun 30, 2022

Hey @jeremy-engel,

Thanks for the great issue report with the test patch! That helped a lot during the investigation. And I am sorry it took us so long to process the report.

@The-Compiler and thank you for sharing another failing case!

I came up with a draft solution in #330, which is not quite elegant (given my lack of experience with typed things).

I'd love to hear your opinions here.

@model-bakers/core please, take a look as well, maybe you have some ideas on improving this piece.

Best,
Rust

@amureki
Copy link
Collaborator

amureki commented Aug 2, 2022

@jeremy-engel @The-Compiler hey! It took us a while, but we released a new version that is addressing this issue.

Please, at your convenience, take a look at 1.7.0 and check if this solves everything for you!

@jeremy-engel
Copy link
Author

This release takes care of it for me. Thanks!

@The-Compiler
Copy link

I'm a bit late to the party as well, but indeed I just upgraded from 1.4.0 to 1.7.0 without any issues - thanks, @amureki!

@amureki
Copy link
Collaborator

amureki commented Sep 30, 2022

Fantastic, thanks everyone, for the cooperation! 💯

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

No branches or pull requests

3 participants