Code

Opened 5 years ago

Closed 4 years ago

Last modified 3 years ago

#11687 closed (fixed)

The 'add' template filter only works for integers, and can fail noisily

Reported by: thepointer Owned by: gruszczy
Component: Template system Version: 1.1
Severity: Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The 'add' template tag attempts to convert both arguments to ints using int() and there's no try/except around it (thus making it noisy if you try and add two objects which are not ints). In addition, there's no reason why it should be limited to only adding together integers, it should be able to support any two objects that can be added together.

Attachments (1)

add_filter.diff (2.6 KB) - added by gruszczy 5 years ago.
coercing to int, then ducktyping + tests + docs

Download all attachments as: .zip

Change History (24)

comment:1 Changed 5 years ago by gruszczy

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

You mean 'add' filter, not the tag right?

Allowing any two objects to be added together isn't that easy. If you use this filter on an object that can be sumed with an int, the filter must somehow know to coerce the argument to int. And what if you want to add a string to a string? Then no coercion should be be applied. And if there was a list or tuple it would have to be turned into one.

Maybe some simpler filter, that would accept some several base types (int, string, list, tuple, set, dict) and allowing adding one to another. If it couldn't turn the argument into something acceptable, it would fail silently and return object on which filter was applied.

comment:2 Changed 5 years ago by gruszczy

Maybe something like this:

{{

def add(value, arg):

"""Adds the arg to the value."""
if isintnace(value, string):

return value + arg

elif isinstance(value, int):

try:

return value + int(arg)

except TypeError:

return value

elif isinstance(value, list):

try:

return value + list(arg.split(','))

except AttributeError:

return value

elif isinstance(value, tuple):

try:

return value + tuple(arg.split(','))

except AttributeError:

return value

return value

}}

Of course it can be extended a little more.

comment:3 Changed 5 years ago by gruszczy

Dammit. Now it's better.

def add(value, arg):
    """Adds the arg to the value."""
    if isintnace(value, string):
		return value + arg
	elif isinstance(value, int):
		try:
			return value + int(arg)
		except TypeError:
			return value
	elif isinstance(value, list):
		try:
			return value + list(arg.split(','))
		except AttributeError:
			return value
	elif isinstance(value, tuple):
		try:
			return value + tuple(arg.split(','))
		except AttributeError:
			return value
	return value

comment:4 Changed 5 years ago by dc

I think better not convert types at all (explicit is better than implicit) and use duck typing.

def add(value, arg):
    try:
        return value + arg
    except TypeError:
        return value

This will work for any object with __add__ or __radd__ method.

comment:5 Changed 5 years ago by gruszczy

This won't work with ints, because value is either an int (and will raise an exception) or a string (and it will concatenate those string).

Furthermore it couldn't be used in following situtation:

x = []

...

x|add:'5,6,7'

which for some could be useful.

comment:6 Changed 5 years ago by gruszczy

  • Owner changed from nobody to gruszczy

comment:7 Changed 5 years ago by gruszczy

OK, I don't know, whether add should have any more functionality, but it certainly needs to be silenced and proper tests should be written. I am posting a patch with just that.

comment:8 Changed 5 years ago by gruszczy

  • Has patch set

comment:9 Changed 5 years ago by SmileyChris

  • Triage Stage changed from Unreviewed to Design decision needed

I agree with dc; don't worry with a load of extra functionality (but IMO it should handle non-ints).

The try/except may as well be a catch-all, too, rather than TypeError or ValueError specific.

Hrm... to be totally backwards compatible, I guess that we'd need to continue with the current behaviour of trying to cast to int first (in case people are relying on adding string-integers), then fall back to a ducktype add.
That's pretty annoying if you wanted to add floats/decimals though.

I'm going to push to a design decision regarding which direction to go. Bring it up on the google dev group if you want some more discussion.

comment:10 Changed 5 years ago by SmileyChris

Non-int adding would be useful for #11689 - if the non-int part of this ticket is accepted, that can be closed as a duplicate.

comment:11 Changed 5 years ago by gruszczy

I don't like sucking all exceptions and this shouldn't be done. Imagine there is a custom object, that can be cast to an int and during coercing it will raise a different exception. Why should user lose this information? This is completely contrary to python zen.

When it comes to ducktyping, you have to remember, that the object on which filter is used, will have to able to accept a string as an argument of add. Most basic types:

In [1]: [] + ''
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)

/home/gruszczy/<ipython console> in <module>()

TypeError: can only concatenate list (not "str") to list

In [2]: () + ''
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)

/home/gruszczy/<ipython console> in <module>()

TypeError: can only concatenate tuple (not "str") to tuple

just can't do this.

comment:12 Changed 5 years ago by SmileyChris

The user should lose this information because a filter should never raise an exception.
Read the first paragraphs here: http://docs.djangoproject.com/en/dev/howto/custom-template-tags/#writing-custom-template-filters

I understand ducktyping.
A filter doesn't only accept strings as an argument, it could be any object type. If they can't be added together, then it should just fail silently - that's the template author's problem. This is not javascript, so no automatic casting.

comment:13 Changed 5 years ago by gruszczy

If they should never raise an exception, then indeed it should be sucking all exceptions.

The filter obviously accepts all objects, because that's just how Python works. However, if used in template only strings will be passed into such filter. Unless object on which filter is run can somehow use string in add, you won't get anything useful. Neither integers, nor lists/tuples can do that. Could you give some example of objects from stdlib/django that could benefit from using add with a string, that aren't strings?

comment:14 Changed 5 years ago by SmileyChris

I'm not sure where your delusion comes from that a template only deals with strings. I already gave a perfect example of a real life useful non-string addition in a template: see ticket #11689.

Here's another arbitrary example:

{% for obj in obj.list_a|add:obj2.list_b %}...{% endfor %}

comment:15 Changed 5 years ago by gruszczy

It was truly a delusion, because I had no idea that filter could work like this. If so, I'll provide proper patch and tests for a few different cases.

comment:16 Changed 5 years ago by SmileyChris

You may have had that view because Django templates always applie unicode() to the final output of variable tags. Glad we got to the conclusion in the end :)

I'll review your patch when it comes in and push forwards if it's all good. Remember, we need to keep the current behaviour so it should look something like this:

try:
    value, arg = int(value), int(arg)
except (TypeError, ValueError):
    pass
try:
    return value + arg
except:
    return value

comment:17 Changed 5 years ago by gruszczy

Yup, I know. The code itself is rather short, I just need to write some more tests to keep it safe. There is only one thing that isn't very nice - if someone wants to concatenate '123' and '456', he will get 579, which might not be exactly the thing he wants. I'll write the docs too.

Changed 5 years ago by gruszczy

coercing to int, then ducktyping + tests + docs

comment:18 Changed 5 years ago by gruszczy

OK, done.

comment:19 Changed 5 years ago by SmileyChris

  • Triage Stage changed from Design decision needed to Ready for checkin

Looking good.

comment:20 Changed 5 years ago by emulbreh

See #10143.

comment:21 Changed 4 years ago by jacob

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

(In [12497]) Fixed #11687: the add filter is now less failsome when faced with things that can't be coerced to integers.

comment:22 Changed 4 years ago by jacob

(In [12499]) [1.1.X] Fixed #11687: the add filter is now less failsome when faced with things that can't be coerced to integers.

Backport of [12497] from trunk, although the fix here is slightly different to
avoid adding new behavior to a bugfix branch.

comment:23 Changed 3 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

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.