Add support for named parameters #47

mp911de · 2019-01-04T13:48:26Z

DatabaseClient now supports named parameters prefixed with a colon such as :name in addition to database-native bind markers. Named parameters thus are supported in annotated repository query methods which also increases portability of queries across database vendors.

Named parameter support unrolls collection arguments to reduce the need for argument-specific SQL statements:

db.execute()
    .sql("SELECT id, name, state FROM table WHERE age IN (:ages)")
    .bind("ages", Arrays.asList(35, 50));

Results in a query: SELECT id, name, state FROM table WHERE age IN (35, 50)

Collection arguments containing nested object arrays can be used to use select lists:

List<Object[]> tuples = new ArrayList<>();
tuples.add(new Object[] {"John", 35});
tuples.add(new Object[] {"Ann",  50});

db.execute()
    .sql("SELECT id, name, state FROM table WHERE (name, age) IN (:tuples)")
    .bind("tuples", tuples);

translates to: SELECT id, name, state FROM table WHERE (name, age) IN (('John', 35), ('Ann', 50))

Related ticket: #23.

schauder

I added one more polishing commit.

Please consider my comments.

schauder · 2019-01-09T14:30:14Z

src/main/asciidoc/reference/r2dbc-repositories.adoc

@@ -104,7 +104,7 @@ Defining such a query is a matter of declaring a method on the repository interf
 ----
 public interface PersonRepository extends ReactiveCrudRepository<Person, Long> {

-    @Query("SELECT * FROM person WHERE lastname = $1")
+    @Query("SELECT * FROM person WHERE lastname = :lastname")
    Flux<Person> findByLastname(String lastname);                      <1>

    @Query("SELECT firstname, lastname FROM person WHERE lastname = $1")


Is the $1 syntax left in there an oversight or an attempt to demonstrate database dependent options?

If the latter I think we need an explanatory note to that extent.

It's intentionally there, to occasionally use native bind markers. See the callout description:

The annotated query uses native bind markers, which are Postgres bind markers in this example.

schauder · 2019-01-09T14:31:28Z

src/main/asciidoc/reference/r2dbc-repositories.adoc

 ====

 NOTE: R2DBC repositories do not support query derivation.

-NOTE: R2DBC repositories require native parameter bind markers that are bound by index.
+NOTE: R2DBC repositories bind parameters to placeholders by index.


"by index" -> "by name"

That's perhaps misleading in this context. It refers to R2DBC SPI Statement.bind(index).

schauder · 2019-01-09T14:39:15Z

src/main/asciidoc/reference/r2dbc.adoc

+.R2DBC Native Bind Markers
+****
+R2DBC uses database-native bind markers that depend on the actual database.
+If you are familiar with JDBC, then you're also familiar with `?` (question mark) bind markers.


While technically correct I think this is really easy to misunderstand it to mean that we support ? markers as well.
I'd structure it like this:

R2DBC supports native bind markers.
An example are postgres bind markers ... Another example are SQL Server markers ...

This is different from JDBC which requires ? as bind markers.

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

schauder · 2019-01-10T07:20:11Z

src/main/java/org/springframework/data/r2dbc/function/DatabaseClient.java

+		 * @see NamedParameterExpander#enabled()
+		 * @see NamedParameterExpander#disabled()
+		 */
+		Builder namedParameters(NamedParameterExpander namedParameters);


I don't like the shortening of NamedParameterExpander to namedParameters. The last name part is what it actually is so removing that seems to drastically change the intuitive meaning from something expanding SQL statements to something containing parameters.

If we really want to shorten I'd vote for dropping the named part.

Also applies to several other places in the code.

I just saw what you did with enabled / disabled.
For this to work nicely I'd be willing to keep the method name here, or change it to namedParameterExpansion.
I'd still like to change the parameter name and field names from namedParameters to namedParametersExpander.

schauder · 2019-01-10T07:31:23Z

src/main/java/org/springframework/data/r2dbc/function/DefaultDatabaseClient.java

+
+				Statement<?> statement = it.createStatement(operation.toQuery());
+
+				byName.forEach((name, o) -> {


doing the binding by name inline but calling a method for binding by index looks unpleasant unsymmetrically.

schauder · 2019-01-10T07:32:31Z

src/main/java/org/springframework/data/r2dbc/function/DefaultDatabaseClient.java

+					}
+				});
+
+				doBind(statement, Collections.emptyMap(), byIndex);


