Opened 13 years ago

Closed 11 years ago

#16137 closed Cleanup/optimization (fixed)

.get behaviour is inconsistent with .get_or_create when no kwargs are given

Reported by: wilfred@… Owned by: nobody
Component: Database layer (models, ORM) Version: 1.3
Severity: Normal Keywords:
Cc: timograham@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

It's really useful to be able to do

MyModel.objects.get()

when you've got a singleton model. Sadly, .get_or_create doesn't allow you to call it in the same way:

MyModel.objects.get_or_create()

This throws an AssertionError "get_or_create() must be passed at least one keyword argument". Could this requirement be dropped?

Attachments (3)

get_or_create_no_kwargs.diff​ (2.1 KB ) - added by wilfred@… 13 years ago.
removed assertion, added a simple test
16137.diff​ (2.6 KB ) - added by Dan Poirier 13 years ago.
16137.2.diff​ (2.6 KB ) - added by Simon Charette 13 years ago.
With a more descriptive test failure

Download all attachments as: .zip

Change History (17)

comment:1 by Aymeric Augustin, 13 years ago

Triage Stage: Unreviewed → Accepted

The check is enforced at the very beginning of django.db.models.query.get_or_create():

        assert kwargs, 'get_or_create() must be passed at least one keyword argument'

This line has been there, unchanged, since the merge of queryset-refactor. Actually, it was even in the initial proposal by Adrian: ​https://groups.google.com/group/django-developers/msg/1c39b286a3e9e2cb

As far as I can tell, it is not necessary.

comment:2 by Aymeric Augustin, 13 years ago

Needs tests: set
Type: Uncategorized → Cleanup/optimization

comment:3 by Stephen Burrows, 13 years ago

Summary: .get behaviour is inconsisten with .get_or_create → .get behaviour is inconsistent with .get_or_create when no kwargs are given
UI/UX: unset

Removing that line causes no unexpected test failures.

by wilfred@…, 13 years ago

Attachment: get_or_create_no_kwargs.diff​ added

removed assertion, added a simple test

comment:4 by Tobias McNulty, 13 years ago

Has patch: set

comment:5 by Aymeric Augustin, 13 years ago

Needs documentation: set
Patch needs improvement: set

Patch doesn't apply cleanly.

Also, this needs a mention in the release notes -- nothing fancy, but people could be relying on the current behavior, even if it isn't really justified.

comment:6 by Dan Poirier, 13 years ago

Owner: changed from nobody to Dan Poirier
Status: new → assigned

by Dan Poirier, 13 years ago

Attachment: 16137.diff​ added

comment:7 by Dan Poirier, 13 years ago

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

Updated patch to apply to Django trunk.
Added brief note to 1.4 release notes.

by Simon Charette, 13 years ago

Attachment: 16137.2.diff​ added

With a more descriptive test failure

comment:8 by Simon Charette, 13 years ago

Updated patch with a more descriptive test failure (instead of self.assertTrue(False))

comment:9 by Dan Poirier, 13 years ago

Owner: changed from Dan Poirier to nobody
Status: assigned → new

comment:10 by Aymeric Augustin, 13 years ago

#10993 was closed in favor of this ticket.

comment:11 by Tim Graham, 11 years ago

Cc: timograham@… added

Updated patch to apply cleanly to master. ​https://github.com/django/django/pull/1214

comment:12 by Claude Paroz, 11 years ago

Triage Stage: Accepted → Ready for checkin

comment:13 by Aymeric Augustin, 11 years ago

I left two minor comments on the PR, otherwise this is indeed RFC.

comment:14 by Tim Graham, 11 years ago

Resolution: → fixed
Status: new → closed

In 90af278203963e3e3f96e443971cd38a2dad34e4:

Fixed #16137 - Removed kwargs requirement for QuerySet.get_or_create

Thanks wilfred@, poirier, and charettes for work
on the patch.

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