New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #28198 -- Prevented model attributes from overriding deferred fields. #9126

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@denys-tarykin

denys-tarykin commented Sep 22, 2017

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

This comment has been minimized.

Show comment
Hide comment
@denys-tarykin

denys-tarykin Sep 22, 2017

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

denys-tarykin commented Sep 22, 2017

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

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Sep 22, 2017

Member

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.

Member

timgraham commented Sep 22, 2017

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

This comment has been minimized.

Show comment
Hide comment
@ryanhiebert

ryanhiebert Sep 26, 2017

Contributor

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?

Contributor

ryanhiebert commented Sep 26, 2017

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

This comment has been minimized.

Show comment
Hide comment
@denys-tarykin

denys-tarykin Sep 27, 2017

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

denys-tarykin commented Sep 27, 2017

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

This comment has been minimized.

Show comment
Hide comment
@scorpp

scorpp Oct 18, 2017

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

scorpp commented Oct 18, 2017

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

This comment has been minimized.

Show comment
Hide comment
@ryanhiebert

ryanhiebert Nov 3, 2017

Contributor

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

Contributor

ryanhiebert commented Nov 3, 2017

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.

Show outdated Hide outdated django/db/models/base.py Outdated
Show outdated Hide outdated django/db/models/base.py Outdated
Show outdated Hide outdated django/db/models/base.py Outdated
Show outdated Hide outdated django/db/models/base.py Outdated
Show outdated Hide outdated django/db/models/base.py Outdated
Show outdated Hide outdated tests/defer/tests.py Outdated
Show outdated Hide outdated tests/defer/tests.py Outdated
Show outdated Hide outdated django/db/models/base.py Outdated
Show outdated Hide outdated tests/defer/models.py Outdated
Show outdated Hide outdated tests/defer/tests.py Outdated

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

Show outdated Hide outdated django/db/models/base.py Outdated
Show outdated Hide outdated django/db/models/base.py Outdated
Show outdated Hide outdated docs/releases/1.11.7.txt Outdated
@denys-tarykin

This comment has been minimized.

Show comment
Hide comment
@denys-tarykin

denys-tarykin Nov 9, 2017

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

denys-tarykin commented Nov 9, 2017

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

This comment has been minimized.

Show comment
Hide comment
@ryanhiebert

ryanhiebert Nov 16, 2017

Contributor

@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.

Contributor

ryanhiebert commented Nov 16, 2017

@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.

Fixed #28198 -- Model attributes does not overridden by attributes
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

This comment has been minimized.

Show comment
Hide comment
@denys-tarykin

denys-tarykin Nov 20, 2017

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

denys-tarykin commented Nov 20, 2017

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():

# 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()?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment