#282 - Support of query derivation #295

rchigvintsev · 2020-02-03T13:10:20Z

This PR introduces basic query derivation without support of embedded properties and collection properties. I decided not to scatter changes among different projects so to get a clean implementation of query derivation some changes in the spring-data-relational project would be required.

Issue: #282

mp911de · 2020-02-03T13:31:46Z

Thanks for your PR. It will take a bit to review this change. Looking at a few operators, there are some facilities such as SimpleFunction.create("UPPER", column). Probably we require some extensions to Spring Data Relational's SQL model.

rchigvintsev · 2020-02-03T13:41:51Z

@mp911de I tried to use SimpleFunction but it is taken into account only in SELECT expressions, but not in WHERE conditions. Some changes in spring-data-relational definitely won't be superfluous. In the source code I left several TODOs related to required improvements.

schauder

This looks very good.

I noted some smaller stuff.

The change also needs rebasing onto master since we introduced SqlIdentifier in the meantime. (Sorry for that.)

I created an issue for adding essential infrastructure to spring-data-relational: https://jira.spring.io/browse/DATAJDBC-502
It is probably easiest if you would do these changes as well.
But feel free to let us know if you want us to do this.

schauder · 2020-03-10T15:31:43Z

src/main/java/org/springframework/data/r2dbc/core/DefaultStatementMapper.java

@@ -38,6 +34,10 @@
 import org.springframework.lang.Nullable;
 import org.springframework.util.Assert;

+import java.util.ArrayList;


Please make sure your formatter is configured as described in https://github.com/spring-projects/spring-data-build/tree/master/etc/ide

schauder · 2020-03-10T15:32:53Z

src/main/java/org/springframework/data/r2dbc/repository/query/ConditionFactory.java

