Opened 7 years ago

Closed 17 months ago

Last modified 17 months ago

#27654 closed Cleanup/optimization (fixed)

Propogate alters_data value to subclasses

Reported by: vinay karanam Owned by: nobody
Component: Core (Other) Version: dev
Severity: Normal Keywords:
Cc: 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

Current implementation to prevent data changes within templates relies on the attribute alters_data = True.

By default this is set for all Model, QuerySet and ModelForm methods that can alter data.

But if someone overrides these methods, they need to explicitly set this attribute again.
It is common to override save method and not set this attribute.

Even a lot of popular third-party packages end up making this mistake.

These classes need to make sure that the attribute value is set for their subclasses methods if not explicitly overwritten.

Change History (7)

comment:1 by Tim Graham, 7 years ago

Component: UncategorizedCore (Other)
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

WIP PR

This ticket is a result of discussion in #27638 which suggested to solve the problem by issuing a rollback after rendering templates (closed as wontfix).

Last edited 7 years ago by Tim Graham (previous) (diff)

comment:2 by Asif Saifuddin Auvi, 6 years ago

Patch needs improvement: unset
Version: 1.10master

comment:3 by Carlton Gibson, 6 years ago

Resolution: wontfix
Status: newclosed

After looking at the implementation on the PR, the proposed metaclass solution is too much to given the relatively small improvement over the current alters_data solution.

The remaining suggestion from #27638 was to use the system checks framework to warn users of a problem. That might be worth a go. It would be able to capture the most common cases on Model classes certainly. (Whether we want to add that isn't clear: in lots of cases save is overridden without setting alters_data — to make that an error might be more annoying than a benefit, but it could at least be silenced.)

comment:4 by Shai Berger, 17 months ago

Resolution: wontfix
Status: closednew

Reopening the ticket following a discussion on the security mailing list, which led to https://github.com/django/django/pull/16149

comment:5 by Shai Berger, 17 months ago

Triage Stage: AcceptedReady for checkin

(in the last review the PR still had a couple of minor issues, but I believe it is ready for a merger review)

comment:6 by Mariusz Felisiak <felisiak.mariusz@…>, 17 months ago

Resolution: fixed
Status: newclosed

In e20c9eb:

Fixed #27654 -- Propagated alters_data attribute to callables overridden in subclasses.

Thanks Shai Berger and Adam Johnson for reviews and the implementation
idea.

comment:7 by Mariusz Felisiak <felisiak.mariusz@…>, 17 months ago

In e20c9eb:

Fixed #27654 -- Propagated alters_data attribute to callables overridden in subclasses.

Thanks Shai Berger and Adam Johnson for reviews and the implementation
idea.

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