At the very least we should lose the second parameter of doBind since the only usage is passing an empty map and introduce a second doBind for the named parameters.

schauder · 2019-01-10T12:07:18Z

...springframework/data/r2dbc/function/AbstractTransactionalDatabaseClientIntegrationTests.java

@@ -91,7 +92,9 @@ public void before() {
 	/**
 	 * Get a parameterized {@code INSERT INTO legoset} statement setting id, name, and manual values.
 	 */
-	protected abstract String getInsertIntoLegosetStatement();
+	protected String getInsertIntoLegosetStatement() {


I find it super weird that binding by index can be used for named bind parameters (executeInsertInManagedTransaction).
I wonder if this causes problems when native and named parameters are used.

As of now, drivers support both variants. Indexed by the order of appearance and named by using the binding marker itself.

schauder · 2019-01-10T12:36:09Z

src/test/java/org/springframework/data/r2dbc/function/NamedParameterUtilsUnitTests.java

+	public void parseSqlStatementWithBracketDelimitedParameterNames() {
+
+		String expectedSql = "select foo from bar where baz = b$1$2z";
+		String sql = "select foo from bar where baz = b:{p1}:{p2}z";


What is the purpose of this feature?
And we should document it. I guess it is comming from NamedParameterJdbcTemplate but I couldn't find any documentation either.

It's originating in Spring's named parameter support to allow parameter declaration within a string.

schauder · 2019-01-10T12:45:00Z

src/test/java/org/springframework/data/r2dbc/function/NamedParameterUtilsUnitTests.java

+		assertThat(psql.getTotalParameterCount()).isEqualTo(4);
+		assertThat(psql.getNamedParameterCount()).isEqualTo(3);
+
+		String sql2 = "xxx &a yyyy ? zzzzz";


Does this mean we also support ? as parameter marker? May I veto that?

That's a left-over. We should get rid of it as this is likely to conflict with MySQL.

schauder · 2019-01-10T14:07:05Z

src/main/java/org/springframework/data/r2dbc/function/NamedParameterUtils.java

+ * @author Juergen Hoeller
+ * @author Mark Paluch
+ */
+abstract class NamedParameterUtils {


I guess this works, is well tested in its incarnation from Spring JDBC and probably fast and efficient.
It makes me nervous though that we one day might want to support SpELs or similar.

We included all the test cases and fixed bugs 🙃 (and not discovered bugs as well 😬)

DatabaseClient now supports named parameters prefixed with a colon such as :name in addition to database-native bind markers. Named parameters thus are supported in annotated repository query methods which also increases portability of queries across database vendors. Named parameter support unrolls collection arguments to reduce the need for argument-specific SQL statements: db.execute() .sql("SELECT id, name, state FROM table WHERE age IN (:ages)") .bind("ages", Arrays.asList(35, 50)); Results in a query: SELECT id, name, state FROM table WHERE age IN (35, 50) Collection arguments containing nested object arrays can be used to use select lists: List<Object[]> tuples = new ArrayList<>(); tuples.add(new Object[] {"John", 35}); tuples.add(new Object[] {"Ann", 50}); db.execute() .sql("SELECT id, name, state FROM table WHERE (name, age) IN (:tuples)") .bind("tuples", tuples); translates to: SELECT id, name, state FROM table WHERE (name, age) IN (('John', 35), ('Ann', 50))

Minor doc fixes. Add Javadoc, align license headers to comply with inlined Spring Framework code.

@test

Formatting. Made Tests simpler and stricter by using `containsExactly`. Added @test annotation to ignored database specific tests so they actually show up as ignored.

DatabaseClient now supports named parameters prefixed with a colon such as :name in addition to database-native bind markers. Named parameters thus are supported in annotated repository query methods which also increases portability of queries across database vendors. Named parameter support unrolls collection arguments to reduce the need for argument-specific SQL statements: db.execute() .sql("SELECT id, name, state FROM table WHERE age IN (:ages)") .bind("ages", Arrays.asList(35, 50)); Results in a query: SELECT id, name, state FROM table WHERE age IN (35, 50) Collection arguments containing nested object arrays can be used to use select lists: List<Object[]> tuples = new ArrayList<>(); tuples.add(new Object[] {"John", 35}); tuples.add(new Object[] {"Ann", 50}); db.execute() .sql("SELECT id, name, state FROM table WHERE (name, age) IN (:tuples)") .bind("tuples", tuples); translates to: SELECT id, name, state FROM table WHERE (name, age) IN (('John', 35), ('Ann', 50)) Original pull request: #47.

@test

Formatting. Made Tests simpler and stricter by using `containsExactly`. Added @test annotation to ignored database specific tests so they actually show up as ignored. Original pull request: #47.

Original pull request: #47.

mp911de · 2019-01-18T13:07:24Z

That's merged now.

mp911de requested review from schauder and odrotbohm January 9, 2019 10:33

schauder approved these changes Jan 10, 2019

View reviewed changes

mp911de and others added 5 commits January 11, 2019 10:54

#23 - Prepare issue branch.

2193e79

Polishing

065e27f

Minor doc fixes. Add Javadoc, align license headers to comply with inlined Spring Framework code.

#23 - Polishing.

76f7a5a

Formatting. Made Tests simpler and stricter by using `containsExactly`. Added @test annotation to ignored database specific tests so they actually show up as ignored.

#23 - Address review comments.

e7b0572

mp911de force-pushed the issues/23-named-parameters branch from d4c258d to e7b0572 Compare January 11, 2019 09:54

mp911de pushed a commit that referenced this pull request Jan 18, 2019

#23 - Polishing.

29f1edc

Formatting. Made Tests simpler and stricter by using `containsExactly`. Added @test annotation to ignored database specific tests so they actually show up as ignored. Original pull request: #47.

mp911de added a commit that referenced this pull request Jan 18, 2019

#23 - Address review comments.

3d1041c

Original pull request: #47.

mp911de added the type: enhancement A general enhancement label Jan 18, 2019

mp911de added this to the 1.0 M2 milestone Jan 18, 2019

mp911de closed this Jan 18, 2019

mp911de deleted the issues/23-named-parameters branch January 18, 2019 13:07

oshai mentioned this pull request Jan 17, 2023

r2dbc named parameters binding jasync-sql/jasync-sql#331

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for named parameters #47

Add support for named parameters #47

mp911de commented Jan 4, 2019

schauder left a comment

schauder Jan 9, 2019

mp911de Jan 10, 2019

schauder Jan 9, 2019

mp911de Jan 11, 2019

schauder Jan 9, 2019

schauder Jan 10, 2019

schauder Jan 10, 2019

schauder Jan 10, 2019

schauder Jan 10, 2019

schauder Jan 10, 2019

mp911de Jan 11, 2019

schauder Jan 10, 2019

mp911de Jan 11, 2019

schauder Jan 10, 2019

mp911de Jan 11, 2019

schauder Jan 10, 2019

mp911de Jan 10, 2019

mp911de commented Jan 18, 2019


		Statement<?> statement = it.createStatement(operation.toQuery());

		byName.forEach((name, o) -> {

Add support for named parameters #47

Add support for named parameters #47

Conversation

mp911de commented Jan 4, 2019

schauder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mp911de commented Jan 18, 2019