Add spring-integration DSL support for JDBC. #10081
Conversation
issues: spring-projects#8467 Signed-off-by: Jiandong Ma <jiandong.ma.cn@gmail.com>
artembilan
left a comment
artembilan
left a comment
There was a problem hiding this comment.
In general, it is very cool and smooth.
Just left a couple remarks, but I can address them on merge.
However I'd like to have a discussion since this is like the first time that someone else contribute this Java DSL API to Spring Integration, so other opinion really matters.
Thank you!
spring-integration-jdbc/src/test/java/org/springframework/integration/jdbc/dsl/JdbcTest.java
Show resolved
Hide resolved
|
|
||
| List<Integer> primeNumbers = (List<Integer>) payload; | ||
|
|
||
| assertThat(primeNumbers.size() == 4).isTrue(); |
There was a problem hiding this comment.
There is a hasSize(4) API in AssertJ.
| assertThat(message).isNotNull(); | ||
| Object payload = message.getPayload(); | ||
| assertThat(payload).isInstanceOf(Map.class); | ||
| assertThat(((Map<?, ?>) payload).get("UPDATED")).isEqualTo(1); |
There was a problem hiding this comment.
There is containsEntry() API in AssertJ.
| * @return the storedProcExecutor | ||
| * @see StoredProcExecutor#setProcedureParameters(List) | ||
| */ | ||
| public StoredProcExecutorConfigurer procedureParameters(List<ProcedureParameter> procedureParameters) { |
There was a problem hiding this comment.
Can we have this as vararg to not make end-user to use that List.of()?
Or... We can have just procedureParameter(ProcedureParameter procedureParameter) API and explain in its JavaDocs that it is safe to call it several times for more params.
| * @return the storedProcExecutor | ||
| * @see StoredProcExecutor#setReturningResultSetRowMappers(Map) | ||
| */ | ||
| public StoredProcExecutorConfigurer returningResultSetRowMappers(Map<String, RowMapper<?>> returningResultSetRowMappers) { |
There was a problem hiding this comment.
Same idea here:
returningResultSetRowMapper(String name, RowMapper<?> rowMapper)
Then we build map internally.
(Same for the mentioned procedureParameters list to be built internally).
My point is that Java DSL should be as smooth as possible without extra hops.
| * @param configurer the configurer. | ||
| * @return the spec | ||
| */ | ||
| public JdbcStoredProcOutboundGatewaySpec configurerStoredProcExecutor(Consumer<StoredProcExecutorConfigurer> configurer) { |
There was a problem hiding this comment.
Interesting idea, but this is not the style we follow in the project already.
I mean it feels like for the existing and familiar DSL API , it would be natural to have this as a dedicated StoredProcExecutorSpec and provide its factory method in that Jdbc main entry point.
But we can think about this callback as well.
I mean leave this configurerStoredProcExecutor method but have it like this instead:
configurerStoredProcExecutor(Consumer<StoredProcExecutorSpec> configurer)
So, then end-user would have a choice for their preferences: Use Jdbc.storedProcExecutor() factory or this configurer.
|
sure, I ll leave it you. Thank you |
| Message<?> message = this.inboundFlowPollerChannel.receive(10_000); | ||
| List<?> rows = (List<?>) message.getPayload(); | ||
| assertThat(rows.size()).isEqualTo(2); | ||
| assertThat(rows.get(0) instanceof Inbound).isTrue(); |
There was a problem hiding this comment.
This line can be updated to assertThat(rows.get(0)).isInstanceOf(Inbound.class);. There are other tests that you can use this same pattern
artembilan
left a comment
artembilan
left a comment
There was a problem hiding this comment.
@mjd507 ,
please, let us know if you are going to address reviews, or you agreed with them and we can solve them ourselves on subsequent commits after merging your.
There is another part we have missed from our previous review cycle.
Docs. The whats-new.adoc deserves an entry for this new feature and target jdbc directory for docs has to have a new dsl.adoc file with explanation for this new API you have just introduced.
The package-info.java file also has to be places in this new jdbc.dsl package.
Thanks
|
@artembilan Thanks for the feedback, kindly help me to merge or solve that, I will learn from your commits about Docs part as well! Thank you! |
artembilan
left a comment
artembilan
left a comment
There was a problem hiding this comment.
OK. I'll take it from here then.
Thanks
Follow up for: #10081 * Rename `StoredProcExecutorConfigurer` to `StoredProcExecutorSpec` * Add `Jdbc.storedProcExecutorSpec()` factory * Improve logic of `StoredProcExecutor`-based Java DSL classes to handle an external `StoredProcExecutor` as well * Add `package-info.java` into the `jdbc/dsl` package * Add `/jdbc/dsl.adoc` and respective `What's New` entry
|
Looks very good to me. Thank you! |
3 participants
issues: #8467