-
Notifications
You must be signed in to change notification settings - Fork 622
Add support for wildcards on the CustomClassMapper. #792
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
Conversation
CustomClassMapper can now handle GenericTypeIndicators with wildcards, instead of strictly requiring the type specified.
@@ -1763,6 +1763,15 @@ public void subclassingGenericTypeIndicatorIsAllowed() { | |||
assertEquals("foo", bean.value); | |||
} | |||
|
|||
@Test | |||
public void usingWildcardInGenericTypeIndicatorIsAllowed() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add tests for:
- wildcard with lowerbound throws
- wildcard with multiple upperBounds uses the first one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL.
// that this array always has at least one element, since the unbounded wildcard <?> always | ||
// has at least an upper bound of Object. | ||
Type[] upperBounds = ((WildcardType) type).getUpperBounds(); | ||
hardAssert(upperBounds.length > 0, "Unexpected type bounds on wildcard " + type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assert message doesn't seem to match what is being asserted. While this problem exists in the original implementation, we could still want to take a stab at improving it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL.
An additional fix from Firestore for handling TypeVariables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Please do add a changelog.
@schmidt-sebastian Added entry to the changelog. PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome. Thanks!
/test new-smoke-tests |
CustomClassMapper can now handle GenericTypeIndicators with wildcards, instead of strictly requiring the type specified.