#27638 closed Cleanup/optimization (wontfix)
Rollback data changes made inside templates
Reported by: | vinay karanam | Owned by: | nobody |
---|---|---|---|
Component: | Template system | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Someday/Maybe | |
Has patch: | no | 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 endup making this mistake.
I propose that we rollback any data changes that are done during template rendering.
Either by savepoint rollback or by setting autocommit to False
during rendering.
Change History (6)
comment:1 by , 8 years ago
Triage Stage: | Unreviewed → Someday/Maybe |
---|---|
Type: | Uncategorized → Cleanup/optimization |
comment:2 by , 8 years ago
I've sent a PR with initial draft.
All the testcases are passing except for StartProject
.
Any suggestions in dealing with that.
comment:3 by , 8 years ago
Coupling the database layer with the template one looks like a really bad idea to me.
What if you want to use the template layer without configuring a database? What if your template ends up taking a lot of time to render and keeps a transaction open the whole time?
While the alters_data
approach is not perfect I think that it offers a good enough solution paired with a convention of naming functions and methods appropriately.
Now a small things that could be done to improve the situation could be to either automatically mark overridden save()
/delete()
methods as alters_data = True
in ModelBase.__new__
or have system checks to warn developers about this edge case.
comment:4 by , 8 years ago
I agree that such hardcoded coupling is inappropriate.
This is reasonably easy to achieve with a custom template backend — all you have is to override the render method. I think that solution is appropriate for that use case.
comment:5 by , 8 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
Vinay, please open a new ticket if you think either solution suggested by Simon are worth pursuing.
comment:6 by , 8 years ago
I'll try implementing Simon's approach.
Thank you all for the feedback.
I'm not sure if it's feasible and/or a good idea. Backwards-compatibility could be a concern. Can you provide a patch or raise the idea on the DevelopersMailingList to get feedback?