JSONObject and JSONArray initialization:

[treyerl]

JSONObject(Map<String, ?> map) allows to initialize the JSONObject with
a Map<String, String>

JSONArray(Collection<?> collection) allows to initialize a JSONArray
with a Collection<JSONObject>

tests:

public static void testJSONObjectWithStringMap(){Map<String, String> pairs = new HashMap<String, String>(); pairs.put("one", "two"); JSONObject j = new JSONObject(pairs); System.out.println(j); System.out.println(pairs); // what is wrong? --> since Map<String, String> cannot be mapped to Map<String, Object> // the new JSONObject is being initialized with JSONObject(Object bean) } public static void testJSONArrayWithCollectionOfJSONObject(){List<JSONObject> jsonObjects = new ArrayList<JSONObject>(); jsonObjects.add(new JSONObject().put("one", 1)); jsonObjects.add(new JSONObject().put("two", 2)); JSONArray jsonArray = new JSONArray(jsonObjects); System.out.println(jsonArray); // what is wrong? --> since Collection<Object> cannot be mapped to Collection<JSONObject> // the new JSONArray is being initialized with JSONArray(Object array) } 

[stleary]

Thanks for the proposed code change and for including unit tests.

This looks like an enhancement to some of the ctors to convert a map param to JSONObject and a list param to JSONArray, rather than to process them as beans. I don't call it a bug fix, since this may have been the intended behavior, although it is possible that it was just a matter of generics not being available at the time.

Any code that results in behavior changes has a high bar for acceptance. We only want to allow changes that are clearly needed, and won't cause problems to current users who expect the pre-change behavior. When possible, it is better to enhance the API with new methods. I don't think we can accept this, but will leave the pull request open for a few days to get more input.

[johnjaylward]

I don't think this is changing any functionality. It appears that it is just allowing you to have more generic inputs placed into the ctor without needing a conversion on the call

Before:

Collection<Integer> myC = Collections.singleton(Integer.TEN); JSONArrayja = newJSONArray((Collection<Object>)myC);

After:

Collection<Integer> myC = Collections.singleton(Integer.TEN); JSONArrayja = newJSONArray(myC);

both calls should result in the same generated JSONArray.

[stleary]

The proposed change passed unit tests, so it does not introduce regressions. Agreed, users who have up til now had to cast their params will benefit. But it seems to me that some users may be counting on the bean behavior. In that case, could it not break their applications?

[johnjaylward]

I'm not sure any user would want a Map translated as a Bean. It seems like an odd thing to depend on the internals of a Collection or a Map to convert those to Json. Personally I implemented this change years ago in my own code upon the switch to java 5 or 6.

[johnjaylward]

Actually, in mine, I took the Map 1 step further and made both the key and the value generic:

publicJSONObject(finalMap<?, ?> map) throwsJSONException{if (LOG.isTraceEnabled()){this.map = newTreeMap<>()} else{this.map = newHashMap<>()} if (map != null){for (finalEntry<?, ?> e : map.entrySet()){finalObjectvalue = e.getValue(); if (value != null){this.map.put(String.valueOf(e.getKey()), wrap(value))} } } }

I used a treemap so when debugging I could see the keys in an ordered fashion

[treyerl]

I recently switched to maven in one of my projects, and now I depend on json-java. Before that I was using the old json.org library as a jar. Actually it was these generics breaking the code for me. If you think it should be general practice to cast everything to Map<String, Object> and Collection I can adapt of course. I'm just not sure if that's good practice.

[stleary]

We see a similar issue with Enum, which is handled by the JSONObject and JSONArray ctors as a bean. I think a reasonable argument could made that, although the library has been brought to 1.8 compatibility, it would benefit from a rewrite to incorporate new features like enum and "?" parameters.

On the other hand, the API clearly advertises what types it supports, and within those limits works correctly, so far as I have been able to determine, and as evidenced by the unit tests.

So far as I know, the author of this lib has no plans for a non-backwards-compatible "release 2.0" which explicitly supports the latest Java features (but please correct me if I am wrong).

I think the proposed changes have merit. But I don't think we can accept them at this time. If you think you can rework the changes as an enhancement, please do so, as long as it does not break backwards compatibility with existing apps.

Or if you think I am totally off-base, please let me know as well :)

