Update enum support to be more fully featured. #271

johnjaylward · 2016-08-11T16:38:08Z

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".

stleary · 2016-08-11T17:31:14Z

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.

johnjaylward · 2016-08-11T17:43:47Z

done.

stleary · 2016-08-12T05:49:28Z

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

johnjaylward · 2016-08-12T12:19:37Z

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" [email protected] 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
.

stleary · 2016-08-13T06:45:39Z

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.

johnjaylward · 2016-08-13T13:09:41Z

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" [email protected] 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
.

johnjaylward · 2016-08-13T13:14:03Z

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" [email protected] 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" [email protected] 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
.

stleary · 2016-08-13T16:35:12Z

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

johnjaylward · 2016-08-13T16:42:34Z

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" [email protected] 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
.

stleary · 2016-08-15T06:01:26Z

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.

johnjaylward · 2016-08-15T06:16:39Z

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" [email protected] 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
.

johnjaylward · 2016-08-15T14:30:09Z

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

stleary · 2016-08-15T14:41:17Z

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update enum support to be more fully featured. #271

Update enum support to be more fully featured. #271

johnjaylward commented Aug 11, 2016 •

edited by stleary

Loading

stleary commented Aug 11, 2016

johnjaylward commented Aug 11, 2016

stleary commented Aug 12, 2016

johnjaylward commented Aug 12, 2016

stleary commented Aug 13, 2016 •

edited

Loading

johnjaylward commented Aug 13, 2016

johnjaylward commented Aug 13, 2016

stleary commented Aug 13, 2016

johnjaylward commented Aug 13, 2016

stleary commented Aug 15, 2016

johnjaylward commented Aug 15, 2016

johnjaylward commented Aug 15, 2016 •

edited

Loading

stleary commented Aug 15, 2016

Update enum support to be more fully featured. #271

Update enum support to be more fully featured. #271

Conversation

johnjaylward commented Aug 11, 2016 • edited by stleary Loading

stleary commented Aug 11, 2016

johnjaylward commented Aug 11, 2016

stleary commented Aug 12, 2016

johnjaylward commented Aug 12, 2016

stleary commented Aug 13, 2016 • edited Loading

johnjaylward commented Aug 13, 2016

johnjaylward commented Aug 13, 2016

stleary commented Aug 13, 2016

johnjaylward commented Aug 13, 2016

stleary commented Aug 15, 2016

johnjaylward commented Aug 15, 2016

johnjaylward commented Aug 15, 2016 • edited Loading

stleary commented Aug 15, 2016

johnjaylward commented Aug 11, 2016 •

edited by stleary

Loading

stleary commented Aug 13, 2016 •

edited

Loading

johnjaylward commented Aug 15, 2016 •

edited

Loading