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

JsonArray is now a List and Cloneable #2169

Merged
merged 0 commits into from
Aug 21, 2022
Merged

JsonArray is now a List and Cloneable #2169

merged 0 commits into from
Aug 21, 2022

Conversation

xtay2
Copy link

@xtay2 xtay2 commented Aug 5, 2022

The JsonArray is now a List and supports all of its operations.

  • This includes add(index, elem), remove(index, elem), contains(elem) and more.
  • All operations that don't directly copy the behaviour of the underlying ArrayList have tests.
  • There is a new Constructor to create a JsonArray from any Collection of JsonElements (null-safe).
  • All add(primitive) methods now support var-args.
  • 100% backwards-compatible

Its also a Cloneable

  • Because its support of deepCopy(), it is now also a Cloneable, like ArrayList.

@xtay2
Copy link
Author

xtay2 commented Aug 5, 2022

If this gets approved, JsonObject will get the same treatment with the Map-Interface.

@eamonnmcmanus
Copy link
Member

See #683, which was substantially the same. I see at least one major problem, which is that it is not possible to implement equals such that it is compatible both with the current equals and with the contract of List.equals:

  JsonArray jsonArray = ...
  List<JsonElement> list = new ArrayList<>(jsonArray);
  assert list.equals(jsonArray) && !jsonArray.equals(list); // oops

We might consider adding a method List<JsonElement> asList() to JsonArray, which would return a modifiable List view. I think that would be compatible. Possibly it could just return the internal elements field.

If you want to tackle that or any other issue, please respect the existing formatting of the project: two-space indentation; no wildcard imports; control statement bodies must use braces even if there is only one statement. See the Google Java Style Guide.

@Marcono1234
Copy link
Collaborator

Marcono1234 commented Aug 5, 2022

We might consider adding a method List<JsonElement> asList() to JsonArray, which would return a modifiable List view. I think that would be compatible. Possibly it could just return the internal elements field.

The issue with this is that it would not protect against null elements (!= JsonNull) anymore. This seems to also be an issue with this PR, the listIterator and subList methods directly return the result of the delegate. Though there is already a PR for adding an asList() method: #1209

Also, did you delete ReflectionHelper intentionally?

@xtay2
Copy link
Author

xtay2 commented Aug 6, 2022

Equals

See #683, which was substantially the same. I see at least one major problem, which is that it is not possible to implement equals such that it is compatible both with the current equals and with the contract of List.equals:

Yes. I didn't wan't to touch existing implementations for the sake of backwards compatibility, but from my point of view this method could get enhanced flawlessly by using:

@Override
public boolean equals(Object o) {
	return o == this || o instanceof List<?> && ((List<?>) o).equals(elements);
}

Given there would be an existing scenario where equals would be used on a JsonArray and a List, it already would be an extremely weird typecheck like !(obj instanceof List<?>). I cannot imagine that there is serious critical business code out there which does something like this, and even then, now that JsonArray is a List, this statement would be true. So lets just implement it?

asList

We might consider adding a method List<JsonElement> asList() to JsonArray, which would return a modifiable List view. I think that would be compatible. Possibly it could just return the internal elements field.

My motivation to change this were the missing "add multiple elements", and "contains"-method. Because of the added linear time complexity, converting to a List and back is just not that nice.

Formatting

If you want to tackle that or any other issue, please respect the existing formatting of the project: two-space indentation; no wildcard imports; control statement bodies must use braces even if there is only one statement. See the Google Java Style Guide.

Sorry, I have an autoformatter and thought you guys had one too.

Reply

From my point of view, the implementation of List is integral to what a JsonArray should be. The equals-method should be changed, and the request should get merged with the current branch. If you are not happy with this, I'm offering to create a new class JsonList, that is syntactically identical to the JsonArray, and deprecate the JsonArray (without removal). Two constructors should get offered to transfer between one and another.

@xtay2
Copy link
Author

xtay2 commented Aug 6, 2022

Also, did you delete ReflectionHelper intentionally?

Yes, my autoformatter has deleted one unused import-statement, but I didn't want to touch a class I don't know.

}

@Override
public boolean equals(Object o) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should get replaced by:

@Override
public boolean equals(Object o) {
	return o == this || o instanceof List<?> && ((List<?>) o).equals(elements);
}

but this is debatable because of backwards-compatibility.

@Marcono1234
Copy link
Collaborator

Sorry, I have an autoformatter and thought you guys had one too.

