Opened 9 years ago

Closed 6 years ago

#4540 closed Cleanup/optimization (wontfix)

Template.render() should verify context instance

Reported by: cephelo@… Owned by: munhitsu
Component: Template system Version: master
Severity: Normal Keywords:
Cc: tom@…, mateusz@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX:

Description

Currently, Template.render() doesn't assert that "context" really is a Context object. This leads to all sorts of fun problems when debugging code that happens to be sending a dict() instead of Context(). While most tags work fine with a dict instead, there are a few that don't, notably {% for %} and {% filter %} (possibly a few others). While I'm not a huge fan of forcing data types, you get really wonky error messages.

The change itself is trivial, but I'm not sure how to handle a regression test for this, as the tests currently force-cast the dicts into Contexts.

Attachments (1)

django-4540.diff (2.7 KB) - added by munhitsu 9 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 9 years ago by Chris Beaven

Triage Stage: UnreviewedDesign decision needed

I agree, but I'll push to a design decision from one of the core.

comment:2 Changed 9 years ago by Thomas Steinacher <tom@…>

Cc: tom@… added
Triage Stage: Design decision neededAccepted

I think that it should definitely do a check. Otherwise you get strange errors when using autoescaping: 'dict' object has no attribute 'autoescape'.

comment:3 Changed 9 years ago by anonymous

Owner: changed from nobody to munhitsu

comment:4 Changed 9 years ago by Tony Becker

Would it make sense to do the check AFTER an error has occurred, and report at that time that "Hey, you are sending a dict instead of a context?" Would there be any performance benefit versus doing the check every single time? It seems to me that type checking every time is a little unpythonic, and the main goal is to get better error messages.

comment:5 Changed 9 years ago by James Bennett

Personally I'd rather have the eventual error message be friendlier to say something like "did you pass a dictionary instead of a Context object?"; doing type checks or assertions to verify that we've actually been handed a Context is optimization for a case where things wouldn't work anyway (and would introduce some negligible overhead into the process).

comment:6 in reply to:  5 Changed 9 years ago by munhitsu

Replying to ubernostrum:

Personally I'd rather have the eventual error message be friendlier to say something like "did you pass a dictionary instead of a Context object?"; doing type checks or assertions to verify that we've actually been handed a Context is optimization for a case where things wouldn't work anyway (and would introduce some negligible overhead into the process).

Rising this error message will require a type check so I don't see difference

Replying to tbecker:

Would it make sense to do the check AFTER an error has occurred, and report at that time that "Hey, you are sending a dict instead of a context?" Would there be any performance benefit versus doing the check every single time? It seems to me that type checking every time is a little unpythonic, and the main goal is to get better error messages.

Every time can mean only on each template render execution. Not on every Node render. So we have already an optimisation here.

Changed 9 years ago by munhitsu

Attachment: django-4540.diff added

comment:7 Changed 9 years ago by munhitsu

Cc: mateusz@… added
Has patch: set

comment:8 Changed 7 years ago by Adam Nelson

Patch needs improvement: set

comment:9 Changed 6 years ago by Gabriel Hurley

Severity: Normal
Type: Cleanup/optimization

comment:10 Changed 6 years ago by Chris Beaven

Easy pickings: unset
Resolution: wontfix
Status: newclosed

I'm going to say that we don't need this as critically now. The error messages are at least raised in the render method now, since we do context.render_context.push() in there.

And like ubernostrum stated 3 years ago, doing type checks introduces overhead and no matter how negligible, it's still overhead which we don't need.

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