Code

Opened 6 years ago

Closed 10 months 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 Owned by: saz
Component: Template system Version: master
Severity: Normal Keywords:
Cc: bmispelon@…, boblefrag, 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 mattmcc 6 years ago.
type_check.diff (1.1 KB) - added by boblefrag 15 months 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 saz 14 months ago.
Fixes the test and improves code

Download all attachments as: .zip

Change History (17)

comment:1 Changed 6 years ago by mtredinnick

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 6 years ago by tobias

Sorry, here is how to reproduce it:

from django.template import Variable
Variable({})

Changed 6 years ago by mattmcc

comment:3 Changed 6 years ago by mattmcc

  • Has patch set
  • Triage Stage changed from Unreviewed to Design decision needed

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

comment:4 Changed 3 years ago by lukeplant

  • Severity set to Normal
  • Type set to New feature

comment:5 Changed 3 years ago by Alex

  • Easy pickings unset
  • Triage Stage changed from Design decision needed to Accepted
  • 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 15 months ago by bmispelon

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

comment:7 Changed 15 months ago by boblefrag

  • Cc boblefrag added

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

comment:8 Changed 15 months ago by bmispelon

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 15 months ago by boblefrag

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 14 months ago by saz

  • Owner changed from nobody to saz
  • Status changed from new to assigned

Changed 14 months ago by saz

Fixes the test and improves code

comment:10 Changed 14 months ago by saz

  • Cc me@… added

comment:11 Changed 14 months ago by saz

  • Needs tests unset
  • Patch needs improvement unset

comment:12 Changed 14 months ago by boblefrag

@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 14 months ago by saz

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 10 months ago by Tim Graham <timograham@…>

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

In 7fec5a2240835af7c7f3accd64d9d894d4f92782:

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

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

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.