Opened 4 years ago

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

Download all attachments as: .zip

Change History (17)

comment:1 Changed 4 years ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to 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 Changed 4 years ago by aaugustin

  • Needs tests set
  • Type changed from Uncategorized to Cleanup/optimization

comment:3 Changed 4 years ago by melinath

  • Summary changed from .get behaviour is inconsisten with .get_or_create to .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 4 years ago by wilfred@…

removed assertion, added a simple test

comment:4 Changed 4 years ago by tobias

  • Has patch set

comment:5 Changed 4 years ago by aaugustin

  • 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 4 years ago by poirier

  • Owner changed from nobody to poirier
  • Status changed from new to assigned

Changed 4 years ago by poirier

comment:7 Changed 4 years ago by 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 4 years ago by charettes

With a more descriptive test failure

comment:8 Changed 4 years ago by charettes

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

comment:9 Changed 4 years ago by poirier

  • Owner changed from poirier to nobody
  • Status changed from assigned to new

comment:10 Changed 4 years ago by aaugustin

#10993 was closed in favor of this ticket.

comment:11 Changed 2 years ago by timo

  • Cc timograham@… added

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

comment:12 Changed 2 years ago by claudep

  • Triage Stage changed from Accepted to Ready for checkin

comment:13 Changed 2 years ago by aaugustin

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

comment:14 Changed 2 years ago by timo

  • Resolution set to fixed
  • Status changed from new to 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