Make defaults a required argument for QuerySet.get_or_create() and update_or_create()

[Álex Córcoles]

Hi,

I think this would prevent errors as code which puts fields in defaults which should be kwargs will work the first time and would pass basic testing, but fail the second time it's executed.

Kind regards,

Álex

[Tim Graham]

I'm not sure if I've understand the problem. Could you add a code example of what you mean? Even if a good idea, it sounds like it'll break backwards compatibility?

[Álex Córcoles]

Hi, yeah, sorry, typed that out in a hurry.

Yesterday we had a bug where someone wrote:

models.Foo.objects.update_or_create(
    foo=foo,
    bar=bar,
    spam=spam,
)

, when the correct code was:

models.Foo.objects.update_or_create(
    foo=foo,
    bar=bar,
    defaults={
        spam=spam,
    }
)

This passed code review and even had 100% test coverage, but as it just had a test case for the update case and not for the or_create case, we didn't prevent the bug.

My argument here is that most calls to update_or_create or get_or_create need a non-empty defaults parameter, so making it a mandatory parameter can prevent bugs, at the cost of the actual calls not needing defaults just need to type defaults={},. While people still can write that when they shouldn't, it catches the eye in a code review.

Yes, this breaks compat, but it probably will also uncover hidden bugs, and maybe it has a net positive effect.

Kind regards,

Álex

[Ankur Gupta]

The example you have provided is not a shortcoming, and does not warrant the feature enhancement. It was an improper use of one of Django's features. Since the proposed change will suddenly break things, I do not think it is worth it.

[Álex Córcoles]

I think this is in line with changes such as:

​https://docs.djangoproject.com/en/2.0/releases/2.0/#form-fields-no-longer-accept-optional-arguments-as-positional-arguments

, where breaking changes are introduced to improve ergonomics. I am not aware of the criteria required to accept them (I didn't see much at https://code.djangoproject.com/ticket/28192 ), however I think this also fits well "explicit is better than implicit".

[Tim Graham]

I doubt there would be consensus for this change (I don't think the problem you described is a common mistake and worth making the entire Django ecosystem add defaults={} as needed) but you could write to the DevelopersMailingList to get other opinions.