FactoryBoy / factory_boy

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

Design issue: Post-generation declaration and overrides #359

Open rbarrois opened 7 years ago

rbarrois commented 7 years ago

As part of an ongoing refactor of factory_boy's core engine (which should remove lots of quirks and asymmetry between different kinds of declarations), we need to decide how to handle a specific case with post-generation declaration.

Context

Suppose we have the following factory:

class UserFactory(factory.Factory):
    class Meta:
        model = User

    username = factory.Faker('user_name')
    # For some reason, we have to set the password *after* creating the user
    password = factory.PostGenerationMethodCall('set_password', 'password123')

    @factory.post_generation
    def groups(self, create, extracted, **extra):
        targets = extracted if extracted is not None else Group.objects.all()[:3]
        for group in targets:
            self.group_set.add(group)

With the current code, a user may do:

UserFactory(password='test')  # Uses 'test' as the password
UserFactory(groups=[some_group])  # Uses the given groups

On the other hand, when typing:

UserFactory(
    password=factory.PostGenerationMethodCall('set_unusable_password')
)

One would expect that this changes the password generation to call set_unusable_password, instead of trying to use the PostGenerationMethodCall object as a password (unlikely to work :wink:).

Issue

This means that post-generation declarations aren't handled as pre-generation are:

This means that:

Options

I see a few options to address this point:

  1. Keep going as presently done
  2. Use a specific marker to pass a parameter to the post-generation hook, e.g UserFactory(password__='test')
  3. Require that all post-generation methods accept keyword parameters, and pass options as such: UserFactory(password__cleartext='test')
rbarrois commented 7 years ago

Other issues with the current design:

RelatedFactory

If the factory has a related factory (e.g profile = factory.RelatedFactory(ProfileFactory, 'user'), what should happen when the user calls UserFactory(profile=a_profile):

PostGenerationMethodCall

When a method accepts several positional args, how should we handle them?

For instance, with a field password = factory.PostGenerationMethodCall('set_password', 'pwd123', 'pbkdf2'):

rbarrois commented 7 years ago

After a couple of discussions with factory_boy users, the idea is to go as follow:

Decisions

The chosen options from the above list of choices are:

Remaining questions

How should we handle post-declaration/pre-declaration relations?

With the above factory, when the user calls UserFactory(password=factory.SelfAttribute('username')), should we:

  1. Crash: they are trying to shadow the value, won't happen
  2. Replace the post-declaration with the pre-declaration (i.e call User(username='john', password='john'); not what they would expect
  3. Resolve the pre-declaration in the context of the UserFactory, and pass the generated value to the post-declaration
  4. Resolve the pre-declaration in the context of the parameters passed to the PostGenerationMethodCall

If we go with option 3, does this work with other post-generation declarations?

bors-ltd commented 7 years ago

Not sure this will help you but as a user and consumer of factories written by others, I would have typed:

user = UserFactory()
user.set_unusable_password()

in my test code.

I didn't know about PostGenerationMethodCall (or never paid attention) but this writing is straightforward to read and doesn't need to remember all of the factory-boy API for every case.

rbarrois commented 7 years ago

@bors-ltd yes, it works well ;)

However, it doesn't scale if you want to write, for instance, a DisabledUserFactory

volksport commented 6 years ago

If I can throw in my opinion: I'd like to be able to use a SubFactory and/or RelatedFactory in such a way that when I pass a value to the factory (e.g. UserFactory(profile=a_profile)) the SubFactory and/or the RelatedFactory is not called at all, and I won't have to override it in the post_generation or anywhere else.

I don't see a reason why it should be called at all if I'm passing the value. This is creating a situation where extra unintended objects are being written to the db.

stevelacey commented 6 years ago

You can support passing a raw_password to factory_boy like so:

class UserFactory(Factory):
    password = factory.PostGeneration(lambda user, create, extracted: user.set_password(extracted))

The create arg is irrelevant here, that simply specifies whether you're using the build or create strategy – set_password doesn't save so it doesn't matter which

The extracted arg is what was passed to the factory call – by default this is None so you end up with an unusable password – which is probably fine as a default

In [1]: UserFactory().has_usable_password()
Out[1]: False

In [2]: UserFactory(password='test').has_usable_password()
Out[2]: True

In [3]: UserFactory(password='test').check_password('test')
Out[3]: True

In [4]: UserFactory(password='test').password
Out[4]: 'argon2$argon2i$v=19$m=512,t=2,p=2$blahblahblah'

To clarify, the reason you have to set the password in PostGeneration is because up until that point (e.g. when LazyAttribute's are evaluated) – factory_boy is working with a user stub – not a user object – so the set_password method is not available – the stub is later used to build the model instance – this design is to support building things other than model instances (like dicts)