Fixed #20429 -- Added QuerySet.update_or_create #1248

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
Owner

timgraham commented Jun 6, 2013

Thanks tunixman for the suggestion.

timgraham referenced this pull request Jun 6, 2013

Closed

Ticket 20429 #1132

@apollo13 apollo13 and 1 other commented on an outdated diff Jun 18, 2013

tests/update_or_create/models.py
@@ -0,0 +1,30 @@
+"""
+update_or_create()
+
+``update_or_create()`` tries to look up an object with the given parameters and
+if an object isn't found, it creates one with the given parameters.
+"""
+
+from __future__ import unicode_literals
+
+from django.db import models
+from django.utils.encoding import python_2_unicode_compatible
+
+
+@python_2_unicode_compatible
+class SalesRank(models.Model):
@apollo13

apollo13 Jun 18, 2013

Owner

Please no new models unless absolutely neccessary

@timgraham

timgraham Jun 18, 2013

Owner

I could likely rewrite the tests using the models in the get_or_create app, but then I think I'd have to put the tests in there instead of making a new app. Thoughts?

@timgraham

timgraham Jun 19, 2013

Owner

I added the tests to the get_or_create app. If we want to rename it, I'm open to suggestions as something like get_or_create_and_update_or_create seems too long.

@loic loic commented on an outdated diff Jul 4, 2013

docs/ref/models/querysets.txt
+ obj.save()
+ except Person.DoesNotExist:
+ updated_values.update({'first_name': 'John', 'last_name': 'Lennon'})
+ obj = Person(**updated_values)
+ obj.save()
+
+This pattern gets quite unwieldy as the number of fields in a model goes up.
+The above example can be rewritten using ``update_or_create()`` like so::
+
+ obj, created = Person.objects.update_or_create(
+ first_name='John', last_name='Lennon', defaults=updated_values)
+
+For detailed description how names passed in ``kwargs`` are resolved see
+:meth:`get_or_create`.
+
+As described above in:meth:`get_or_create`, this method is prone to a
@loic

loic Jul 4, 2013

Member

Missing space before :meth:get_or_create.

@loic loic and 1 other commented on an outdated diff Jul 4, 2013

docs/ref/models/querysets.txt
+~~~~~~~~~~~~~~~~
+
+.. method:: update_or_create(**kwargs)
+
+.. versionadded:: 1.7
+
+A convenience method for updating an object with the given ``kwargs``, creating
+a new one if necessary.
+
+Returns a tuple of ``(object, created)``, where ``object`` is the created or
+updated object and ``created`` is a boolean specifying whether a new object was
+created.
+
+The ``update_or_create`` method tries to fetch an object from database based on
+the given ``kwargs``. If a match is found, it updates the fields passed in the
+``defaults`` ``dict`` in ``kwargs``.
@loic

loic Jul 4, 2013

Member

Nitpicking but shouldn't we change the method signature to (defaults=None, **kwargs)?

Because the meaning of "in the defaults dict in kwargs" is not immediately obvious.

This applies to get_or_create() as well.

@timgraham

timgraham Jul 4, 2013

Owner

I don't think it's technically correct because wouldn't that suggest you could pass an arg which would be be interpreted as defaults?

maybe the opening sentence should be revised a bit:

A convenience method for updating an object with the given kwargs (excluding a kwarg named defaults which is a dictionary of (field, value) pairs used to update the object), creating a new one if necessary.

@loic

loic Jul 4, 2013

Member

I meant changing the signature in the code which would require a special handling of None (to cast it to {}). That would be self documenting and also enable IDEs to autocomplete the defaults argument.

For the docs, what about:
A convenience method for updating an object that matches the given kwargs (with the exception of a kwarg named defaults), creating a new one if necessary. The optional kwarg named defaults is a dictionary of (field, value) pairs used to update the object.

@loic

loic Jul 4, 2013

Member

Actually reading again "wouldn't that suggest you could pass an arg which would be be interpreted as defaults?": yes it would enable a new behavior where you could pass defaults as an arg instead of a kwargs but that remains backward compatible.

Member

loic commented Jul 6, 2013

@timgraham: #20625 is almost ready and it's conflicting with this changeset since I'll need to remove the Manager method and add a new entry for update_or_create in a test.

Do you want to commit this one first and I'll rebase on top of it?

Owner

timgraham commented Jul 6, 2013

No, don't let this block #20625. It needs some more work and another review, I think.

@charettes charettes and 2 others commented on an outdated diff Jul 10, 2013

docs/releases/1.7.txt
@@ -30,6 +30,12 @@ In addition, the widgets now display a help message when the browser and
server time zone are different, to clarify how the value inserted in the field
will be interpreted.
+Minor features
@charettes

charettes Jul 10, 2013

Member

Should we mention get_or_create signature change?

@loic

loic Jul 10, 2013

Member

I don't think we should, it's cleaner to use defaults as a kwarg so in that respect the signature hasn't changed.

@timgraham

timgraham Jul 10, 2013

Owner

It's more of a code cleanup in my mind. I don't think it's backwards incompatible in any way (or a real "feature") -- the reason I put a versionchanged note about it on the docs is in case someone is reading a later version of the docs than the version of Django they are actually using and thinks he can pass defaults as an arg. What do you think?

@charettes

charettes Jul 10, 2013

Member

Well I think we should remove the versionadded clause then since we don't want to document this as a feature.

Edit: versionchanged.

@timgraham

timgraham Jul 10, 2013

Owner

That's fine with me.

@sicarrots @timgraham sicarrots Fixed #20429 -- Added QuerySet.update_or_create
Thanks tunixman for the suggestion and Loic Bistuer for the review.
c0a9dd2
Owner

timgraham commented Jul 12, 2013

merged in 6272d2f

timgraham closed this Jul 12, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment