Fix JsonArray contains for non-wrapped JsonArray/JsonObject

[kristian]

Fixes #5397

[tsegismont]

Thank you for this PR @kristian

[tsegismont]

In the worst scenario, we will iterate over the list three times. Besides, we want to be able to check if the JsonArray contains a JsonArray or JsonObject regardless of how it's stored, not to check if a JsonArray contains an arbitrary list or map.

I would suggest to use something like this:

  @SuppressWarnings("unchecked")
  public boolean contains(Object value) {
    if (value == null) {
      for (Object entry : list) {
        if (entry == null) {
          return true;
        }
      }
    } else {
      for (Object entry : list) {
        if (entry instanceof Map && value instanceof JsonObject) {
          return value.equals(new JsonObject((Map<String, Object>) entry));
        }
        if (entry instanceof List && value instanceof JsonArray) {
          return value.equals(new JsonArray((List<String>) entry));
        }
        if (value.equals(entry)) {
          return true;
        }
      }
    }
    return false;
  }

[kristian]

Thanks for the suggestion @tsegismont. Yes I was debating back and forth whether to essentially re-implement ArrayList#contains or whether the better code quality with the downside of iterating over the list a maximum number of 2 times (not 3, because it is either a JsonArray or JsonObject you want to check for) is the better solution. I can agree that we should go for the option that is better from a performance point of view. In my solution I did optimize for doing less comparisons, however I agree that the iteration itself is much more time consuming than doing the comparison operations.

Besides, we want to be able to check if the JsonArray contains a JsonArray or JsonObject regardless of how it's stored, not to check if a JsonArray contains an arbitrary list or map.

Mhm, as far as I am concerned, I would argue that a Map / List inside of a JsonArray is virtually equivalent to it's wrapped JsonArray / JsonObject counterpart. Reason for me is the public interface of JsonArray does wrapping on getting anyways, so classic duck-typing principle: If it walks like a duck and it quacks like a duck, then it must be a duck. An arbitrary Map / List is not supported inside of a JsonArray, it is just a "vehicle" for representing nested arrays anyways. This is why for your solution I would argue that creating temporary JsonObject / JsonArray depending on the size of the JsonArray creates a lot of temporary objects and thus a lot of pressure on the heap / garbage collection.

I would maybe suggest to "mix" both solutions and:

a) Keep the assumption that Map / List inside of JsonArray is equivalent to JsonObject / JsonArray and
b) Re-implement contains based on ArrayList#contains as you suggested.

Let me make a proposal to continue our discussion. 👍 Thanks for the good suggestion.

[kristian]

@tsegismont I made a new suggestion in my latest force push.

[halber]

I do not think that there is a good way to fix this behavior. I think the best thing you can do is to document this.

For example, if you apply this fix, then the following would work:

JsonArray jsonArray = JsonArray.of(Map.of("foo", "bar"));
assertTrue(jsonArray.contains(JsonObject.of("foo", "bar")));
assertTrue(jsonArray.contains(Map.of("foo", "bar")));

That is also a behavior that is not expected. When you decided to change the returned value (e.g., io.vertx.core.json.impl.JsonUtil#wrapJsonValue) but didn’t apply the same transformation to the stored values, you broke the contains contract. The good thing is that you do not implement the List interface. Therefore, you don’t have to comply with this contract. However, it is still an expected behavior and, of course, a trap. With this fix, the behavior becomes even weirder, in my opinion. Also, you would need to apply this logic for all transformations you perform in JsonUtil#wrapJsonValue. For example:

Instant now1 = Instant.now();
JsonArray jsonArray = JsonArray.of(now1);
assertTrue(jsonArray.contains(now1));

for (Object o : jsonArray) {
  assertTrue(jsonArray.contains(o));
}

This test will fail, which is unexpected to me. You can’t easily fix this because o is, in this example, a String. The same is true for byte[], Buffer, and Enum. You can’t reverse these transformations easily.

I consider the contains method as broken. Possibly consider deprecating the JsonArray#contains method.

[tsegismont]

@halber with the suggestions made in the review, there would be no weird cases with JsonObject/JsonArray entries.

But you're right, we need some Javadoc on the contains method.

[kristian]

Thanks for the review @vietj, all changes are included in my latest force push

[vietj]

actually I don't think this should pass

[vietj]

instead rewrite it as jsonArray.getList().add(...)

[vietj]

json array should not return contains true for map/list structures

[kristian]

json array should not return contains true for map/list structures

@vietj I get the point, however I think I need some guidance of how you want to see it fixed:

Generally JSONArray accepts an unwrapped (Map/List) object in all of it's interface, you can add a Map / List and the moment you get it again, it is automatically wrapped into a JSONObject / JSONArray. Do you want to generally prevent JSONArray to contain Lists/Maps via the interface? So adding a Map/List will result in a type exception? Otherwise the following calls would result in false which is maybe quite strange:

List list = List.of("foobar");
jsonArray.add(list);
jsonArray.contains(list); // false

With the current implementation (without any checks), List is idempotent to JSONArray and Map is idempotent of JSONObject. I don't have a strong preference for either solution, but the current implementation lets any non-JSON type fall through wrapJsonValue, so you could add unsupported types to any JSONObject or JSONArray, which would work "just fine" (if you don't try to encode it) and contains would also return the correct value, just when you go for a Map / List it would return false in any case?

Alternative solution would be, that you could also check contains(Map) or contains(List) and intrinsically it checks for again both JSONObject and JSONArray so something along those lines:

      Object reverseWrappedValue = value;

      if (value instanceof JsonObject) {
        reverseWrappedValue = ((JsonObject) value).getMap();
      } else if (value instanceof JsonArray) {
        reverseWrappedValue = ((JsonArray) value).getList();
      } else if (value instanceof Map) {
        reverseWrappedValue = new JSONObject(value);
      } else if (value instanceof List) {
        reverseWrappedValue = new JSONArray(value);
      }

      // ...

I think this "you can use either" approach fits best to the current implementation of JSONObject / JSONArray. Happy to hear your oppinion.

[vietj]

ok I see your point @kristian , let me think over this and get back to you :-)

[vietj]

I think it is fine as is actually, the API is lenient when Object is used (e.g. add(Object)) and it can be abused. I have been fooled by the tests thinking that we do support adding directly a map or a list.

i.e. I think we can support that we add directly a map/list, however it should not be part of the contract

In other words, the contract of adding an object is, add whatever object you want but you should not rely on the fact that adding a map will turn it into a valid JsonObject later.

[kristian]

In other words, the contract of adding an object is, add whatever object you want but you should not rely on the fact that adding a map will turn it into a valid JsonObject later.

Agreed. Do you still want to also support contains(Map) to return true in case the JsonArray contains a JsonObject of Map?

[vietj]

@kristian thanks for the contribution, can you back port this to the 4.x branch ?

[kristian]

@kristian thanks for the contribution, can you back port this to the 4.x branch ?

@vietj sure, #5414. Happy to help.