paulocheque / django-dynamic-fixture

A complete library to create dynamic model instances for testing purposes.
http://django-dynamic-fixture.readthedocs.io/
Other
391 stars 67 forks source link

Object creation fails if a ForeignKey field has an ID as default #5

Closed tobych closed 11 years ago

tobych commented 12 years ago

I'm using f5784b, and I've a model with this field:

GENERIC_BUNNY_ID = 8
# ...
bunny_type = models.ForeignKey(BunnyType, default=GENERIC_BUNNY_ID)

When I try to instantiate this using G, I get:

File "/usr/local/lib/python2.6/dist-packages/django_dynamic_fixture-1.6.3-py2.6.egg/django_dynamic_fixture/ddf.py", line 370, in set_data_for_a_field
    setattr(instance, field.name, data) # Model.field = data
  File "/usr/local/lib/python2.6/dist-packages/django/db/models/fields/related.py", line 331, in __set__
    self.field.name, self.field.rel.to._meta.object_name))
ValueError: Cannot assign "8": "Asset.category" must be a "Category" instance.

I've not looked super long at the code, but I'll submit a pull request for code that's got it working for me.

Toby

paulocheque commented 12 years ago

Hi, thanks for reporting!

This patch sounds good for me. Do you create some automated test that reproduce this error to include in the pull request?

If you did not create, I will create for you.

Thanks again.

tobych commented 12 years ago

I didn't write a test actually. I think it was the day Github was down for a while and I couldn't do any work! Let me know if you'd still like me to do that.

paulocheque commented 11 years ago

Hi Toby.

I think with more attention about this. I believe this is an anti-pattern, since this will incentivate you to use a magic number in your models, and worst, a magic ID. Of course you can give a name to this constant, but I believe it is a better idea to create a method for this, so it will be possible to handle errors in a more properly way. Check the following example (I added an automated test for this scenario and it is working without any change in the code).

def default_fk_value():
    try:
        return ModelRelated.objects.get(id=1)
    except ModelRelated.DoesNotExist:
        return None

foreignkey_with_default = models.ForeignKey('ModelRelated', related_name='fk2', null=True, default=default_fk_value)

def test_new_deal_with_default_values(self):
    instance = self.ddf.new(ModelWithRelationships)
    self.assertTrue(isinstance(instance.foreignkey_with_default, ModelRelated))

What do you think? Do you agree?

Regards

tobych commented 11 years ago

Hi Paulo. I'm coming back to this rather late in the day, and only because of the subsequent issue you're referencing above. I do see what you're doing above, but my model is as I write, and I'm surprised you want to get involved in recommending certain approaches to modeling over others, I guess, and I know the foreign row will be there, so I don't need this extra code, and wouldn't like to have to change my model even this much to work around a limitation in DDF. Right now I'm using my fork of DDF.

paulocheque commented 11 years ago

I will include this patch in DDF!