DATACMNS-1035 - Moved CustomConversions to Spring Data Commons. #210
Conversation
We now expose the Pageable that was used to create a Slice via its API. Slice itself constructs a default PageRequest so that current implementations of it still work as before.
…on for self link. We now hand the Pageable contained in the Page to the code that constructs the self link. Added toEmptyResource(Page, Class) to force clients to resolve an optional base link before calling the method. Removed deprecated method to append pagination template parameters.
Extracted CustomConversions — whose code has largely been duplicated between the MongoDB, Couchbase and Cassandra modules — into Spring Data Commons. Store-specific extensions can now be contributed via a StoreConversions value type that carries both, store-specific default converters as well as a store-specific SimpleTypeHolder to augment the default list of simple types. Removed SimpleTypeHolders public default constructor in favour of a protected one and a static DEFAULT instance for plain references.
|
|
||
| // Add user provided converters to make sure they can override the defaults | ||
| toRegister.addAll(converters); | ||
| toRegister.addAll(JodaTimeConverters.getConvertersToRegister()); |
There was a problem hiding this comment.
How about holding the default converters in a static held List? They are added in the same order each time the constructor is called.
There was a problem hiding this comment.
I didn't bother to again extract the converters here into a static List as there should be only one production instance of CustomConversions in place anyway. We'd basically keep yet another List around and introduce a layer of indirection. That said, we can definitely extract that list, no problem.
| DEFAULTS.add(Class.class); | ||
| DEFAULTS.add(Enum.class); | ||
| } | ||
| private static final Set<Class<?>> DEFAULTS = new HashSet<Class<?>>() { |
There was a problem hiding this comment.
Creating a new HashSet with an inner static initializer creates a subclass. What's the advantage compared to HashSet creation and initialization in a static block of SimpleTypeHolder?
There was a problem hiding this comment.
It's the only way you can have a static DEFAULT instance see below and get the DEFAULTS HashSet populated already, which is used in the constructor. If DEFAULTS is set up in a static initializer block, it's still empty when the constructor is called to create the DEFAULT instance.
| this.customWriteTargetTypes = new ConcurrentHashMap<>(); | ||
| this.rawWriteTargetTypes = new ConcurrentHashMap<>(); | ||
|
|
||
| List<Object> toRegister = new ArrayList<Object>(); |
There was a problem hiding this comment.
The List could be replaced with Stream.concat(…)
There was a problem hiding this comment.
At least can say new ArrayList<>()
There was a problem hiding this comment.
Is there really a benefit of calling ….stream() on each of the Lists? The constructor invocation doesn't need an explicit generic parameter, right.
| } | ||
|
|
||
| if (!added) { | ||
| throw new IllegalArgumentException( |
There was a problem hiding this comment.
It would make sense to mention the offending object in the exception message
There was a problem hiding this comment.
I'd favor logging each registration. Then exception can say "couldn't find match". Also avoid tracking state of who was registered.
There was a problem hiding this comment.
Exposing the offending instance is a good idea. Is there added value in logging the converters registered? If so, shouldn't this be functionality GenericConversionService?
| */ | ||
| public static StoreConversions of(SimpleTypeHolder storeTypeHolder, Object... converters) { | ||
|
|
||
| Assert.notNull(storeTypeHolder, "SimpleTypeHolder must not be null!"); |
There was a problem hiding this comment.
These two overloads make it quite easy to use by accident the wrong method. Should check here that converters isn't a single Collection element?
There was a problem hiding this comment.
How so? You'd explicitly need to call StoreConversions.of(new Object[] { collection }) to run into this problem, right?
| toRegister.addAll(JodaTimeConverters.getConvertersToRegister()); | ||
| toRegister.addAll(Jsr310Converters.getConvertersToRegister()); | ||
| toRegister.addAll(ThreeTenBackPortConverters.getConvertersToRegister()); | ||
| toRegister.addAll(storeConversions.getStoreConverters()); |
There was a problem hiding this comment.
Store converters should be probably registered after converters and before JodaTimeConverters to retain registration order. MongoDB and Cassandra modules register their converters after converters and before JodaTimeConverters.
|
From the mixture of feedback I conclude the following steps.
|
Default converters are now held in a static list so that they're only obtained once. A failed registration of a converter now exposes the offending converter instance. StoreConversions.of(…) now uses a Collection<?> to avoid accidental use of wrong overload. Original pull request: #210.
Extracted CustomConversions — whose code has largely been duplicated between the MongoDB, Couchbase and Cassandra modules — into Spring Data Commons. Store-specific extensions can now be contributed via a StoreConversions value type that carries both, store-specific default converters as well as a store-specific SimpleTypeHolder to augment the default list of simple types. Removed SimpleTypeHolders public default constructor in favour of a protected one and a static DEFAULT instance for plain references. Original pull request: #210.
Extracted CustomConversions — whose code has largely been duplicated between the MongoDB, Couchbase and Cassandra modules — into Spring Data Commons. Store-specific extensions can now be contributed via a StoreConversions value type that carries both, store-specific default converters as well as a store-specific SimpleTypeHolder to augment the default list of simple types.
Removed SimpleTypeHolders public default constructor in favour of a protected one and a static DEFAULT instance for plain references.