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

RelatedFactory created even when passing None kwargs #62

Closed
gbittoun opened this Issue May 23, 2013 · 7 comments

Comments

Projects
None yet
3 participants
@gbittoun

gbittoun commented May 23, 2013

There is no way to avoid the creation of an object via RelatedFactory even when this related param is set to None via kwargs in factory instantiation. Example:

** Factory file **

MyFactory(factory.DjangoModelFactory):
param1 = factory.RelatedFactory('model_file.MyModel')

** Test file **

obj = MyFactory(param1=None) # param1 exists in db

Regards,

@Dhekke

This comment has been minimized.

Show comment
Hide comment
@Dhekke

Dhekke Jun 1, 2013

I'm facing the same issue... And I'm currently having to resort to some weird stuff using factory.post_generation.
Seems to me there should be a check in base.Factory._generate for RelatedFactory instances, since any other postgen should be called even if we pass a value (right?), something like:

for name, decl in sorted(postgen_declarations.items()):
    extracted, extracted_kwargs = postgen_attributes[name]
    # If I passed any value for a RelatedFactory, don't call the RelatedFactory
    if extracted is not None and isinstance(decl, RelatedFactory):
        continue
    decl.call(obj, create, extracted, **extracted_kwargs)

extracted is None if no argument is passed, so we can't check against it. Maybe PostGenerationDeclaration could return False instead of None, since the extracted value is supposed to always be an object anyway (at least I think so), then extracted == None would mean we don't want the factory to create an object.

But considering that we could want to send an object instead of asking the factory not to create one, maybe the check should be in the RelatedFactory.call method, also adding a barebones way of setting the attribute if we pass an object like (using @gbittoun example) obj = MyFactory(param1=<model_file.MyModel instance>)

Something like:

def call(self, obj, create, extracted=None, **kwargs):
    passed_kwargs = dict(self.defaults)
    passed_kwargs.update(kwargs)
    if self.name:
        passed_kwargs[self.name] = obj

    # If we passed an object of type I'm supposed to create, just set the attribute and save if we have to
    if isinstance(extracted, self.get_factory().FACTORY_FOR):
        setattr(extracted, self.name, obj)
        if create:
            extracted.save()
    # If we passed no other value, then we can generate the object using the factory
    elif extracted is None:
        self.factory.simple_generate(create, **passed_kwargs)

I haven't tried this, so I don't know if it works, or if it breaks something else, but I do believe something like that is needed...

Dhekke commented Jun 1, 2013

I'm facing the same issue... And I'm currently having to resort to some weird stuff using factory.post_generation.
Seems to me there should be a check in base.Factory._generate for RelatedFactory instances, since any other postgen should be called even if we pass a value (right?), something like:

for name, decl in sorted(postgen_declarations.items()):
    extracted, extracted_kwargs = postgen_attributes[name]
    # If I passed any value for a RelatedFactory, don't call the RelatedFactory
    if extracted is not None and isinstance(decl, RelatedFactory):
        continue
    decl.call(obj, create, extracted, **extracted_kwargs)

extracted is None if no argument is passed, so we can't check against it. Maybe PostGenerationDeclaration could return False instead of None, since the extracted value is supposed to always be an object anyway (at least I think so), then extracted == None would mean we don't want the factory to create an object.

But considering that we could want to send an object instead of asking the factory not to create one, maybe the check should be in the RelatedFactory.call method, also adding a barebones way of setting the attribute if we pass an object like (using @gbittoun example) obj = MyFactory(param1=<model_file.MyModel instance>)

Something like:

def call(self, obj, create, extracted=None, **kwargs):
    passed_kwargs = dict(self.defaults)
    passed_kwargs.update(kwargs)
    if self.name:
        passed_kwargs[self.name] = obj

    # If we passed an object of type I'm supposed to create, just set the attribute and save if we have to
    if isinstance(extracted, self.get_factory().FACTORY_FOR):
        setattr(extracted, self.name, obj)
        if create:
            extracted.save()
    # If we passed no other value, then we can generate the object using the factory
    elif extracted is None:
        self.factory.simple_generate(create, **passed_kwargs)

I haven't tried this, so I don't know if it works, or if it breaks something else, but I do believe something like that is needed...

@rbarrois

This comment has been minimized.

Show comment
Hide comment
@rbarrois

rbarrois Jun 1, 2013

Member

@Dhekke Yep, that's indeed a real bug in factory_boy.

I'll try to prepare a fix next week.

Member

rbarrois commented Jun 1, 2013

@Dhekke Yep, that's indeed a real bug in factory_boy.

I'll try to prepare a fix next week.

@Dhekke

This comment has been minimized.

Show comment
Hide comment
@Dhekke

Dhekke Jun 1, 2013

@rbarrois Am I on the right track with those suggestions? factory_boy has been very useful to me and I'd love to contribute, if I can.

Dhekke commented Jun 1, 2013

@rbarrois Am I on the right track with those suggestions? factory_boy has been very useful to me and I'd love to contribute, if I can.

@rbarrois

This comment has been minimized.

Show comment
Hide comment
@rbarrois

rbarrois Jun 13, 2013

Member

@Dhekke Yep, your analysis pretty much nailed the problem here :)

