Opened 19 years ago
Closed 10 years ago
#901 closed New feature (fixed)
Reload method for models
Reported by: | Owned by: | Tim Graham | |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | normal | Keywords: | |
Cc: | simon@…, kmike84@…, aarongc@…, james@…, un33kvu@…, mike@…, mail@… | 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
reload is an additional model method that forces a models attributes to be reloaded from the database:
Index: django/core/meta/__init__.py =================================================================== --- /home/andreas/workspace/django_src/django/core/meta/__init__.py (revision 1396) +++ /home/andreas/workspace/django_src/django/core/meta/__init__.py (working copy) @@ -528,7 +560,8 @@ attrs['save'].alters_data = True attrs['delete'] = curry(method_delete, opts) attrs['delete'].alters_data = True - + attrs['reload'] = curry(method_reload, opts) + if opts.order_with_respect_to: attrs['get_next_in_order'] = curry(method_get_next_in_order, opts, opts.order_with_respect_to) attrs['get_previous_in_order'] = curry(method_get_previous_in_order, opts, opts.order_with_respect_to) @@ -775,6 +812,16 @@ def method_eq(opts, self, other): return isinstance(other, self.__class__) and getattr(self, opts.pk.attname) == getattr(other, opts.pk.attname) +def method_reload(opts, self): + pk_val = getattr(self, opts.pk.attname) + assert pk_val is not None, "%s can't be loaded because it doesn't have an ID." + kwargs = {'%s__exact' % opts.pk.attname : pk_val} + obj_list = function_get_list(opts, self._meta.get_model_module().Klass, **kwargs) + assert len(obj_list) == 1, \ + "Exactly one object should exist for %s %s -- %s objects exist!" % \ + (opts.pk.attname, pk_val, len(obj_list)) + self.__dict__ = obj_list[0].__dict__ + def method_save(opts, self): # Run any pre-save hooks. if hasattr(self, '_pre_save'):
Here's a snippet useful e.g. for Ticket #555 (DateTimeFields with auto_now and auto_now_add don't change in place):
def _post_save(self): self.reload()
Change History (40)
comment:1 by , 19 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
comment:3 by , 17 years ago
Resolution: | wontfix |
---|---|
Status: | closed → reopened |
Triage Stage: | Unreviewed → Design decision needed |
I've needed to reload() during save().
During save(), after calling the super().save(), sometimes I needed to update some related records, via the ORM and/or manual SQL.
I can't seem to set/replace self post save(); and I can't reload(); so I came up with the below hackish workaround for my specific model attribute (rtree) that was causing problems elsewhere by not being updated --
... super(Category, self).save() ... if cascade: self._save_children() # can't set/replace self, no reload() method. weird_magic. weird_magic = Category.objects.get(pk=self.id) self.rtree = weird_magic.rtree
Obviously, I'd prefer to --
... super(Category, self).save() ... if cascade: self._save_children() self.reload()
I was also surprised that the following didn't work. Trying to set self was completely ignored for some reason, presumably because it thought it already had the instance loaded --
... super(Category, self).save() ... if cascade: self._save_children() self = Category.objects.get(id=self.id)
comment:4 by , 17 years ago
Resolution: | → wontfix |
---|---|
Status: | reopened → closed |
I'm really not convinced by Simon's use case -- adding "reload()" only saves you a single line of code. Let's do our best to keep Django as svelte as possible.
comment:5 by , 17 years ago
Resolution: | wontfix |
---|---|
Status: | closed → reopened |
The problem isn't the extra line, it's the logic. If I go and tamper with my database directly, then obviously I would need to reload my models.
Since there is no reload, the next logical step would be to "just re-look-up the object from the database" as Jacob pointed out earlier in the ticket. But alas, lazy-loading prevents this from happening.
comment:6 by , 17 years ago
Can I double-+1 vote for this. I just spent half an hour wracking my brains trying to work out how to do this. Then I googled and found this ticket, and only then did it occur to me that foo = Foo.objects.get(id=foo.id) might work as a reload. So it's _not_ obvious how to do this IMHO.
comment:7 by , 17 years ago
Resolution: | → wontfix |
---|---|
Status: | reopened → closed |
Jacob's already wontfixed this twice. If there's a discussion to be had, have it on the mailing list instead of reopening this ticket over and over again.
comment:8 by , 17 years ago
Cc: | added |
---|
comment:9 by , 15 years ago
Resolution: | wontfix |
---|---|
Status: | closed → reopened |
I gingerly, and with trepidation, would like to re-open this ticket because reload() would cover an important use case that did not get a hearing in the above discussion: that you might already have handed the object off to some other library, which is keeping a reference from somewhere deep in its data structures for use when you make a final call to it. If one or more data structures that are being built already hold the object, then it is not reasonable to have to track down the object and try to swap in a new copy everywhere. There should be a simple way to have the existing object's attributes update in-place.
comment:10 by , 15 years ago
Resolution: | → wontfix |
---|---|
Status: | reopened → closed |
This has been wontfixed by a core contributer twice, plus once by James. If you would like to discuss this issue please bring it up on the mailing list.
comment:11 by , 15 years ago
Sorry if I'm resurrecting this from the dead, but I stumbled upon it googling for this myself, and thought sticking this here might help some people. Sometimes it is useful having a reload method, especially when the object is calling model.update() on it's primary key (for reasons of data consistency, this is necessary sometimes) from a method internal to that object. I just use an abstract base model for all of my classes and implement the following:
def refresh_from_db(self): """Refreshes this instance from db""" from_db = self.__class__.objects.get(pk=self.pk) fields = self.__class__._meta.get_all_field_names() #for each field in Model for field in fields: try: setattr(self, field, getattr(from_db, field)) #update this instances info from returned Model except: continue
No, I don't recommend eating all exceptions... just kept it that way in this example for simplicity. Any critiques are welcome. Hope this helps someone.
comment:12 by , 13 years ago
Cc: | added |
---|---|
Easy pickings: | unset |
UI/UX: | unset |
follow-up: 16 comment:13 by , 12 years ago
I had more luck with:
def refresh_from_db(self): """Refreshes this instance from db https://code.djangoproject.com/ticket/901 """ from_db = self.__class__.objects.get(pk=self.pk) for field in self.__class__._meta.fields: setattr(self, field.name, getattr(from_db, field.name))
But I don't know enough to know if it's correct, only that it seems to work.
comment:14 by , 12 years ago
Resolution: | wontfix |
---|---|
Status: | closed → new |
Shouldn't objects be reloaded from the database by default, after calling the save() method? Say you have stored procedures, triggers or rules modifying the data. Django expects itself to be the only data-modifying user to the database, ever, but in many cases this assumption does not hold true.
comment:15 by , 12 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
For the fourth time in this thread: *please* do not reopen tickets closed wontfix by a core developer. If you'd like to advocate for this feature, do so on the mailing list, and if consensus is reached _then_ the ticket can be reopened.
comment:16 by , 12 years ago
Replying to anonymous:
I had more luck with:
def refresh_from_db(self): """Refreshes this instance from db https://code.djangoproject.com/ticket/901 """ from_db = self.__class__.objects.get(pk=self.pk) for field in self.__class__._meta.fields: setattr(self, field.name, getattr(from_db, field.name))But I don't know enough to know if it's correct, only that it seems to work.
If we're not going to get a method to do this for us, it would be nice if one of the core developers could bless or provide a code sample to update 'self' with the latest data from the db instance.
I've seen this one, as well as one that updates self.dict : http://stackoverflow.com/questions/4377861/reload-django-object-from-database
Which method can we depend upon still working in the future?
comment:17 by , 12 years ago
So I just came across the presentation "Why Django Sucks and How we Can Fix it" at http://blip.tv/djangocon/why-django-sucks-and-how-we-can-fix-it-4131303
As I watched it, this bug immediately came to mind. I feel it is a perfect example of exactly what Eric was talking about in his talk.
There are multiple StackOverflow questions asking how to do this, there are several answers there on how to do this, none of which are ensured to keep future compatibility. Some are stated as incomplete ("Oh, this won't work if you have foreign keys"), and potentially have other quirks. And there is no documented way that suggests the best/supported way to do this.
There's a suggested patch above.
Note: The use case for this request isn't:
obj = Foo.objects.get(pk=1)
<some external side effects>
obj = obj.reload() # i.e., obj = Foo.objects.get(pk=obj.id)
It is people wanting to do a reload from *within* the model. And "self = self.objects.get(pk=self.id)" isn't going to do it, obviously!
The use case is more like:
class Foo(Model):
def blah():
# maybe we need to use .update() for some reason, like
# atomicity. i.e., only update iff x is 0 as a atomicity
# check.
Foo.objects.filter(pk=self.id, x=0).update(x=1, y=10)
<maybe some other side effects>
# make sure I have the latest attributes
self.reload()
...
I don't want the client of the Model Foo to have to know "Oh, well, I need to refresh my copy of the object after I call blah()". And other references to the object that called blah() will still be wrong.
comment:18 by , 12 years ago
Eric makes an important point. I gave up a long time ago trying to make simple feature suggestions like these, no matter how big/small (6yrs ago by the look of this thread).
comment:19 by , 12 years ago
Funny, we've also come to the conclusion that making Django feature requests is a waste of time.
comment:20 by , 12 years ago
Making feature requests is in no way a waste of time. Every version of Django has had features that the previous version didn't have. Those features were requested by *someone*. So it is demonstrably *not* futile.
However, expecting that merely *making* a feature request will in some way magically result in your feature being added? That *is* futile.
The difference between the features that *have* been added, and the features that *haven't* been added, is that someone actually did the work. And I don't *just* mean writing a patch, either. I mean someone engaged with the community. Had a discussion. Resolved any issues or concerns that existed.
Let's take this exact ticket as an example. If you actually *read* the discussion on this ticket, members of the core team have expressed concerns about the idea, and have said on *three separate occasions* what needs to happen in order for this issue to move forward: *Raise the issue on Django-developers*.
So, if this is such an important topic, such an essential feature, the proponents of this idea *must* have raised it on django-developers, right?
Well, no. As far as I can make out, it's been raised exactly once, in 2008, and after a single post from Simon, and a single response from Malcolm, the conversation died. Ticket #901 came up in passing in 2009 on a django-users post. So not only hasn't there been a vibrant developer debate, there's hasn't been a flood of requests for the feature on django-users, either.
Suggesting features is only futile if you are labouring under the illusion that the core team is sitting here waiting for ideas of things to do. Let me shatter that illusion for you. We have plenty of things to do. What we don't have, and what we want, is people offering to help out.
If you want it, you have to make it happen.
comment:21 by , 12 years ago
I think it's important we as a community *acknowledge* there is an issue here. Eric, myself, and plenty of others seem to think so. It's not unfixable either.
comment:22 by , 12 years ago
Yes, it's all about the community, and the community is on the django-developers mailing list, not here.
comment:23 by , 12 years ago
FWIW I think this feature is a good idea - but the mailing list is the right place to discuss this.
comment:24 by , 12 years ago
Omg, huge +1 here. Whenever I use model forms I have to query for objects again if I don't save them (i.e., form data is not valid), which wastes a query.
comment:25 by , 12 years ago
Hi people,
Thanks for taking the time to respond to this ticket. A question has been raised on the Django mailing list if we can reconsider re-opening it. The mailing thread is here:
https://groups.google.com/forum/?fromgroups=#!topic/django-developers/Dcqq7zr2VZo
In addition, if you want to discuss the responsiveness of django community in general or on this ticket, you can do so here:
https://groups.google.com/forum/?fromgroups=#!topic/django-developers/DUQtBrM2iTs
I am very sorry to hear that people feel unfriendly received and not appreciated. For myself, since Luke Plant gave me a long and detailed answer on just a question I had, and which I put on the mailing list, I have been feeling very welcome here.
Thank you all for your time spent in this community.
Wim
PS For the record, in a Google Group you can become a member of a mailing list and read all the mail online, while no mail is being sent to you.
comment:26 by , 12 years ago
Component: | Core (Other) → Database layer (models, ORM) |
---|---|
Has patch: | unset |
Resolution: | wontfix |
Status: | closed → new |
Summary: | [patch] Reload method for models → Reload method for models |
Triage Stage: | Design decision needed → Accepted |
Type: | enhancement → New feature |
Version: | → master |
The discussion on django-developers concluded that this ticket should be reopened, and it contains a concrete proposal.
comment:27 by , 12 years ago
Cc: | added |
---|
comment:28 by , 11 years ago
Cc: | added |
---|
comment:29 by , 11 years ago
Has patch: | set |
---|---|
Patch needs improvement: | set |
A simple first implementation: https://github.com/planop/django/tree/ticket_901 based on the ideas in the django-developers thread.
Adds modelinstance.refresh([*fieldnames]) method to reload (part of) the object from the database.
comment:30 by , 11 years ago
Cc: | added |
---|
comment:31 by , 11 years ago
Looking at the discussion on the django-developers mailing list, several developers have expressed concern about messy corner cases.
Koen: you've set 'patch needs improvement'. What issues are remaining with the patch?
Looking at this issue, I think adding a Model.fresh_copy method would be sufficient for many use cases and would be straightforward to implement. Certainly all my use cases would be satisfied. I'm primarily interested in unit tests, where I've made several requests and I want to verify the state of the database.
comment:32 by , 11 years ago
I've opened a pull request: https://github.com/django/django/pull/2194 that defines a Model.get_again method. I think this is a good compromise. I'd love feedback.
comment:33 by , 11 years ago
You can find the concrete proposal that was agreed to on the mailing list. The latest pull request doesn't address the problem this way.
The patch from Koen looks more on the right track, but I haven't reviewed it in detail.
comment:34 by , 11 years ago
Cc: | added |
---|
comment:35 by , 10 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Now that Model.from_db() has been added I think this will be important addition - the most important use case being deferred field loading and altering the loading behavior in that case. Currently for example implementing save only modified fields feature will be impossible to do for deferred fields.
I am going to go with the concrete API proposal mentioned above
Model.refresh() reloads all non-deferred local field values (that is, all fields in the current model which have a database column). In addition refresh() will make sure that cached values dependent of the reloaded values will be cleared. That is, if you do foo.refresh(), then when foo.user_id is reloaded foo.user will be cleared (assuming the reloaded user_id is different from the original value). This is to ensure that next access to foo.user will reload the related object, too. The refresh() method makes sure descriptors are honored, for example custom field's to_python() method will be called for refreshed values. The refresh() method accepts *args, so that one can specify which fields to reload. This is useful so that deferred attribute loading can use refresh(), and by overriding refresh() it will be possible to customize how deferred loading happens. Deferred field loading is altered to use .refresh().
comment:36 by , 10 years ago
Needs documentation: | set |
---|
I implemented the above design plan in PR https://github.com/django/django/pull/2882.
The changes to the plan are:
- the method name is refresh_from_db()
- no *args, instead kwarg fields containing an iterable of fields to refresh
- added using kwarg, by which one can alter the database to refresh from
In addition Model.get_deferred_fields() was added. This is needed in public API so that it is possible to create a refresh_from_db() method that reloads all deferred fields when one of those is accessed.
As discussed on mailing list, the reloading doesn't affect non-local values. For example, if you have done prefetching, the prefetched values will not be invalidated. It seems impossible to code a refresh_from_db() method that actually clears all database dependent value from the model, for the simple reason that in many cases Django can't even know what database dependent values are.
I wonder if we need better handling for o2o fields. This has two cases:
1) o2o defined on refreshed model - when doing refresh, should we also clear the cached value on the pointed model?
2) o2o defined on remote model - when doing refresh, should we make sure that the cached value on refreshed model is still valid?
So, with models:
class M1: pass class M2: m1 = O2OField(M1)
case 2)'s question is if we load instance m1_1 <-> m2_1 from db, and then m2_1.o2o is changed to point to m1_2, when doing m1_1.refresh() should we notice that m1_1.m2 has changed to None? In case 1), if we refresh m2_1() we do set m1 to None. But should we also set m1_1.m2 to None, too?
Both of these are getting to "clear all database defined state on the model" category. Still, I am not sure if we should do at least 1). If 1) is implemented, then it is possible to force refresh of the reverse o2o by doing m1_1.m2.refersh_from_db().
comment:37 by , 10 years ago
Cc: | added |
---|
comment:38 by , 10 years ago
Needs documentation: | unset |
---|---|
Patch needs improvement: | unset |
I think this is now ready for checkin, but as this is a non-trivial new feature I'd like to get this reviewed before marking ready for checkin.
comment:39 by , 10 years ago
Owner: | changed from | to
---|---|
Triage Stage: | Accepted → Ready for checkin |
Will cleanup docs/docstrings a bit before committing.
comment:40 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
This seems like a needless function; it's already possible to just re-look-up the object from the database.