List is converted to String

[sebfz1]

The List are converted to a String instead of a JSONArray For instance the following code:

JSONObject object = new JSONObject();
object.put("mylist", Arrays.asList("my1", "my2", "my3"));

is producing

{"mylist":"[my1, my2, my3]"}

Instead of:

{"mylist":["my1","my2","my3"]}

Many thanks in advance :) Sebastien

[mfursov]

Original org.json has 3 methods:

• put(String, Object)
• put(String, Collection)
• put(String, Map)

Openjson implementation is based on Android code and has only 1 method

• put(String, Object)

Problem:

• put(String, Collection/Map) breaks 'null' contract of put(String, Object) where null-value can be used to remove property from object. The missing methods in 'org.json' puts empty JSONArray|JSONObject.

What options we have:

1. Keep current minimal API compatible with Android

2. Follow org.json and break our contract for nulls for 2 new put() methods. It will not be possible to remove values using put(String, (Collection|Map)null). This will make us incompatible with Android API on compile level: calls to put(String, null) will not be compilable without explicit casts. Users who migrate from org.json will not have runtime issues.

3. Follow put(String, Object) contract for 2 new methods and, if null is passed as a second parameter, remove the property from object. This will make us incompatible with both org.json and Android API but the package will have better API.

I would follow option 2 here and be compatible with org.json package. People on Android have no need in our API and those who migrate from org.json on server platforms want minimum surprise in runtime.

Other thoughts?

[sebfz1]

I would go for option 2 too, as long as put(String, Object) tests the type of the supplied object (Collection/Map) and calls the appropriate method.

[mfursov]

I believe if you have these 3 methods you can't call put(String, Object) without explicit cast to (Object).

And if the method put(String, Object) will check the type and call to put(String, Collection|Map) it will be API break both with org.json and Android and lead to runtime issues for those who migrate.

[sebfz1]

I believe if you have these 3 methods you can't call put(String, Object) without explicit cast to (Object).

Each methods will be correctly linked whether the object is a known Collection (put(String, Collection)), a known Map (put(String, Map)), or any other object type which is not a Collection nor a Map (put(String, Object)). The linkage is done at compile time.

And if the method put(String, Object) will check the type and call to put(String, Collection|Map) it will be API break both with org.json and Android and lead to runtime issues for those who migrate.

IMO that's the opposite, if the linkage is on put(String, Object), the underlying object - if it's a Collection/Map - should be handled correctly. That's actually the purpose of my issue, the org.json library behave that way.

For instance:

Object value = PropertyResolver.getValue("myList", this.getModelObject()); // gets a List at runtime

JSONObject object = new JSONObject();
object.put("mylist", value); // linked to put(String, Object) at compile time

[mfursov]

You are correct about linking. My comment about explicit casts is valid only for null values.

I would go for option 2 too, as long as put(String, Object) tests the type of the supplied object (Collection/Map) and calls the appropriate method.

Here is the code from Wicket 7.x branch (same as json.org): it does not check if the object passed is collection or map and adds it as is.

public JSONObject put(String key, Object value) throws JSONException {
        if (key == null) {
            throw new NullPointerException("Null key.");
        }
        if (value != null) {
            testValidity(value);
            this.map.put(key, value);
        } else {
            this.remove(key);
        }
        return this;
    }

.

[tdunning]

Just a quick question here.

Is this a theoretical problem (regardless of the attraction), or is there code that uses Open Json right now that breaks because of the single method?

[solomax]

Hello All,

We can add code like

public JSONObject put(String key, Object value) throws JSONException {
        if (key == null) {
            throw new NullPointerException("Null key.");
        }
        if (value != null) {
if (value instanceof Collection) {
            testValidity(value);
            this.map.put(key, value);
} else if ....
        } else {
            this.remove(key);
        }
        return this;
    }

complex logic like this is already used in the code (if I'm not mistaken it is new JSONObject(String))

WDYT?

[mfursov]

Max, for the package with a different name (com.github.openjson) I would try to convert this method (and all «Object» methods) to be package local - for internal use only. As a public option I would add alternatives with concrete types: put(String, JSONObject|JSONArray). In this case users will have no chance to call API with unclear behavior.

For 'org.json' version of the library this may not work.

[solomax]

This might be better from architect point of view, but it would require more code from user

BTW never heard of 'null' contract of put, always used remove for this :)

[mfursov]

I have the same experience here: never used this method for removal or for collections.

To me the worst problem is that this problem is hidden from user. I believe it's better to ask user manually change the code the way he wants it to be used (this is trivial change) than have issues in runtime (or in database state)

[sebfz1]

@mfursov right, I didn't looked into the code. I think overloads are here just for ease. That's ok (and normal) that the object is stored as it is. But the issue is how it will be handled when rendered (to string)

@tdunning I don't think this is due to this method in particular (maybe my description was confusing), the real issue/break is when it's converted to string

[solomax]

@mfursov do you need need help with with?

[mfursov]

@solomax the only help I need to fix 'com.github.openjson' branch is to decide which fix to apply before Wicket8 release.

I like any of these 2 options:

1) v1.0.1: add 2 more methods and make all 3 methods 100% compatible with original 'org.json' In this case users will be required to perform minimal code change and we will know we tried to be as much 'org.json' as we can. We will inherit it's problems and may find more issues latter.

2) v2.0.0: remove «Object» methods (put, constructors, wrap, accumulate). Add dedicated «putJSONObject|putJSONArray» methods. In this case users will be forced to review all the code that has hidden compatibility problems and library will be even simpler than before. Users are also have an option to swtich to original 'org.json', jackson or whatever they like if changes are big.

Both these solutions can be prepared/tested and pushed to maven in 1 day.

[martin-g]

3. Both and mark the methods from 1) as @Deprecated and later remove them in 2.0.0

[solomax]

+1 for 3) :))

[sebfz1]

I don't think removing "Object" methods sounds like a good plan. These methods are quite essential IMO. Still IMO, actually the main problem is not in the put method, but how the object is rendered

[solomax]

So I guess the best option for API users will be: put should be more sophisticated: ie if collection is being put it should be auto-wrapped into collection

[mfursov]

Ok, I will implement option 1 + 2 methods and update maven with v1.0.1 for com.github.openjson today.

@solomax ok, so put(String, Collection|Map) will check for object type and call appropriate method (this is not what original org.json have, btw).

@sebfz1 I thought it will be almost enought to replace put(Object) with putValue(Object) where value is known library type only: JSONArray, JSONObject, String or one of primitive types. This will allow to keep a lot of old scenarios where fields from one object are copied into another, but will not support automatic hierarchy creation from Maps. But anyway, let's keep it for latter.

[sebfz1]

Do you mean having the following?

• put(Collection|Map)
• putValue(JSON*|String|Primitive)

In that case: 1/ Custom Object is not handled (in case it implements JSONString or equivalent). But ok, let's say it can be done later... 2/ There is 2 different method names, one for Collection/Map and one for String. That's a problem: in my use-case I don't know the type of object (defined by the user), that's why I need a unique/generic "Object" method. 3/ From my pov, put(Collection) is just a convenient overload. If you do any processing in it instead of just storing the object, what will happen if the user does a get right after and think he will retrieve its object ? (to be honest, I do not remember how it behave, I just matter of raising potential points :) )

[mfursov]

Here is a fix for 'com.github.openjson' branch:

https://github.com/openjson/openjson/commit/5ce9624000d324cdd5813b1459a185b91a130a4c

1) Checks for Map|Collection types in put(String, Object) 2) Fixes JSONObject(Map) constructor to accept nulls (same as org.json)

I'm going to add tests for these branches and release it as v1.0.1 today if no issues/objections found.

[mfursov]

Released to maven: http://repo1.maven.org/maven2/com/github/openjson/openjson/1.0.1/

[martin-g]

Wicket 8.x has been updated to use it! I'll take a look tomorrow to update 6.x and 7.x as well unless someone beats me to do it.

[mfursov]

This will be a breaking change for Wicket 6.x & 7.x.

If timeline for adoption of this change allows to wait until version Wicket 8.x release I would wait and asked customers to migrate to Wicket 8.x if they do really care about this single line of code in MIT license.

[sebfz1]

Thanks guys for the quick change! I've tested upon wicket-8.0.0-SNAPSHOT and so far it seems to be good! :+1:

[tdunning]

@mfursov It isn't a matter of the users any more. It is a matter of the Apache Board saying no more releases with the non-OSS license.