I'm conflicted about the best solution for this:

Option 1

  • When nothing was passed for extracted, the PostGeneration hook receives a special Empty object
  • When None was explicitly passed, the PostGeneration receives it
  • When another value was passed, the PostGeneration receives it

Option 2

  • When nothing was passed for extracted, the PostGeneration hook receives None
  • When None was explicitly passed, the PostGeneration receives it
  • When another value was passed, the PostGeneration receives it

Option 3

  • When nothing was passed for extracted, the PostGeneration hook receives None, and a factory_extracted=False kwarg
  • When None was explicitly passed, the PostGeneration receives it with a factory_extracted=True kwarg
  • When another value was passed, the PostGeneration receives it with a factory_extracted=True kwarg

The simple in terms of API would be option 1, but I don't really like using a pseudo-None object.

What do you think?

Member

rbarrois commented Jun 13, 2013

@Dhekke Yep, your analysis pretty much nailed the problem here :)

I'm conflicted about the best solution for this:

Option 1

  • When nothing was passed for extracted, the PostGeneration hook receives a special Empty object
  • When None was explicitly passed, the PostGeneration receives it
  • When another value was passed, the PostGeneration receives it

Option 2

  • When nothing was passed for extracted, the PostGeneration hook receives None
  • When None was explicitly passed, the PostGeneration receives it
  • When another value was passed, the PostGeneration receives it

Option 3

  • When nothing was passed for extracted, the PostGeneration hook receives None, and a factory_extracted=False kwarg
  • When None was explicitly passed, the PostGeneration receives it with a factory_extracted=True kwarg
  • When another value was passed, the PostGeneration receives it with a factory_extracted=True kwarg

The simple in terms of API would be option 1, but I don't really like using a pseudo-None object.

What do you think?

@Dhekke

This comment has been minimized.

Show comment
Hide comment
@Dhekke

Dhekke Jun 14, 2013

Yeah, I'm not a fan of Option 1 for the same reason, it just seems a bit overkill.

As for the rest, I believe having None as an argument is something that should only impact the RelatedFactory, right? If I pass None to a PostGeneration, for instance, the function should still be called, while a None to a RelatedFactory should mean that I don't want the object to be created.
So while I do favor Option 3 as a clear and explicit way to solve this, I'm not sure on using extracted for something that would only impact RelatedFactory.

At the same time, extracted is the only place that can tell me if the user sent something to be extracted, so it's the obvious place to do it. So I'm also conflicted about Option 3.

That leaves Option 2 but I don't really see how it would solve the issue =/

All of this is to say that I like Option 3 the most, and if it's paired with something like

for name, decl in sorted(postgen_declarations.items()):
    extracted, extracted_kwargs = postgen_attributes[name]
    # If I passed any value for a RelatedFactory, don't call the RelatedFactory
    if factory_extracted is False and isinstance(decl, RelatedFactory):
        continue
    decl.call(obj, create, extracted, **extracted_kwargs)

we don't even have to change method signatures.

Dhekke commented Jun 14, 2013

Yeah, I'm not a fan of Option 1 for the same reason, it just seems a bit overkill.

As for the rest, I believe having None as an argument is something that should only impact the RelatedFactory, right? If I pass None to a PostGeneration, for instance, the function should still be called, while a None to a RelatedFactory should mean that I don't want the object to be created.
So while I do favor Option 3 as a clear and explicit way to solve this, I'm not sure on using extracted for something that would only impact RelatedFactory.

At the same time, extracted is the only place that can tell me if the user sent something to be extracted, so it's the obvious place to do it. So I'm also conflicted about Option 3.

That leaves Option 2 but I don't really see how it would solve the issue =/

All of this is to say that I like Option 3 the most, and if it's paired with something like

for name, decl in sorted(postgen_declarations.items()):
    extracted, extracted_kwargs = postgen_attributes[name]
    # If I passed any value for a RelatedFactory, don't call the RelatedFactory
    if factory_extracted is False and isinstance(decl, RelatedFactory):
        continue
    decl.call(obj, create, extracted, **extracted_kwargs)

we don't even have to change method signatures.

@rbarrois

This comment has been minimized.

Show comment
Hide comment
@rbarrois

rbarrois Jun 14, 2013

Member

I guess I'll go for Option 3, but I'll pass the factory_extracted to RelatedFactory ; that way, other users may add custom RelatedFactory-like declarations and be able to take advantage from that info.

It still means that all postgen_declarations will suddenly begin receiving a factory_extracted kwarg, but the core design for that feature states that arbitrary kwargs may be received...

Member

rbarrois commented Jun 14, 2013

I guess I'll go for Option 3, but I'll pass the factory_extracted to RelatedFactory ; that way, other users may add custom RelatedFactory-like declarations and be able to take advantage from that info.

It still means that all postgen_declarations will suddenly begin receiving a factory_extracted kwarg, but the core design for that feature states that arbitrary kwargs may be received...

@rbarrois rbarrois closed this in 1ba20b0 Jun 14, 2013

@Dhekke

This comment has been minimized.

Show comment
Hide comment
@Dhekke

Dhekke Jun 14, 2013

Looks great! Thanks!

Dhekke commented Jun 14, 2013

Looks great! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment