Opened 8 years ago

Closed 3 years ago

#7557 closed New feature (fixed)

ValueError does not catch all cases when trying to determine type of variable in template.Variable class

Reported by: Tobias McNulty Owned by: Steffen Zieger
Component: Template system Version: master
Severity: Normal Keywords:
Cc: bmispelon@…, Yohann Gabory, me@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

  File "/usr/lib/python2.5/site-packages/caktus/django/templatetags/__init__.py", line 59, in __init__
    self.kwargs = dict([(k, Variable(arg)) for k, arg in kwargs.items()])
  File "/usr/lib/python2.5/site-packages/django/template/__init__.py", line 628, in __init__
    self.literal = float(var)
TypeError: float() argument must be a string or a number

In my case "var" was a dict. obviously dict is not a supported variable type, so django should raise a more obvious error here.

Attachments (3)

7557-1.diff (564 bytes) - added by Matt McClanahan 8 years ago.
type_check.diff (1.1 KB) - added by Yohann Gabory 4 years ago.
use six.string_type instead of basestring for python3 compatibility. Added a test to check that a non string value raise a TypeError
7557_type_check_test_working.diff (1.1 KB) - added by Steffen Zieger 4 years ago.
Fixes the test and improves code

Download all attachments as: .zip

Change History (17)

comment:1 Changed 8 years ago by Malcolm Tredinnick

It's not clear from the report what you are doing to trigger this error. Without knowing that, it's difficult to choose from amongst possible solutions.

comment:2 Changed 8 years ago by Tobias McNulty

Sorry, here is how to reproduce it:

from django.template import Variable
Variable({})

Changed 8 years ago by Matt McClanahan

Attachment: 7557-1.diff added

comment:3 Changed 8 years ago by Matt McClanahan

Has patch: set
Triage Stage: UnreviewedDesign decision needed

TypeError makes sense, just not on the float call. Patch to re-raise it with a more descriptive error.

comment:4 Changed 6 years ago by Luke Plant

Severity: Normal
Type: New feature

comment:5 Changed 5 years ago by Alex Gaynor

Easy pickings: unset
Triage Stage: Design decision neededAccepted
UI/UX: unset

Marking as accepted, but this patch is wrong, Variable's constructor takes a basestring, and nothing else, we should just typecheck the input IMO.

comment:6 Changed 4 years ago by Baptiste Mispelon

Cc: bmispelon@… added
Easy pickings: set
Needs tests: set
Patch needs improvement: set

comment:7 Changed 4 years ago by Yohann Gabory

Cc: Yohann Gabory added

Just added a patch that raise if "var" is not a basestring.

comment:8 Changed 4 years ago by Baptiste Mispelon

This patch is better, but to ensure python3 compatibility, you need to use six.string_types instead of basestring.

You also need to add some tests for this new feature (maybe in tests/template_tests/test_parser.py seeing as there's already some tests for Variable in there).

Thanks.

Changed 4 years ago by Yohann Gabory

Attachment: type_check.diff added

use six.string_type instead of basestring for python3 compatibility. Added a test to check that a non string value raise a TypeError

comment:9 Changed 4 years ago by Steffen Zieger

Owner: changed from nobody to Steffen Zieger
Status: newassigned

Changed 4 years ago by Steffen Zieger

Fixes the test and improves code

comment:10 Changed 4 years ago by Steffen Zieger

Cc: me@… added

comment:11 Changed 4 years ago by Steffen Zieger

Needs tests: unset
Patch needs improvement: unset

comment:12 Changed 4 years ago by Yohann Gabory

@saz : your attachment "type_check_test_working" is a copy'n'past of "type_check". For what purpose did you made ​​it ?

comment:13 Changed 4 years ago by Steffen Zieger

Almost. Have a look at line 25 in your patch and lines 25 to 27 in my patch.

After applying your patch, running the tests has thrown an exception for me.
Changing the self.assertRaisses line as it is documented (http://docs.python.org/2/library/unittest.html#unittest.TestCase.assertRaises) everything is working as expected.

comment:14 Changed 3 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 7fec5a2240835af7c7f3accd64d9d894d4f92782:

Fixed #7557 -- Added type checking to Variable initialization.

Thanks tobias for the suggestion and boblefrag and saz for work on the
patch.

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