I've opened up a ticket 7252 [1] with patch, but I'm not sure my patch
addresses the problem from the right angle, so if someone knows a
better way to go about fixing it, I'd like to open the discussion here
for better ways to fix this.
Essentially what's happening is that the manager is being inherited
properly from the abstract model, but each manager has a reference to
its parent model class, and that reference is not being updated. My
solution is to check in the descriptor if the calling model instance
matches the manager's reference, and if it doesn't, assign it. This
feels flimsy, as we probably shouldn't have to make this check at
all.
As far as I can tell, this can't be done in ModelBase.__new__, as the
reference is not yet set and therefore can't be checked.
On May 17, 5:38 am, "flo...@gmail.com" <flo...@gmail.com> wrote:
> I've opened up a ticket 7252 [1] with patch, but I'm not sure my patch
> addresses the problem from the right angle, so if someone knows a
> better way to go about fixing it, I'd like to open the discussion here
> for better ways to fix this.
There's a similar ticket to this at 7154 (http://
code.djangoproject.com/ticket/7154). It has a patch that works but I
am not sure if it is the most elegant method.
> Essentially what's happening is that the manager is being inherited
> properly from the abstract model, but each manager has a reference to
> its parent model class, and that reference is not being updated. My
> solution is to check in the descriptor if the calling model instance
> matches the manager's reference, and if it doesn't, assign it. This
> feels flimsy, as we probably shouldn't have to make this check at
> all.
> As far as I can tell, this can't be done in ModelBase.__new__, as the
> reference is not yet set and therefore can't be checked.
> There's a similar ticket to this at 7154 (http://
> code.djangoproject.com/ticket/7154). It has a patch that works but I
> am not sure if it is the most elegant method.
Whoops! Sounds like we're talking about the same thing. I thought
based on the summary that it was a different problem, because the
manager actually _does_ get copied over, but it just needs a slight
modification.
I do tend to like the implementation on that ticket better, as it gets
closer to the root of the problem. Are there any disagreements about
whether that patch should be applied?
On May 17, 10:57 am, "flo...@gmail.com" <flo...@gmail.com> wrote:
> > There's a similar ticket to this at 7154 (http://
> > code.djangoproject.com/ticket/7154). It has a patch that works but I
> > am not sure if it is the most elegant method.
> Whoops! Sounds like we're talking about the same thing. I thought
> based on the summary that it was a different problem, because the
> manager actually _does_ get copied over, but it just needs a slight
> modification.
> I do tend to like the implementation on that ticket better, as it gets
> closer to the root of the problem. Are there any disagreements about
> whether that patch should be applied?
I do not know if there are any disagreements (I found qsrf very
complicated to understand) but I am using the patch for the
newcomments project. There are a couple of other quirks with Abstract
Models which I was assured were trivial and would be done with the nfa
merge.
> > > There's a similar ticket to this at 7154 (http:// > > > code.djangoproject.com/ticket/7154). It has a patch that works > > > but I am not sure if it is the most elegant method.
I am the author of #7154 and its patch. I thought first that it is possible to fix this problem with less code if this is your understanding of elegant. But I had to note that it isn't. ;-P
Look at #7252. The patch of this ticket adds only 2 lines. But it is just an ugly hack. As Eric already noted it does not gets to the root of the problem. The descriptor object's __get__ method is not the place to handle such a case. The contibute_to_class method is the way to go. But because of when contribute_to_class is called for the abstract base model, we can not yet add the manager to the derived models. We have to store the attribute name of the manager in the model options. So the metaclass can handle this when deriving from an abstract model with custom managers.
It think this is the most clean and reliable way to solve this problem. But improvement purposes are welcome. ;)
> > Whoops! Sounds like we're talking about the same thing. I thought > > based on the summary that it was a different problem, because the > > manager actually _does_ get copied over, but it just needs a slight > > modification.
As far as I know it was only present in derived models because of derived models inherits all managers from the parent abstract model. But it is no "copied" as done by _copy_to_model, which was the reason for the behavior of inherited managers.
> > I do tend to like the implementation on that ticket better, as it > > gets closer to the root of the problem. Are there any > > disagreements about whether that patch should be applied?
Thanks. Although I wrote this patch, I have a disagreement. ;-P Tests are still missing. But I think I can wrote some test cases at the weekend or during next week.
Because of I needed the patch more urgent I actually thought by myself, I have already uploaded a new version including test cases.
I found a bug by the way, that breaks the ability to derive from a not abstract model, but I have fixed it. If you are already using the previous version of my patch, you should update now.
I think the patch is ready to get merged into trunk now, but I can not decide this. What does the maintainers think?
