Opened 15 years ago

Closed 14 years ago

Last modified 9 years ago

#11687 closed (fixed)

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

Reported by: Bradley Ayers Owned by: Filip Gruszczyński
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: no UI/UX: no

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 Filip Gruszczyński 15 years ago.
coercing to int, then ducktyping + tests + docs

Download all attachments as: .zip

Change History (23)

comment:1 by Filip Gruszczyński, 15 years ago

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 by Filip Gruszczyński, 15 years ago

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.

Last edited 9 years ago by Tim Graham (previous) (diff)

comment:4 by dc, 15 years ago

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 by Filip Gruszczyński, 15 years ago

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 by Filip Gruszczyński, 15 years ago

Owner: changed from nobody to Filip Gruszczyński

comment:7 by Filip Gruszczyński, 15 years ago

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 by Filip Gruszczyński, 15 years ago

Has patch: set

comment:9 by Chris Beaven, 15 years ago

Triage Stage: UnreviewedDesign 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 by Chris Beaven, 15 years ago

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 by Filip Gruszczyński, 15 years ago

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 by Chris Beaven, 15 years ago

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 by Filip Gruszczyński, 15 years ago

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 by Chris Beaven, 15 years ago

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 by Filip Gruszczyński, 15 years ago

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 by Chris Beaven, 15 years ago

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 by Filip Gruszczyński, 15 years ago

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.

by Filip Gruszczyński, 15 years ago

Attachment: add_filter.diff added

coercing to int, then ducktyping + tests + docs

comment:18 by Filip Gruszczyński, 15 years ago

OK, done.

comment:19 by Chris Beaven, 15 years ago

Triage Stage: Design decision neededReady for checkin

Looking good.

comment:20 by Johannes Dollinger, 15 years ago

See #10143.

comment:21 by Jacob, 14 years ago

Resolution: fixed
Status: newclosed

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

comment:22 by Jacob, 14 years ago

(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 by Jacob, 12 years ago

milestone: 1.2

Milestone 1.2 deleted

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