Fixed #28198 -- Prevented model attributes from overriding deferred fields. by denys-tarykin · Pull Request #9126 · django/django

denys-tarykin · 2017-09-22T09:22:12Z

Attributes from parent classes will not overload attributes in child classes. During model creation process, attributes which are dublicated in parent and child classes will be removed. That attributes will restored for all parent classes after model creation process

denys-tarykin · 2017-09-22T10:23:18Z

Hi. Can you help me with checking of my pull request? Most of tests are failed. But in the logs I see that unit tests are was not started because of jgit exception

timgraham · 2017-09-22T14:49:47Z

If you amend your commit and push it to the branch, it should trigger the build again. You'll need to add a regression test anyway.

ryanhiebert · 2017-09-26T04:54:42Z

How will this affect methods in subclassed models? For example, if Parent has a method, and Child overrides it and calls super(), what will happen?

denys-tarykin · 2017-09-27T09:04:58Z

This fix is not affect to methods and override only fields. Regarding your example, it possible to execute method of parent class in overridden method of child model

scorpp · 2017-10-18T15:07:31Z

any updates on this? who can decide if we accept it?

ryanhiebert

LGTM. The example issue I mentioned on the ticket looks to be resolved, and the test seems to be a sufficient coverage to avoid regression.

ryanhiebert · 2017-11-03T19:30:45Z

Is there a committer willing to look at this patch? The tests pass, the bug is fixed, and a regression test is added. Thanks!

timgraham

Please squash commits when updating and try to follow the commit message format.

timgraham · 2017-11-03T23:35:39Z

django/db/models/base.py

We generally try to describe the issue sufficient so that a ticket reference isn't needed.

timgraham · 2017-11-03T23:35:41Z

django/db/models/base.py

The comment needs improvement. I don't know what "duplicate public attributes" means.

timgraham · 2017-11-03T23:36:42Z

django/db/models/base.py

Use hanging indent as described in Python coding style.

timgraham · 2017-11-03T23:36:44Z

django/db/models/base.py

This condition could use a comment with an explanation. '__' might be replaced with LOOKUP_SEP if that's what it's checking.

isn't it checking for name mangling and/or dunder methods?

timgraham · 2017-11-03T23:37:09Z

django/db/models/base.py

Again, the comment isn't really clear to me.

timgraham · 2017-11-03T23:46:45Z

tests/defer/tests.py

Use assertIs(..., False) to check for boolean attributes.

timgraham · 2017-11-03T23:47:46Z

tests/defer/tests.py

I think a docstring would be useful to explain the expected behavior. See Python coding style for docstring guidelines.

timgraham · 2017-11-03T23:49:28Z

django/db/models/base.py

if attr_per_parent: is simpler -- does the current implementation have an advantage over that? (It looks like it requires more computation.) Same comment below.

timgraham · 2017-11-03T23:50:52Z

tests/defer/models.py

I think you could edit some existing models rather than adding three new models. We try to avoid adding new models since it decreases the runtime of the tests.

timgraham · 2017-11-03T23:50:58Z

tests/defer/tests.py

I'm not sure that a separate test class is needed.

adamchainz · 2017-11-07T15:52:15Z

django/db/models/base.py

this doesn't look right

adamchainz · 2017-11-07T15:52:33Z

django/db/models/base.py

no line continuations, instead use parentheses, see https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/coding-style/

adamchainz · 2017-11-07T15:53:40Z

docs/releases/1.11.7.txt

it looks like you've merged too much stuff into this branch. you probably want to rebase on top of master, rather than merge. use git rebase --interactive to select only your commits when you do so, to avoid pulling in these other changes.

denys-tarykin · 2017-11-09T11:19:46Z

I've updated my commit. But I did not update tests models. Because any changes in existing models will affect on existing test, and in this case my commit will contains updates which are not related to issue

ryanhiebert · 2017-11-16T00:12:15Z

@denys-tarykin : It looks like there are a couple comments, even without the model changes requested, that have not been addressed. Would you like some assistance addressing those? If so I might be able to make a PR to your branch.

