I agree with you in principle but Django has a very strict policy about only documented behavior being part of the public API contract. You are right that switching to .update() would break even more things and we try to keep backwards incompatible changes to a minimum so we wouldn’t try that. I understand that every change affects someone and in the worst case it really breaks a lot for some people. But if we would keep everything the same we could just freeze Django and stop developing it. In this case the performance and correctness gains seemed to be worth it (imo), and while annoying for you you can still work around the new behavior (if I understood correctly). Yes it might be more work for you and we try to keep it minimal, but sometimes it hits some people harder.
This is fair. I appreciate you empathizing and I do understand that any change has the possibility to effect someone and that is sometime the cost of progress.
Yes I understand (and have already implemented) mitigation for the behavior change.
However, I still argue it would be valuable to give the user some control/flexibility over the manner in which update_or_create calls the objects save method. But if this breaks Django’s design rules or is just not that popular of a feature request because of my unique use case, I guess we have simply come to an impasse. I Appreciate everyone’s time discussing this matter.
The main problem that I see with your suggestion is that there might be code in Django (like Simon pointed out) where you cannot control what is passed to save/update_or_create and we cannot expose this fully up the stack. So it feels like a half-baked solution to me (I fully understand that it might work in your specific case but it doesn’t feel like a solution that covers the 80% usecase)
Yeah I suppose if update_or_create was exclusively a public method its a no brainer. Since it is apparently not, you would need to modify your model to be prepared to handle any method of being called anyway. I get it. I’m still grumpy but I get it.
Hey everyone!
I recently created a ticket to report a problem I experienced regarding this same update_or_create and the fields selection for update: #35125 (Parent model fields are ignored when preparing the update_fields) – Django
tl;dr:
Parent model fields are not picked up for update, meaning that if parent models contain auto_now=True fields, then they are not updated.
class A(model.Model):
updated_at = models.DateTimeField(auto_now=True)
something = models.BooleanField(default=True)
# in another app
class B(A):
name = models.CharField()
When B.objects.update_or_create(name="x", defaults={"something": False}) is called, the updated_at field is not picked up for update, staying the same.
Took a look at the source code and this seems to be the troubling line. Perhaps it should read for field in self.model._meta._non_pk_concrete_field_names instead? I am unfamiliar with Django codebase, I am not sure if there is a better method to pick all model’s fields, including parent. I noticed the check before that line uses that attribute instead, which seems to pick up all model fields (including parent’s).
I can submit a patch if I get some thumbs up on this change. Not sureof any other repercussions of this change so I accept some guidance from someone more experience with the codebase to test this change.
Side note: Please open up a new topic for this discussion. This is a different topic than the original post.
When you do so, please identify whether “A” is an abstract parent class or a concrete parent class. It would make a difference.
Is it a different topic though? Because it seems to me like it is another example of the change to update_or_create behavior having unintended consequences on legacy code. That fits quite well with the topic at hand here.
I was told in the ticket to mention in this specific topic as might be related - it seemed to me.
In the ticket I did mention it was not an abstract class but forgot here. However, the code does not mention being abstract because it isn’t.
If not related, I can open up a new discussion.
Yes, please open up a different topic.
I agree, it is related. Both issues point back to update_or_create changing its behavior on passing update_fields which is clearly having unintended consequences.
Since the ticket has a small amount of new life I’ll make another interjection. I still haven’t heard an argument to the upside of the behavior change besides the following:
1.) The very marginal performance improvement stated in the docs “There will be a slight performance benefit from preventing all of the model fields from being updated in the database”
and
2.) The principle argument of it feels more correct.
These upsides don’t seem to be worth the issues it appears to be causing, especially without providing any ability to bypass the new behavior.
Also let me ask this question to make my own principle argument. When you exclude fields from a ModelForm in an UpdateView, does it it make sure update_fields has only the fields present in the form when the save is called? If not it seems like there is the same “performance” and “correctness” opportunity there. (for the record I am not suggesting to do this.)