Skip to content
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

Support equality testing #573

Closed
holgerbrandl opened this issue Nov 15, 2020 · 15 comments · Fixed by #575
Closed

Support equality testing #573

holgerbrandl opened this issue Nov 15, 2020 · 15 comments · Fixed by #575

Comments

@holgerbrandl
Copy link

It would be great if the library would support equals for JSONObject.

An important use case would be equality/regression testing. See here for an example which hacks around the problem by using toString (which somehow works in this particular case but is certainly not good practise).

Another use case would be the use of JSONObject as map keys.

Since there seems to be just a single data fields (map) in JSONObject, it may be possible to simple delegate to HashMap.equals. Clearly, also JSONArray would need to be equipped with an equals (see https://stackoverflow.com/questions/4082416/equality-between-2-hashmap or https://www.baeldung.com/java-compare-hashmaps).

Alternatively, the docs https://github.com/stleary/JSON-java#notes shall detail out better why an equals implementation can not be provided.

@stleary
Copy link
Owner

stleary commented Nov 16, 2020

Please try the JSONObject and JSONArray methods boolean similar(Object) and see if they do what you want.

Agreed that the docs need a lot of improvement. I will be getting to that.

@holgerbrandl
Copy link
Author

Thanks for the quick support and the pointer to similar.

However, since java builds a lot on top of equals (and hashCode with which it shares a common contract), similar is no real substitute for equals/hashCode imho.

@holgerbrandl
Copy link
Author

@johnjaylward
Copy link
Contributor

The contract is

Both methods are recursive and call each other as needed.

  1. Json Objects must have the same keys.
  2. Json Objects must have "similar" values for keys of the same names. This means that underlying object types must be the same. for each key. Non-Json types (anything except JSONObject and JSONArray) are compared via their own equals methods.
  3. Json Arrays must have the same number of elements
  4. Json Array elements must have "similar" values in the same index.

From our internal test cases using "similar" we have not had issues crop up recently, but there may be an outstanding bug we have missed. If you can provide us with your test data so we can reproduce your unexpected result, that would be appreciated.

Since you are using floating point numbers from what I can see, my guess it that the failure around those comparisons.

@holgerbrandl
Copy link
Author

holgerbrandl commented Nov 19, 2020

The reason for similar not returning true in my example/test whereas toString.equals does, is that one field (duration) is a Double in one JSONObject and an Integer in the second JSONObject. This is rendered in the same way (no digits for the double) but similar detects it as being different. The value is 50 in both cases, so its not the value but the type mismatch which causes the false.
image

Not sure if this qualifies as a bug, but its an API inconsistency imho at least.

@johnjaylward
Copy link
Contributor

Agreed. It would make sense to compare numeric values by value only.

johnjaylward pushed a commit to johnjaylward/JSON-java that referenced this issue Nov 19, 2020
johnjaylward pushed a commit to johnjaylward/JSON-java that referenced this issue Nov 19, 2020
johnjaylward pushed a commit to johnjaylward/JSON-java that referenced this issue Nov 19, 2020
johnjaylward pushed a commit to johnjaylward/JSON-java that referenced this issue Nov 19, 2020
johnjaylward pushed a commit to johnjaylward/JSON-java that referenced this issue Nov 19, 2020
@jdiasamaro
Copy link

Hi @johnjaylward thank you for the similar() explanation. Indeed it is much better now.

However, I can only agree with @holgerbrandl in the sense that these JSON object and array should implement a stable equals/hashCode/toString.

Lots of platform types (for instance, collections) and also assertion libraries depend on a stable contract of equals/hashCode to handle equality.

For JSON arrays, 2 arrays are equal if:

  • sizes are the same
  • every position is equal on this internal array and the other array.

For JSON objects, 2 objects are equal if:

  • number of keys is the same
  • for every key on this objects, the other has the same key with an equal value.

Which is the same as similar. Wouldn't it make sense to have this?

public boolean equals(Object other) {
   return this.similar(other);
}

Also hash codes should be the same if the 2 JSON arrays or objects share the same internal state. For instance:

List<String> list = Arrays.asList("foo", "bar");

JSONArray a1 = JSONArray().putAll(list);
JSONArray a2 = JSONArray().putAll(list);

// this is false
boolean theSame = a1.hashCode() == a2.hashCode();

@johnjaylward
Copy link
Contributor

Our API contract that is shared with Android and maybe other implementations is that equals and hashcode are not implemented (uses base Object equals and hashcode).

I don't see us changing this anytime soon unless it can be shown that other implementations that share the namespace/api (like Android) have also moved to implementing those methods.

@jdiasamaro
Copy link

Hey! Thank you so much for getting back on this.

Not sure if I am looking into the correct Android source code location, but do you mean this? https://android.googlesource.com/platform/libcore/+/android-4.4_r1.1/json/src/main/java/org/json/JSONArray.java#607 (lines 607 - 614)

@johnjaylward
Copy link
Contributor

More accurately it would be here: https://android.googlesource.com/platform/libcore/+/refs/heads/master/json/src/main/java/org/json/

Due to licensing, we can not use any implementation details from that project. Or at least I'm not willing to get into that quagmire.

@johnjaylward
Copy link
Contributor

johnjaylward commented Dec 8, 2021

A quick look shows that Android is inconsistent in their implementation. It looks like JSONArray.equals is the only one implemented, and their JSONArray.hashcode implementation specifically notes that it diverges from our master (we are the "original")

@jdiasamaro
Copy link

jdiasamaro commented Dec 8, 2021

I see, is it for licensing reasons that the "original" can't be changed?

Indeed Android is inconsistent even on JSONObject. I can't find the equals/hash on JSONObject.

Just wondering how this could create Android licensing issues since this would just be a simple equals/hashCode or even a redirection to the similar() method.

@johnjaylward
Copy link
Contributor

for why we (likely) won't be changing the original, see the FAQ:
https://github.com/stleary/JSON-java/wiki/FAQ#how-do-you-decide-which-pull-requests-to-accept

Does it change the behavior of the lib? If so, it will not be accepted, unless it fixes an egregious bug. This is happening less frequently now.

At this point, the library is 12+ years old (10+ on github), changing the way hashcode/equals work would be a fairly major change in how the library works.

@johnjaylward
Copy link
Contributor

Licensing was not a concern for the change, only for sharing of implementation details. Android was brought up because it is the most widely known & used rewrite of this library, and we try to remain API compatible with it when possible.

@jdiasamaro
Copy link

I understand. It makes sense. It might be breaking something somewhere.

Thank you so much for your help @johnjaylward :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants