JSON-java icon indicating copy to clipboard operation
JSON-java copied to clipboard

Preserve insertion order when stringifing JSONObject

Open holgerbrandl opened this issue 5 years ago • 6 comments
trafficstars

Sure, the specs don't specify an ordering (as correctly pointed out in the javadocs), but a particular implementation - such as this one - can provide some ordering semantics. It is not wrong if an implementation is opinionated, there is just no definition in the json spec about this should be done.

So why not using LinkedHashMap instead of HashMap in https://github.com/stleary/JSON-java/blob/master/src/main/java/org/json/JSONObject.java#L178? This would improve (human) readability by preserving insertion order when printing json.

holgerbrandl avatar Nov 14 '20 20:11 holgerbrandl

@holgerbrandl Thanks for offering the suggestion. Keeping this issue open for a few days in case anyone else wants to post.

stleary avatar Nov 15 '20 06:11 stleary

Sounds like a good idea to me, but maybe implement it as a subclass that programmers can opt for when appropriate, e.g. JSONOrderedObject, to avoid unexpected compatibility or performance issues in existing apps?

janicki avatar Dec 10 '20 23:12 janicki

I'm still not a fan of an implicit order internally. I'm not opposed to a subclass that makes it explicit though. The JsonTokener would need the most modifications in that case in order to know what subclass of JSONObject to instantiate.

If we were to bump the minimum required JVM from 6-8, this would be fairly easy to do by adding a lamba parameter to the tokener which returns a JSONObject.

// Original JSONObject class
public class JSONObject {
// ...
   protected JSONObject(Map<String, object> backingMapStore) {
        this.map = backingMapStore;
   }

    public JSONObject(String source) throws JSONException {
        this(new JSONTokener(source, (JSONTokener t)->new JSONObject(t)));
    }

    // also extract the "tokener" code out into a new `protected final` method from the constructor
// ...
}

// new sub-class
public class JsonObjectInsertionOrder : JSONObject {

    public JsonObjectInsertionOrder() {
        super(new LinkedHashMap());
    }

    public JsonObjectInsertionOrder(String source) throws JSONException {
        super(new LinkedHashMap());
        this.populateFromTokener(new JSONTokener(source, (JSONTokener t)->new JsonObjectInsertionOrder(t)));
    }
}

// then would need to modify/add-to the tokener constructor(s) to take the new lambda
public class JSONTokener {
    /** constructor to use **/
    private Function<JSONTokener, JSONObject> jsonObjectConstructor;
    public JSONTokener(Reader reader, Function<JSONTokener,JSONObject> constructor) {
        // ... setup internal fields
        this.jsonObjectConstructor = constructor
    }


    public Object nextValue() throws JSONException {
        char c = this.nextClean();
        String string;

        switch (c) {
        case '"':
        case '\'':
            return this.nextString(c);
        case '{':
            this.back();
            return this.jsonObjectConstructor.apply(this);
        case '[':
            this.back();
            return this.(this);
        }
        // ... rest of function
}

johnjaylward avatar Dec 11 '20 21:12 johnjaylward

I tried to argue for a similar feature in issue #564. My proposal was to make the use of LinkedHasMap configurable in a system property. In the commit bellow you can see the changes needed to test the use of LinkedHasMap:

https://github.com/kovax/JSON-java/commit/dab6c125bd2eb546dc534a94e7ce1d9b952e0b25

kovax avatar Jan 11 '21 11:01 kovax

I'd like to propose a design that is minimally invasive and allows the support of custom Map implementation (like LinkedHashMap to have insertion ordered entries).

The rationale behind using this approach was:

  • I like to keep map property final for a lot of reasons, and replacing the internal map after the default instantiation of the HashMap was overkill
  • I couldn't use JSONObject(Map) constructor because it's already used to copy a map into the object
  • Having a protected constructor mandate extending the JSONObject class (not necessary, it could be public)
  • This allows also using hyper optimized map implementation (like Trove collections or Guava) if needed

The change would be to create a new protected constructor JSONObject( Class<? extends Map>) passing the class to instantiate at creation time. You can use that in this way:

class OrderedJSONObject extends JSONObject {
  public OrderedJSONObject() {
    super( LinkedHashMap.class );
  }
}

JSONObject json = new OrderedJSONObject();

And make map property of JSONObject class as protected instead of private.

Also, in JSONTokener a new property has been added with getter and setter:

private Class<? extends Map> mapImplementation = null;

So you can have an insertion ordered map starting from the tokener. Example of setting the map implementation in the JSONObject first to populate it with 100 entries, and then using the tokener.

class OrderedJSONObject extends JSONObject {
    public OrderedJSONObject() {
        super( LinkedHashMap.class );
    }
}

JSONObject jsonObject = new OrderedJSONObject();
for (int i = 0; i < 100; i++)
    jsonObject.put(String.valueOf(i), i);

JSONTokener tokener = new JSONTokener(jsonObject.toString()).setMapImplementation( LinkedHashMap.class );
JSONObject orderedObject = new JSONObject( tokener );

WDYT?

For more information about the code, I've already created a PR with the code and test: https://github.com/stleary/JSON-java/pull/658.

lvca avatar Dec 23 '21 20:12 lvca

Any news on this? Just checking.

lvca avatar Jan 18 '22 17:01 lvca

Closing due to unordered objects being a central part of the JSON spec.

stleary avatar Sep 03 '23 15:09 stleary

Unordered doesn't mean can't be ordered...I think this was a missed opportunity for this project to be used when the requirement is having an ordered map. Would it break the JSON spec? No, because there is no indication about how the properties are ordered. So having them ordered in some way would still be fine. After waiting for months on this we decided to use something else

lvca avatar Sep 03 '23 18:09 lvca

@lvca That is a reasonable opinion, and in fairness, most other JSON libraries do allow this option. However, I think the original author has let us know his preferences, and given that he wrote the original JSON spec, this project will honor his guidance.

stleary avatar Sep 04 '23 16:09 stleary

Came across this issue while writing automated tests against a REST API that had a defect that required certain JSON keys to appear before others in a POST body. That was clearly a defect in the API, but unfortunately also meant I couldn’t use JSONObject (at least easily) as the means to construct and then send the POST body to that API. In my case I was fortunate that I could get that API fixed, but for a 3rd party API, users wishing to use JSONObject may be stuck, at the mercy of the API maintainers, and need to pursue an alternative library that offers a way to honor insertion order.

I can say I appreciate that JSONObject defaults to not honor insertion order, as that helped us discover this defect. But offering a means by which one could explicitly say to honor insertion order would provide a way to continue to use JSONObject when calling APIs that require a fixed order without needing to resort to a different JSON library.

snowymike avatar Feb 07 '24 16:02 snowymike