enum.IntEnum is not compatible with JSON serialisation

@gvanrossum

BPO	18264
Nosy	@gvanrossum, @warsaw, @rhettinger, @amauryfa, @ncoghlan, @pitrou, @giampaolo, @ezio-melotti, @ethanfurman, @ericsnowcurrently
Files	issue18264.1-prelim.patch issue18264.stoneleaf.01.patch issue18264.stoneleaf.02.patch issue18264.stoneleaf.03.patch issue18264.stoneleaf.04.patch issue18264.stoneleaf.05.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/ethanfurman'
closed_at = <Date 2013-08-10.20:01:54.289>
created_at = <Date 2013-06-19.13:45:41.046>
labels = ['type-feature', 'library']
title = 'enum.IntEnum is not compatible with JSON serialisation'
updated_at = <Date 2013-08-10.20:01:54.287>
user = 'https://github.com/ncoghlan'

bugs.python.org fields:

activity = <Date 2013-08-10.20:01:54.287>
actor = 'python-dev'
assignee = 'ethan.furman'
closed = True
closed_date = <Date 2013-08-10.20:01:54.289>
closer = 'python-dev'
components = ['Library (Lib)']
creation = <Date 2013-06-19.13:45:41.046>
creator = 'ncoghlan'
dependencies = []
files = ['31134', '31141', '31155', '31156', '31172', '31173']
hgrepos = []
issue_num = 18264
keywords = ['patch']
message_count = 50.0
messages = ['191467', '191470', '191471', '191473', '191474', '191475', '191476', '191477', '191478', '191479', '191480', '191481', '191491', '191498', '191586', '191587', '191588', '191601', '191625', '191626', '191628', '191630', '191631', '191632', '191633', '191634', '191635', '194219', '194220', '194222', '194223', '194266', '194280', '194283', '194289', '194306', '194322', '194327', '194330', '194360', '194401', '194403', '194536', '194539', '194541', '194543', '194554', '194556', '194558', '194828']
nosy_count = 13.0
nosy_names = ['gvanrossum', 'barry', 'rhettinger', 'amaury.forgeotdarc', 'ncoghlan', 'pitrou', 'giampaolo.rodola', 'ezio.melotti', 'eli.bendersky', 'cvrebert', 'ethan.furman', 'python-dev', 'eric.snow']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue18264'
versions = ['Python 3.4']

Here's the relevant routine from _json.c:
-----------------------------------------

static PyObject *
encoder_encode_float(PyEncoderObject *s, PyObject *obj)
{
    /* Return the JSON representation of a PyFloat */
    double i = PyFloat_AS_DOUBLE(obj);
    if (!Py_IS_FINITE(i)) {
        if (!s->allow_nan) {
            PyErr_SetString(PyExc_ValueError, "Out of range float values are not JSON compliant");
            return NULL;
        }
        if (i > 0) {
            return PyUnicode_FromString("Infinity");
        }
        else if (i < 0) {
            return PyUnicode_FromString("-Infinity");
        }
        else {
            return PyUnicode_FromString("NaN");
        }
    }
    /* Use a better float format here? */
    return PyObject_Repr(obj);
}

Possible solutions
------------------

Use PyObject_Str() instead of PyObject_Repr()

I was unable to find any references to why it isn't currently PyObject_Str(), but switching over to it did not break any tests
Coerce the obj to a PyFloat, and take the repr of that (just use the i)

float subclasses would always lose the subclass status, but that is lost on deserialization anyway unless a custom decoder is used; and if a custom decoder is being used I would think a custom encoder is also being used?

Summary
-------

Having hybrid Enums not change __str__ would solve most of the json serialization issues;
either of the above two changes will solve the json issue of enumerated floats.

Thoughts on which route to take for json?

The proposal to change json from using repr() to str() has unknown dangers.

I don't want the str() of IntEnum to return just the decimal string (e.g. "42"), since that breaks half of the usefulness of using the enum in the first place -- people will write print(x) and be confused.

Unfortunately the json module doesn't have a way to define *in the object* how to customize its serialization -- this is always done in the json encoder/decoder. Maybe we can add something to the JSON encoder and decoder class that looks for a special method, e.g. __json_encode__ etc.? (OR maybe this would be a use case for PEP-443?)