from parent classes Implemented logic that update the model creating process. The bases in `super_new(cls, name, bases, new_attrs)` can contain the implementation for a attributes which also provided in 'new_attrs'.Before model creating this implementations are removing from parent classes. This fix is necessary because during defer reading of field the result object will not contain implementation from parent class

denys-tarykin · 2017-11-20T08:49:15Z

I've updated the code that is related to your comments

carltongibson

I'm not, as yet, convinced by this.

The root cause of the issue here comes from Field.contribute_to_class():

django/django/db/models/fields/__init__.py

Lines 699 to 703 in a87189f

    
           # Don't override classmethods with the descriptor. This means that 
        
           # if you have a classmethod and a field with the same name, then 
        
           # such fields can't be deferred (we don't have a check for this). 
        
           if not getattr(cls, self.attname, None): 
        
               setattr(cls, self.attname, DeferredAttribute(self.attname, cls))

It would be nice to address it there, rather than in ModelBase.__new__().

If we're going to remove and then replace attributes on parent classes,
we need tests to ensure that logic is correct.

I suggest extracting two methods, or helper functions, to encapsulate the logic
in the two added blocks.

Each of these can then have test cases demonstrating the expected behaviour, and
that it's correct. The two can also be tested together to demonstrate that the
replacement always works as expected.

In general, rather than actually removing/replacing attributes, could we merely
generate a list that could be checked against in Field.contribute_to_class()?

carltongibson

I'm going to close this in favor of pursuing the fix in Field.contribute_to_class() that's being worked on in #11337.

IMO, only if a fix there is unavailable should we consider anything along these lines in ModelBase.__new__().

Thanks for the effort @denys-tarykin.

ryanhiebert approved these changes Oct 19, 2017

View reviewed changes

timgraham reviewed Nov 3, 2017

View reviewed changes

timgraham changed the title ~~Bug#28198. Fixed model attributes.~~ Fixed #28198 -- Prevented model attributes from overriding deferred fields. Nov 4, 2017

adamchainz reviewed Nov 7, 2017

View reviewed changes

denys-tarykin force-pushed the fixed_dublicated_attributes branch 9 times, most recently from 0cb453a to 3a9e5c2 Compare November 9, 2017 09:03

scorpp mentioned this pull request Nov 16, 2017

bug#28198. Fixed attributes. Now all attributes which are in parent a… #9118

Closed

denys-tarykin force-pushed the fixed_dublicated_attributes branch from 3a9e5c2 to 1cfe955 Compare November 17, 2017 12:30

denys-tarykin force-pushed the fixed_dublicated_attributes branch from 1cfe955 to 307a82f Compare November 17, 2017 12:39

carltongibson suggested changes Jan 31, 2018

View reviewed changes

carltongibson self-requested a review June 21, 2019 14:06

carltongibson self-assigned this Jan 22, 2020

carltongibson reviewed Jan 29, 2020

View reviewed changes

carltongibson closed this Jan 29, 2020

	# Don't override classmethods with the descriptor. This means that
	# if you have a classmethod and a field with the same name, then
	# such fields can't be deferred (we don't have a check for this).
	if not getattr(cls, self.attname, None):
	setattr(cls, self.attname, DeferredAttribute(self.attname, cls))

Uh oh!

Conversation

denys-tarykin commented Sep 22, 2017

Uh oh!

denys-tarykin commented Sep 22, 2017

Uh oh!

timgraham commented Sep 22, 2017

Uh oh!

ryanhiebert commented Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

denys-tarykin commented Sep 27, 2017

Uh oh!

scorpp commented Oct 18, 2017

Uh oh!

ryanhiebert left a comment

Choose a reason for hiding this comment

Uh oh!

ryanhiebert commented Nov 3, 2017

Uh oh!

timgraham left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

denys-tarykin commented Nov 9, 2017

Uh oh!

ryanhiebert commented Nov 16, 2017

Uh oh!

denys-tarykin commented Nov 20, 2017

Uh oh!

carltongibson left a comment

Choose a reason for hiding this comment

Uh oh!

carltongibson left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ryanhiebert commented Sep 26, 2017 •

edited

Loading