-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Fix type parameters not being passed to fallback property codec #1765
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
@rozza , I am not sure about the validity of the code changes proposed. For one thing, it breaks 6 unit testing cases; secondly it seems FallbackPropertyCodecProvider opted for calling CodecRegistry#get(Type) intentionally, for the reason that the other get(Type, List<Type>() might not be overridden, thus defeating the purpose of fallback as the last resort.
How do you think?
@GliczDev looks like this PR needs some tests and as @NathanQingyangXu mentioned this does appear to break some existing test cases.
To run the bson tests and checks run ./gradlew bson:check from the shell.
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.
I added a couple of requested changes - to support java 8+ and to ensure that types are only provided if the list is not empty.
The LookupCountingCodecRegistry implementation in tests will need to implement <T> Codec<T> get(Class<T> clazz, List<Type> typeArguments) inorder not to throw an exception.
eg:
@Override
public <T> Codec<T> get(final Class<T> clazz, final List<Type> typeArguments) {
incrementCount(clazz);
for (CodecProvider provider : codecProviders) {
Codec<T> codec = provider.get(clazz, typeArguments, this);
if (codec != null) {
return codec;
}
}
return null;
}
It would also be good to add a regression test where this code path is used.
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.
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.
For some reason, type parameters weren't passed to fallback property codec, which caused an exception when trying to access them. Now, this issue is fixed.