Modifying json to use str(int(value)) (if it detects isinstance(value, int)) is fine with me too.

And similar for floats, really.

I'd be in favor of the __protocol__ route first and the PEP-443 route second.

The problem with just tacking in str(int(value)) or str(float(value)) is where does it stop? StrEnum, TupleEnum, BytesEnum, ComplexEnum, SomeOtherInterestingConstantEnum, etc., etc..

If we go the protocol route we add one method to Enum which returns the str of its .value, and we're set for every Enum.* As a bonus, any other object that needs special json (or yaml or whatever) consideration has a fairly easy way to make it happen.

*Every normal Enum, anyway. ;)

Can I vote for something like "__builtin__" as the protocol, rather than
something entirely specific to serialisation? As in "return the most
appropriate builtin type with the same value"? Then a converter
("operator.builtin"?) could coerce builtin subclasses to their base classes
by default, rather than needing to implement the protocol on every type.

Such a design would need a PEP, of course.

Hm. Then I prefer just calling the appropriate builtin, e.g. int().

--Guido van Rossum (sent from Android phone)
On Jun 21, 2013 6:08 PM, "Nick Coghlan" <report@bugs.python.org> wrote:

Nick Coghlan added the comment:

Can I vote for something like "__builtin__" as the protocol, rather than
something entirely specific to serialisation? As in "return the most
appropriate builtin type with the same value"? Then a converter
("operator.builtin"?) could coerce builtin subclasses to their base classes
by default, rather than needing to implement the protocol on every type.

Such a design would need a PEP, of course.

----------

Python tracker <report@bugs.python.org>
<http://bugs.python.org/issue18264\>

On Jun 22, 2013, at 01:08 AM, Nick Coghlan wrote:

Can I vote for something like "__builtin__" as the protocol, rather than
something entirely specific to serialisation? As in "return the most
appropriate builtin type with the same value"? Then a converter
("operator.builtin"?) could coerce builtin subclasses to their base classes
by default, rather than needing to implement the protocol on every type.

