FactoryBoy / factory_boy

A test fixtures replacement for Python
https://factoryboy.readthedocs.io/
MIT License
3.52k stars 397 forks source link

Circular DjangoModelFactory RelatedFactory instances are not created #782

Open incidentist opened 4 years ago

incidentist commented 4 years ago

Description

I have two Django models that refer to eachother. StudentPhone has a ForeignKey relationship to Student, and a Student has a primary_phone field that refers to a StudentPhone. I want a StudentFactory that uses StudentPhoneFactory to create its primary_phone field. This is exactly the scenario shown in the example https://factoryboy.readthedocs.io/en/latest/reference.html#factory.RelatedFactory . With the following models.py and test.py, the assert at the end fails.

models.py:

from django.db import models

class Student(models.Model):
    email = models.EmailField(blank=True)
    primary_phone = models.ForeignKey(
        "StudentPhone",
        on_delete=models.SET_NULL,
        null=True,
        related_name="primary_students",
    )

class StudentPhone(models.Model):

    NUMBER_TYPES = (
        ("mobile", "Mobile"),
        ("home", "Home"),
    )

    student = models.ForeignKey(
        "Student", related_name="phones", on_delete=models.CASCADE
    )
    type = models.CharField(max_length=16, choices=NUMBER_TYPES)
    is_valid = models.BooleanField(default=True)

tests.py:

import factory
from factory import post_generation
from factory.django import DjangoModelFactory

from main import models

# Create your tests here.
class StudentPhoneFactory(DjangoModelFactory):
    class Meta:
        model = models.StudentPhone

    student = factory.SubFactory("test.factories.StudentFactory")
    type = "mobile"
    is_valid = True

class StudentFactory(DjangoModelFactory):
    class Meta:
        model = models.Student

    email = factory.Faker("email")
    primary_phone = factory.RelatedFactory(
        StudentPhoneFactory, factory_related_name="student"
    )

s = StudentFactory()
assert(s.primary_phone is not None)

Django 3.0.10 FactoryBoy 3.0.1

To Reproduce

See the attached django app. Run ./manage.py test main. The assert should succeed but it fails. factorybug.zip

incidentist commented 4 years ago

Never mind. A duplicate of #684. I made a RelatedFactory when I should have made a SubFactory. Except I'm trying to pass the parent object to the SubFactory (not an attribute, but the whole object) and it's not clear how to do that, whereas it's quite clear how to do this with a RelatedFactory.

Hmm, after some investigation it seems that this is not possible. It is fine if the primary_phone attribute is set after the Student object is created. I guess I don't understand why running the RelatedFactory after the parent object creation results in the attribute being None.

It's also not clear from documentation that this is not possible, in either the CityFactory/CountryFactory example or the UserFactory/GroupFactory example. Because neither of those examples provide the class definition that the factories are based on, it is hard to tell.

rbarrois commented 4 years ago

Hey, thanks for the very detailed report!

This is indeed a common use case, tricky to build with factory_boy; and, as you've pointed, the docs could be improved.

For such complex cases, I always find it easier to start by thinking in terms of pure Django code; once the sequence of calls is clear, I then add them to the factory. Here, I guess you would go like this:

  1. student = Student.objects.create(primary_phone=None)
  2. phone = StudentPhone.objects.create(student=student)
  3. student.primary_phone = phone; student.save()

The important bit is that third step; there are a few options to handle it:

In the Django model

You could decide that, whenever a StudentPhone is created, if its student doesn't have a primary_phone, it gets attached:

class StudentPhone(models.Model):
    # Definitions here
    def save(self, *args, **kwargs):
        result = super().save(*args, **kwargs)
        if self.student.primary_phone is None:
            self.student.primary_phone = self
            self.student.save()
       return result

In the factory

We've discovered that the phone should be attached to the student once the phone has been created. For that task, the right FactoryBoy tool is a post-generation declaration:

class StudentPhoneFactory(DjangoModelFactory):
    # Usual declarations here
    @factory.post_generation
    def set_primary_phone(self, obj, *args, **kwargs):
        if self.student.primary_phone is None:
            self.student.primary_phone = self
            self.student.save()

Mixed, through a helper

Maybe you have a helper on StudentPhone to mark it as a primary? In that case, the factory code can be simplified:

class StudentPhone(models.Model):
    ...
    def set_primary(self):
        self.student.primary_phone = self
        self.student.save()

class StudentPhoneFactory(DjangoModelFactory):
    ...
    # Always set ourselves as the primary phone of the user
    set_primary = factory.PostGenerationMethodCall('set_primary')

However, with this last option, the latest StudentPhoneFactory call would always be defined as the primary phone.

incidentist commented 4 years ago

This is very helpful, thank you. I went with option 2, and it's working great.