-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JSONObject and JSONArray initialization: #153
Conversation
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>
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. |
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);
JSONArray ja = new JSONArray((Collection<Object>)myC); After: Collection<Integer> myC = Collections.singleton(Integer.TEN);
JSONArray ja = new JSONArray(myC); both calls should result in the same generated JSONArray. |
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? |
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. |
Actually, in mine, I took the Map 1 step further and made both the key and the value generic: public JSONObject(final Map<?, ?> map) throws JSONException {
if (LOG.isTraceEnabled()) {
this.map = new TreeMap<>();
} else {
this.map = new HashMap<>();
}
if (map != null) {
for (final Entry<?, ?> e : map.entrySet()) {
final Object value = 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 |
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. |
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 :) |
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 |
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. |
wildcard generic types, e.g. Collection<?> instead of Collection<Object>. This was proposed by other pull requests (#111, #112) already. Consider this commit as merge with #111 and #112. JSONArray: - put(Collection<?> value) {...} - put(Map<String, ?> value) {...} - put(int index, Collection<?> value) throws JSONException {...} - put(int index, Map<String, ?> value) throws JSONException {...} JSONObject: - put(String key, Collection<?> value) throws JSONException {...} - put(String key, Map<String, ?> value) throws JSONException {...} Changed all code affected by new JSONObject and JSONArray constructors: JSONObject: - valueToString(Object value) throws JSONException { - value instanceof Map - value instanceof Collection } - wrap(Object object) { - value instanceof Map - value instanceof Collection } - writeValue(Writer writer, Object value, int indentFactor, int indent){ - value instanceof Map - value instanceof Collection }
Thank you @bguedas for adding your comments and pull requests. I included them in my latest commit. |
changed Iterator to foreach loop in JSONArray ctor JSONArray(Collection<?> collection) and JSONObject ctor JSONObject(Map<?,?> map)
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 |
(sorry was logged in as the wrong account) public JSONObject(final Map<?, ?> map) throws JSONException {
this.map = new HashMap<>();
if (map != null) {
for (final Entry<?, ?> e : map.entrySet()) {
final Object value = 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 As for JSONArray, the implementation in this pull request should be functionally equivalent to the original pre-java8 ctor The new |
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. |
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 |
@treyerl would you also be able to update the "put", "wrap", "writeValue", and "valueToString" methods in this pull request:
JSONObject:
|
Wow, I need to get better at reading diffs. It looks like all the conversions are already handled properly. Thanks @treyerl ! |
@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 |
I think
|
@treyerl what package error messages? |
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. |
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 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. |
Thanks @johnjaylward, for pointing me at it. If I need to create another pull request I will try to follow this ;-) |
It is the same use case as the constructors. If we want to maintain backwards compatibility, then these need to be updated. |
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? |
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);
JSONArray ja = new JSONArray(myC);
@SuppressWarnings("rawtypes")
Collection myRawC = Collections.singleton(Integer.TEN);
JSONArray ja2 = new JSONArray(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");
JSONObject jo = new JSONObject(myC);
@SuppressWarnings("rawtypes")
Collection myRawC = Collections.singletonMap("Key","Value");
JSONObject jo2 = new JSONObject(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);
JSONArray ja = new JSONArray();
ja.put(myC);
@SuppressWarnings("rawtypes")
Collection myRawC = Collections.singleton(Integer.TEN);
JSONArray ja2 = new JSONArray();
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");
JSONObject jo = new JSONObject();
jo.put("someKey",myC);
@SuppressWarnings("rawtypes")
Collection myRawC = Collections.singletonMap("Key","Value");
JSONObject jo2 = new JSONObject();
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. |
If the existing code does not pass the tests, then changes to the put methods would be needed. |
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) |
Think I need to reopen in order to pull to a local branch. |
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 |
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>? |
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. |
@stleary: did you mean "<String, Object> instead of <?,?>" ? |
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. 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. |
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 |
What problem does this code solve? Root cause analysis Mitigation
Risks
Will this result in changes to the API?
Changes to how the code behaves?
Does it break the unit tests? Will this require a new release? Should the documentation be updated? |
Just noticed a typo in the last comment under the JSONObject API changes: |
Fixed, thanks. |
Merging now, but will wait a week or so to cut a release and update Maven, in case any users report problems. |
JSONObject and JSONArray initialization for Map<?,?> and Collection<?>
Test cases for PR stleary#153 in JSON-Java
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: