Opened 18 years ago

Closed 9 years ago

#901 closed New feature (fixed)

Reload method for models

Reported by: andreas@… 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 Jacob, 18 years ago

Resolution: wontfix
Status: newclosed

This seems like a needless function; it's already possible to just re-look-up the object from the database.

comment:2 by anonymous, 17 years ago

but how? the docs aren't clear about this.

comment:3 by Simon Litchfield <simon@…>, 17 years ago

Resolution: wontfix
Status: closedreopened
Triage Stage: UnreviewedDesign 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 Jacob, 16 years ago

Resolution: wontfix
Status: reopenedclosed

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 Simon Litchfield <simon@…>, 16 years ago

Resolution: wontfix
Status: closedreopened

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 mpe, 16 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 James Bennett, 16 years ago

Resolution: wontfix
Status: reopenedclosed

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 Simon Litchfield <simon@…>, 16 years ago

Cc: simon@… added

comment:9 by brandon, 14 years ago

Resolution: wontfix
Status: closedreopened

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 Alex Gaynor, 14 years ago

Resolution: wontfix
Status: reopenedclosed

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 jnadro52, 14 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 Mikhail Korobov, 12 years ago

Cc: kmike84@… added
Easy pickings: unset
UI/UX: unset

comment:13 by anonymous, 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 izzaddin.ruhulessin@…, 11 years ago

Resolution: wontfix
Status: closednew

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 Carl Meyer, 11 years ago

Resolution: wontfix
Status: newclosed

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.

in reply to:  13 comment:16 by anonymous, 11 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 anonymous, 11 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 Simon Litchfield, 11 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 anonymous, 11 years ago

Funny, we've also come to the conclusion that making Django feature requests is a waste of time.

comment:20 by Russell Keith-Magee, 11 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 Simon Litchfield, 11 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 Aymeric Augustin, 11 years ago

Yes, it's all about the community, and the community is on the django-developers mailing list, not here.

comment:23 by Anssi Kääriäinen, 11 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 anonymous, 11 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 wim@…, 11 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 Aymeric Augustin, 11 years ago

Component: Core (Other)Database layer (models, ORM)
Has patch: unset
Resolution: wontfix
Status: closednew
Summary: [patch] Reload method for modelsReload method for models
Triage Stage: Design decision neededAccepted
Type: enhancementNew feature
Version: master

The discussion on django-developers concluded that this ticket should be reopened, and it contains a concrete proposal.

comment:27 by aarongc@…, 11 years ago

Cc: aarongc@… added

comment:28 by James Aylett, 11 years ago

Cc: james@… added

comment:29 by Koen Biermans <koen@…>, 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 Val Neekman, 10 years ago

Cc: un33kvu@… added

comment:31 by me@…, 10 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 me@…, 10 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 Tim Graham, 10 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 Mike Fogel, 10 years ago

Cc: mike@… added

comment:35 by Anssi Kääriäinen, 10 years ago

Owner: changed from Adrian Holovaty to Anssi Kääriäinen
Status: newassigned

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 Anssi Kääriäinen, 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 Danilo Bargen, 9 years ago

Cc: mail@… added

comment:38 by Anssi Kääriäinen, 9 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 Tim Graham, 9 years ago

Owner: changed from Anssi Kääriäinen to Tim Graham
Triage Stage: AcceptedReady for checkin

Will cleanup docs/docstrings a bit before committing.

comment:40 by Tim Graham <timograham@…>, 9 years ago

Resolution: fixed
Status: assignedclosed

In c7175fcdfe94be60c04f3b1ceb6d0b2def2b6f09:

Fixed #901 -- Added Model.refresh_from_db() method

Thanks to github aliases dbrgn, carljm, slurms, dfunckt, and timgraham
for reviews.

Note: See TracTickets for help on using tickets.
Back to Top