Repository navigation
enum.IntEnum is not compatible with JSON serialisation #62464
Description
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']
Activity
ncoghlan commentedon Jun 19, 2013
Replacing an integer constant with the current incarnation of enum.IntEnum breaks JSON serialisation:
It breaks for floats as well, but in a slightly different way:
Allowing __str__ to be inherited from Enum rather than from the concrete type will similarly break any serialisation protocol that relies on str() to handle integers. The float case is even trickier, since failing to inherit __repr__ would rather miss the point of the whole exercise...
We're going to have to be *very* careful with swapping out existing constants with enums, and we should be warning about the pitfalls in the docs too (where we can't change enum to avoid them).
warsaw commentedon Jun 19, 2013
On Jun 19, 2013, at 01:45 PM, Nick Coghlan wrote:
Serialization isn't the only issue - you have to know how to deserialize as
well. I use JSON extensively in Mailman's REST API. For enums I always
encode them to just the member name, because I always know what enum class
will be used to deserialize them.
(TBH, I wish the json module had better hooks for extending both serialization
and deserialization of non-basic types.)
ericsnowcurrently commentedon Jun 19, 2013
It's for times like this that I wonder if a simple serialization protocol might be worth it--something separate from __str__() but much simpler than the pickle protocol. __str__() could be the fallback. In the end, though, I always conclude that it's not worth adding yet another optional protocol to objects. Still, it would help in this case (at least for anything that knows to make use of the simple serialization protocol).
Alternately, would it make sense to add special handling of enums to the json module? Do we already with pickle?
amauryfa commentedon Jun 19, 2013
in encoder.py:
...
elif instance(value, int):
yield buf + str(value)
...
What if we use int(str(value)) instead?
ericsnowcurrently commentedon Jun 19, 2013
+1
+1
There's at least one stdlib module that does this pretty well (sqlite3, I think). This is where a simple serialization (and deserialization of course) protocol would come in handy.
ethanfurman commentedon Jun 19, 2013
Nick, in your example it looks like json is using the __str__ for int, but the __repr__ for float -- is this correct?
ethanfurman commentedon Jun 19, 2013
Eric,
The pickle support is solely in Enum. No changes were made to pickles.
pitrou commentedon Jun 19, 2013
You mean str(int(value)). Sounds good to me.
pitrou commentedon Jun 19, 2013
I think this is pretty much besides the point. IntEnums are meant to be substitutible with plain ints, so you can deserialize as a plain int.
Moreoven, JSON doesn't fill the same use cases as pickle: it is meant to communicate with services written in any languages, and those services won't have the same enum mnemonics as your Python library.
warsaw commentedon Jun 19, 2013
On Jun 19, 2013, at 02:09 PM, Eric Snow wrote:
Yeah, my database layer also has to be taught how to handle enums!
ncoghlan commentedon Jun 19, 2013
While I agree with forcing int subclasses to true integers in the JSON module, that may not be enough - the problem will affect third party serialisers as well. Whiel the debugging gains won't be as high, we may need to override __str__() in enum.IntEnum to give the "raw" form (leaving the debugging form solely for repr).
As for the float behaviour, the JSON serialiser is almost certainly playing games to avoid the truncation that used to occur in float.__str__ (I don't recall if we actually got rid of that, or if it's just a lot harder to trigger these days - it definitely changed when float.__repr__ was updated to prefer human friendly representations that produce the same floating point number). I'm less concerned about that, since we don't plan to swap out any standard library floats for enums. We may still want to mention the potential compatibility issue somewhere in the docs, though.
Backwards compatibility is such fun, isn't it? :)
ethanfurman commentedon Jun 19, 2013
I have no problem with leaving __str__ as the inherited type's, and just keeping the __repr__ as the enum add-on; this could be one of the differences between a pure Enum and a hybrid Enum.
Is it safe to change the json behaviour back to using __str__ for floats? If so, those two updates and we could be good to go. (Famous last words? ;)
ethanfurman commentedon Jun 19, 2013
I was unable to find any references to previous problems with json and floats. A quick-n-dirty patch yields the following:
--> from json import dumps, loads
--> from enum import Enum
--> class FE(float, Enum):
... pass
...
--> class Test(FE):
... one = 1.0
...
--> Test.one
<Test.one: 1.0>
--> str(Test.one)
'1.0'
--> dumps(Test.one)
'1.0'
--> loads(dumps(Test.one))
1.0
All json and enum tests are still passing.
If this is an acceptable solution I'll create a nicer patch and post for review.
25 remaining items
ncoghlan commentedon Aug 4, 2013
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.
elibendersky commentedon Aug 4, 2013
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.
ethanfurman commentedon Aug 4, 2013
This patch handles both float and int subclasses, and includes fixes/improvements from the review.
ethanfurman commentedon Aug 4, 2013
Forgot to add float tests. Included in this patch.
ethanfurman commentedon Aug 6, 2013
Hopefully the final patch. :)
ethanfurman commentedon Aug 6, 2013
Forgot to back out core dump tests before creating previous patch. This one even passes the tests. :/
elibendersky commentedon Aug 6, 2013
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.
ethanfurman commentedon Aug 6, 2013
I'll plan on committing no sooner than Friday unless somebody else has comments/corrections.
Thanks, Eli.
ethanfurman commentedon Aug 6, 2013
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?
elibendersky commentedon Aug 6, 2013
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.
ezio-melotti commentedon Aug 6, 2013
FWIW
make patchcheckshould remind you about that.python-dev commentedon Aug 10, 2013
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