django_get_or_create with SubFactory

[VRohou]

Description

The django_get_or_create feature is working well when the concerned Factory is used as a BaseFactory but not when it's used as a SubFactory
When used as a SubFactory, the following error occurs :
AttributeError: type object 'CurrencyFactory' has no attribute '_original_params'

To Reproduce

With the below code, the error will occur randomly when the same currency code/name is used.

To reproduce it systematically, just change the PairFactory with the following line :

base_currency = factory.SubFactory(CurrencyFactory, name="Bitcoin")

It will force the IntegrityError in the Currency Factory, so it will try the get_or_create feature.
And then call the PairFactory() two times and voila.

E           AttributeError: type object 'CurrencyFactory' has no attribute '_original_params'

../../../.cache/pypoetry/virtualenvs/belfort-backend-KmGKt2wB-py3.10/lib/python3.10/site-packages/factory/django.py:151: AttributeError

Model / Factory code

# Models
class Currency(models.Model):
    name = models.CharField(max_length=100, unique=True)
    symbol = models.CharField(max_length=20, unique=True)

class Pair(models.Model):
    base_currency = models.ForeignKey(
        Currency, on_delete=models.CASCADE, related_name="pairs_as_base"
    )
    quoted_currency = models.ForeignKey(
        Currency, on_delete=models.CASCADE, related_name="pairs_as_quoted"
    )

# Factories
class CurrencyFactory(factory.django.DjangoModelFactory):
    class Meta:
        model = "Currency"
        django_get_or_create = ("name", "symbol")

    name = factory.Faker("cryptocurrency_name")
    symbol = factory.Faker("cryptocurrency_code")

class PairFactory(factory.django.DjangoModelFactory):
    class Meta:
        model = "Pair"

    base_currency = factory.SubFactory(CurrencyFactory)
    quoted_currency = factory.SubFactory(CurrencyFactory)

Notes

I believe this is linked to the _generate() method which is not called for SubFactory, while the _original_params is set only in this method.

[rbarrois]

That's surprising, the error message you mention doesn't seem to match the existing codebase (neither master or 3.2.1 use _original_params on line 151). Which version are you using?

[VRohou]

I'm using the 3.2.1 version.
The line is different because I added some logs to try to understand what was going on. It allowed me to see that the _generate method wasn't called for Subfactory.
Sorry for the confusion

[foarsitter]

In my project a model has an unique char field of two characters. The factory fills this fields with a factory.Faker("pystr", min_chars=2, max_chars=2), there comes the non-determinism from. Generating enough cases will raise an IntegrityError(duplicate key value violates unique constraint) and the missing _original_params story. But the error is not related to the django_get_or_create column.

[foarsitter]

Ok, took some time to provide a failing testcase (forgive me for the bad naming).

Do you agree that the problem can be solved by initiating the class attribute _original_params as an empty dict {}?

[francoisfreitag]

The test case exhibits the problem well, thank you, it’s very helpful! 🎉

Do you agree that the problem can be solved by initiating the class attribute _original_params as an empty dict {}?

I do not agree. _original_params distinguishes the user lookup (for use in .get()) from the values generated by the factory (for use in .create()). See #606 for history.
Setting it to an empty dict isn’t correct, as the lookup from #981 is {"is_bool_true": True}. With an empty dict, Model.objects.get() would eventually be called, which has a strong chance of raising a MultipleObjectsReturned (without filters, there are likely more than a single object returned).
Saving the lookup requires deeper changes to the library, to preserve where the kwarg should be used for the lookup (when calling the factory, or the kwargs from the definition as in #981) or if it’s a value generated by the factory for use in create(). I don’t see an obvious point to do that in the library now, we may need to add something, but requires deeper investigation.

[foarsitter]

Thanks for providing some context! Checking for None will be the fix over here then at this moment I think. Raising the IntegrityError gives the user enough information to fix it I suppose (at least it would have give me).