Such a protocol needs a way to deserialize as well. You can't necessarily
assume (or shouldn't impose) that the __init__() can do that conversion. In
any case...

Such a design would need a PEP, of course.

...yes, definitely.

On Fri, Jun 21, 2013 at 6:59 PM, Barry A. Warsaw <report@bugs.python.org>wrote:

Barry A. Warsaw added the comment:

On Jun 22, 2013, at 01:08 AM, Nick Coghlan wrote:

>Can I vote for something like "__builtin__" as the protocol, rather than
>something entirely specific to serialisation? As in "return the most
>appropriate builtin type with the same value"? Then a converter
>("operator.builtin"?) could coerce builtin subclasses to their base
classes
>by default, rather than needing to implement the protocol on every type.

Such a protocol needs a way to deserialize as well. You can't necessarily
assume (or shouldn't impose) that the __init__() can do that conversion.
In
any case...

>Such a design would need a PEP, of course.

...yes, definitely.

Practically speaking, what should be done to make enum play well with JSON
without writing new PEPs? I think we still want to convert those stdlib
constants to IntEnums...

Eli

Change json to call int() first.

--Guido van Rossum (sent from Android phone)
On Jun 21, 2013 7:45 PM, "Eli Bendersky" <report@bugs.python.org> wrote:

Eli Bendersky added the comment:

On Fri, Jun 21, 2013 at 6:59 PM, Barry A. Warsaw <report@bugs.python.org
>wrote:

>
> Barry A. Warsaw added the comment:
>
> On Jun 22, 2013, at 01:08 AM, Nick Coghlan wrote:
>
> >Can I vote for something like "__builtin__" as the protocol, rather than
> >something entirely specific to serialisation? As in "return the most
> >appropriate builtin type with the same value"? Then a converter
> >("operator.builtin"?) could coerce builtin subclasses to their base
> classes
> >by default, rather than needing to implement the protocol on every type.
>
> Such a protocol needs a way to deserialize as well. You can't
necessarily
> assume (or shouldn't impose) that the __init__() can do that conversion.
> In
> any case...
>
> >Such a design would need a PEP, of course.
>
> ...yes, definitely.

Practically speaking, what should be done to make enum play well with JSON
without writing new PEPs? I think we still want to convert those stdlib
constants to IntEnums...

Eli

----------

Python tracker <report@bugs.python.org>
<http://bugs.python.org/issue18264\>

On 06/21/2013 07:49 PM, Guido van Rossum wrote:

Eli Bendersky added the comment:
>
> Practically speaking, what should be done to make enum play well with JSON
> without writing new PEPs? I think we still want to convert those stdlib
> constants to IntEnums...

Change json to call int() first.

Should we have json call float() and str() first as well? I'm thinking less of the stdlib and more of the unsuspecting
user.

Yes for float() -- but for str() it would seem redundant? (Or what's
the context?)

On Fri, Jun 21, 2013 at 8:23 PM, Ethan Furman <report@bugs.python.org> wrote:
>
> Ethan Furman added the comment:
>
> On 06/21/2013 07:49 PM, Guido van Rossum wrote:
>> Eli Bendersky added the comment:
>>>
>>> Practically speaking, what should be done to make enum play well with JSON
>>> without writing new PEPs? I think we still want to convert those stdlib
>>> constants to IntEnums...
>>
>> Change json to call int() first.
>
> Should we have json call float() and str() first as well?  I'm thinking less of the stdlib and more of the unsuspecting
> user.
>
>

Python tracker <report@bugs.python.org>
<http://bugs.python.org/issue18264\>

Guido van Rossum added the comment:

Yes for float() -- but for str() it would seem redundant? (Or what's
the context?)

If a user has

     class Color(StrEnum):
         red = 'ff0000'
         green = '00ff00'
         blue = '0000ff'

..
..
..

oh. str() isn't going to give is the value's string, is it?

Hmmm...

Instead of calling int() or float() on an enum member, perhaps we could make json smart enough to use the value? That
would also cover pure Enums.

Whatever we do needs to be something third party serialisation libraries
can also adopt with minimal compatibility risk for older versions of Python.

Yes, that serialisation will lose the new debugging information. That's
fine - if people want to map from a standard format like JSON to enums,
they have to handle that themselves anyway.

I've been reading the discussion again to figure out what we need to move forward; it appears that a simple approach of using str(int(obj)) in json encoding when isinstance(obj, int) was acceptable for many of the participants, and it helps solve the most common problem - the one with IntEnum. We can solve it now, move on to converting socket.* and other constants to IntEnum and perhaps change the solution to something more grandiose later.

I'm attaching a proof-of-concept (probably incomplete) patch that implements this for the Python and C implementations of json.

With it applied, Nick's example goes like this:

>>> from enum import IntEnum
>>> import json
>>> class Example(IntEnum): x = 1
... 
>>> json.dumps(Example.x)
'1'
>>> json.loads(json.dumps(Example.x))
1
>>>

When thinking about the more generic approaches, we must consider what was raised here already - JSON interoperates with other languages, for which Python's enum is meaningless (think of JavaScript, for instance). So the only logical way to JSON-encode a IntEnum is str(int(...)).

[please note that the patch is POC/hacky at best - it's likely to leak memory and be careless and incomplete in other ways. I'm just trying to demonstrate the approach]

I'll check you patch later against big numbers (which is where I had difficulties). If it works I'll try to make it more complete. If it doesn't, I've been working on just extraction the Enum member's value and using that (works fine on the Python side ;) .

Big Question: if the Enum member was used as a key, do we use .name or .value? The decision I went with was to compare the hashes of the member name vs the member itself -- if they're the same, use the .name, otherwise use the value.

I don't think my idea of always extracting .value is grandiose (and didn't take that long to implement on the Python side), but it is definitely forcing me to learn more about C and how Python is put together.

It this point I have successfully imported Enum into the _json module; now I'm working on a function to quietly replace the object being encoded with its .value if it is, in fact, an Enum member.

Just been real short on time lately. :/

Thank goodness for C API section of the docs!

Eli, your method is good. I thought I had tried something similar but I obviously had the wrong PyLong constructor.

I'll get it implemented.

Yes, AFAIU PyNumber_Long is the equivalent of Python-level int(obj). With other constructors of PyLong you are limited by long long (while Python integers may be arbitrarily large).

Ethan - If you're still short on time I can pretty up this patch and put it for review with some tests. Let me know what you prefer.

Thanks for the offer, Eli, but I almost have the tests done. :)

Okay, patch attached with C code (thanks, Eli!), more python code, and some new tests.

Only the int case is handled.

Posted a Rietveld code review

It occurs to me that operator.index() (without a preceding type check) is
likely the more ducktyping friendly option here.

I don't understand.

Type checks are already performed throughout the code (int, float, True, False, NaN, Inf, etc.).

What will opereator.index buy us?

The two "isinstance" checks that bother me are the ones for int and float. However, given that the JSON serialiser *already* includes those explicit checks, I now think it makes sense to just do the minimal fix of coercing subclasses to the base type for both of those cases, especially since such a change should be able to be ported to simplejson as well.

Anything more elaborate (including making JSON explicitly enum aware, or else offering a new encoder that supports singledispatch) should be handled in a separate issue.

I also think that exchanging the explicit type checks by __index__ merits more thought and is outside the scope of this local fix. The proposed patch does not add any new type checks, but acts within the bounds of code for which the type is already established.

This patch handles both float and int subclasses, and includes fixes/improvements from the review.

Forgot to add float tests. Included in this patch.

Hopefully the final patch. :)

