Opened 6 years ago

Closed 4 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@… 5 years ago.
removed assertion, added a simple test
16137.diff (2.6 KB) - added by Dan Poirier 5 years ago.
16137.2.diff (2.6 KB) - added by Simon Charette 5 years ago.
With a more descriptive test failure

Download all attachments as: .zip

Change History (17)

comment:1 Changed 6 years ago by Aymeric Augustin

Triage Stage: UnreviewedAccepted

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 Changed 6 years ago by Aymeric Augustin

Needs tests: set
Type: UncategorizedCleanup/optimization

comment:3 Changed 6 years ago by melinath

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.

Changed 5 years ago by wilfred@…

removed assertion, added a simple test

comment:4 Changed 5 years ago by Tobias McNulty

Has patch: set

comment:5 Changed 5 years ago by Aymeric Augustin

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 Changed 5 years ago by Dan Poirier

Owner: changed from nobody to Dan Poirier
Status: newassigned

Changed 5 years ago by Dan Poirier

Attachment: 16137.diff added

comment:7 Changed 5 years ago by Dan Poirier

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.

Changed 5 years ago by Simon Charette

Attachment: 16137.2.diff added

With a more descriptive test failure

comment:8 Changed 5 years ago by Simon Charette

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

comment:9 Changed 5 years ago by Dan Poirier

Owner: changed from Dan Poirier to nobody
Status: assignednew

comment:10 Changed 5 years ago by Aymeric Augustin

#10993 was closed in favor of this ticket.

comment:11 Changed 4 years ago by Tim Graham

Cc: timograham@… added

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

comment:12 Changed 4 years ago by Claude Paroz

Triage Stage: AcceptedReady for checkin

comment:13 Changed 4 years ago by Aymeric Augustin

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

comment:14 Changed 4 years ago by Tim Graham

Resolution: fixed
Status: newclosed

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