berinhard / model_mommy

No longer maintained, please migrate to model_bakery
http://model-bakery.readthedocs.org/
Other
903 stars 141 forks source link

Set foreign key fields using the primary key #104

Closed lucasicf closed 11 years ago

lucasicf commented 11 years ago

What do you think about this? This would be useful by turning the syntax simpler in some tests I'm doing. For example:

mommy.make(SomeModel, related_model=RelatedModel.objects.get(id=10))

This could be done with:

mommy.make(SomeModel, related_model_id=10)

setting the attribute related_model_id manually before calling save().

berinhard commented 11 years ago

@lucasicf i undesrtand your point of view, but, in my opinion, i don't know if the __ api would be good for this feature because they already have a behaviour attached to them that is the field population lookup...

lucasicf commented 11 years ago

@berinhard, but I'm not using the double underscore feature in this example. It's not related_model__id. It's just the actual primary key's field name.

You can create normally an object, in Django, like that:

SomeModel.objects.create(related_model_id=10)
# either like above or below
sm = SomeModel(related_model_id=10)
sm.save()

For some reason it throws an exception with the mommy. Might I investigate that?

berinhard commented 11 years ago

@lucasicf I see your point here, but I think de API might get a little confusing... This is because we already have an API for __ that overrides the fk object field value generation and we'll also have an API for _id... Another problem is if we have a model like the following:

class Foo(models.Model):
    product_id = models.IntegerField()

The point of this model is to just store a product id but without a FK relationship. This may sound strange but I've already seen this pattern in a few projects. What should be the mommy behaviour when this field is used with a value to override its random generation? I think we'll have a conflict with the API that you're suggesting...

But, if you say that Django already accepts this approach you're suggesting, maybe model_mommy already supports it also because it just passes the arguments do an object creation call. I think an investigation is valid so we can have more information to make any decision...

lucasicf commented 11 years ago

Ok, I took a short look on the _make() function and I figured out the problem.

This works:

SomeModel(related_model_id=10)

This doesn't:

SomeModel(related_model=RelatedModel.objects.get(pk=10), related_model_id=10)

And that's why I can't create it by this way:

mommy.make(SomeModel, related_model_id=10)

The error:

  File "/path/to/model_mommy/model_mommy/mommy.py", line 220, in make
    return self._make(commit=True, **attrs)
  File "/path/to/model_mommy/model_mommy/mommy.py", line 261, in _make
    return self.instance(model_attrs, _commit=commit)
  File "/path/to/model_mommy/model_mommy/mommy.py", line 270, in instance
    instance = self.model(**attrs)
  File "/path/to/site-packages/django/db/models/base.py", line 367, in __init__
    raise TypeError("'%s' is an invalid keyword argument for this function" % kwargs.keys()[0])
TypeError: 'related_model_id' is an invalid keyword argument for this function

As I do not pass related_model to kwargs, it creates the RelatedModel instance and adds to the model_attrs, keeping both related_model and related_model_id keys. That being said, I agree with you @berinhard, it could confuse the simplicity of the API by treating a raw foreign key's id field.

Maybe it could support passing also ids to a FK, like that:

mommy.make(SomeModel, related_model=10)

But it must also be investigated later and depends on the decisions of contributors here.

Thanks

vandersonmota commented 11 years ago

Well, i think there is little benefit from adding this feature when weighed against the complexity it can generate.