#5903 DecimalField returns default value as unicode string

[Brian Rosner]

Hey all,

I recently came across the issue described in #5903 [1] earlier. There
are two distinct patches that fix the problem, but at different
levels. My inclination is to fix this issue at the model field level
and properly override get_default. My feeling is that allowing Decimal
objects to pass through force_unicode (when strings_only=True) might
cause ill-effects in other parts of Django, but I am not entirely sure
(running the test suite with the fix in force_unicode didn't cause any
failed test, but that doesn't make it right to me :). I don't see much
reason to do so. Perhaps someone can shed some light on this?

[1]: http://code.djangoproject.com/ticket/5903

--
Brian Rosner
http://oebfare.com

[Alex Gaynor]

It seems force_unicode is the wrong level to fix this at, and doing it in get_default() is the right place.

Alex

--
"I disapprove of what you say, but I will defend to the death your right to say it." --Voltaire
"The people's good is the highest law."--Cicero

[Alex Gaynor]

I sent that last email off rather hastily, and didn't think about it enough, and of course I've changed my mind :).  It seems to me this is the exact purpose of the strings_only kwarg, for when what we want to do is make sure a string becomes unicode, but not to mess with other objects.  Further it's clearly how other fields(integer, date, etc.) do it, so that further the argument that that is the correct way to handle it/use strings_only.

[Russell Keith-Magee]

On Wed, Feb 11, 2009 at 9:06 AM, Brian Rosner <bro...@gmail.com> wrote:
>
> Hey all,
>
> I recently came across the issue described in #5903 [1] earlier. There
> are two distinct patches that fix the problem, but at different
> levels. My inclination is to fix this issue at the model field level
> and properly override get_default. My feeling is that allowing Decimal
> objects to pass through force_unicode (when strings_only=True) might
> cause ill-effects in other parts of Django, but I am not entirely sure
> (running the test suite with the fix in force_unicode didn't cause any
> failed test, but that doesn't make it right to me :). I don't see much
> reason to do so. Perhaps someone can shed some light on this?

Our usage of Decimals has historically been undertested, so the fact
that you get no test failures doesn't necessarily mean that changing
force_unicode won't cause problems :-)

However, in this case, I'm reasonably convinced it is the right thing
to do. The list of 'ignored' types for force_unicode is essentially
the list of data types that we use for native data representations. I
can't think of any reason that Decimals shouldn't be on that list.

If you're looking for a little more background on exactly what
force_unicode "should" do, here's a discussion from way back, when we
expanded the non-string types to include dates, times, etc:

http://groups.google.com/group/django-developers/browse_thread/thread/c74e881a5f0dc8a6

My reading of that (then, as now), is that
force_unicode(strings_only=True) exists to catch string proxy objects,
not non-string objects; adding Decimal to the list of non-string
objects should be fine.

However, before you check anything in, I would suggest making sure
that we have got good test coverage for this change. In particular, I
would check that we have good Decimal tests for the following:

* Populating initial values on forms. One of the few places that
might be relying on get_default() returning a string is in an initial
value for a form

* Serialization, especially of an object with a decimal default value.

Yours,
Russ Magee %-)

[Malcolm Tredinnick]

On Wed, 2009-02-11 at 22:50 +0900, Russell Keith-Magee wrote:
[...]

> However, in this case, I'm reasonably convinced it is the right thing
> to do. The list of 'ignored' types for force_unicode is essentially
> the list of data types that we use for native data representations. I
> can't think of any reason that Decimals shouldn't be on that list.
>
> If you're looking for a little more background on exactly what
> force_unicode "should" do, here's a discussion from way back, when we
> expanded the non-string types to include dates, times, etc:
>
> http://groups.google.com/group/django-developers/browse_thread/thread/c74e881a5f0dc8a6
>
> My reading of that (then, as now), is that
> force_unicode(strings_only=True) exists to catch string proxy objects,
> not non-string objects; adding Decimal to the list of non-string
> objects should be fine.

Yes, that's the idea. I agree that allowing Decimal objects to
pass-through natively should be fine.

/me makes a note to add that summary to internal documentation in that
future when I have time to write some.

Regards,
Malcolm