The google/styleguide project provides formatter configs for Eclipse (might need some additional settings tweaks to get correct formatting, see google/styleguide#273 (comment)) and IntelliJ. Additionally there is https://github.com/google/google-java-format (have not used that myself yet).

Personally I just have configured the formatter to automatically adjust indentation, remove trailing whitespace and organize imports; otherwise this might cause too many differences for pull requests.
Though I am not a Google / Gson member; maybe these configurations and plugins can be used more effectively.

Also, did you delete ReflectionHelper intentionally?

Yes, my autoformatter has deleted one unused import-statement, but I didn't want to touch a class I don't know.

Could you please add it back? That class is definitely used.

One of the main problems would probably still be binary compatibility. This was not mentioned in great detail in #683, but I think the issue here is changing void add(JsonElement) to boolean add(JsonElement). Even though source-wise this might be backward compatible (maybe except for some corner cases with lambdas or method references), it is not binary backward compatible. The reason for this is that the class file encodes the method signature including the return type. Therefore changing the return type would lead to a confusing NoSuchMethodError unless users recompile their code (which might not always be possible if Gson is used by third-party dependencies). This is also described in this blog post. There are multiple tools for working around this, but it would require adjusting the Maven build setup.

@xtay2
Copy link
Author

xtay2 commented Aug 6, 2022

Formatting

The google/styleguide project provides formatter configs for Eclipse

Thanks, I will look into that!

ReflectionHelper

Also, did you delete ReflectionHelper intentionally?

Yes, my autoformatter has deleted one unused import-statement, but I didn't want to touch a class I don't know.

Could you please add it back? That class is definitely used.

Is it possible to change this pull request? I didn't want to delete the file in the first place, I just wanted to remove the changes from the request.

Binary Compatibility

One of the main problems would probably still be binary compatibility. This was not mentioned in great detail in #683, but I think the issue here is changing void add(JsonElement) to boolean add(JsonElement). Even though source-wise this might be backward compatible (maybe except for some corner cases with lambdas or method references), it is not binary backward compatible. The reason for this is that the class file encodes the method signature including the return type. Therefore changing the return type would lead to a confusing NoSuchMethodError unless users recompile their code (which might not always be possible if Gson is used by third-party dependencies). This is also described in this blog post. There are multiple tools for working around this, but it would require adjusting the Maven build setup.

But if another project would use the updated library, wouldn't they have to recompile either way?
If this is really a problem I would gladly double down on implementing a JsonList.

@Marcono1234
Copy link
Collaborator

Marcono1234 commented Aug 17, 2022

But if another project would use the updated library, wouldn't they have to recompile either way?

You are right. The problem would then only occur when projects have Gson as transitive dependency. In that case the third-party dependency using Gson could not be easily re-compiled.

@eamonnmcmanus, how do you think should we proceed? Would you prefer the asList(): List<JsonElement> approach which exposes the internal list (for which pull requests already exist)? Maybe I am also too concerned about null detection.

@eamonnmcmanus
Copy link
Member

eamonnmcmanus commented Aug 20, 2022

So, a number of thoughts here.

I knew this reminded me of something. I wrote this comment probably in about 2004 when I was on the JMX team at Sun. Very similar compatibility problems arose then, specifically around the return type of add. Just as RoleList couldn't implement List<Role> then, JsonArray can't implement List<JsonElement> now. I think it should have implemented that interface from the outset, rather than just Iterable<JsonElement>, but it's too late to change that.

So that leaves us with the asList() option, which incidentally was also the solution in the RoleList case. I agree with @Marcono1234 that the situation with null is delicate. The field List<JsonElement> elements cannot currently contain null and we should preserve that situation. We don't want to end up with two representations for JSON nulls, null and JsonNull.INSTANCE. So we need to intercept operations that would add null to elements and either replace it with JsonNull.INSTANCE or throw an exception.

I think there are two implementation strategies:

  1. We make a subclass of ArrayList that intercepts add operations.
  2. We have asList() return a wrapper that intercepts nulls before forwarding add to elements.

The first strategy is too fragile in my opinion. We'd have to override the 5 methods that can add or replace elements (2 × add, 2 × addAll, set) as well as the two listIterator methods to be completely safe, and even then new methods might be added in the future.

So I think the second strategy is the only viable one. asList() returns a List object that wraps elements. It can be an AbstractList that overrides the required 5 methods for a variable-size mutable list, adding null logic for the add and set ones before forwarding, and forwarding the other ones directly.

I would be inclined to return a new object every time asList() is returned rather than trying to cache. It's a very lightweight object and is likely to be used once then discarded.

I think canonicalizing null to JsonNull.INSTANCE is probably better than throwing an exception, though I could be convinced otherwise.

We would probably want to consider the same problem regarding JsonObject and Map<String, JsonElement>. I don't think there's a compatibility problem with signatures there (like the void add one here), but adding asMap would still be better than implementing Map because of the asymmetric-equals problem I mentioned earlier.

@xtay2
Copy link
Author

xtay2 commented Aug 21, 2022

Third strategy: Why not create a new class? These points would all be no problem with a fresh implementation.

Very similar compatibility problems arose then, specifically around the return type of add. Just as RoleList couldn't implement List then, JsonArray can't implement List now. I think it should have implemented that interface from the outset, rather than just Iterable, but it's too late to change that.

We cannot extend ArrayList because our legacy add(Role) method would then override add(E) in ArrayList, and our return value is void whereas ArrayList.add(E)'s is boolean.

Likewise for set(int, Role). Grrr. We cannot use covariance to override the most important methods and have them return Role, either, because that would break subclasses that override those methods in turn (using the original return type of Object).

Finally, we cannot implement Iterable so you could write for (Role r : roleList) because ArrayList<> implements Iterable<> and the same class cannot implement two versions of a generic interface. Instead we provide the asList() method so you can write for (Role r : roleList.asList())

The whole null-problem is easily avoided by using the implemented methods with their boolean return. (like add)

@xtay2 xtay2 merged commit 517d3b1 into google:master Aug 21, 2022
@Marcono1234
Copy link
Collaborator

Marcono1234 commented Aug 22, 2022

I think canonicalizing null to JsonNull.INSTANCE is probably better than throwing an exception, though I could be convinced otherwise.

There are two concepts: Gson's JsonArray which performs implicit null conversion, and Java's List.
Personally I think it would be better to not have implicit null conversion for the List returned by asList(). Otherwise users who obtain the List<JsonElement> (or maybe even List<?>) without knowing that it came from a JsonArray might be surprised by the null conversion. Similarly it might cause issues (or at least interesting corner cases) where for example: list.add(null) returns true but calling list.contains(null) afterwards returns false.
If we add a JsonObject.asMap() it would also make it more consistent since there we prevent null values already (#2167).
After all a user could simply provide JsonNull as value instead of null; the additional work for this is most likely not that great.


Third strategy: Why not create a new class?

Do you mean a JsonArray2 implements List<JsonElement> (or similar; probably needs a better name)? Not completely sure how well that would work, you would then need to provide conversion methods between JsonArray and JsonArray2.


Also, for future reference; it appears the force-push to this PR branch caused GitHub to erroneously think this PR was merged:

grafik

However, it looks like this PR was not actually merged; I have reported this issue to GitHub.
Edit: Looks like GitHub has fixed this behavior (at least for new force-pushes), see blog post.

@eamonnmcmanus
Copy link
Member

I don't think we want to create a new class just for this. An asList() method is a good enough solution.

Regarding null, @Marcono1234 has persuaded me that the returned List should be null-hostile, which is explicitly allowed by the List contract.

It looks as if this PR is borked for some reason but a new PR implementing asList() would be welcome.

@Marcono1234
Copy link
Collaborator

Another question might be whether we want to enforce that elements are really instances of JsonElement (similar to java.util.Collections.checkedList(...)), but I guess checking for that might be a bit too much. We can just hope that users don't perform any unchecked conversions or use raw types.


@xtay2, are you interested in giving the asList() (and optionally asMap()) approach mentioned above a try? No worries if you are not interested; in that case I might have a look.

@xtay2
Copy link
Author

xtay2 commented Aug 27, 2022

@Marcono1234 I will do it in the next few days, when I find the time :)

@Marcono1234
Copy link
Collaborator

@xtay2, are you still interested on working on this? No hurry in case you are still planning to do this; I just wanted to make sure this is not forgotten.

@Marcono1234
Copy link
Collaborator

@xtay2, I have added the JsonArray.asList() method now in #2225, I hope that is fine for you (sorry if you were working on this already as well). Feel free to give feedback on that pull request if you spot anything which could be improved. And thank you for your pull request here nonetheless!

@Marcono1234
Copy link
Collaborator

Marcono1234 commented Oct 25, 2022

@eamonnmcmanus, the 2.10 release notes erroneously refer to this PR here:

JsonArray is now a List and Cloneable by @xtay2 in #2169
@xtay2 made their first contribution in #2169

Could you please remove this to avoid any confusion since this PR was not merged (instead the view methods were added as mentioned above).

@eamonnmcmanus
Copy link
Member

Oh yes, the problem with GitHub thinking this PR has been merged, which you mentioned above, also caused it to include the PR in the auto-generated list for the release notes. I've removed it.

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 this pull request may close these issues.

3 participants