Code

Opened 7 years ago

Closed 6 years ago

#3192 closed enhancement (fixed)

[patch] Noisier bugs in context processors

Reported by: Jeremy Dunck <jdunck@…> Owned by: nobody
Component: Template system Version: master
Severity: normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description (last modified by jdunck)

I just spent an embarrassingly long time figuring out the cause of "'NoneType' object has no attribute 'has_key'" when rendering a template.

I had a buggy context processor which returned None under some conditions, which then made it onto the context stack.

It seems to me that Context.update could verify that it's getting an object that supports getitem since it counts on that (and give a helpful error if it's not the case), but I understand context needs to be high-performance, and generally update is called for context processors anyway.

Perhaps RequestContext.init could raise a TypeError.

Patch attached. Feel free to ignore if I'm the only one to be bitten by this. :-/

Attachments (2)

dict-from-context-processor.diff (553 bytes) - added by Jeremy Dunck <jdunck@…> 7 years ago.
dict-to-update.diff (559 bytes) - added by Jeremy Dunck <jdunck@…> 7 years ago.
Another diff in case preference is to enforce in Context.update rather than RequestContext.init

Download all attachments as: .zip

Change History (9)

Changed 7 years ago by Jeremy Dunck <jdunck@…>

Changed 7 years ago by Jeremy Dunck <jdunck@…>

Another diff in case preference is to enforce in Context.update rather than RequestContext.init

comment:1 Changed 7 years ago by anonymous

  • milestone Version 1.0 deleted

Milestone Version 1.0 deleted

comment:2 Changed 7 years ago by SmileyChris

  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Design decision needed

Looks just about ready for checkin - needs decision on best way to implement.

I feel the patch should be using a try/except, however

comment:3 Changed 7 years ago by ubernostrum

  • Resolution set to invalid
  • Status changed from new to closed

Unless I'm misunderstanding, this is invalid: a context processor *must* return a dictionary, even if it's empty, and the pattern for a processor which wants to return nothing is to return an empty dictionary (much as template nodes can render to an empty string if they don't want to output anything).

comment:4 Changed 7 years ago by jdunck

  • Description modified (diff)
  • Resolution invalid deleted
  • Status changed from closed to reopened

Re-opening for clarification. I'm not fighting the invalid decision, if my point is understood, but ubernostrum suggested that maybe it wasn't.

I agree that a context processor *must* return a dictionary. What I'm trying to address, here, is that it isn't immediately obvious when a bug exists in a context processor which causes it not to return the expected type.

If a context processor returns an object which can't be used as a dictionary, an error does not occur until much later, and then that error 1) kills template rendering rather than using TEMPLATE_STRING_IF_INVALID, and 2) doesn't hint that a context processor may have caused the problem.

...And I'm using hasattr...'getitem' rather than isinstance(..., dict) because Context itself doesn't inherit from dict, and smashing contexts together currently works, so I didn't want the change to start insisting on a dictionary.

Finally, I'm not using "try...except" because Context.update doesn't throw an exception when handed something other than a dictionary-- it happily appends None to the stack of *expected* dictionaries to be checked upon context lookup, i.e. Context.getitem.

Feel free to close as invalid before, but I hope I've made my rationale clearer.

comment:5 Changed 7 years ago by anonymous

+1 on this one, it's a 2-liner and if it saves anybody an hour or two it should go in. I'd make the message more explicit though.

comment:6 Changed 6 years ago by jacob

  • Triage Stage changed from Design decision needed to Accepted

comment:7 Changed 6 years ago by lukeplant

  • Resolution set to fixed
  • Status changed from reopened to closed

Fixed in r8181 (which mistakenly says it fixed #3912 instead of #3192

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.