Repository navigation
Use the runtime type for a wildcard element type - #3112
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
@googlebot I signed it! |
a field like List<? extends Base> holding a Sub lost the fields that Sub adds , because getRuntimeTypeIfMoreSpecific only looked at the runtime type for a Class or a type variable , and ? extends Base is neither . so List<? extends Base> and List<Base> gave different json for the same value . a wildcard says nothing more than its upper bound , so this reads the bound and then decides the same way as before . when the bound is a parameterized type the declared type is kept , same as gson already does for a plain parameterized type , because the runtime class would drop the type arguments . closes google#1870
1b2b61a to
5843050
Compare
Marcono1234
left a comment
Marcono1234
left a comment
There was a problem hiding this comment.
Thanks for the changes! I have one question regarding wildcards with lower bounds (see comment).
Note that I am not a direct project member; feel free to consider my comment only as suggestion.
| if (declaredType instanceof WildcardType) { | ||
| declaredType = ((WildcardType) declaredType).getUpperBounds()[0]; | ||
| } |
There was a problem hiding this comment.
Have you also considered what effect your changes will have for wildcards with lower bound, e.g. List<? super String>?
The PR description and the comment here only covers upper bounds. With your changes, getUpperBounds()[0] returns for a wildcard with lower bounds just Object.
I think this is fine though because it is effectively the same behavior as the previous code:
- previously: wildcard with lower bound was kept as is, but
TypeToken#getRawType()treats that asObject, so eventually it uses the built-inObjectTypeAdapterwhich uses the runtime type during serialization - with your changes: it directly chooses the runtime type adapter here within
TypeAdapterRuntimeTypeWrapper
But could you please double-check to be safe?
There was a problem hiding this comment.
checked it , and it comes out the way you describe .
a field typed Collection<? super Sub> holding a Sub gives {"collection":[{"s":3,"b":2}]} , and that is the same string before and after this change . the subclass field is there in both , so nothing moves for lower bounds .
the two routes end in the same place , as you said . getUpperBounds()[0] is Object for ? super Sub , so this picks the runtime type . before , the wildcard was kept and getRawType() reads a lower bounded wildcard as Object , so it ended up in ObjectTypeAdapter , which also writes the runtime type . i printed the type this method is handed to be sure the wildcard branch is the one that runs , and it is , kind=WildcardTypeImpl type=? super ...Sub .
eamonnmcmanus
left a comment
eamonnmcmanus
left a comment
There was a problem hiding this comment.
Thanks! This looks fine, and it passed all of Google's internal tests.
3 participants
Fixes #1870
getRuntimeTypeIfMoreSpecificonly looks at the runtime type for aClassor aTypeVariable.? extends Baseis aWildcardType, so it is neither , and the adapter forthe declared bound is used instead :
same value , same field , different json .
a wildcard says nothing more than its upper bound , so this reads the bound and then decides
the same way it already does for a plain type . the bound is only used to make the decision ,
the declared type is still what gets kept when it is parameterized , so
? extends List<String>behaves like
List<String>and does not lose its type arguments . that is why this reads thebound rather than just adding
|| type instanceof WildcardType, which would swap in the rawruntime class and drop them .
while testing i found a worse case of the same cause that is not in the report .
List<? extends CharSequence>holding aStringand aStringBuilderserialized as[{},{}], because gson fell back to a reflective adapter for the interface . the strings weresimply gone .
List<CharSequence>gives["s","t"]. that is fixed by the same change .four tests added , covering a wildcard list , a wildcard map value , and a wildcard reached
through a type variable . the fourth goes the other way and pins that
Collection<? extends ParameterizedBase<String>>still keeps the declared type , so the changeis not too loose . it passes with and without .
4637 tests pass in the gson module , 0 failures , and the full reactor is 4856 across all eight
modules .
spotless:checkis clean .one related thing i deliberately left out .
registerTypeAdapter(Base.class, adapter)is stillignored when the element type is
? extends Base, because the lookup token never matches oneregistered for
Base.class. that needs the wildcard stripped at adapter lookup time , wherethere is already a
// TODO: strip wildcards?inGsonTypes, and it is a wider change toyour internals , so it seemed better to leave that call to you . this change still improves that
case , the subclass fields stop disappearing .