Uses superclass Number instead of specific sub classes

[johnjaylward]

see issue #126

[johnjaylward]

this is also adding in support for Enum directly again since we are depending on java 1.8

[stleary]

Sorry I could not get to this sooner. Going to look at Number support in the context of @FritzMock's commit. Java enum seems a good idea. Unit test support first, then see how implementation goes. Thanks, Sean

[johnjaylward]

yes, it makes sense to grab a complete solution, as this only adjusts the output to JSON, and does nothing about the input from string to JSON.

[stleary]

Regarding enums, it seems to me that the goal should not be to represent the enum structure, but rather the values of an instance. The bean parsing code already handles optional getters, all that is needed is to add code to parse the assigned constant value. However, there does not appear to be a straightforward way to represent the value and getters together in a JSONObject. One possibility might be something like this:

{ 
    "name": "(the assigned constant value string)", 
    "value" : { 
        "(getter1 name)": "(getter1 value)", 
        "(getter2 name)": "(getter2 value)"
    }
}

Don't really like the use of arbitrary names and nested JSONObjects. Let me know if you have any thoughts about how best to do this.

[johnjaylward]

With an Enum, all the values in that enum should be constant. I think just representing the enum name should be enough.

public enum MyEnum {
    enumVal1("String 1"), enumVal2("String 2");
    private String someValue;
   //... constructor
}

public class MyObject {
    private String someVal;
    private MyEnum someEnum;
}

JSONObject o = new JSONObject(new MyObject("my String value", MyEnum.enumVal1));

Then the output of "o.toString()" would look like this:

{
    "someVal" : "my String value",
    "someEnum" : "enumVal1"
}

To be honest, short of using annotations or other reflective techniques like Jackson uses, I can't see being able to reliably deserializing the JSON object fully into MyObject automatically. The best option would probably be a getters like:

public <E> E getEnum(Class<E> clazz, String key){
    try {
        return Enum.valueOf(clazz, this.optString(key));
    } catch (IllegalArgumentException |  NullPointerException e) {
        // JSONException should really take a throwable argument
        throw new JSONException("Unable to process enum value for "+key);
    }
}
public <E> E optEnum(Class<E> clazz, String key){
    try {
        return Enum.valueOf(clazz, this.optString(key));
    } catch (IllegalArgumentException |  NullPointerException e) {
    }
    return null;
}

[johnjaylward]

ok, I updated my pull request to have the example getters added for reading Enum values from the JSON object.

It's possible I should do a check to see if the value is of type enum first, but the optString call I'm making should handle converting an existing Enum value into its String value for me. The only worry I have there is if someone overrode their "toString()" method on their enum where it doesn't return Enum.name() anymore.

[johnjaylward]

one last commit that should fix most cases for the optEnum method. Also cleans up the getEnum exception message to better match existing messages.

[stleary]

Can you provide a code fragment please where a JSONObject is initialized with an enum value?

[johnjaylward]

the change to the "wrap" method allows enums to be placed directly into the map. So anything like my example above, or even something like this:

EDITED (example copied from above):
Eaxmple 1:

public enum MyEnum {
    enumVal1("String 1"), enumVal2("String 2");
    private String someValue;
   //... constructor
}

public class MyObject {
    private String someVal;
    private MyEnum someEnum;
}

// the private populateMap method will be called,
// and the enum will be placed in the underlying map
JSONObject o = new JSONObject(new MyObject("my String value", MyEnum.enumVal1));

Example 2:

JSONObject jo = new JSONObject();
// there is no check here, it will be inserted the same
// as any object. testValidity check is only verifying
// Double and Float instances. The enum will be inserted
// directly into the underlying map.
// This is possibly a bug, I'd think that this should be calling "wrap"
// but it is not.
jo.put("someKey", someEnumReference);

[stleary]

Thanks for clarifying. What if the user tries something like this:

   MyEnum myEnum = MyEnum.enumVal1;
   JSONObject jo = new JSONObject(myEnum);

Currently, the jo.toString() output is:

{}

Should that be changed?

[douglascrockford]

Just a reminder to everyone. This is stable 14 year old code. The rules for the quality of submissions needs to be much higher than in current projects. Nothing gets changed unless it is absolutely right.

[johnjaylward]

It probably should. I would expect that to error as it would be similar to doing something like this:

JSONObject jo = new JSONObject("Some String that is invalid JSON");

I personally consider an Enumeration to be a single value, so trying to create a JSONObject from a value should fail.

