Opened 3 years ago

Closed 18 months ago

Last modified 18 months ago

#22728 closed Bug (invalid)

get_or_create with field lookups cause empty values

Reported by: Patrick Bregman Owned by: Andriy Sokolovskiy
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: get_or_create
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

First of, I'm actually using 1.7b4, but there seems to be no version for that (yet)...

When trying to be smart and using a case insensitive filter for get_or_create, it doesn't actually fill in the value, because you passed it in with a field lookup instead of just the field name. An example explains this better:

from django.db import models

class Tag(models.Model):
    name = models.CharField(unique=True)
    slug = models.SlugField(unique=True)

tags = ['django', 'python', 'web', 'Django', 'html5', 'Python'] # Just a list of tags to make this work

for tag in tags:
    obj, created = Tag.objects.get_or_create(name__iexact=tag, defaults={'slug': tag)
    print("Tag {0} created? {1}".format(obj.name, created)

This will give a IntegrityError while going through the list, complaining that there already is a tag with an empty value for name.

IntegrityError: duplicate key value violates unique constraint "blog_tag_name_key"
DETAIL:  Key (name)=() already exists.

A work-around is to also specify the name field in the defaults, but I was actually expecting the ORM to be "smart" enough to not require this. There is no documentation describing it should happen either way. Is this a documentation issue or a code issue?

Change History (13)

comment:1 Changed 3 years ago by valberg

Don't mind me, totally misread the issue :)

Last edited 3 years ago by valberg (previous) (diff)

comment:2 Changed 3 years ago by Aymeric Augustin

Your expectations are a bit higher than what the ORM is actually able to do ;-)

This is really a feature request. Can you submit it to the django-developers mailing-list?

comment:3 Changed 3 years ago by Patrick Bregman

I'd swear I used something like this in a previous version... But I guess you know Django better than I do :)
I'll post it to the django-developers mailing-list, thanks.

comment:4 Changed 3 years ago by Baptiste Mispelon

Triage Stage: UnreviewedAccepted

For reference, the mailing list discussion is here: https://groups.google.com/forum/#!topic/django-developers/EbLRpYB_1WI

As someone else mentionned on the mailing list, I'm not sure either if we can do something that works in the general case.

The fact that this "works" currently and that it silently drops the field that has a lookup seems quite dangerous (and not intentional).
For this reason, I'd be in favor of throwing an error if a user attempts to use lookups in a get_or_create call (similar to what happens if you try and use one in create()).

I'll mark this as accepted on this basis.

Thanks.

comment:5 Changed 2 years ago by Tim Graham

Version: 1.7-beta-2master

comment:6 Changed 2 years ago by Andriy Sokolovskiy

Owner: changed from nobody to Andriy Sokolovskiy
Status: newassigned

comment:7 Changed 2 years ago by Andriy Sokolovskiy

Has patch: set

comment:8 Changed 2 years ago by Berker Peksag

Patch needs improvement: set

comment:9 Changed 18 months ago by Andriy Sokolovskiy

Patch needs improvement: unset

comment:10 Changed 18 months ago by Carl Meyer

FWIW, I have code in a current project, written only a few months ago, that uses join lookups with get_or_create, and takes advantage of the fact that they are used in the lookup but not the create. I don't think it's an exotic use case, it's simply "find me a user that matches these criteria, where the criteria are actually based on related objects, and if no such user exists, create a user with the given defaults." That's exactly the use case that get_or_create is intended for, with the addition of "the criteria are based on related objects."

Even requiring any field mentioned in a lookup to be present in defaults is potentially a backwards-incompatible breakage to such use cases. I don't know how that change would even handle my case, as the relation I'm querying on is not even a field present on User, it's a reverse FK from another model, so there's no sensible corresponding value that could be included in defaults.

I see the possible confusion here, but I don't see any way to address it without reducing the utility of get_or_create in a backwards-incompatible way. So I think we should make sure the documentation of get_or_create is crystal clear on this point, and otherwise leave the behavior alone.

comment:11 Changed 18 months ago by Marc Tamlyn

Resolution: invalid
Status: assignedclosed

The documentation of get_or_create does devote a full paragraph and sample code to this, so I don't think there's much to improve.

There is a case for an extension of the functionality which allows type-idempotent transforms such as unaccent or iexact, but I feel this will be super hard and prone to bugs. It's a possibility, but not worth keeping this lying around for.

For longer discussion of why this ticket is being closed, see https://github.com/django/django/pull/4779

comment:12 Changed 18 months ago by Andriy Sokolovskiy <me@…>

In fc19f93:

Refs #22728 - Added missing tests for defaultsexact case

comment:13 Changed 18 months ago by Marc Tamlyn <marc.tamlyn@…>

In 1f28521:

Merge pull request #4786 from coldmind/refs_22728

Refs #22728 - Added missing tests for defaultsexact case

Note: See TracTickets for help on using tickets.
Back to Top