Code

Opened 7 years ago

Closed 3 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 7 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 7 years ago by SmileyChris

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

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

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

  • Cc tom@… added
  • Triage Stage changed from Design decision needed to Accepted

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 7 years ago by anonymous

  • Owner changed from nobody to munhitsu

comment:4 Changed 7 years ago by 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.

comment:5 follow-up: Changed 7 years ago by 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).

comment:6 in reply to: ↑ 5 Changed 7 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 7 years ago by munhitsu

comment:7 Changed 7 years ago by munhitsu

  • Cc mateusz@… added
  • Has patch set

comment:8 Changed 4 years ago by adamnelson

  • Patch needs improvement set

comment:9 Changed 3 years ago by gabrielhurley

  • Severity set to Normal
  • Type set to Cleanup/optimization

comment:10 Changed 3 years ago by SmileyChris

  • Easy pickings unset
  • Resolution set to wontfix
  • Status changed from new to closed

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.