Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#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 Changed 6 years ago by Tim Graham

Triage Stage: UnreviewedSomeday/Maybe
Type: UncategorizedCleanup/optimization

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?

comment:2 Changed 6 years ago by vinay karanam

I've sent a PR with initial draft.

All the testcases are passing except for StartProject.
Any suggestions in dealing with that.

comment:3 Changed 6 years ago by Simon Charette

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 Changed 6 years ago by Aymeric Augustin

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 Changed 6 years ago by Tim Graham

Resolution: wontfix
Status: newclosed

Vinay, please open a new ticket if you think either solution suggested by Simon are worth pursuing.

comment:6 Changed 6 years ago by vinay karanam

I'll try implementing Simon's approach in #27654.

Thank you all for the feedback.

Last edited 6 years ago by Tim Graham (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top