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

Allow propagation of ignored_fields to automatically generated Foreign Key #88

Closed k4rl85 closed 7 years ago

k4rl85 commented 7 years ago

Hi, i have some problem with the integration between django-polymorphic and django-dynamic-fixtures.

This was talked in different issues and some fix has been done like #85. The problem for me is the automatic generation of a foreign key releated to a polymorphic instance.

The solution suggested in #85 is to set DDF_IGNORE_FIELDS = ['*_ptr'] but this solution is ineffective in my case, because the DDF code does not use ignored_fields with an auto generated related object.

Example:

# settings.py
DDF_IGNORE_FIELDS = ['*_ptr']

# models.py
class Location(PolymorphicModel):
    objects = PolymorphicManager()

class Country(Location):
    name = models.CharField(max_length=200)

class Publisher(models.Model):
    country = models.ForeignKey(Country, null=True, blank=True)

# test.py
from django_dynamic_fixture import G

G(Country)  # <-- no problem
G(Publisher)  # <-- raise Attribution error

The error generated in 2nd call is caused by the implicit creation of a Country for the Publisher instance. The ignored fields are not correctly set for the publisher country because it receive an empty ignored_fields.

I wonder if there is a reason to avoid this propagation, i will prepare a PR to fix it if is not needed.

Carlo

paulocheque commented 7 years ago

Hi there, first of all, thanks for the report and for the PR.

The reason to avoid this propagation is because two models can have fields with the same name.

class ModelA:
    name = charfield

class ModelB:
    name = charfield
    a = FK(A)

G(ModelA, ignore_fields=['name']) # should ignore just name in ModelA.

Do you think it is possible to adjust the PR to support both situations?

Maybe you could create a new option like ignore_all_fields or something?

Or maybe we can add some prefix to tell DDF to propagate, like ignore_fields=['*name'].

What do you think?

k4rl85 commented 7 years ago

I would stay with the current G(ModelA, ignore_fields=[...]) behavior, i.e. explicit ignore_fields will be applied only to ModelA and NOT to related objects (ModelB), created together with the A instance. While ignore fields declared in settings should be applied to A and B, being them intended to be applied globally.

I will modify the PR to show my solution

paulocheque commented 7 years ago

Thanks again.

But I did not understand the new PR completely. What is the difference of behavior with and without the PR https://github.com/paulocheque/django-dynamic-fixture/pull/90?

k4rl85 commented 7 years ago

I have dug a lot to find a solution so prepare for a very long message :D

The codebase have 3 point where a DynamicFixture object are initialized:

the problem without the PR is that only in the first case use the DDF_IGNORE_FIELDS setting so with my PR i ensure that any call of DynamicFixture() will use the generic ignore_fields.

BTW the solution proposed in #85 is wrong because by ignoring *_ptr it does not return error on G() call but the created obj is not valid! When you try to retrieve from db the new obj it will fail. https://docs.djangoproject.com/en/1.11/topics/db/models/#multi-table-inheritance

# settings
DDF_IGNORE_FIELDS = ['*_ptr']

# model.py
class ModelA(PolymorphicModel):
    ...

class ModelB(ModelA):
    ...

class ModelC(models.Model):
    a = FK(ModelB)

# test.py
obj = G(ModelB)
ModelB.objects.get(pk=obj.pk) # <-- Here django throw error because he cannot find a valid parent instance of ModelA relative the created obj

The real solution to fix the integration with django-polymorphic is to avoid creation of polymorphic_ctype field with this code:

DDF_IGNORE_FIELDS = ['polymorphic_ctype']

and modify set_data_for_a_field method to handle an AttributeError raised by *_ptr fields.

I don't know why multi table inheritance does not raise an AttributeError while django-polymorphic does it.

paulocheque commented 7 years ago

What amazing explanation and contribution! =)

I merged the PR.

Thanks!