[treyerl]

I thought that I clearly pointed out that the process of bringing the lib to 1.8 DID break backwards compatibility. I don't see why "?-Parameters" (wildcard generic types) would break the code. Could you provide an example for this? Or in other words: why do you think my code is no enhancement? What else would I need to provide to convince you that this is an enhancement? As shown by the comments people start to create their own versions of this library. You would argue that they create their own enhancements. For me it's bugfixing to bring back the capabilities we had with the original version. Here's why: http://docs.oracle.com/javase/tutorial/extra/generics/wildcards.html

[stleary]

I think the point being made in this pull request and in #155 is that upgrading to the latest Maven release is breaking some applications. Re-opening for discussion of this change as a bug fix instead of an enhancement. If your previously working application broke because you upgraded to (Java 1.8 compatible) 20150729, please continue the discussion here. So far I see an issue with the JSONObject(Map<String, Object>) ctor and the JSONArray(Collection<Object>) ctor.

[ghost]

Thanks for reopening this pull request which is more complete than mine.

Other related requests: #111, #112 and #114

[treyerl]

Thank you @bguedas for adding your comments and pull requests. I included them in my latest commit.
@johnjaylward: as stated on json.org, pairs need to have string keys. Therefore I stick to Map<String, ?> atm. Nevertheless, I see that your proposal takes care of this. So I will perhaps add this too, if everybody agrees.
@stleary: I was thinking about another future pull request on adding the generic types to JSONObject<K,V> and JSONArray<E> but ran into troubles after two lines already. It made me thinking that this would go too far and would imply a rewrite of the library as you stated earlier. But perhaps others did that job already with other libraries (e.g. jackson). I don't know other libraries well enough to judge on that. As for the wildcards I added in this pull request, I don't think they will urge us to do a rewrite. I think the ArrayList that backs JSONArray needs to remain a ArrayList<Object> as any object in an JSONArray must be nullable since [, "some", , "thing"] is valid json for instance. Similarly, the map that backs JSONObject must be a Map<String, Object> to my opinion. Even with @johnjaylward's proposal this would be granted.

[stleary]

The various ctors are not core functionality, in my opinion. It is sometimes convenient for the lib to automatically populate JSONObject and JSONArray instances. The API advertises what types it does and does not support. However, it is easy for a user to misinterpret the API, e.g. to expect a Map<String, String> to be handled by the JSONObject<String, Object> ctor. Also, it seems that some users have experienced incompatibilities when upgrading to the 20150829 release.

It is probably best to avoid mixing bug fixes with enhancements or redesigns. At work, I shamelessly push enhancements while fixing random bugs, but try to hold to a higher standard here. For now, the goal is to fix the incompatibility while minimizing changes to the code and avoiding any change to intended behavior. Reverting to the previous ctor API should probably be on the table, too. Any proposed solution will need to be rigorously tested to avoid regressions or unintended behavior.

Let me know if you see the problem statement and/or proposed scope of the fix differently

[johnjaylward]

(sorry was logged in as the wrong account)
Sean, I believe reverting to the old constructor would have the same functional implication as my code sample posted above (just remove my tree map)

publicJSONObject(finalMap<?, ?> map) throwsJSONException{this.map = newHashMap<>(); if (map != null){for (finalEntry<?, ?> e : map.entrySet()){finalObjectvalue = e.getValue(); if (value != null){this.map.put(String.valueOf(e.getKey()), wrap(value))} } } }