However, it should work fine to insert it directly into a JSONArray, as those hold values.

JSONArray ja = new JSONArray();
ja.put(myEnum)

[johnjaylward]

we should also consider what happens in the case of:

JSONObject jo = new JSONObject(Long.valueOf(1));

The enum case, and the Number case should be handled similarly. I'd argue that anything that returns itself from the "wrap" method should be handled the same way. Specifically: Number, Character, Boolean, String, and Enum

[stleary]

@douglascrockford Thanks for the reminder, point taken.

[johnjaylward]

Thanks. I'm hoping we get it 100% right as well. I've been using this for 8+ years and have mostly kept my changes to myself. It's nice being able to share them once in a while.

[stleary]

Let's regroup and look at how enums should be handled vs how JSON-Java actually handles them. Here is my analysis, please let me know if I have missed or miss-stated anything.

A Java enum can be thought of as a restricted kind of class with some public static values, and possibly some getters. Here is a simple enum that includes a getter.

public enum MyEnum  {
    VAL1(1), VAL2(2);
    private Integer val;
    private MyEnum(Integer val) { this.val = val; }
    public getVal() { return val; }
}

Because an enum is like a class, we expect JSONObject and JSONArray to treat it similarly to a POJO and to work correctly in the following API methods that take or return an object:

• JSONObject(Object)
• JSONObject(Object, String[])
• Object jo.get(String)
• Object jo.opt(String)
• jo.put(String, Object)
• jo.putOnce(String, Object)
• jo.putOpt(String, Object)
• jo.getNames(Object)
• jo.valueToString(Object)
• jo.wrap(Object)
• jo.toString()
• ja.put(Object)
• ja.put(int, Object)
• Object ja.get(int)
• Object ja.opt(int)
• Object ja.remove(int)
• ja.toString()

An enum has 3 different views. JSON-Java should provide the ability to serialize an enum by each view:

• The value assigned to the enum: Use the put() methods
• The getter values: Use JSONObject(Object)
• The defined values: Use getNames() with JSONObject(Object, String[])

I am going to work on the unit tests next, to confirm how JSON-Java handles all of the above.
Note: I found in the Javadoc for jo.put(), and some other methods, text which I think may be incomplete, since objects (and enums) seem to be supported as well:

     * @param value
     *            An object which is the value. It should be of one of these
     *            types: Boolean, Double, Integer, JSONArray, JSONObject, Long,
     *            String, or the JSONObject.NULL object.

[johnjaylward]

That sounds like a good plan to me. We should try to keep it as close to the current functionality as possible, but extending it would also be good. Let me know if you want any help with the unit tests. I'm more familiar with TestNG than Junit, but they are similar enough.

[stleary]

no worries on the code, writing these particular unit tests is easier than getting time off from weekend chores.

[stleary]

Unit tests are completed and merged (see EnumTest.java). I think I have a better idea of how to get enum information into and out of JSONObject and JSONArray objects, despite not having explicit support for the enum type. It should all be documented in the test file. Let me know if you think any functionality is missing or incorrect.

[johnjaylward]

Great, those tests look good upon eyes only review. I'll try to take some time today to clone and run them and see if anything is missing.

[stleary]

It seems reasonable to have some enum getters similar to what was done for big numbers. Also, will look again at the number replacement code, just want to make sure we are not allowing for otherwise unsupported numeric types. Thanks for being patient.

[johnjaylward]

I rebased my branch off the master here, so it should be up to date now.

[stleary]

Proposal:
jsonObject.get|optEnum should be implemented. optEnum is overloaded with a default.
jsonArray.get|optEnum should be implemented. optEnum is overloaded with a default.

API changes:
New methods, no changes to existing API
JSONObject
getEnum(Key)
optEnum(Clazz, Key)
optEnum(Clazz, Key, Default)
JSONArray
getEnum(Index)
optEnum(Clazz, Index)
optEnum(Clazz, Index, Default)

Changes to existing behavior:
None

Files touched:
JSONObject.java
JSONArray.java

Effect on existing apps:
Don't expect any effect on existing apps

Unit test changes:
EnumTest enumAPI() tests the new API. Currently on the enum-support branch, will be moved to master if/when this code is pushed.

Next steps:
The proposed changes are on the enum-support branch. A pull request will be created and left up for some time for comments.

What was not changed:
Did not implement the change to type support in wrap(). Needs thorough testing.

[stleary]

Please see pull request #140