Fix Gson.getDelegateAdapter not working properly for JsonAdapter
#2435
Conversation
| * types, our stats factory will not count the number of String or primitives that will be | ||
| * read or written. | ||
| * | ||
| * <p>If {@code skipPast} is {@code null} or a factory which has neither been registered |
There was a problem hiding this comment.
The section starting here is new; the changes above only remove one redundant leading space after the * (unfortunately the GitHub diff does not indicate that very well).
| T instance = instanceCreator.createInstance(type); | ||
| if (instance == null) { | ||
| throw new RuntimeException("InstanceCreator " + instanceCreator + " returned null for type " + type); | ||
| } | ||
|
|
||
| if (!rawType.isInstance(instance)) { | ||
| throw new ClassCastException("InstanceCreator " + instanceCreator + " created instance of wrong type;" | ||
| + " expected " + rawType.getName() + " but got instance of unrelated type " + instance.getClass().getName()); | ||
| } | ||
| return instance; |
There was a problem hiding this comment.
Added this for sanity, and to avoid some checks in JsonAdapterAnnotationTypeAdapterFactory.isClassJsonAdapterFactory to handle bad InstanceCreators. Most likely Gson was already failing for bad InstanceCreators before, but probably at a completely unrelated place which would have made debugging more difficult.
gson/src/main/java/com/google/gson/internal/bind/JsonAdapterAnnotationTypeAdapterFactory.java
Outdated
Show resolved
Hide resolved
gson/src/main/java/com/google/gson/internal/bind/JsonAdapterAnnotationTypeAdapterFactory.java
Show resolved
Hide resolved
|
Thanks! The fact that I am a bit afraid that users unwittingly rely on the current broken behavior, or that users are currently intentionally calling |
|
Still waiting for test reruns, but looking good so far. (There are so many tests that invariably a few of them fail for unrelated reasons.) |
|
Did a code search on GitHub now and unfortunately found a few cases of
These libraries don't seem to be that popular so maybe the risk of causing issues for them is not that big (we can also warn the authors about their problematic Also, it appears commit 83e5a49 which introduced the |
|
I'm hesitating a bit here because the internal tests did show up one test failure (or actually a group of failures in related tests, probably having the same cause. I'm getting a failure like this: Maybe there is some sort of interaction between this private static class ImmutableListTypeAdapterFactory implements TypeAdapterFactory {
@Nullable
@Override
public <T> TypeAdapter<T> create(Gson gson, TypeToken<T> type) {
if (!ImmutableList.class.isAssignableFrom(type.getRawType())) {
return null;
}
final TypeAdapter<T> delegate = gson.getDelegateAdapter(this, type);
return new TypeAdapter<T>() {
@Override
public void write(JsonWriter out, T value) throws IOException {
delegate.write(out, value);
}
@Override
@SuppressWarnings("unchecked")
public T read(JsonReader in) throws IOException {
return (T) ImmutableList.copyOf((List) delegate.read(in));
}
};
}
public static <E> InstanceCreator<List<E>> newCreator() {
return type -> new ArrayList<>();
}
}Registered like this: private static final GsonBuilder GSON_BUILDER = new GsonBuilder();
static {
GSON_BUILDER.registerTypeAdapterFactory(new ImmutableListTypeAdapterFactory());
GSON_BUILDER.registerTypeAdapter(
ImmutableList.class, ImmutableListTypeAdapterFactory.newCreator());
}
public static Gson get() {
return GSON_BUILDER.create();
}I'm not sure if this code is doing something unusual. I do notice that the code in That's as much as I had time for here but I may be able to look into it further later. Let me know if this much is enough to get an idea of the problem, and in particular whether it is something other users might run into. |
|
Thanks a lot for the feedback! If I understand the code of
However, the way this is implemented relies heavily on Java's type erasure and Gson implementation details (possibly even to a degree where you could call it "misuse"?):
Most likely these implementation details of Maybe a proper implementation would be to have Technically the changes to |
That sounds like a great idea. If those changes are not strictly necessary, I think everything else is pretty uncontroversial. Even taking into account your code-search findings. We could then have a separate PR for the |
|
Have reverted the Though while thinking a bit more about these changes, what do you think about rejecting |
|
@eamonnmcmanus, would it be ok if I changed |
I can certainly try that. Just adding a second |
|
Ah sorry, I should have been more explicit: Rejecting If the test results indicate that rejecting |
|
I have confirmed that rejecting |
Previously `getDelegateAdapter` called `factories.contains(skipPast)`, but unlike the other comparisons which check for reference equality, that would have used the `equals` method. This could lead to spurious "GSON cannot serialize ..." exceptions if two factory instances compared equal, but the one provided as `skipPast` had not been registered yet.
|
Thanks a lot for checking! I have made a few small additional adjustments; sorry for the trouble. It should be functionally nearly identical to the previous implementation (except for corner cases with factories with custom I think these changes should be ready for merging now. |
…oogle#2435) * Fix `Gson.getDelegateAdapter` not working properly for `JsonAdapter` * Address review feedback and add comments regarding thread-safety * Revert InstanceCreator instance validation * Disallow `null` as `skipPast` * Avoid `equals` usage in `getDelegateAdapter` & minor other changes Previously `getDelegateAdapter` called `factories.contains(skipPast)`, but unlike the other comparisons which check for reference equality, that would have used the `equals` method. This could lead to spurious "GSON cannot serialize ..." exceptions if two factory instances compared equal, but the one provided as `skipPast` had not been registered yet.
Purpose
Fixes #1028
Supersedes #1690
Description
Unlike #1690 this implementation here tracks which
TypeAdapterFactoryinstances have been created for@JsonAdapterplaced on classes, and only for them letsGson.getDelegateAdapterskip past theJsonAdapterAnnotationTypeAdapterFactory.This pull request also in general changes the behavior of
Gson.getDelegateAdapterto hopefully be more reasonable whenskipPastisnull(which was previously supported, but undocumented) or whenskipPastis not a known factory. In these cases the method now behaves likeGson.getAdapter(instead of assuming that the factory came from@JsonAdapteron a class).Checklist
null@since $next-version$(
$next-version$is a special placeholder which is automatically replaced during release)TestCase)mvn clean verify javadoc:jarpasses without errors