paulocheque / django-dynamic-fixture

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

Nullable field with a default should get a value even with DDF_FILL_NULLABLE_FIELDS=False #121

Closed jklaiho closed 4 years ago

jklaiho commented 4 years ago

One of the models in our app has a NullBooleanField with default=True. Back in DDF 2.0.0, it used to reliably get a True value when creating instances with G() without having that model field explicitly specified.

Turns out this was not (primarily) due to the default value, but the fact that DDF_FILL_NULLABLE_FIELDS used to be True by default. We were unaware of this and were very confused when upgrading to DDF 3.0.2 started breaking a whole bunch of tests that relied on the old behavior, until stepping through the logic with pdb revealed the culprit:

https://github.com/paulocheque/django-dynamic-fixture/blob/bd253b5ef4e94209dbee5e448710cfc17bd03dad/django_dynamic_fixture/ddf.py#L362-L370

The combined "nullability/should fill nullable fields" check happens before the default value check, and actually did so in 2.0.0, only working for us thanks to DDF_FILL_NULLABLE_FIELDS being True back then.

I set that setting back to True in 3.0.2 and it fixed our immediate issue, but it got me thinking: if a field has a default value set, it seems to me that DDF should respect that default before checking for nullability. Having a default value is rather a strong signal from a developer that if left unspecified, this is the value that the field should have. (And that is how it works outside testing when creating objects without specifying values for those fields.) So basically, in the above section of code, the logic on lines 364-365 should be moved after lines 366-370 and modified accordingly.

This is a backwards-incompatible change in itself, but since 3.0.0 has other ones and is still young and not widely adopted, I think now would be a great time to do it for 3.0.3.

paulocheque commented 4 years ago

This PR should fix this issue: https://github.com/paulocheque/django-dynamic-fixture/pull/125

paulocheque commented 4 years ago

Please, try the version 3.0.3. Thanks again for reporting.