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
CodecRegistry usability improvements #62
Conversation
Making copying them easier - refs JAVA-1655
Created a CodecRegistryHelper simplifyin the creation and combining of registries. New CodecRegistries accessible from the helper: * SimpleCodecRegistry is a simple wrapper providing a registry for a specific codec * ProviderCodecRegistry takes a list of codec providers to use for finding the correct codec * PreferredCodecRegistry takes two codec registries and uses the first for codec lookups then falls back to the second registry Drivers internally use the internal RootCodecRegistry which wraps the user provided codec registry and throws a CodecConfigurationException if a codec can't be found JAVA-1655
/cc @evanchooly / @craiggwilson |
Comments from the review on Reviewable.io Reviewed files:
bson/src/main/org/bson/codecs/configuration/CodecRegistryHelper.java, line 27 [r1] (raw file): bson/src/main/org/bson/codecs/configuration/CodecRegistryHelper.java, line 45 [r1] (raw file): bson/src/main/org/bson/codecs/configuration/PreferredCodecRegistry.java, line 23 [r1] (raw file): bson/src/main/org/bson/codecs/configuration/ProviderCodecRegistry.java, line 26 [r1] (raw file): bson/src/main/org/bson/codecs/configuration/ProviderCodecRegistry.java, line 47 [r1] (raw file): driver-core/src/main/com/mongodb/internal/codecs/RootCodecRegistry.java, line 58 [r1] (raw file): I don't think we can have it both ways. Seems like this is only an issue for the PreferredCodecRegistry. What if we just take the hit there and catch CodecConfigurationException when asking the preferred provider for a Codec? Yes, it's using exceptions for control flow, but it's better than this IMO. |
Comments from the review on Reviewable.io bson/src/main/org/bson/codecs/configuration/PreferredCodecRegistry.java, line 23 [r1] (raw file): bson/src/main/org/bson/codecs/configuration/SimpleCodecRegistry.java, line 24 [r1] (raw file): driver-async/src/main/com/mongodb/async/client/ListIndexesIterableImpl.java, line 46 [r1] (raw file): driver-core/src/main/com/mongodb/internal/codecs/RootCodecRegistry.java, line 58 [r1] (raw file): |
Comments from the review on Reviewable.io In general, I'm wondering why we are now passing RootCodecRegistry around instead of just CodeRegistry. It doesn't seem like depending on RootCodecRegistry is a good thing. |
Comments from the review on Reviewable.io I've renamed the protected classes to the suggestions and followed up why I think Theres still an open question about bson/src/main/org/bson/codecs/configuration/CodecRegistryHelper.java, line 27 [r1] (raw file): bson/src/main/org/bson/codecs/configuration/CodecRegistryHelper.java, line 45 [r1] (raw file): bson/src/main/org/bson/codecs/configuration/PreferredCodecRegistry.java, line 23 [r1] (raw file): bson/src/main/org/bson/codecs/configuration/ProviderCodecRegistry.java, line 26 [r1] (raw file): bson/src/main/org/bson/codecs/configuration/ProviderCodecRegistry.java, line 47 [r1] (raw file): If this scenario isn't feasible - I'm happy to remove the catch, I'm not sure on what the intent was and the last thing I want to do is provide a codec that isn't correctly configured especially when a correct one for the bson/src/main/org/bson/codecs/configuration/SimpleCodecRegistry.java, line 24 [r1] (raw file): It followed on from the driver-async/src/main/com/mongodb/async/client/ListIndexesIterableImpl.java, line 46 [r1] (raw file): driver-core/src/main/com/mongodb/internal/codecs/RootCodecRegistry.java, line 58 [r1] (raw file): So essentially this class is here to DRY all those null checks up and is not intended for public consumption hence being in internal. I'm happy to not have it both ways but it'll come at the cost of adding lots of checking duplication throughout the code - with the easy risk of missing one. Also, this isn't here for just |
Comments from the review on Reviewable.io Reviewed files:
bson/src/main/org/bson/codecs/configuration/ProviderCodecRegistry.java, line 47 [r1] (raw file): driver-core/src/main/com/mongodb/internal/codecs/RootCodecRegistry.java, line 58 [r1] (raw file): |
Comments from the review on Reviewable.io bson/src/main/org/bson/codecs/configuration/CodecRegistry.java, line 36 [r2] (raw file): driver-async/src/main/com/mongodb/async/client/ListIndexesIterableImpl.java, line 46 [r1] (raw file): driver-core/src/main/com/mongodb/internal/codecs/RootCodecRegistry.java, line 58 [r1] (raw file): |
Comments from the review on Reviewable.io driver-async/src/main/com/mongodb/async/client/ListIndexesIterableImpl.java, line 46 [r1] (raw file): |
Comments from the review on Reviewable.io Thanks, I've finally made it to the same page! I'm now happy to remove RootCodecRegistry - I didn't get Jeff's point about it would only really need to be checked on the CompoundCodecRegistry but that makes sense given the original nature of the registries. And that in turn will make Craig happy again as its CodecRegistries all round! We still blindly trust any given CodecRegistry will throw on get() and not return null, thats the rabbit hole I went down to solve - are we happy that isn't a problem? All methods that accept a codec should null check - so it would be caught just not necessarily obvious as to why it happened. Thanks again, I'll update tomorrow. bson/src/main/org/bson/codecs/configuration/CodecRegistry.java, line 36 [r2] (raw file): driver-async/src/main/com/mongodb/async/client/ListIndexesIterableImpl.java, line 46 [r1] (raw file): driver-core/src/main/com/mongodb/internal/codecs/RootCodecRegistry.java, line 58 [r1] (raw file): |
Comments from the review on Reviewable.io bson/src/main/org/bson/codecs/configuration/CodecRegistry.java, line 36 [r2] (raw file): |
Comments from the review on Reviewable.io Much happier now its CodecRegistries all round |
* | ||
* @return a codec registry that has a preferred registry when looking for codecs. | ||
*/ | ||
public static CodecRegistry compoundRegistry(final CodecRegistry firstRegistry, final CodecRegistry secondRegistry) { |
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.
Change to fromRegistries to be consistent with the other method names?
I like this much better. Just one comment on a method name, and then an implementation issue with caching of Codec instances in the registry implementations. |
Updated :) |
I see you made the listCollectionNames/databaseNames in async. Let's do it for sync as well. Otherwise, LGTM. |
Finally got there! Rebased and merged |
Two commits to help make codec registries easier for users and for wrapping libraries
CodecRegistryHelper