[WIP] Add support for mandatory single result in LdapClient search operations#1446
[WIP] Add support for mandatory single result in LdapClient search operations#1446therepanic wants to merge 1 commit intospring-projects:mainfrom
Conversation
Closes: spring-projectsgh-1404 Signed-off-by: Andrey Litvitski <andrey1010102008@gmail.com>
| default <T> MappedSearchSpec<T> map(ContextMapper<T> mapper) { | ||
| return new MappedSearchSpec<T>() { | ||
| @Override | ||
| public List<T> list() { | ||
| return toList(mapper); | ||
| } | ||
|
|
||
| @Override | ||
| public Stream<T> stream() { | ||
| return toStream(mapper); | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
This is currently a poorly implemented implementation that uses methods we've depricated. Should we literally take the default implementation for these methods from DefaultLdapClient?
There was a problem hiding this comment.
I'd prefer we not use deprecated code where possible. If needed, the copy-paste can be abstracted away in a package-private utility class. Are there any other concerns other than the duplication?
| interface MappedSearchSpec<T extends @Nullable Object> { | ||
|
|
||
| /** | ||
| * Retrieve the result as a lazily resolved stream of mapped objects, retaining | ||
| * the order from the original database result. | ||
| * @return the result Stream, containing mapped objects, needing to be closed once |
There was a problem hiding this comment.
The implementation of this class is essentially copied from JdbcClient
| */ | ||
| default <O extends LdapDataEntry> O single() { | ||
| ContextMapper<O> cast = (ctx) -> (O) ctx; | ||
| return map(cast).single(); | ||
| } |
There was a problem hiding this comment.
Perhaps there's a better idea for the default implementation?
Also, should we create tests for the default implementations of these methods? Besides, I don't think we need to reimplement them in DefaultLdapClient?
There was a problem hiding this comment.
Yes, let's please test them. It will help us to know when we inadvertently make a non-passive change down the road.
jzheaux
left a comment
jzheaux
left a comment
There was a problem hiding this comment.
Looking good so far, @therepanic! If you feel good about it, let's go ahead and proceed with testing and with updating the documentation.
| /** | ||
| * Retrieve a single search result as a required {@link LdapDataEntry} instance. | ||
| * @return the single result object (never {@code null}) | ||
| */ |
There was a problem hiding this comment.
Let's add @since 4.1 to new classes and methods.
| */ | ||
| default <O extends LdapDataEntry> O single() { | ||
| ContextMapper<O> cast = (ctx) -> (O) ctx; | ||
| return map(cast).single(); | ||
| } |
There was a problem hiding this comment.
Yes, let's please test them. It will help us to know when we inadvertently make a non-passive change down the road.
| default <T> MappedSearchSpec<T> map(ContextMapper<T> mapper) { | ||
| return new MappedSearchSpec<T>() { | ||
| @Override | ||
| public List<T> list() { | ||
| return toList(mapper); | ||
| } | ||
|
|
||
| @Override | ||
| public Stream<T> stream() { | ||
| return toStream(mapper); | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
I'd prefer we not use deprecated code where possible. If needed, the copy-paste can be abstracted away in a package-private utility class. Are there any other concerns other than the duplication?
2 participants
This commit added the single, optional, list, and stream methods to the
LdapClient#SearchSpecand deprecated the implementations that will be deprecated by this change.Please note that this is a WIP: I have a couple of comments that would be helpful to address.
Closes: gh-1404