The old ctor was not converting the key here, but instead at output time. (see https://github.com/douglascrockford/JSON-java/blob/34f327e6d070568256b314479be158589d391891/JSONObject.java#L250 and https://github.com/douglascrockford/JSON-java/blob/34f327e6d070568256b314479be158589d391891/JSONObject.java#L1604)

The Map<?,?> erases to the original raw type Map. So the new constructor will accept all the same parameter values as the original. Further, my change for the Key makes this conversion null-safe for null keys as it will print "null" instead of thrown a null pointer exception. If we want to stick with non-null keys, then replace it with this.map.put(e.getKey().toString(), wrap(value));

As for JSONArray, the implementation in this pull request should be functionally equivalent to the original pre-java8 ctor
(https://github.com/douglascrockford/JSON-java/blob/7ff3fa4e40cfd5b44570297080810aed5cce6086/JSONArray.java#L153)

The new Collection<?> erases to the same raw type Collection as the original, so should accept all the same parameters passed. The rest of the logic in the function is the same, only updated to use the new for loop syntax.

[treyerl]

BUG: as stated in the oracle tutorial:

Collection<Object>, which, as we've just demonstrated, is not a supertype of all kinds of collections!

Collection<?> is the supertype.

Various constructors? This pull request fixes the affected JSONArray constructor and the affected JSONObject constructor as well as all the code that is calling those constructors.

The only question that remains: should we throw an exception upon null keys in a map? I think we should have it behave as close to the existing code as possible. I will think about it.

Again, this pull request is no enhancement. It fixes a small bug introduced with generics.

[johnjaylward]

Yes, I'm not sure I was clear in my previous comments, but this is a bug fix. When the "Java8" (a9a0762) was committed, @douglascrockford improperly (if trying to maintain backwards compatability) used exact typing for the parameter to the ctors in JSONArray and JSONObject.

The correct syntax for full backwards compatibility would be to use the Map<?,?> and Collection<?> for the parameters. The underlying store should have exact parameters as @treyerl has stated previously.

[johnjaylward]

@treyerl would you also be able to update the "put", "wrap", "writeValue", and "valueToString" methods in this pull request:
JSONArray:

• public JSONArray put(Collection<Object> value){
• public JSONArray put(Map<String, Object> value){
• public JSONArray put(int index, Collection<Object> value)
• public JSONArray put(int index, Map<String, Object> value)

JSONObject:

• public JSONObject put(String key, Collection<Object> value)
• public JSONObject put(String key, Map<String, Object> value)
• public static Object wrap(Object object){, static final Writer writeValue(Writer writer, Object value,, public static String valueToString(Object value) Here you'll need to update the type conversions:

  • if (objectinstanceofCollection){returnnewJSONArray((Collection<Object>) object)} if (object.getClass().isArray()){returnnewJSONArray(object)} if (objectinstanceofMap){returnnewJSONObject((Map<String, Object>) object)}

[johnjaylward]

Wow, I need to get better at reading diffs. It looks like all the conversions are already handled properly. Thanks @treyerl !

[johnjaylward]

@stleary I'm trying to run the test cases but am failing one that this pull request would fix. Would you like me to file an issue in the Test project? Specifically it's the jsonObjectPut() test, and it fails because the expected value is a JSONArray, but the actual value is ArrayList. The comparison is comparing the toString() value but the ArrayList outputs with spaces, while the JSONArray does not.

[treyerl]

I think String.valueOf(e.getKey()) as proposed by @johnjaylward is the closest counterpart to the pre-Java8 version of the lib. A NullPointerException was never thrown. Nevertheless I expect* a different JSON serialisation output. When initializing a JSONObject of the old version of the lib with a map{null="string"} its toString() method produces "null". With this pull request I expect* it to output "{"null":"string"}, which is as opposed to the earlier version still valid json. So yes, here we might speak of an enhancement. A really small one though... in an edge case I would question to rely on: why should I want to have a map with null keys and except the JSONObject I initiate with it to serialize to "null"?

• can somebody tell me a smart way to get around the org.json package error messages?

[johnjaylward]

@treyerl what package error messages?

[treyerl]

package error messages: to test the lib I have to copy all classes into a org/json/ folder in a different eclipse project, I assume. I verified my assumptions with such a test. I will also remove the "Unnecessary @SuppressWarnings("unchecked")" warnings.

[johnjaylward]

I wrapped them into a common gradle project and used git submodules. See https://github.com/johnjaylward/Json-Java-Combined

It's a little clunky using the sub modules, but I can do things like git submodule foreach 'git checkout -b featureA' to create a common branch between the test project and the main project to help me stay in sync.

You can see the project layout from my project. Forking it may or may not be useful for you since the submodules point to my own forks.

[treyerl]

Thanks @johnjaylward, for pointing me at it. If I need to create another pull request I will try to follow this ;-)

[johnjaylward]

It is the same use case as the constructors. If we want to maintain backwards compatibility, then these need to be updated.

[stleary]

We need to be able to justify each change. A case can be made for the ctors because there is evidence of existing apps using String objects breaking on upgrade. But I don't have a reason why the map key should not be required to be String, nor why the put methods should change. What will break, or not work as intended, if those changes are not made?

[johnjaylward]

I guess the best way to check would be to create some test cases of what we expect to happen vs what does happen.

What I expect to happen: (I did not compile or run these at all, may need tweeking)

Test the ctors:

Collection<Integer> myC = Collections.singleton(Integer.TEN); JSONArrayja = newJSONArray(myC); @SuppressWarnings("rawtypes") CollectionmyRawC = Collections.singleton(Integer.TEN); JSONArrayja2 = newJSONArray(myRawC); assertTrue("The RAW Collection should give me the same type as the Typed Collection", ja.similar(ja2) );

Map<String,String> myC = Collections.singletonMap("Key","Value"); JSONObjectjo = newJSONObject(myC); @SuppressWarnings("rawtypes") CollectionmyRawC = Collections.singletonMap("Key","Value"); JSONObjectjo2 = newJSONObject(myRawC); assertTrue("The RAW Collection should give me the same type as the Typed Collection", jo.similar(jo2) );

Test the put methods:

Collection<Integer> myC = Collections.singleton(Integer.TEN); JSONArrayja = newJSONArray(); ja.put(myC); @SuppressWarnings("rawtypes") CollectionmyRawC = Collections.singleton(Integer.TEN); JSONArrayja2 = newJSONArray(); ja2.put(myRawC); assertTrue("The RAW Collection should give me the same type as the Typed Collection", ja.similar(ja2) );

Map<String,String> myC = Collections.singletonMap("Key","Value"); JSONObjectjo = newJSONObject(); jo.put("someKey",myC); @SuppressWarnings("rawtypes") CollectionmyRawC = Collections.singletonMap("Key","Value"); JSONObjectjo2 = newJSONObject(); jo2.put("someKey",myRawC); assertTrue("The RAW Collection should give me the same type as the Typed Collection", jo.similar(jo2) );

We'd do this for each ctor and put method affected.

[johnjaylward]

If the existing code does not pass the tests, then changes to the put methods would be needed.

[stleary]

Opened stleary/JSON-Java-unit-test#27

[johnjaylward]

I added test cases for the regression testing as well as directions on how to run them here: stleary/JSON-Java-unit-test#28

You will see that everything in this PR is needed when run against all 3 commits (baseline, master, and this PR)

[stleary]

Think I need to reopen in order to pull to a local branch.

[johnjaylward]

it's probably best to leave it open until all testing is completed. Then it shows as a known issue and will hopefully prevent more duplicates from being posted

[stleary]

Conceded that the code both addresses the bug fix and is consistent with https://github.com/douglascrockford/JSON-java/tree/JSON-java-1.4, which makes for a nice implementation.

Why do you think the 1.8 commit used <String, Object>, instead of <Object, Object>?

[johnjaylward]

It was likely just an oversight. As @douglascrockford said before, he hasn't used the library in a while, so it probably just passed an initial test using a Map<String, Object> and not a thorough test using different types of maps.

[treyerl]

@stleary: did you mean "<String, Object> instead of <?,?>" ?
I agree with @johnjaylward. He most likely didn't test with maps other than <String, Object>.

[stleary]

I am not explaining my thoughts well, let me try to rephrase:

The API provides a mechanism via the put() methods for users to populate a JSONOjbect.
There are also JSONObject(Map<String,Object>) and put(String, Map<String,Object>) methods, which provide direct conversion to JSONObject of certain maps. My thought is that the these methods were intended to be a convenience for objects that already have a "JSON-like" structure (e.g. having String keys)

I can see the benefit of using <?,?> in the case where a non-String key class easily converts to String, but what about the user whose key class has no useful toString() method? Do we want to provide an API method that does not always work, where we cannot tell if it is not working?

It seems to me that the prudent thing to do is to open the API to allow all map values, but keep the restriction on String map keys.

[johnjaylward]

I see, so you'd like to change the (java1.4) functionality to now restrict all keys as strings.

I'm not sure that is the best idea. I like being able to pass in a Map<Integer, Object> for instance to have numbered keys for some things. I think maybe the best option would be to update the javadoc on the put(String, Map<?,?>), put(Map<?,?>) ,and JSONObject(Map<?,?>) methods to make it clear that the key portion of the map will be converted to a string even if the toString() method is not overridden on the key's type.

[stleary]

What problem does this code solve?
Starting with the https://github.com/douglascrockford/JSON-java/tree/JSON-java-1.8 commit, users have been unable to use a constructor or put() API method to add some maps as contained JSONObject instances in a single operation. For example, converting a Map<String,String> to JSONObject requires adding each entry via put(String, String). If the map is passed to a constructor or put() method, then JSONObject(Object) or put(String, Object) is invoked, which only adds the map getter values, not the map entries. It is still possible to add Map<String,Object> instances in a single operation.

Root cause analysis
The 1.4 commit did not include generics since they were not supported by Java. All maps and collections were accepted by the API. The only presumption in the code was that map keys were strings. Support for generics was added in the 1.8 commit. The API was updated to allow containers as they were stored internally - Map<String,Object> for JSONObject, and Collection<Object> for JSONArray. Generic containers inherit through the collection type, not the type parameters, so only the exact type parameters specified in the API signatures were accepted by these methods. This caused some existing code that invoked the changed methods with unsupported maps and collections to break. For example, calling the JSONObject constructor with a Map<String,String> parameter. This issue was previously noted in #111, #112, and #114.

Mitigation
The proposed code fix changes the API parameters wherever they appear, from Map<String,Object> to Map<?,?> and from Collection<Object> to Collection<?>.

• Alternate solution 1: Limit the scope of the Map change to Map<String,?>. This is still being investigated. See limited ctor change for 153 #164.
• Alternate solution 2: No change to the code. Except for ClassCastException in JSONObject.valueToString() #167 (which can be fixed separately), the API functions correctly and advertises its capabilities accurately.

Risks

1. Code that expects some maps and collections to be processed as objects may not work as intended. While it seems unlikely that anyone would want this behavior, it is possible that applications receiving the JSON have been written so as not to expect the new content, and will then fail.
2. The proposed fix opens the API for all Maps. Code that processes maps with keys lacking a useful toString() method may produce unexpected JSON. For example, the contents of a map may be reduced to a single entry if the toString() method emits the same string for each entry. It is not obvious from the method signatures that this behavior can occur.
3. At the end of the day, the proposed changes are for convenience methods, not core functionality. Users can always iterate through a container to add each entry individually. The existing code advertises exactly what types it accepts, and users can invoke the code accordingly. If preservation of the API is determined to be a higher priority than ease of use, then a change would not improve the library.

Will this result in changes to the API?
The following API methods may be affected:

Module	Method
JSONArray	JSONArray(Collection<Object>)
	put(Collection<Object>)
	put(Map<String,Object>)
	put(index, Collection<Object>)
	put(index, Map<String,Object>)
JSONObject	JSONObject(Map<String,Object>)
	put(String,Collection<Object>
	put(String,Map<String,Object>)

Changes to how the code behaves?
No Map and Collection parameters will be handled as objects when the changed API methods are invoked. JSONObjects may contain keys which are not derived from simple Strings. In the following JSONObject methods, type parameters for containers will be changed to wildcards:

Method
valueToString(Object)
wrap(Object)
writeValue(Writer, Object, int, int)

Does it break the unit tests?
No, but changes to functionality will require stleary/JSON-Java-unit-test#28 to be merged. However, additional tests will need to be added, in light of #167.

Will this require a new release?
Yes, this is a substantial change to the existing API which fixes a problem in the production code. A new GitHub release and a new Maven release will be required.

Should the documentation be updated?
Yes, The Javadocs for the affected methods will need to be updated. Additional text should be added to the README as well.

[johnjaylward]

Just noticed a typo in the last comment under the JSONObject API changes:
put(String,Collection<String,Object>) should be put(String,Collection<Object>)

[stleary]

Fixed, thanks.

[stleary]

Merging now, but will wait a week or so to cut a release and update Maven, in case any users report problems.