Skip to content

#189 - Accept StatementFilterFunction in DatabaseClient #308

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

#189 - Accept StatementFilterFunction in DatabaseClient #308

wants to merge 5 commits into from

Conversation

mp911de
Copy link
Member

@mp911de mp911de commented Feb 18, 2020

We now accept StatementFilterFunction and ExecuteFunction via DatabaseClient to filter Statement execution. StatementFilterFunctions can be used to pre-process the statement or post-process Result objects.

databaseClient.execute(…)
		.filter((s, next) -> next.execute(s.returnGeneratedValues("my_id")))
		.filter((s, next) -> next.execute(s.fetchSize(25)))

databaseClient.execute(…)
		.filter(s -> s.returnGeneratedValues("my_id"))
		.filter(s -> s.fetchSize(25))

Related ticket: #189.

@mp911de mp911de linked an issue Feb 18, 2020 that may be closed by this pull request
execute(...) should be extended with returning generated keys #189
Closed
@mp911de mp911de requested a review from schauder February 18, 2020 13:57
@mp911de mp911de added this to the 1.1 M4 (Neumann) milestone Feb 18, 2020
@mp911de mp911de added the type: enhancement A general enhancement label Feb 18, 2020
schauder
schauder reviewed Feb 25, 2020
View reviewed changes
Copy link
Contributor

@schauder schauder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two minor issues and once more I'm not happy with a name without a real good alternative.

I also rebased and prepared two polishing commits.

src/main/asciidoc/reference/r2dbc-sql.adoc
@@ -134,7 +134,7 @@ In JDBC, the actual drivers translate `?` bind markers to database-native marker

Spring Data R2DBC lets you use native bind markers or named bind markers with the `:name` syntax.

Named parameter support leverages a `R2dbcDialect` instance to expand named parameters to native bind markers at the time of query execution, which gives you a certain degree of query portability across various database vendors.
Named parameter support leverages a `R2dbcDialect` instance to expand named parameters to native bind markers at the time of query execution, which gives you a certain degree of query portability across various database vendors.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I like the name filter. I would expect it to accept a Predicate.

I don't have a name I really like but maybe somthing like registerStatementCallback, or modifyStatement?
Or to go with the more functional nomenclature it would be a map, maybe a mapStatement?

src/main/java/org/springframework/data/r2dbc/core/DefaultDatabaseClient.java
Map<String, SettableValue> byName, Supplier<String> sqlSupplier) {
return createTypedExecuteSpec(byIndex, byName, sqlSupplier, this.typeToRead);
Map<String, SettableValue> byName, Supplier<String> sqlSupplier, StatementFilterFunction filterFunction) {
return createTypedExecuteSpec(byIndex, byName, sqlSupplier, filterFunction, this.typeToRead);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typeToRead is nullable but gets passed as an argument to a parameter which is not nullable and eventually actually gets dereferenced.
I'm not sure if there is something really wrong or if the nullability annotations need tweaking.

src/main/java/org/springframework/data/r2dbc/core/DefaultDatabaseClient.java
this.byIndex = byIndex;
this.byName = byName;
this.sqlSupplier = sqlSupplier;
this.filterFunction = filterFunction;
}

<T> FetchSpec<T> exchange(Supplier<String> sqlSupplier, BiFunction<Row, RowMetadata, T> mappingFunction) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually about the line Function<Connection, Statement> executeFunction = it -> { a few lines down.

It now clashes awkwardly with ExecuteFunction and the member of that type also named executeFunction. I think something should get a name change here.

mp911de and others added 5 commits March 16, 2020 14:05
@mp911de
#189 - Prepare issue branch.
581329d
@mp911de
#189 - Accept StatementFilterFunction in DatabaseClient.
1675af7 
We now accept StatementFilterFunction and ExecuteFunction via DatabaseClient to filter Statement execution. StatementFilterFunctions can be used to pre-process the statement or post-process Result objects.

databaseClient.execute(…)
		.filter((s, next) -> next.execute(s.returnGeneratedValues("my_id")))
		.filter((s, next) -> next.execute(s.fetchSize(25)))

databaseClient.execute(…)
		.filter(s -> s.returnGeneratedValues("my_id"))
		.filter(s -> s.fetchSize(25))
@schauder @mp911de
#189 - Polishing.
c3fb8b3 
Made assertions in tests more strict.
@schauder @mp911de
#189 - Polishing.
46e3fb6 
Refactored DefaultDatabaseClientUnitTests in order to make the relevant differences in setup easier to spot.
@mp911de
#189 - Incorporate review feedback.
5c078db 
Fix nullability annotations. Relax generics at DatabaseClient.StatementFilterSpec.filter(…).
schauder pushed a commit that referenced this pull request Mar 17, 2020
@mp911de @schauder
#189 - Accept StatementFilterFunction in DatabaseClient.
366c10b 
We now accept StatementFilterFunction and ExecuteFunction via DatabaseClient to filter Statement execution. StatementFilterFunctions can be used to pre-process the statement or post-process Result objects.

databaseClient.execute(…)
		.filter((s, next) -> next.execute(s.returnGeneratedValues("my_id")))
		.filter((s, next) -> next.execute(s.fetchSize(25)))

databaseClient.execute(…)
		.filter(s -> s.returnGeneratedValues("my_id"))
		.filter(s -> s.fetchSize(25))

Original pull request: #308.
schauder added a commit that referenced this pull request Mar 17, 2020
@schauder
#189 - Polishing.
6a8ab60 
Made assertions in tests more strict.

Original pull request: #308.
schauder added a commit that referenced this pull request Mar 17, 2020
@schauder
#189 - Polishing.
4a51848 
Refactored DefaultDatabaseClientUnitTests in order to make the relevant differences in setup easier to spot.

Original pull request: #308.
schauder pushed a commit that referenced this pull request Mar 17, 2020
@mp911de @schauder
#189 - Incorporate review feedback.
10ec677 
Fix nullability annotations. Relax generics at DatabaseClient.StatementFilterSpec.filter(…).

Original pull request: #308.
schauder added a commit that referenced this pull request Mar 17, 2020
@schauder
#289 - Polishing.
edc6d9b 
Minor formatting.

Original pull request: #308.
@schauder
Copy link
Contributor

That's merged. Thanks.

@schauder schauder closed this Mar 17, 2020
@schauder schauder deleted the issue/gh-189 branch March 17, 2020 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schauder schauder schauder left review comments

Assignees
No one assigned
Labels
type: enhancement A general enhancement
Projects
None yet
Milestone
1.1 M4 (Neumann)
Development

Successfully merging this pull request may close these issues.

execute(...) should be extended with returning generated keys
2 participants
@mp911de @schauder