Forgot to back out core dump tests before creating previous patch. This one even passes the tests. :/

LGTM now. Make sure to run the test(s) in refleak mode and let's wait for a couple of days before committing, in case someone else has comments.

I'll plan on committing no sooner than Friday unless somebody else has comments/corrections.

Thanks, Eli.

Eli Bendersky added the comment:

Make sure to run the test(s) in refleak mode . . .

How extensive should testing be?

I plan on always running the refleak mode tests (now that I know how ;) .

Should I also run non-refleak tests?

Should I run tests with a python built without --with-pydebug?

IMHO it's very much dependent on the change. When making C code changes, I usually run the relevant tests with refleaks, and then run the whole test suite; all of this --with-pydebug. Personally I've not encountered cases where non-debug builds failed where debug suceeded for CPython, but for very significant changes I may do some extra runs in the release mode as well (or try to test it on a specific OS). It's very important to watch the bots after committing, because they run a wide variety of configurations you'll be hard pressed to reproduce on your own - so they exercise the change pretty well.

I plan on always running the refleak mode tests (now that I know how ;)

FWIW make patchcheck should remind you about that.

New changeset ae1a7c420f08 by Ethan Furman in branch 'default':
Close bpo-18264: int- and float-derived enums now converted to int or float.
http://hg.python.org/cpython/rev/ae1a7c420f08

Navigation Menu

enum.IntEnum is not compatible with JSON serialisation #62464

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

enum.IntEnum is not compatible with JSON serialisation #62464

Description

Activity

ethanfurman commented on Jun 20, 2013

gvanrossum commented on Jun 21, 2013

gvanrossum commented on Jun 21, 2013

gvanrossum commented on Jun 21, 2013

ethanfurman commented on Jun 21, 2013

ncoghlan commented on Jun 22, 2013

gvanrossum commented on Jun 22, 2013

warsaw commented on Jun 22, 2013

elibendersky commented on Jun 22, 2013

gvanrossum commented on Jun 22, 2013

ethanfurman commented on Jun 22, 2013

gvanrossum commented on Jun 22, 2013

ethanfurman commented on Jun 22, 2013

ncoghlan commented on Jun 22, 2013

elibendersky commented on Aug 2, 2013

elibendersky commented on Aug 2, 2013

ethanfurman commented on Aug 2, 2013

ethanfurman commented on Aug 3, 2013

ethanfurman commented on Aug 3, 2013

elibendersky commented on Aug 3, 2013

ethanfurman commented on Aug 3, 2013

ethanfurman commented on Aug 3, 2013

elibendersky commented on Aug 3, 2013

ncoghlan commented on Aug 4, 2013

ethanfurman commented on Aug 4, 2013

ncoghlan commented on Aug 4, 2013

elibendersky commented on Aug 4, 2013

ethanfurman commented on Aug 4, 2013

ethanfurman commented on Aug 4, 2013

ethanfurman commented on Aug 6, 2013

ethanfurman commented on Aug 6, 2013

elibendersky commented on Aug 6, 2013

ethanfurman commented on Aug 6, 2013

ethanfurman commented on Aug 6, 2013

elibendersky commented on Aug 6, 2013

ezio-melotti commented on Aug 6, 2013

python-dev commented on Aug 10, 2013

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions