Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ticket #4102 - Allow UPDATE of only specific fields in model.save() #41

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
4 participants
@niwinz
Copy link
Contributor

commented May 3, 2012

@alex

View changes

django/db/models/base.py Outdated
@@ -2,6 +2,7 @@
import sys
from functools import update_wrapper
from itertools import izip
from sets import ImmutableSet

This comment has been minimized.

Copy link
@alex

alex May 3, 2012

Member

You don't need this, it's just frozenset.

@alex

This comment has been minimized.

Copy link
Member

commented May 3, 2012

A thought that just popped into my head: what's the interaction between this and defer()/only(). If I use one of those should the save automatically only save fields that I fetched?

@niwinz

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2012

No, first get all the fields that have deferred and then save everything.

Example:

>>> a = User.objects.defer("username").get(pk=1)
>>> a.save()

This generates the following queries:

(0.000) SELECT "auth_user"."id", "auth_user"."first_name", "auth_user"."last_name", "auth_user"."email", "auth_user"."password", "auth_user"."is_staff", "auth_user"."is_active", "auth_user"."is_superuser", "auth_user"."last_login", "auth_user"."date_joined" FROM "auth_user" WHERE "auth_user"."id" = 1 ; args=(1,)
(0.000) SELECT (1) AS "a" FROM "auth_user" WHERE "auth_user"."id" = 1  LIMIT 1; args=(1,)
(0.000) SELECT "auth_user"."id", "auth_user"."username" FROM "auth_user" WHERE "auth_user"."id" = 1 ; args=(1,)
(0.000) UPDATE "auth_user" SET "username" = andrei, "first_name" = Andrei, "last_name" = Antoukh, "email" = niwi@niwi.be ....
@akaariai

This comment has been minimized.

Copy link
Member

commented May 7, 2012

The idea is that .only()/.defer() interaction will be handled later on. No need to handle it in this patch yet, but definitely should be done before 1.5.

@niwinz

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2012

Sorry, I misread the question! The interaction with "only" and "difer" would be interesting to be implemented for 1.5.

I'll work on it, the next free time!

@niwinz

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2012

Now patch updated to django master and ImmutableSet replaced with frozenset!

@niwinz

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2012

I just added a possible candidate for the interaction of parameter update_fields with methods only() and defer().

@akaariai

This comment has been minimized.

Copy link
Member

commented May 11, 2012

Lets defer the only() and defer() interaction in a separate pull request (I am just finishing off some last changes to this patch, will send you a pull request).

@niwinz

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2012

Well! I have reverted the last commit.

@akaariai

This comment has been minimized.

Copy link
Member

commented May 11, 2012

Scratch that pull request: for some reason I can't find your github repo to make that pull... So, if you can manually download the commit d66527f into your branch, rebase it to just one commit and remove the only()/defer() interactions for now. I suggest this as the sole commit message:

Fixed #4102 -- Allow update of specific fields in model.save()

Added the ability to update only part of the model's fields in
model.save() by introducing a new kwarg "update_fields". Thanks
to all the numerous reviewers and commenters in the ticket

(to download the commit:
curl https://github.com/akaariai/django/commit/d66527f79d1555abb04dd933019db4ca8d949988.patch | git am
)

@niwinz

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2012

Already updated the pull-request with your patch! Thank you for your corrections.

@akaariai

This comment has been minimized.

Copy link
Member

commented May 11, 2012

Thanks a lot! As far as I am concerned this is 100% ready for checkin. A final cursory check (especially of the doc changes) would be welcome.

niwibe: thanks for your patience. We all are still trying to figure how to best work using Trac + github together, and how to use pull requests, so you have been a test subject... :) If you have any opinions to share about this process please post them! It would be informative to hear how this process looks from your perspective...

@niwinz

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2012

I am very happy to have django on github, I'm much more easy to track changes.

The combination of using trac and github is not bad. At first I was confused, did not know if what I did was right or wrong, but gradually I have been adapting and learning.

I, on this issue, I have kept updated, patches in trac and pull-request at github. In my opinion there should be only a single site where they are patches. In github or traction, but not both at once.

Now, once this patch is part of the official repo django, I'm thinking of alex proposal (defer / only methods) and really do not know if I have to create a ticket in trac or not. Or just pull it with a request.

I think it should be (if not present) a site that explains the basic procedures to contribute patches, now that django is on github.

thanks

@akaariai

This comment has been minimized.

Copy link
Member

commented May 11, 2012

I think I have volunteered to write those guidelines... At the risk of totally side-tracking this pull request, my suggestion is the following: you should create a ticket (the addition is complex enough to warrant one), you should work in a branch of your github repo, and you can announce that latest work from you is available from that branch in the trac. Once the work nears completition, create a pull request. That is, branches are used to work on stuff, pull requests are used only when you really thing you have something ready for a pull.

@akaariai

View changes

docs/ref/signals.txt Outdated
@@ -154,6 +158,10 @@ Arguments sent with this signal:
``using``
The database alias being used.

``update_fields``
List or tuple of fields to update explicitly specified in the ``save()`` method.

This comment has been minimized.

Copy link
@akaariai

akaariai May 11, 2012

Member

Still one error here - sorry for not spotting this earlier. This should of course say a frozenset of fields to update... I can fix this when committing.

@freakboy3742

View changes

tests/modeltests/update_only_fields/tests.py Outdated
self.assertEqual(s.gender, 'F')
self.assertEqual(s.name, 'Ian')

def test_update_fields_m2n(self):

This comment has been minimized.

Copy link
@freakboy3742

freakboy3742 May 12, 2012

Member

Typo - m2m not m2n

product.name = 'Name changed again'
product.save(update_fields=['name'])

The ``update_fields`` argument can be any iterable containing strings. An

This comment has been minimized.

Copy link
@freakboy3742

freakboy3742 May 12, 2012

Member

"An not None" should be "A non-None"; but the whole sentence is a bit confusing anyway. Consider rewording the whole sentence. "An empty update_fields iterable will skip the save. A value of None will perform an update on all fields".

Fixed #4102 -- Allow update of specific fields in model.save()
Added the ability to update only part of the model's fields in
model.save() by introducing a new kwarg "update_fields". Thanks
to all the numerous reviewers and commenters in the ticket
@niwinz

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2012

Now I fixed this typos. ;)

@akaariai

This comment has been minimized.

Copy link
Member

commented May 12, 2012

Closed in commit 365853d - some final editorializing done based on the above comments.

Thanks for everybody participating. Of course, biggest thanks go to niwibe.

@akaariai akaariai closed this May 12, 2012

@akaariai

This comment has been minimized.

Copy link
Member

commented May 12, 2012

I created a ticket for .only()/.defer() interaction, see #18306.

I missed your latest changes and instead did the final cleanup myself - concurrent working has its downsides.

@niwinz

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2012

Well! I responded on the ticket, and indicated where my proposal.

sztrovacsek pushed a commit to sztrovacsek/django that referenced this pull request Mar 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.