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

Fix ConstructorConstructor creating mismatching Collection and Map instances #2068

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

Marcono1234
Copy link
Collaborator

@Marcono1234 Marcono1234 commented Jan 29, 2022

Fixes #419
Fixes #1708
Fixes #2730 (the proper exception that interfaces cannot be instantiated will be thrown instead)

Gson's ProtoTypeAdapter (in the non-published proto module) indirectly relied on ConstructorConstructor's broken behavior:
During deserialization of 'repeated' fields it peeks at the type of the internal Java field. The problem is that this field does not have the type List<E> is all cases but for some types has specialized Protobuf subinterfaces. For example for List<Long> the internal field has the type com.google.protobuf.Internal.LongList.
So previously Gson erroneously created ArrayList instances even though Protobuf's LongList is not related to ArrayList. This worked because LongList is only an implementation detail, the Protobuf builder API seems to expect just List<E> so it did not cause any problems that Gson used an ArrayList. (Still the behavior that Gson created ArrayList when asked for a LongList is arguably wrong.)

@eamonnmcmanus
Copy link
Member

The current behaviour seems hard to understand, but I am concerned if we change it to be more logical we may break projects that are implicitly depending on the way it is now. I ran this PR against Google's internal tests and encountered four failures in four unrelated projects. All appeared to be related to protobufs, some directly via the ProtoTypeAdapter in this project and others via what appear to be modified clones of that class. My inclination is to leave this alone.

@Marcono1234
Copy link
Collaborator Author

Hmm, ok that is unfortunate. I have split EnumMap support into a separate pull request (#2071). If you want you can close this pull request here then, if you don't think there is any chance this will be integrated.

@Marcono1234
Copy link
Collaborator Author

Marcono1234 commented Sep 3, 2024

@eamonnmcmanus, can / should I revive this pull request? As seen with #2730, the current behavior is broken and leads to quite confusing exceptions, for example in cases where you deserialize a custom List subtype but Gson creates an ArrayList instead.
Only in rare cases this actually works, where the user code is not completely type-safe.

In case this still applies to the Protobuf handling, then if necessary we could hardcode support for that, checking the class name and using reflection.

@eamonnmcmanus
Copy link
Member

OK, if you want to bring this up to date I will retry the experiment of running it against Google's tests and we can possibly come up with an adjustment that makes those tests pass.

@Marcono1234 Marcono1234 changed the title Improve ConstructorConstructor Fix ConstructorConstructor creating mismatching Collection instances Sep 6, 2024
…ields

For example a `List<Long>` Protobuf field might internally have the
Protobuf-internal `LongList` interface type.
Previously Gson's ConstructorConstructor was nonetheless creating an
ArrayList for this, which is wrong but worked. Now with the changes in
ConstructorConstructor Gson will fail to create an instance, so this commit
tries to solve this properly in ProtoTypeAdapter.
@Marcono1234 Marcono1234 changed the title Fix ConstructorConstructor creating mismatching Collection instances Fix ConstructorConstructor creating mismatching Collection and Map instances Sep 6, 2024
Copy link
Collaborator Author

@Marcono1234 Marcono1234 left a comment

Choose a reason for hiding this comment

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

Have updated this pull request now, and also tried to solve the Protobuf issue in a (hopefully) reasonable way, see changes in ProtoTypeAdapter.

However, it will require that users update their local copy of ProtoTypeAdapter.
Other ways of solving this would probably require special hardcoded Protobuf fallback behavior in ConstructorConstructor, which is possible, but probably not a very clean approach.

@@ -1360,7 +1358,19 @@ public <T> T fromJson(JsonReader reader, TypeToken<T> typeOfT)
JsonToken unused = reader.peek();
isEmpty = false;
TypeAdapter<T> typeAdapter = getAdapter(typeOfT);
return typeAdapter.read(reader);
T object = typeAdapter.read(reader);
Class<?> expectedTypeWrapped = Primitives.wrap(typeOfT.getRawType());
Copy link
Collaborator Author

@Marcono1234 Marcono1234 Sep 6, 2024

Choose a reason for hiding this comment

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

It seems all other fromJson methods delegate to this one, so I have added the type check (which was previously Primitives.wrap(classOfT).cast(object) in the other fromJson methods) here instead.
However, this method did not actually have this check before.

Copy link
Member

Choose a reason for hiding this comment

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

I ran this change against all of Google's internal tests and found a lot of failures with this exception, or with an exception about not being able to instantiate the abstract class com.google.common.collect.ImmutableList. In both cases I think the code in question should be fixed, because it is relying on accidental behaviour. A pattern I saw quite often was this:

@AutoValue
public abstract Foo {
  public abstract ImmutableList<Bar> bars();

  public static TypeAdapter<Foo> typeAdapter(Gson gson) {
    return new AutoValue_Foo.GsonTypeAdapter(gson);
  }

  @AutoValue.Builder
  public interface Builder {
    public Builder setBars(List<Bar> bars);
    public Foo build();
  }
}

This is using auto-value-gson to serialize and deserialize AutoValue classes. It works even if no TypeAdapterFactory has been registered for ImmutableList, because Gson deserializes a plain List. Auto-value-gson constructs the Foo instance using its builder, and the deserialized List works with the setBars method. That method calls ImmutableList.copyOf(List) so we do get the right ImmutableList type. But the additional checks in this PR don't let us get that far.

As I say, code like that only works accidentally. My plan is to add a GuavaCollectionsTypeAdapterFactory to extras and update the internal code to use that. I already found several custom ImmutableListTypeAdapterFactory and ImmutableMapTypeAdapterFactory classes which could be switched to use this.

Some of the other failures were just plain using the wrong types, which again happened to work through erasure and the like.

So in short, I think this change is OK, but it may require some client code to be updated. Most of the updates should be straightforward and will lead to client code that is more correct.

I think it will be a while before we can complete this because of all the adjustments that will need to be made.

Comment on lines +1364 to +1365
throw new ClassCastException(
"Type adapter '"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not completely sure if this custom ClassCastException is really worth it, maybe a Class#cast call would suffice, as done before.

This check will only help when directly using fromJson for an incorrect adapter, but if the adapter is used indirectly (for example for a list value) it would still cause other less descriptive exceptions later.

Comment on lines +341 to +342
// Unsupported type; try other means of creating constructor
return null;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In #419 (comment) the concern was mentioned that the fallback will be Unsafe which might leave the Collection or Map implementation in a broken state because fields have not been initialized. And the built-in Collection and Map adapters might then encounter confusing exceptions when trying to add elements.

Not sure if we should throw an exception then in case of Collection or Map? Or we could maybe add an allowUnsafe parameter here (which is considered in addition to GsonBuilder#disableJdkUnsafe), which the built-in Collection and Map adapters can set to false because calling add and put will likely fail afterwards.

Comment on lines +216 to +217
@Test
public void testMapStringKeyDeserialization() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is some overlap between the newly added tests here and in ConstructorConstructorTest, should we keep both nonetheless?

@Marcono1234 Marcono1234 force-pushed the marcono1234/Collection-default-instance-creation branch from 921bbb4 to f8ca948 Compare September 6, 2024 22:42
This reverts a previous commit of this pull request, and matches the
original behavior again.
Copy link
Member

@eamonnmcmanus eamonnmcmanus left a comment

Choose a reason for hiding this comment

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

I ran this against all of Google's internal tests and there were several failures. I plan to investigate in more detail next week.

gson/src/main/java/com/google/gson/Gson.java Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants