[BUG] v1.2.1 breaks usage of baker.make(related_model__field=value)

[cb109]

Specifying field values of related model instances within the baker.make() signature (e.g. baker.make(my_related_model__field=value) is not possible anymore with v1.2.1. This still works in v1.2.0.

Expected behavior

The model instance AND its required related model instances should be created, populated with random defaults except the values we explicitly passed in the call.

Actual behavior

ValueError: Cannot assign "<fk-id>": "<model>.<fk-field>" must be a "<fk-model>" instance.

Reproduction Steps

Try creating a model instance with baker.make() using related field assignments for an FK field that has a default value/callable returning an ID as seen in the following example. Any related models will do, these are just for illustration:

# models.py

def get_default_category():
    """ 
    Returning an ID here will make model_bakery fail, returning an instance is fine, but see:
    https://docs.djangoproject.com/en/3.1/ref/models/fields/#django.db.models.Field.default 
    """
    return 1


class SpotCategory(models.Model):
    name = models.CharField(max_length=128)


class TVSpot(models.Model):
    category = models.ForeignKey(SpotCategory, on_delete=models.CASCADE, default=get_default_category)


# tests.py

class TestBugWithModelBakeryVersion_1_2_1:

    def test_creating_related_models_separately_works(self, db):
        """This seems to always work, no matter the version of model_bakery."""

        category = baker.make("SpotCategory", name="Special")
        baker.make("TVSpot", category=category)

    def test_specifying_related_model_fields_within_constructor_fails_with_v1_2_1(
        self, db
    ):
        """This worked up until 1.2.0, but fails with 1.2.1 as follows:

        ValueError: Cannot assign "1990": "TVSpot.category" must be a "SpotCategory" instance.

        """
        baker.make("TVSpot", category__name="Special")

Run tests with different model_bakery versions like:

$ pip install model_bakery==1.2.0 && pytest -k TestBugWithModelBakeryVersion_1_2_1  # should pass
$ pip install model_bakery==1.2.1 && pytest -k TestBugWithModelBakeryVersion_1_2_1  # should fail

Versions

OS: Ubuntu 20.04
Database: MySQL 5.7.32
Python: 3.8
Django: 2.2.17
pytest: 6.1.2
Model Bakery: 1.2.1

Maybe you do have an idea which of the changes in 1.2.1 is related to this. Let me know what I can do to help fix this, thanks.

[cb109]

Full pytest output:

===================================================================================================== FAILURES ======================================================================================================
__________________________________________________________ test_create_tvspot_with_specific_category __________________________________________________________

    def test_create_tvspot_with_specific_category(db):
>       assert baker.make(
            "TVSpot", category__name="Name"
        ).some_flag

msrcms/api/tests/test_misc.py:2274: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
../../.virtualenvs/venv/lib/python3.8/site-packages/model_bakery/baker.py:80: in make
    return baker.make(
../../.virtualenvs/venv/lib/python3.8/site-packages/model_bakery/baker.py:302: in make
    return self._make(**params)
../../.virtualenvs/venv/lib/python3.8/site-packages/model_bakery/baker.py:357: in _make
    instance = self.instance(
../../.virtualenvs/venv/lib/python3.8/site-packages/model_bakery/baker.py:388: in instance
    instance = self.model(**attrs)  # type: Model
msrcms/utils/__init__.py:479: in new_init
    new_init._original_init(self, *args, **kwargs)
../../.virtualenvs/cms/lib/python3.8/site-packages/django/db/models/base.py:483: in __init__
    _setattr(self, field.name, rel_obj)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <django.db.models.fields.related_descriptors.ForwardManyToOneDescriptor object at 0x7f73ec861070>
instance = <TVSpot: wAtkPGavzuyoryTgNxCgfaCiJMYlnRAESEHodYenUmcxldioNl | VgluBXBRPnczSqzDcuenBJRnokEUIDBvYzhsUkcrflgMjKfLdiszTOaL...jyQgyBCwMzIYtuSRTCSopKqyDHrTqtEsFfWrRIruxvbdwfvnuMFXoCvfNwmlqxenAkNlswTkrgLFwdUTuUInBGcmvgBptoKczSidMeceFvdFTNXpdAiGBO>
value = 2586

    def __set__(self, instance, value):
        """
        Set the related instance through the forward relation.
    
        With the example above, when setting ``child.parent = parent``:
    
        - ``self`` is the descriptor managing the ``parent`` attribute
        - ``instance`` is the ``child`` instance
        - ``value`` is the ``parent`` instance on the right of the equal sign
        """
        # An object must be an instance of the related class.
        if value is not None and not isinstance(value, self.field.remote_field.model._meta.concrete_model):
>           raise ValueError(
                'Cannot assign "%r": "%s.%s" must be a "%s" instance.' % (
                    value,
                    instance._meta.object_name,
                    self.field.name,
                    self.field.remote_field.model._meta.object_name,
                )
E               ValueError: Cannot assign "2586": "TVSpot.category" must be a "SpotCategory" instance.

[berinhard]

Hi @cb109 thanks for opening this issue. I didn't have time to try to reproduce this and explore why this error is happening, but if you want to investigate here's a good starting point: the 1.2.1 changelog with the updates and links for the PRs.

[cb109]

Looking into it, the culprit is the baker.Baker.generate_value() method as changed via #116 / #117.

    def generate_value(self, field: Field, commit: bool = True) -> Any:
        is_content_type_fk = isinstance(field, ForeignKey) and issubclass(
            self._remote_field(field).model, contenttypes.models.ContentType
        )
-        if field.name in self.attr_mapping:
+        if field.has_default():
+            if callable(field.default):
+                return field.default()
+            return field.default
+        elif field.name in self.attr_mapping:
             generator = self.attr_mapping[field.name]
         elif getattr(field, "choices"):
             generator = random_gen.gen_from_choices(field.choices)
@@ -520,13 +545,12 @@ class Baker(object):
             generator = generators.get(field.__class__)
         elif field.__class__ in self.type_mapping:
             generator = self.type_mapping[field.__class__]
-        elif field.has_default():
-            return field.default
         else:
             raise TypeError("%s is not supported by baker." % field.__class__)

It changes the order of checks, checking for default first, and later for type_mapping. For my problem, swapping that order back to check type_mapping first and default afterwards solves my issue. It breaks two tests though:

• tests/test_baker.py::TestFillBlanksTestCase::test_fill_optional_with_default
• tests/test_extending_bakery.py::TestLessSimpleExtendBaker::test_string_to_generator_required

Will take a closer look tomorrow, maybe there is a compromise that satisfies all use cases.

[berinhard]

Thanks for the help @cb109! You've saved me a lot of investigation time =)

[cb109]

One more observation:

The problem in my case is that I'm using tvspot = baker.make("TVSpot", category__name="My Category") and the TVSpot.category field uses a default function that returns an ID, not an instance. See example:

def get_default_spotcategory_id():
    try:
        return SpotCategory.objects.get(name=settings.DEFAULT_SPOTCATEGORY_NAME).id
    except SpotCategory.DoesNotExist:
        return None
        
class TVSpot(models.Model):
    category = models.ForeignKey(
        "SpotCategory",
        null=True,
        default=get_default_spotcategory_id,
        on_delete=models.SET_DEFAULT,
    )

AFAIK it's what Django wants you to do:

https://docs.djangoproject.com/en/3.1/ref/models/fields/#django.db.models.Field.default

For fields like ForeignKey that map to model instances, defaults should be the value of the field they reference (pk unless to_field is set) instead of model instances.

model_bakery used to tolerate it, ends up with the ValueError above now. If I instead return a model instance in that default function, everything works.

[berinhard]

Ok @cb109 now we got into the real problem! For me the initial description was very odd because I'm also using 1.2.1 in my Django projects with "field lookups" for related model and it was working fine.

Now it's even easier for me to create a unit test reproducing this scenario! Thanks for the help =)

If you wanna go ahead and give yourself a try, I advise you to:

1. Create a new regression model with a less specific name as yours or add a new ForeignKey with a callable as the default value. Our test models are defined here;
2. Write a unit test reproducing this scenario (maybe a good place to write the test is near to this one);
3. Try to make it work;

If you find trouble with the third step, feel free to open a PR with the breaking test and I'll be glad to help you from there. Otherwise, it's possible that I can find some time in the weekend to fix this.

[cb109]

@berinhard Thanks for having a look at it. I gave it a first try, see #137. Reverted my failed fixing attempt, but was able to reproduce the issue as a test.

[yigor]

I found issue that might be related:
If field has explicit default, _fill_optional=True now fills it with that default value:

class Issue(models.Model):
    comment = models.TextField(default='', blank=True)

then in test

        issue = baker.make('Issue', _fill_optional=True)
>       assert issue.comment
E       AssertionError: assert ''

It wasn't the case before 1.2.1.
If you remove default='', the test passes.

[amureki]

@cb109 hey Christoph, thank you for raising this issue and providing a test case! This is very helpful and got me to the point of making a "dirty" draft fix:
#164

I still need to check if I am not missing anything there, add some code comments (as it becomes complicated) and maybe extend test cases.
I'd also love to get your opinion on the solution.

[cb109]

@amureki Thank you for working on this 😃 I think we are close! I have installed your branch locally and ran our tests against it: It seems that creating related model instances is now problematic, when those are optional and use default=None. I tried to reproduce that with a test, maybe you can try to double-check:

 # tests/generic/models.py
 
class DummyOptionalForeignKey(models.Model):
   named_thing = models.ForeignKey(
       "NamedThing",
       null=True,
       default=None,
       on_delete=models.SET_NULL,
   )


# tests/test_filling_fields.py

@pytest.mark.django_db
class TestFillingOptionalForeignKeyField:
   def test_filling_optional_foreignkey_implicitly(self):
       dummy = baker.make(models.DummyOptionalForeignKey, named_thing__name="Optional")
       assert dummy.named_thing.name == "Optional"

(It does not seem to matter whether we use CASCADE or SET_NULL here and there are other test models already that have optional FK fields, so maybe reusing one of those makes more sense)

The test above fails for me with your changes, as no NamedThing instance is created, although baker.make(..., named_thing__name="Optional") used to do that... Let me know if I can help with anything else.

EDIT: Fixed test class and method names.

[amureki]

@cb109 great, thanks for running your tests against your setup! This helps to discover cases for which we do not have tests yet. 👍

I am trying provided test case now, however it is also related to the original issue introduced in 1.2.1 (enforcing default) and has nothing to do with my draft change. I'll try to see how can we cover this case.

Update:
So far I see these test cases to be covered:

@pytest.mark.django_db
class TestFillingForeignKeyFieldWithDefaultFunctionReturningId:
    def test_filling_foreignkey_with_default_id(self):
        dummy = baker.make(
            models.DummyForeignKeyWithDefaultIdModel
        )
        assert dummy.named_thing.id == models.get_default_namedthing_id()
        assert dummy.named_thing.name == "Default"

    def test_filling_foreignkey_with_default_id_with_custom_arguments(self):
        dummy = baker.make(
            models.DummyForeignKeyWithDefaultIdModel, named_thing__name="Not default"
        )
        assert dummy.named_thing.__class__.objects.count() == 1
        assert dummy.named_thing.id == 1
        assert dummy.named_thing.name == "Not default"


@pytest.mark.django_db
class TestFillingOptionalForeignKeyField:
   def test_filling_optional_foreignkey_implicitly(self):
       dummy = baker.make(models.DummyOptionalForeignKey, named_thing__name="Optional")
       assert dummy.named_thing.name == "Optional"

[amureki]

@cb109 would you mind running your tests against 73ac0d3 (in #164, so it would be the same branch for you)?
I went in a different direction and put an extra check before applying a default value (this solution I also do not like, but we need to find a stable point and play from there...).

[cb109]

@amureki just pulled it and ran our custom tests, all is green ✔️ Well done! And so much more elegant!