diff --git a/src/main/java/org/springframework/data/r2dbc/core/NamedParameterExpander.java b/src/main/java/org/springframework/data/r2dbc/core/NamedParameterExpander.java index 3750ddb..adcffec 100644 --- a/src/main/java/org/springframework/data/r2dbc/core/NamedParameterExpander.java +++ b/src/main/java/org/springframework/data/r2dbc/core/NamedParameterExpander.java @@ -29,6 +29,9 @@ import org.springframework.data.r2dbc.dialect.BindMarkersFactory; * This class expands SQL from named parameters to native style placeholders at execution time. It also allows for * expanding a {@link java.util.List} of values to the appropriate number of placeholders. *

+ * References to the same parameter name are substituted with the same bind marker placeholder if a + * {@link BindMarkersFactory} uses {@link BindMarkersFactory#identifiablePlaceholders() identifiable} placeholders. + *

* NOTE: An instance of this class is thread-safe once configured. * * @author Mark Paluch diff --git a/src/main/java/org/springframework/data/r2dbc/core/NamedParameterUtils.java b/src/main/java/org/springframework/data/r2dbc/core/NamedParameterUtils.java index 1c89c69..936995c 100644 --- a/src/main/java/org/springframework/data/r2dbc/core/NamedParameterUtils.java +++ b/src/main/java/org/springframework/data/r2dbc/core/NamedParameterUtils.java @@ -31,6 +31,7 @@ import org.springframework.data.r2dbc.dialect.BindMarkers; import org.springframework.data.r2dbc.dialect.BindMarkersFactory; import org.springframework.data.r2dbc.dialect.BindTarget; import org.springframework.util.Assert; +import org.springframework.util.CollectionUtils; /** * Helper methods for named parameter parsing. @@ -38,6 +39,9 @@ import org.springframework.util.Assert; * Only intended for internal use within Spring's Data's R2DBC framework. Partially extracted from Spring's JDBC named * parameter support. *

+ * References to the same parameter name are substituted with the same bind marker placeholder if a + * {@link BindMarkersFactory} uses {@link BindMarkersFactory#identifiablePlaceholders() identifiable} placeholders. + *

* This is a subset of Spring Frameworks's {@code org.springframework.r2dbc.namedparam.NamedParameterUtils}. * * @author Thomas Risberg @@ -260,7 +264,7 @@ abstract class NamedParameterUtils { public static PreparedOperation substituteNamedParameters(ParsedSql parsedSql, BindMarkersFactory bindMarkersFactory, BindParameterSource paramSource) { - BindMarkerHolder markerHolder = new BindMarkerHolder(bindMarkersFactory.create()); + NamedParameters markerHolder = new NamedParameters(bindMarkersFactory); String originalSql = parsedSql.getOriginalSql(); List paramNames = parsedSql.getParameterNames(); @@ -276,9 +280,11 @@ abstract class NamedParameterUtils { int startIndex = indexes[0]; int endIndex = indexes[1]; actualSql.append(originalSql, lastIndex, startIndex); + NamedParameters.NamedParameter marker = markerHolder.getOrCreate(paramName); if (paramSource.hasValue(paramName)) { Object value = paramSource.getValue(paramName); if (value instanceof Collection) { + Iterator entryIter = ((Collection) value).iterator(); int k = 0; while (entryIter.hasNext()) { @@ -294,19 +300,19 @@ abstract class NamedParameterUtils { if (m > 0) { actualSql.append(", "); } - actualSql.append(markerHolder.addMarker(paramName)); + actualSql.append(marker.addPlaceholder()); } actualSql.append(')'); } else { - actualSql.append(markerHolder.addMarker(paramName)); + actualSql.append(marker.addPlaceholder()); } } } else { - actualSql.append(markerHolder.addMarker(paramName)); + actualSql.append(marker.getPlaceholder()); } } else { - actualSql.append(markerHolder.addMarker(paramName)); + actualSql.append(marker.getPlaceholder()); } lastIndex = endIndex; } @@ -387,22 +393,79 @@ abstract class NamedParameterUtils { } /** - * Holder for bind marker progress. + * Holder for bind markers progress. */ - private static class BindMarkerHolder { + static class NamedParameters { private final BindMarkers bindMarkers; - private final Map> markers = new TreeMap<>(); + private final boolean identifiable; + private final Map> references = new TreeMap<>(); - BindMarkerHolder(BindMarkers bindMarkers) { - this.bindMarkers = bindMarkers; + NamedParameters(BindMarkersFactory factory) { + this.bindMarkers = factory.create(); + this.identifiable = factory.identifiablePlaceholders(); } - String addMarker(String name) { + /** + * Get the {@link NamedParameter} identified by {@code namedParameter}. + * + * @param namedParameter + * @return + */ + NamedParameter getOrCreate(String namedParameter) { - BindMarker bindMarker = this.bindMarkers.next(name); - this.markers.computeIfAbsent(name, ignore -> new ArrayList<>()).add(bindMarker); - return bindMarker.getPlaceholder(); + List reference = this.references.computeIfAbsent(namedParameter, ignore -> new ArrayList<>()); + + if (reference.isEmpty()) { + NamedParameter param = new NamedParameter(namedParameter); + reference.add(param); + return param; + } + + if (this.identifiable) { + return reference.get(0); + } + + NamedParameter param = new NamedParameter(namedParameter); + reference.add(param); + return param; + } + + List getMarker(String name) { + return this.references.get(name); + } + + class NamedParameter { + + private final String namedParameter; + private final List placeholders = new ArrayList<>(); + + NamedParameter(String namedParameter) { + this.namedParameter = namedParameter; + } + + /** + * Create a placeholder to translate a single value into a bindable parameter. + *

+ * Can be called multiple times to create placeholders for array/collections. + * + * @return + */ + String addPlaceholder() { + + BindMarker bindMarker = NamedParameters.this.bindMarkers.next(this.namedParameter); + this.placeholders.add(bindMarker); + return bindMarker.getPlaceholder(); + } + + String getPlaceholder() { + + if (this.placeholders.isEmpty()) { + return addPlaceholder(); + } + + return this.placeholders.get(0).getPlaceholder(); + } } } @@ -414,13 +477,13 @@ abstract class NamedParameterUtils { private final String expandedSql; - private final Map> markers; + private final NamedParameters parameters; private final BindParameterSource parameterSource; - ExpandedQuery(String expandedSql, BindMarkerHolder bindMarkerHolder, BindParameterSource parameterSource) { + ExpandedQuery(String expandedSql, NamedParameters parameters, BindParameterSource parameterSource) { this.expandedSql = expandedSql; - this.markers = bindMarkerHolder.markers; + this.parameters = parameters; this.parameterSource = parameterSource; } @@ -435,13 +498,7 @@ abstract class NamedParameterUtils { return; } - if (bindMarkers.size() == 1) { - bindMarkers.get(0).bind(target, value); - } else { - - Assert.isInstanceOf(Collection.class, value, - () -> String.format("Value [%s] must be an Collection with a size of [%d]", value, bindMarkers.size())); - + if (value instanceof Collection) { Collection collection = (Collection) value; Iterator iterator = collection.iterator(); @@ -460,6 +517,10 @@ abstract class NamedParameterUtils { bind(target, markers, valueToBind); } } + } else { + for (BindMarker bindMarker : bindMarkers) { + bindMarker.bind(target, value); + } } } @@ -483,16 +544,26 @@ abstract class NamedParameterUtils { return; } - if (bindMarkers.size() == 1) { - bindMarkers.get(0).bindNull(target, valueType); - return; + for (BindMarker bindMarker : bindMarkers) { + bindMarker.bindNull(target, valueType); } - - throw new UnsupportedOperationException("bindNull(…) can bind only singular values"); } - private List getBindMarkers(String identifier) { - return this.markers.get(identifier); + List getBindMarkers(String identifier) { + + List parameters = this.parameters.getMarker(identifier); + + if (parameters == null) { + return null; + } + + List markers = new ArrayList<>(); + + for (NamedParameters.NamedParameter parameter : parameters) { + markers.addAll(parameter.placeholders); + } + + return markers; } @Override diff --git a/src/main/java/org/springframework/data/r2dbc/dialect/BindMarkersFactory.java b/src/main/java/org/springframework/data/r2dbc/dialect/BindMarkersFactory.java index 10e3900..f86c1ce 100644 --- a/src/main/java/org/springframework/data/r2dbc/dialect/BindMarkersFactory.java +++ b/src/main/java/org/springframework/data/r2dbc/dialect/BindMarkersFactory.java @@ -26,6 +26,17 @@ public interface BindMarkersFactory { */ BindMarkers create(); + /** + * Return whether the {@link BindMarkersFactory} uses identifiable placeholders. + * + * @return whether the {@link BindMarkersFactory} uses identifiable placeholders. {@literal false} if multiple + * placeholders cannot be distinguished by just the {@link BindMarker#getPlaceholder() placeholder} + * identifier. + */ + default boolean identifiablePlaceholders() { + return true; + } + /** * Create index-based {@link BindMarkers} using indexes to bind parameters. Allow customization of the bind marker * placeholder {@code prefix} to represent the bind marker as placeholder within the query. @@ -40,6 +51,7 @@ public interface BindMarkersFactory { static BindMarkersFactory indexed(String prefix, int beginWith) { Assert.notNull(prefix, "Prefix must not be null!"); + return () -> new IndexedBindMarkers(prefix, beginWith); } @@ -56,7 +68,19 @@ public interface BindMarkersFactory { static BindMarkersFactory anonymous(String placeholder) { Assert.hasText(placeholder, "Placeholder must not be empty!"); - return () -> new AnonymousBindMarkers(placeholder); + + return new BindMarkersFactory() { + + @Override + public BindMarkers create() { + return new AnonymousBindMarkers(placeholder); + } + + @Override + public boolean identifiablePlaceholders() { + return false; + } + }; } /** diff --git a/src/test/java/org/springframework/data/r2dbc/core/AbstractDatabaseClientIntegrationTests.java b/src/test/java/org/springframework/data/r2dbc/core/AbstractDatabaseClientIntegrationTests.java index 1f3b83d..a48e6e9 100644 --- a/src/test/java/org/springframework/data/r2dbc/core/AbstractDatabaseClientIntegrationTests.java +++ b/src/test/java/org/springframework/data/r2dbc/core/AbstractDatabaseClientIntegrationTests.java @@ -152,6 +152,18 @@ public abstract class AbstractDatabaseClientIntegrationTests extends R2dbcIntegr }).verifyComplete(); } + @Test // gh-2 + public void executeSelectNamedParameters() { + + DatabaseClient databaseClient = DatabaseClient.create(connectionFactory); + + databaseClient.execute("SELECT id, name, manual FROM legoset WHERE name = :name or manual = :name") // + .bind("name", "unknown").as(LegoSet.class) // + .fetch().all() // + .as(StepVerifier::create) // + .verifyComplete(); + } + @Test // gh-2 public void insert() { diff --git a/src/test/java/org/springframework/data/r2dbc/core/NamedParameterUtilsUnitTests.java b/src/test/java/org/springframework/data/r2dbc/core/NamedParameterUtilsUnitTests.java index b48bed5..b96aa30 100644 --- a/src/test/java/org/springframework/data/r2dbc/core/NamedParameterUtilsUnitTests.java +++ b/src/test/java/org/springframework/data/r2dbc/core/NamedParameterUtilsUnitTests.java @@ -19,7 +19,10 @@ import static org.assertj.core.api.Assertions.*; import static org.mockito.Mockito.*; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; +import java.util.LinkedHashMap; +import java.util.Map; import org.junit.Test; @@ -27,6 +30,7 @@ import org.springframework.data.r2dbc.dialect.BindMarkersFactory; import org.springframework.data.r2dbc.dialect.BindTarget; import org.springframework.data.r2dbc.dialect.PostgresDialect; import org.springframework.data.r2dbc.dialect.SqlServerDialect; +import org.springframework.data.r2dbc.mapping.SettableValue; /** * Unit tests for {@link NamedParameterUtils}. @@ -111,7 +115,7 @@ public class NamedParameterUtilsUnitTests { String sql1 = "/*+ HINT */ xxx /* comment ? */ :a yyyy :b :c :a zzzzz -- :xx XX\n"; ParsedSql psql1 = NamedParameterUtils.parseSqlStatement(sql1); - assertThat(expand(psql1)).isEqualTo("/*+ HINT */ xxx /* comment ? */ $1 yyyy $2 $3 $4 zzzzz -- :xx XX\n"); + assertThat(expand(psql1)).isEqualTo("/*+ HINT */ xxx /* comment ? */ $1 yyyy $2 $3 $1 zzzzz -- :xx XX\n"); MapBindParameterSource paramMap = new MapBindParameterSource(new HashMap<>()); paramMap.addValue("a", "a"); @@ -120,7 +124,7 @@ public class NamedParameterUtilsUnitTests { String sql2 = "/*+ HINT */ xxx /* comment ? */ :a yyyy :b :c :a zzzzz -- :xx XX"; ParsedSql psql2 = NamedParameterUtils.parseSqlStatement(sql2); - assertThat(expand(psql2)).isEqualTo("/*+ HINT */ xxx /* comment ? */ $1 yyyy $2 $3 $4 zzzzz -- :xx XX"); + assertThat(expand(psql2)).isEqualTo("/*+ HINT */ xxx /* comment ? */ $1 yyyy $2 $3 $1 zzzzz -- :xx XX"); } @Test // gh-23 @@ -283,6 +287,129 @@ public class NamedParameterUtilsUnitTests { assertThat(psql2.getParameterNames()).containsExactly("xxx"); } + @Test // gh-138 + public void shouldAllowParsingMultipleUseOfParameter() { + + String sql = "SELECT * FROM person where name = :id or lastname = :id"; + + ParsedSql parsed = NamedParameterUtils.parseSqlStatement(sql); + + assertThat(parsed.getTotalParameterCount()).isEqualTo(2); + assertThat(parsed.getNamedParameterCount()).isEqualTo(1); + assertThat(parsed.getParameterNames()).containsExactly("id", "id"); + } + + @Test // gh-138 + public void multipleEqualParameterReferencesBindsValueOnce() { + + String sql = "SELECT * FROM person where name = :id or lastname = :id"; + + BindMarkersFactory factory = BindMarkersFactory.indexed("$", 0); + + PreparedOperation operation = NamedParameterUtils.substituteNamedParameters(sql, factory, + new MapBindParameterSource(Collections.singletonMap("id", SettableValue.from("foo")))); + + assertThat(operation.toQuery()).isEqualTo("SELECT * FROM person where name = $0 or lastname = $0"); + + operation.bindTo(new BindTarget() { + @Override + public void bind(Object identifier, Object value) { + throw new UnsupportedOperationException(); + } + + @Override + public void bind(int index, Object value) { + assertThat(index).isEqualTo(0); + assertThat(value).isEqualTo("foo"); + } + + @Override + public void bindNull(Object identifier, Class type) { + throw new UnsupportedOperationException(); + } + + @Override + public void bindNull(int index, Class type) { + throw new UnsupportedOperationException(); + } + }); + } + + @Test // gh-138 + public void multipleEqualParameterReferencesForAnonymousMarkersBindsValueMultipleTimes() { + + String sql = "SELECT * FROM person where name = :id or lastname = :id"; + + BindMarkersFactory factory = BindMarkersFactory.anonymous("?"); + + PreparedOperation operation = NamedParameterUtils.substituteNamedParameters(sql, factory, + new MapBindParameterSource(Collections.singletonMap("id", SettableValue.from("foo")))); + + assertThat(operation.toQuery()).isEqualTo("SELECT * FROM person where name = ? or lastname = ?"); + + Map bindValues = new LinkedHashMap<>(); + + operation.bindTo(new BindTarget() { + @Override + public void bind(Object identifier, Object value) { + throw new UnsupportedOperationException(); + } + + @Override + public void bind(int index, Object value) { + bindValues.put(index, value); + } + + @Override + public void bindNull(Object identifier, Class type) { + throw new UnsupportedOperationException(); + } + + @Override + public void bindNull(int index, Class type) { + throw new UnsupportedOperationException(); + } + }); + + assertThat(bindValues).hasSize(2).containsEntry(0, "foo").containsEntry(1, "foo"); + } + + @Test // gh-138 + public void multipleEqualParameterReferencesBindsNullOnce() { + + String sql = "SELECT * FROM person where name = :id or lastname = :id"; + + BindMarkersFactory factory = BindMarkersFactory.indexed("$", 0); + + PreparedOperation operation = NamedParameterUtils.substituteNamedParameters(sql, factory, + new MapBindParameterSource(Collections.singletonMap("id", SettableValue.empty(String.class)))); + + assertThat(operation.toQuery()).isEqualTo("SELECT * FROM person where name = $0 or lastname = $0"); + + operation.bindTo(new BindTarget() { + @Override + public void bind(Object identifier, Object value) { + throw new UnsupportedOperationException(); + } + + @Override + public void bind(int index, Object value) { + throw new UnsupportedOperationException(); + } + + @Override + public void bindNull(Object identifier, Class type) { + throw new UnsupportedOperationException(); + } + + @Override + public void bindNull(int index, Class type) { + assertThat(index).isEqualTo(0); + assertThat(type).isEqualTo(String.class); + } + }); + } + private String expand(ParsedSql sql) { return NamedParameterUtils.substituteNamedParameters(sql, BIND_MARKERS, new MapBindParameterSource()).toQuery(); } diff --git a/src/test/java/org/springframework/data/r2dbc/core/PostgresDatabaseClientIntegrationTests.java b/src/test/java/org/springframework/data/r2dbc/core/PostgresDatabaseClientIntegrationTests.java index 7f855d6..7b18125 100644 --- a/src/test/java/org/springframework/data/r2dbc/core/PostgresDatabaseClientIntegrationTests.java +++ b/src/test/java/org/springframework/data/r2dbc/core/PostgresDatabaseClientIntegrationTests.java @@ -55,6 +55,11 @@ public class PostgresDatabaseClientIntegrationTests extends AbstractDatabaseClie @Override public void insert() {} + @Ignore("Postgres considers multiple references to the same parameter as multiple bindings (where name = $1 OR lastname = $1)") + @Test + @Override + public void executeSelectNamedParameters() {} + @Ignore("Adding RETURNING * lets Postgres report 0 affected rows.") @Test @Override