#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 , 8 years ago
| Component: | Uncategorized → Core (Other) |
|---|---|
| Patch needs improvement: | set |
| Triage Stage: | Unreviewed → Accepted |
comment:2 by , 7 years ago
| Patch needs improvement: | unset |
|---|---|
| Version: | 1.10 → master |
comment:3 by , 7 years ago
| Resolution: | → wontfix |
|---|---|
| Status: | new → closed |
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 , 2 years ago
| Resolution: | wontfix |
|---|---|
| Status: | closed → new |
Reopening the ticket following a discussion on the security mailing list, which led to https://github.com/django/django/pull/16149
comment:5 by , 2 years ago
| Triage Stage: | Accepted → Ready 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)
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).