@@ -0,0 +1,307 @@
+/*
+ * Copyright 2018-2020 the original author or authors.


The copyright for new files should be only for the current year.

schauder · 2020-03-12T15:22:03Z

src/main/java/org/springframework/data/r2dbc/repository/query/PartTreeR2dbcQuery.java

+    }
+
+    private static boolean parameterIsCollectionLike(RelationalParameters.RelationalParameter parameter) {
+        return Iterable.class.isAssignableFrom(parameter.getType()) || parameter.getType().isArray();


We learned the hard way that we shouldn't use Iterable here but Collection. Also we should check first if the type of the parameter matches the type of the property. See https://jira.spring.io/browse/DATAJPA-1682

schauder · 2020-03-12T15:28:25Z

src/test/java/org/springframework/data/r2dbc/repository/query/LikeEscaperTest.java

+/**
+ * @author Roman Chigvintsev
+ */
+public class LikeEscaperTest {


Test names should be named ....UnitTests or ....IntegrationTests this applies to all tests.

schauder · 2020-03-12T15:28:50Z

src/test/java/org/springframework/data/r2dbc/repository/query/LikeEscaperTest.java

+ */
+public class LikeEscaperTest {
+    @Test
+    public void ignoresNulls() {


I think this class deserves some additional tests to ensure the escaping actually works.

I relied on integration tests here, but yes, you're right, unit tests are required too.

rchigvintsev · 2020-03-12T16:42:43Z

@schauder I don't mind to make changes in 'spring-data-relational' too.

Also I'm planning to add query caching (in the similar way as it is done in 'spring-data-jpa') along with corrections to this PR. Is it OK or should I create another request?

schauder · 2020-03-12T17:04:15Z

Sounds great. Just don't let the PR grow to much. I rather have something working on master then something fast somewhere in the process.

mp911de

I took a review pass from a R2DBC perspective. Thanks for the great work, the approach looks really good. I left a few comments about aspects to consider and some R2DBC specifics that make it easier to work with various database dialects.

mp911de · 2020-03-13T10:00:00Z

src/main/java/org/springframework/data/r2dbc/repository/query/ConditionFactory.java

+                BindMarker secondBindMarker = createBindMarker(parameterMetadataProvider.next(part));
+
+                // TODO: why do not we have BETWEEN condition?
+                return Conditions.isGreaterOrEqualTo(pathExpression, firstBindMarker)


Good candidate for an enhancement in Spring Data Relational. On a related note: We support in some stores also a Range data type (findByDateBetween(Range<LocalDate>)) that allows for more fine-grained control of inclusion/exclusion regarding boundaries.

mp911de · 2020-03-13T10:07:42Z

src/main/java/org/springframework/data/r2dbc/repository/query/ConditionFactory.java

+    @NotNull
+    private BindMarker createBindMarker(ParameterMetadata parameterMetadata) {
+        if (parameterMetadata.getName() != null) {
+            return SQL.bindMarker(":" + parameterMetadata.getName());


Using named parameters manifests the requirement that named parameter processing is enabled in DatabaseClient. We tried to not stick with such an assumption. Ideally, we generate directly SQL that does not require post-processing by accessing Dialect when generating SQL to use the proper BindMarkersFactory.

I can suggest the following solution:

at first we should expose Dialect because we'll need it to create correct bind markers; in my opinion ReactiveDataAccessStrategy interface is a good candidate (if so I'm not sure whether default method is required since this interface has only one implementation);

pass Dialect to the constructor of ParameterMetadataProvider

public class PartTreeR2dbcQuery extends AbstractR2dbcQuery { ... protected BindableQuery createQuery(RelationalParameterAccessor accessor) { ... ParameterMetadataProvider parameterMetadataProvider = new ParameterMetadataProvider(accessor, likeEscaper, dataAccessStrategy.getDialect()); ...

in ParameterMetadataProvider create BindMarkers using current BindMarkersFactory

class ParameterMetadataProvider { ... private ParameterMetadataProvider(Parameters<?, ?> parameters, @Nullable Iterator<Object> bindableParameterValueIterator, LikeEscaper likeEscaper, R2dbcDialect dialect) { ... this.bindMarkers = dialect.getBindMarkersFactory().create(); } ...

save parameter placeholder in ParameterMetadata

class ParameterMetadataProvider { ... public ParameterMetadata next(Part part) { ... ParameterMetadata metadata = ParameterMetadata.builder() .type(parameter.getType()) .partType(part.getType()) .name(parameterName) .isNullParameter(getParameterValue() == null && Part.Type.SIMPLE_PROPERTY.equals(part.getType())) .placeholder(getPlaceholder(parameterName)) .likeEscaper(likeEscaper) .build(); ... } private String getPlaceholder(String parameterName) { return bindMarkers.next(parameterName).getPlaceholder(); } ...

use placeholder to create bind marker

class ConditionFactory { ... @NonNull private BindMarker createBindMarker(ParameterMetadata parameterMetadata) { return SQL.bindMarker(parameterMetadata.getPlaceholder()); } ...

always use placeholder to bind parameter value

class PartTreeBindableQuery implements BindableQuery { ... public <T extends DatabaseClient.BindSpec<T>> T bind(T bindSpec) { T bindSpecToUse = bindSpec; int index = 0; for (Object value : accessor.getValues()) { ParameterMetadata metadata = parameterMetadataProvider.getParameterMetadata(index++); Class<?> parameterType = metadata.getType(); if (value == null) { bindSpecToUse = bindSpecToUse.bindNull(metadata.getPlaceholder(), parameterType); } else { bindSpecToUse = bindSpecToUse.bind(metadata.getPlaceholder(), metadata.prepare(value)); } } return bindSpecToUse; } ...

I've just realized that my approach won't work for databases where question mark is used as a placeholder (MySQL for example). Maybe during parameter value binding I should check whether placeholder is '?' and use binding by index if it is true.

mp911de · 2020-03-13T10:13:06Z

src/main/java/org/springframework/data/r2dbc/repository/query/LikeEscaper.java

+ *
+ * @author Roman Chigvintsev
+ */
+public class LikeEscaper {


This type looks as it would be a good candidate for Spring Data Relational, to be defined as part of a Dialect. Please note that SQL Server (probably the only exception for now) uses square brackets ([]) for a regex-y style declaring character ranges. We should design the class with this behavior in mind. Turning this class into an interface and adding a default implementation for % would be a good first step. In the SQL Server-specific dialect, we can add an adaption for square brackets.

mp911de · 2020-03-13T10:16:13Z

src/main/java/org/springframework/data/r2dbc/repository/query/PartTreeBindableQuery.java

+            String parameterName = metadata.getName();
+            Class<?> parameterType = metadata.getType();
+
+            if (parameterName != null) {


The way how parameters get bound in R2DBC varies from database to database. Postgres and SQL Server use named parameters ($1 or @MyParam) while MySQL uses anonymous bind markers (?) that are only settable via index. Our Dialect abstraction along with BindMarkers caters already for these differences. We don't require parameter names from the interface declaration here.

mp911de · 2020-03-13T10:18:10Z

src/main/java/org/springframework/data/r2dbc/repository/query/R2dbcQueryCreator.java

+        Collection<? extends Expression> selectExpressions = getSelectionExpressions(fromTable);
+        SelectFromAndJoin selectBuilder = StatementBuilder.select(selectExpressions).from(fromTable);
+
+        if (tree.isExistsProjection()) {


Nit: PartTree exposes a isLimiting() method that allows imposing a limit on the actual query (findTop3). It would make sense to consider that feature while being at query derivation.

mp911de · 2020-03-13T10:20:13Z

src/main/java/org/springframework/data/r2dbc/repository/query/ParameterMetadata.java

+        if (String.class.equals(type)) {
+            switch (partType) {
+                case STARTING_WITH:
+                    return String.format("%s%%", likeEscaper.escape(value.toString()));


As much as I like String.format(), the number of % contributes to the fact that the code is less comprehensible. For these methods, I'm a fan of plain "%"+value+"%".

mp911de · 2020-03-13T10:20:33Z

src/main/java/org/springframework/data/r2dbc/repository/query/ParameterMetadata.java

+ */
+@Builder
+@AllArgsConstructor
+class ParameterMetadata {


Please let's not use Lombok in new code as we try to reduce Lombok usage.

mp911de · 2020-03-13T10:21:38Z

src/main/java/org/springframework/data/r2dbc/repository/query/ConditionFactory.java

+     * <p>
+     * Results in a rendered function: {@code UPPER(<expression>)}.
+     */
+    private class Upper implements Expression {


Can we make use of SimpleFunction so that we are not required to introduce new types for each function we want to support? It makes sense to expose that functionality as Functions.upper/lower in Spring Data Relational.

mp911de · 2020-03-13T10:23:17Z

src/main/java/org/springframework/data/r2dbc/repository/query/ParameterMetadata.java

+    public Object prepare(Object value) {
+        Assert.notNull(value, "Value must not be null!");
+        if (String.class.equals(type)) {
+            switch (partType) {


Note: Carrying out PartTree details to the outside of the query creator isn't ideal. Maybe we can find a better approach during the merge.

…cQueryCreator

…ndition

…o be able to build SQL manually

…itions

… operators

…GATING_SIMPLE_PROPERTY' conditions

…'NOT CONTAINING' condition

… whether parameter is scalar

…ng convention

mp911de · 2020-03-20T07:48:29Z

Not necessary to keep all the commits as we would have asked you right before the merge to squash all commits into a single one. Let us know when we should have another look.

rchigvintsev · 2020-03-20T08:10:39Z

@mp911de OK. By the way could you leave some comment related to issue with named parameters in ConditionFactory please?

mp911de · 2020-03-20T08:54:39Z

I'm addressing the binding issue in a separate comment as the thread above is already quite lengthy. Bind markers follow their own strategy and therefore, we have a BindMarkersFactory that creates BindMarkers and BindMarker. BindMarkers is associated with a state (e.g. counter) so the way to use that API is creating a BindMarker object per bindable parameter (or element in the collection to bind) and keep track of both, the BindMarker and the actual value.

We have a similar setup in DefaultStatementMapper. I'm wondering whether it would actually make sense to reuse the Criteria API and StatementMapper to accomplish query derivation.

There are three reasons for that idea:

We have only a single approach to construct queries
By reusing the criteria API and StatmentMapper we ensure that Dialect does not leak outside the strategy class (which could be implemented by a different library such as jOOQ and they have their own mechanics to deal with dialect specifics)
We make sure that all operations that can be used from derived queries are also available through the Criteria API. If we miss an operation right now, then we can extend the Criteria API

Let me know what you think.

rchigvintsev · 2020-03-20T09:47:31Z

@mp911de Maybe there is some misunderstanding here. I asked you to review the solution that I proposed above - in comments to your note regarding bind markers and ConditionFactory. But perhaps in the new light of requirement of integration StatementMapper with Criteria API this solution does not make sense anymore.
When I thought about ways of building SQL of course I considered StatementMapper and I didn't find an easy way to create Criteria from Condition for SelectSpec so I choose to use StatementBuilder. Perhaps we need an option to pass Condition to SelectSpec directly and avoid unnecessary conversions since Criteria finally becomes a Condition with help of QueryMapper.

rchigvintsev · 2020-03-20T10:16:49Z

@mp911de I believe it is my misunderstanding. If by "Criteria API" you mean API that is provided by org.springframework.data.r2dbc.query.Criteria class so in my opinion the best approach is to transform ConditionFactory to CriteriaFactory and use Criteria instead of Conditions but improvements in spring-data-relational will still be required.

rchigvintsev · 2020-03-20T12:11:14Z

@mp911de Continuing the idea of using Criteria instead of Conditions and CriteriaFactory there is still issue with bind markers. It looks like Criteria API operates only with actual parameter values. For performance reasons it makes sense to keep SelectSpec reusable so Criteria API should have a way to specify bind marker instead of actual value (pass marker of bind marker as a value? :))

mp911de · 2020-03-20T13:02:33Z

The spec objects are lightweight objects. For now, let's not consider caching until we really require caching of query objects.

… instead of 'StatementBuilder'

rchigvintsev · 2020-03-23T16:52:16Z

@mp911de I made a few changes in query building mechanism according to your suggestions. Please review them.
Notes:

I temporarily moved classes that are supposed to be in spring-data-relational from ConditionFactory to QueryMapper;
I have a little concern regarding to case-insensitive comparisons in query conditions - when IgnoreCaseType is WHEN_POSSIBLE there is a theoretical possibility when database field will be uppercased but parameter won't (and vice versa); for example, if I use spring-data-jpa I can write method findByFirstNameAllIgnoreCase(StringBuilder firstName) that will be translated (simplified) to SELECT * FROM table WHERE UPPER(first_name) = :firstName provided that "firstName" property is a string; finally on the try to use this method I will get an exception thrown by Hibernate so I don't know is it a real scenario or not; here in this PR I don't apply UPPER function when at least one part of expression doesn't support it.

mp911de · 2020-03-27T08:02:33Z

Thanks a lot. @schauder and I discussed how to proceed with this PR and figured it would make a lot of sense to migrate most of the types into Spring Data Relational so we have already the infrastructure in place that would be required for Spring Data JDBC's query derivation.

I'm going to pick up your pull request for merge.

rchigvintsev · 2020-03-27T08:42:09Z

@mp911de OK. Feel free to let me know if you need some help with spring-data-relational or spring-data-jdbc.

… Relational. Original pull request: spring-projects/spring-data-r2dbc#295.

Use AssertJ version specified by Spring Data Build. Original pull request: spring-projects/spring-data-r2dbc#295.

…nd SQL generation. We now support Conditions.between, notBetween, and notLike as additional criteria conditions and support case-insensitive comparisons. For LIKE escaping we pick up the Escaper configured at Dialect level. The newly introduced ValueFunction allows string transformation before computing a value by applying the Escaper to the raw value. Escaping is required for StartingWith, Contains and EndsWith PartTree operations. Original pull request: spring-projects/spring-data-r2dbc#295.

We now support query derivation for R2DBC repositories: interface ReactivePersonRepository extends ReactiveSortingRepository<Person, String> { Flux<Person> findByFirstname(String firstname); Flux<Person> findByFirstname(Publisher<String> firstname); Mono<Person> findByFirstnameAndLastname(String firstname, String lastname); Flux<Person> findFirstByLastnameLike(String pattern); } Original pull request: #295.

Move query derivation infrastructure to Spring Data Relational. Adapt to newly introduced ValueFunction for deferred value mapping. Use query derivation in integration tests. Tweak javadoc, add since and author tags, reformat code. Related ticket: https://jira.spring.io/browse/DATAJDBC-514 Original pull request: #295.

Original pull request: #295.

mp911de · 2020-03-27T15:54:33Z

Thanks a lot for your contribution. That's merged and polished now. During the merge, we decided to move more components over to Spring Data Relational so that we just need to glue things together to get query derivation for Spring Data JDBC.

rchigvintsev mentioned this pull request Feb 3, 2020

Support of query derivation #282

Closed

mp911de requested review from schauder and mp911de February 3, 2020 13:32

mp911de added the type: enhancement A general enhancement label Feb 3, 2020

schauder requested changes Mar 12, 2020

View reviewed changes

mp911de requested changes Mar 13, 2020

View reviewed changes

rchigvintsev force-pushed the issue-282 branch from 9dc1a57 to da94d96 Compare March 13, 2020 11:59

rchigvintsev added 19 commits March 14, 2020 11:21

spring-projects#282 - Starting point for PartTree-based query derivation

e266d0d

spring-projects#282 - Generate simplest SQL without condition in R2db…

46bae29

…cQueryCreator

spring-projects#282 - Allow to create queries with simple equality co…

90c84df

…ndition

spring-projects#282 - Add test of 'IS NULL' condition

a13c2c8

spring-projects#282 - Expose 'RenderContext' from 'StatementMapper' t…

3871f0b

…o be able to build SQL manually

spring-projects#282 - Limit query in case of 'exists' projection

0d415df

spring-projects#282 - Add Copyright License headers to new files

497e604

spring-projects#282 - Implement conjunction of conditions

d552ec1

spring-projects#282 - Implement disjunction of conditions

b222607

spring-projects#282 - Implement query parameter binding

21e35b2

spring-projects#282 - Add support of 'BETWEEN' operator

bab4c7e

spring-projects#282 - Add support of 'LESS/GREATER THAN (EQUAL)' cond…

0944f8a

…itions

spring-projects#282 - Add support of 'AFTER/BEFORE' conditions

61083bf

spring-projects#282 - Add support of 'IS (NOT) NULL' and '(NOT) LIKE'…

39c745a

… operators

spring-projects#282 - Add support of 'STARTING/ENDING WITH' conditions

cca24c8

spring-projects#282 - Add support of 'CONTAINING' conditions

7c135fe

spring-projects#282 - Support ordering

7095113

spring-projects#282 - Add support of '(NOT) IN', 'TRUE/FALSE' and 'NE…

91b4660

…GATING_SIMPLE_PROPERTY' conditions

spring-projects#282 - Add support of case insensitive conditions and …

6358eed

…'NOT CONTAINING' condition

rchigvintsev added 3 commits March 16, 2020 12:51

spring-projects#282 - Use Collection instead of Iterable to determine…

a00ffb8

… whether parameter is scalar

spring-projects#282 - Rename new test classes in accordance with nami…

b40f9c0

…ng convention

spring-projects#282 - Add more unit tests for LikeEscaper

d1d55d7

rchigvintsev requested a review from schauder March 16, 2020 11:49

rchigvintsev added 3 commits March 18, 2020 01:48

spring-projects#282 - Add support of limiting query results

53d01ba

spring-projects#282 - Improve code readability

37b2dbf

spring-projects#282 - Get rid of Lombok annotations in new code

ab7a3ac

spring-projects#282 - Use 'StatementMapper' to create 'BindableQuery'…

e5abd58

… instead of 'StatementBuilder'

rchigvintsev force-pushed the issue-282 branch from 1e282e1 to e5abd58 Compare March 23, 2020 16:43

mp911de added this to the 1.1 RC1 (Neumann) milestone Mar 27, 2020

mp911de pushed a commit to spring-projects/spring-data-relational that referenced this pull request Mar 27, 2020

DATAJDBC-514 - Add infrastructure for query derivation in Spring Data…

7454032

… Relational. Original pull request: spring-projects/spring-data-r2dbc#295.

mp911de added a commit to spring-projects/spring-data-relational that referenced this pull request Mar 27, 2020

DATAJDBC-514 - Polishing.

d61651a

Use AssertJ version specified by Spring Data Build. Original pull request: spring-projects/spring-data-r2dbc#295.

mp911de added a commit that referenced this pull request Mar 27, 2020

#282 - Add documentation.

020d46b

Original pull request: #295.

mp911de closed this Mar 27, 2020

This was referenced Dec 31, 2020

Add infrastructure for query derivation in Spring Data Relational [DATAJDBC-514] spring-projects/spring-data-relational#695

Closed

Additional SQL generation features for support of query derivation. [DATAJDBC-502] spring-projects/spring-data-relational#726

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#282 - Support of query derivation #295

#282 - Support of query derivation #295

rchigvintsev commented Feb 3, 2020 •

edited

Loading

mp911de commented Feb 3, 2020

rchigvintsev commented Feb 3, 2020

schauder left a comment

schauder Mar 10, 2020

schauder Mar 10, 2020

schauder Mar 12, 2020

schauder Mar 12, 2020

schauder Mar 12, 2020

rchigvintsev Mar 16, 2020

rchigvintsev commented Mar 12, 2020 •

edited

Loading

schauder commented Mar 12, 2020

mp911de left a comment

mp911de Mar 13, 2020

mp911de Mar 13, 2020

rchigvintsev Mar 17, 2020 •

edited

Loading

rchigvintsev Mar 17, 2020

mp911de Mar 13, 2020

mp911de Mar 13, 2020

mp911de Mar 13, 2020

mp911de Mar 13, 2020

mp911de Mar 13, 2020

mp911de Mar 13, 2020

mp911de Mar 13, 2020

mp911de commented Mar 20, 2020

rchigvintsev commented Mar 20, 2020

mp911de commented Mar 20, 2020

rchigvintsev commented Mar 20, 2020

rchigvintsev commented Mar 20, 2020

rchigvintsev commented Mar 20, 2020

mp911de commented Mar 20, 2020

rchigvintsev commented Mar 23, 2020

mp911de commented Mar 27, 2020

rchigvintsev commented Mar 27, 2020

mp911de commented Mar 27, 2020

		@@ -0,0 +1,307 @@
		/*
		* Copyright 2018-2020 the original author or authors.

#282 - Support of query derivation #295

#282 - Support of query derivation #295

Conversation

rchigvintsev commented Feb 3, 2020 • edited Loading

mp911de commented Feb 3, 2020

rchigvintsev commented Feb 3, 2020

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

rchigvintsev commented Mar 12, 2020 • edited Loading

schauder commented Mar 12, 2020

mp911de left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchigvintsev Mar 17, 2020 • edited Loading

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 Mar 20, 2020

rchigvintsev commented Mar 20, 2020

mp911de commented Mar 20, 2020

rchigvintsev commented Mar 20, 2020

rchigvintsev commented Mar 20, 2020

rchigvintsev commented Mar 20, 2020

mp911de commented Mar 20, 2020

rchigvintsev commented Mar 23, 2020

mp911de commented Mar 27, 2020

rchigvintsev commented Mar 27, 2020

mp911de commented Mar 27, 2020

rchigvintsev commented Feb 3, 2020 •

edited

Loading

rchigvintsev commented Mar 12, 2020 •

edited

Loading

rchigvintsev Mar 17, 2020 •

edited

Loading