Update enum support to be more fully featured. by johnjaylward · Pull Request #271 · stleary/JSON-java · GitHub

Adds support to JSONObject wrap and write methods to explicitly handle Enums.

What problem does this code solve?
Enums were handled inconsistently as noted in the original improvement (#140 and #127) and brought up again in #145 and #255

The new way enums are handled is to always place the actual enum in the JSONObject/JSONArray (not wrapped, similar to other value types). When writing, we always write the actual "name" of the enum, so even with a toString override on the enum class, the value remains consistent and compatible with the optEnum/getEnum methods.

The constructor JSONObject(Object) functions the same way as before when passing an enum and is consistent with other "value" types. For example, when creating a JSONObject with Long, Boolean, BigDecimal as the constructor parameter, the value will be treated as a "bean".

Risks
Low risk. Currently inserting enums into JSONObject and JSONArray are not consistent. Anyone depending on the existing handling of enums probably does so with if blocks to ensure they have the correct data. Additionally, some cases of inserting an enum would produce results that could not be read back using get|optEnum methods.

Set<MyEnumField> set = new EnumSet.of(MyEnumField.VAL1);
JSONObject jo = new JSONObject();
jo.put("value", set);

assert (new JSONObject(jo.toString()).optEnum(MyEnumField.class, "value") != null
    : "Oops, couldn't read back the enum value.";

Given that the data is not always reversible, it's unlikely that people are depending on this behaviour.

Changes to the API?
No, but there are functional changes

Will this require a new release?
Can be rolled into the next release, pending comments.

Should the documentation be updated?
Not required.

Does it break the unit tests?
Yes, new unit tests have been added.

Review status
ACCEPTED. Starting 3-day comment window 15 Aug 2016

…e Enums. The new way enums are handled is to always place the actual enum in the JSONObject/JSONArray. When writing, we always write the actual "name" of the enum, so even with a toString override on the enum class, the value remains consistant and compatible with the optEnum/getEnum methods. The constructor JSONObject(Object) functions the same way as before when passing an enum and is consistent with other "value" types. For example, when creating a JSONObject with Long, Boolean, BigDecimal as the constructor parameter, the value will be treated as a "bean".

Thanks, will take a look at this tonight. Please add a Risk section with your evaluation of the risk of this change. You can see examples in #259 and #261. For example, in what ways might existing users be impacted by the functional change?

Agreed that enums are currently not well supported.

done.

Is there a reasonable way to implement this such that existing users are unaffected, but new users can choose the new behavior?

I don't think so since it was inconsistent. Sometimes values would be
generated as a single string, and others as a Bean. However, the Bean
output was not that useful because neither the name or the ordinal of the
enum was ever output.

On Aug 12, 2016 01:49, "Sean Leary" notifications@github.com wrote:

Is there a reasonable way to implement this such that existing users are
unaffected, but new users can choose the new behavior?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#271 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAXa1xF8duK04l3quQS15CtxXQ4HODkdks5qfAlpgaJpZM4JiT7M
.

Enum support is connected in an odd way to BigNumber: both are Java features that did not exist when the lib was first implemented. At the times we added support for both, Douglas gave us pretty clear guidance on what we should and should not do, when implementing support for new features. That is, it needs to be exactly correct, and it can't break existing code. Of course, there will be a little give and take between those two constraints. I am still trying to get a sense of whether this enhancement is within the gray area ("Some rules can be bent; some can be broken" - Morpheus), or if we are straying too far into the realm of breaking existing code.

I am going to be working all weekend - chores and job. Anything you can do to nail down precisely what is going to change, plus any discussion you want to throw in, would be humbly appreciated.

I'd say that since the current implementation is inconsistent that this is
closer to a bug fix than an enhancement.

On Aug 13, 2016 02:45, "Sean Leary" notifications@github.com wrote:

Enum support is connected in an odd way to Big numbers: both represent
Java features that did not exist when the lib was first implemented. At the
time, we were fortunate that Douglas gave us pretty clear guidance on what
we should and should not do, when implementing support for new features.
That is, it needs to be exactly correct, and it can't break existing code.
Of course, there will be a little give and take between those two
constraints. I am still trying to get a sense of whether this enhancement
is within the gray area ("Some rules can be bent; some can be broken" -
Morpheus), or if we are straying too far into the realm of breaking
existing code.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#271 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAXa1xG7c7yGsSYNNYOtCa4sc_1dnjjiks5qfWgUgaJpZM4JiT7M
.

Also to note, this library did have support for enums a while ago, but
Douglas removed it when I pointed out the readme said it compiles on java
1.4.

On Aug 13, 2016 09:09, "John Aylward" john@johnanddalene.net wrote:

I'd say that since the current implementation is inconsistent that this is
closer to a bug fix than an enhancement.

On Aug 13, 2016 02:45, "Sean Leary" notifications@github.com wrote:

Enum support is connected in an odd way to Big numbers: both represent
Java features that did not exist when the lib was first implemented. At the
time, we were fortunate that Douglas gave us pretty clear guidance on what
we should and should not do, when implementing support for new features.
That is, it needs to be exactly correct, and it can't break existing code.
Of course, there will be a little give and take between those two
constraints. I am still trying to get a sense of whether this enhancement
is within the gray area ("Some rules can be bent; some can be broken" -
Morpheus), or if we are straying too far into the realm of breaking
existing code.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#271 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAXa1xG7c7yGsSYNNYOtCa4sc_1dnjjiks5qfWgUgaJpZM4JiT7M
.

Good information, thanks. Would that be the change and revert to JSONObject.wrap() in 2012? Or was there more?

Yes, just those 2 commits as far as I know. One in July adding support,
then the reversal in October.

On Aug 13, 2016 12:35, "Sean Leary" notifications@github.com wrote:

Good information, thanks. Would that be the change and revert to
JSONObject.wrap() in 2012? Or was there more?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#271 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAXa1ySIlQM-8qlxmWVVMY-udWGC2GtWks5qffJBgaJpZM4JiT7M
.

Taking small steps...

Of course the unit tests will fail if they are positively tracking the expected behavior, so that is not, in itself, a problem.
Would prefer to omit any changes not directly related to Enum, so it would be better if the Number changes are not included in this commit.
Have not analyzed it yet, but re-implementing Douglas' enum change should be fine. That would be the wrap() change, I guess.
It seems that the valueToString() change is a necessary action if wrap() inserts the enum. Assuming that was the purpose, and this was missed in the 2012 checkin because there were no (or at least no good) unit tests at the time.

Let me know what you think.

That sounds good. The changes to valueToString are indeed needed to prevent
default use of the enums toString method. We want to garuntee that the name
of the enum is output.

For the number changes, I'll revert those here and place them in a separate
pull request.

On Aug 15, 2016 02:01, "Sean Leary" notifications@github.com wrote:

Taking small steps...

Of course the unit tests will fail if they are positively tracking
the expected behavior, so that is not, in itself, a problem.

Would prefer to omit any changes not directly related to Enum, so it
would be better if the Number changes are not included in this commit.

Have not analyzed it yet, but re-implementing Douglas' enum change
should be fine. That would be the wrap() change, I guess.

It seems that the valueToString() change is a necessary action if
wrap() inserts the enum. Assuming that was the purpose, and this was missed
in the 2012 checkin because there were no (or at least no good) unit tests
at the time.

Let me know what you think.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#271 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAXa14sQ6u2wRkBmt_6G07vMD4uaOVK2ks5qgAC2gaJpZM4JiT7M
.

ok, cleanup up the commit as well as merging in the latest changes you made for the comments. Also updated my notes in comment#1

Thanks, starting a 3 day comment window.

Revert Enum changes introduced in upstream pull request stleary#271 and revert the Enum unit tests to how they were around PR stleary#127 on 2015-07-08. This reintroduces the original JSON-java Enum handling that did not have explicit handling for Enums and handled them inconsistently between JSONObject and JSONArray, and also relied on Enum.toString() when wrapping them instead of storing them as value and serializing them to their Enum.name() value.

johnjaylward mentioned this pull request Aug 11, 2016

test updates to make sure Enums are handled consistently. stleary/JSON-Java-unit-test#57

Merged

John J. Aylward added 2 commits August 15, 2016 10:24

revert back changes to Number support

7851e9b

Merge remote-tracking branch 'upstream/master' into EnumCleanup

349a209

stleary merged commit ebe69df into stleary:master Aug 19, 2016

johnjaylward mentioned this pull request Aug 26, 2016

JSONObject#populateMap() calls methods that aren't valid object getters #279

Closed

johnjaylward deleted the EnumCleanup branch February 10, 2017 14:58

srl295 mentioned this pull request Apr 2, 2025

JSONObject.toString does not use an Enum's toString() but its name() #838

Open

Update enum support to be more fully featured. by johnjaylward · Pull Request #271 · stleary/JSON-java · GitHub

Update enum support to be more fully featured. - #271

Update enum support to be more fully featured.#271stleary merged 3 commits intostleary:masterstleary/JSON-java:masterfrom johnjaylward:EnumCleanupjohnjaylward/JSON-java:EnumCleanupCopy head branch name to clipboard

Conversation

johnjaylward commented Aug 11, 2016 • edited by stleary Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stleary commented Aug 11, 2016

Uh oh!

johnjaylward commented Aug 11, 2016

Uh oh!

stleary commented Aug 12, 2016

Uh oh!

johnjaylward commented Aug 12, 2016

Uh oh!

stleary commented Aug 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnjaylward commented Aug 13, 2016

Uh oh!

johnjaylward commented Aug 13, 2016

Uh oh!

stleary commented Aug 13, 2016

Uh oh!

johnjaylward commented Aug 13, 2016

Uh oh!

stleary commented Aug 15, 2016

Uh oh!

johnjaylward commented Aug 15, 2016

Uh oh!

johnjaylward commented Aug 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stleary commented Aug 15, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Update enum support to be more fully featured.#271
stleary merged 3 commits into
stleary:masterfrom
johnjaylward:EnumCleanup

johnjaylward commented Aug 11, 2016 •
edited by stleary
Loading

stleary commented Aug 13, 2016 •
edited
Loading

johnjaylward commented Aug 15, 2016 •
edited
Loading