Opened 12 years ago

Closed 10 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: no UI/UX: no

Description (last modified by Jeremy Dunck)

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@…> 12 years ago.
dict-to-update.diff (559 bytes) - added by Jeremy Dunck <jdunck@…> 12 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 12 years ago by Jeremy Dunck <jdunck@…>

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

Attachment: dict-to-update.diff added

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

comment:1 Changed 12 years ago by (none)

milestone: Version 1.0

Milestone Version 1.0 deleted

comment:2 Changed 12 years ago by Chris Beaven

Patch needs improvement: set
Triage Stage: UnreviewedDesign 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 11 years ago by James Bennett

Resolution: invalid
Status: newclosed

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 11 years ago by Jeremy Dunck

Description: modified (diff)
Resolution: invalid
Status: closedreopened

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 11 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 11 years ago by Jacob

Triage Stage: Design decision neededAccepted

comment:7 Changed 10 years ago by Luke Plant

Resolution: fixed
Status: reopenedclosed

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

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