Support Single Query Loading for queries with where clauses #1617

schauder · 2023-09-19T12:02:16Z

This PR depends on #1615 and the branch currently actually includes those changes.

mp911de

How far does Query work? Do we support top-level/embedded properties only or can we issue queries into one-to-one/map/list relations?

Other than that, left a few comments to address.

mp911de · 2023-09-20T13:05:07Z

spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/AggregateReader.java

@@ -89,6 +96,35 @@ public Iterable<T> findAllById(Iterable<?> ids) {
 		return jdbcTemplate.query(sqlGenerator.findAllById(), Map.of("ids", convertedIds), this::extractAll);
 	}

+	public Iterable<T> findAllBy(Query query) {


findAll and others should be rewritten to use this method. I'm also not sure why we always return Iterable.

Returning Iterable is kind of "inherited" from CrudRepository I guess.
How far should we go with switching to List? All the way up to SimpleJdbcRepository?
It could break inheritance for users extending one of the affected classes.
Although only in a mild way I guess, since of the few inheriting from one of the classes, very few will work with anything else than lists, I assume.

We should return List types in the Template API at least. Not everything needs to be used in the repository although almost all modules return List from their Simple…Repository.

Then let's just go all the way to SimpleRepository

I filed #1623 to keep track of that change.

mp911de · 2023-09-20T13:11:38Z

...ional/src/main/java/org/springframework/data/relational/core/sqlgeneration/SqlGenerator.java

@@ -28,5 +34,7 @@ public interface SqlGenerator {

 	String findAllById();

+	String findAllByCondition(BiFunction<Table, RelationalPersistentEntity, Condition> conditionSource);


The independence of SqlGenerator towards Condition is problematic as there is no control over binding order of parameters nor their names. Ideally, the creation of a statement returns something that encapsulates bindings and either the statement that can be rendered or the finally rendered statement.

I don't think I understand what you are aiming for here.
The link between bind parameters and their values is controlled by the QueryMapper and therefore completely outside the SqlGenerator.

What would we achieve by dragging this into the SqlGenerator?

mp911de · 2023-09-20T13:12:40Z

...jdbc/src/test/java/org/springframework/data/jdbc/testing/CombiningActiveProfileResolver.java

+ *
+ * @author Jens Schauder
+ */
+public class CombiningActiveProfileResolver implements ActiveProfilesResolver {


Probably easier to review once #1620 is merged and this PR is rebased on top of main.

The rebase is done.

mp911de · 2023-09-20T13:14:09Z

spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/QueryMapper.java

-			ValueFunction<Object> valueFunction = (ValueFunction<Object>) criteria.getValue();
-			Object value = valueFunction.apply(getEscaper(comparator));
-
-			mappedValue = convertValue(comparator, value, propertyField.getTypeHint());


The conversion is entirely gone. Please reinstate conversion by wrapping ValueFunction in another function that applies convertValue

There really never was a conversion. ValueFunction is always a ValueFunction<String> obtained using toString of whatever was passed in as an argument. It also should only be converted to String since it is used in LIKE expressions and nothing else.

I therefore don't really consider removing the call to the converter a problem.
Although I also agree, that we should probably implement proper conversion, instead of just calling toString. And maybe at the same time at least warn, when LIKE is used with a non String property.
But I'd rather do that in a separate issue.
What do you think?

One is how it is used. The other is the concept of abstractions. Moving such parts out of sight easily breaks the conceptual flow and then we end up with code that we don't understand. I also think that we have areas in our code that require much more cleanup than this one.

And maybe at the same time at least warn, when LIKE is used with a non String property.

I do not agree. One can have an Enum property and run a LIKE query handing in a String value.

I introduced conversion of ValueFunction in the correct order: first conversion, than escaping, for both JDBC and R2DBC.

schauder · 2023-09-22T14:12:01Z

How far does Query work? Do we support top-level/embedded properties only or can we issue queries into one-to-one/map/list relations?

Queries beyond those based on the top-level class are and will only be supported for Single Query Loading as long as they are formulated as
exists <subselect> or <id> in <subselect> constructs.

We currently don't support deep predicates for the default query strategy either.

The refactoring does not honour the distinction between the default escaper and the like escaper, but since ValueFunctions are only used in the context of LIKE operations I don't see the point. See #1601

Closes #1601

mp911de · 2023-09-26T07:18:25Z

exists or in constructs.
We currently don't support deep predicates for the default query strategy either.

Asking users to run a id IN <subselect> for a query that dives into a relation (one-to-one/one-to-many) is a too high barrier when using Where queries. While it can be a temporary solution, we should translate Where predicates pointing at relationships into proper exists/IN queries.

Simplify ValueFunction mapping. Remove invariants of findBy SQL generation in favor of the Condition-based variant. Reduce visibility.

schauder · 2023-09-26T08:35:20Z

While it can be a temporary solution, we should translate Where predicates pointing at relationships into proper exists/IN queries.

I agree, but think we might have to rethink the abstraction level on which Query is operating. Currently it is just a thin layer over SQL. But we might have to go to proper aggregate support as well.

See #1601 Original pull request: #1617

Closes #1601 Original pull request: #1617

Simplify ValueFunction mapping. Remove invariants of findBy SQL generation in favor of the Condition-based variant. Reduce visibility. Change return value of AggregateReader to List See #1601 Original pull request: #1617

See #1601 Original pull request: #1617

Simplify type and interface arrangement. See #1601 Original pull request: #1617

mp911de · 2023-09-26T12:42:12Z

That's merged now.

schauder requested a review from mp911de September 19, 2023 12:02

schauder force-pushed the issue/1601-where-clause branch from 45a1595 to b3759db Compare September 19, 2023 12:03

mp911de requested changes Sep 20, 2023

View reviewed changes

schauder added 6 commits September 22, 2023 16:19

1601 Single Query Loading with where clause - Prepare branch

cdb7f7c

Removed dialect dependency from QueryMapper.

95fb0c4

The refactoring does not honour the distinction between the default escaper and the like escaper, but since ValueFunctions are only used in the context of LIKE operations I don't see the point. See #1601

Add support for arbitrary where clauses in Single Query Loading.

1ddb157

Closes #1601

change return value of AggregateReader to List

1e63e31

introduced proper implementation of ValueFunction

d43740e

support conversion for ValueFunction

8fe6fda

schauder force-pushed the issue/1601-where-clause branch from b3759db to 8fe6fda Compare September 25, 2023 11:13

mp911de added the type: enhancement A general enhancement label Sep 26, 2023

mp911de linked an issue Sep 26, 2023 that may be closed by this pull request

Support Single Query Loading for template methods with arbitrary where clauses #1601

Closed

Polishing.

1ed34e8

Simplify ValueFunction mapping. Remove invariants of findBy SQL generation in favor of the Condition-based variant. Reduce visibility.

mp911de added 2 commits September 26, 2023 11:10

Add JMH benchmarks and optimizations to affected components.

83530f4

Polishing.

efd8052

mp911de pushed a commit that referenced this pull request Sep 26, 2023

Remove dialect dependency from QueryMapper.

6fb6110

See #1601 Original pull request: #1617

mp911de pushed a commit that referenced this pull request Sep 26, 2023

Add support for arbitrary where clauses in Single Query Loading.

0fdeaeb

Closes #1601 Original pull request: #1617

mp911de added a commit that referenced this pull request Sep 26, 2023

Add JMH benchmarks and optimizations to affected components.

7b27d0e

See #1601 Original pull request: #1617

mp911de added a commit that referenced this pull request Sep 26, 2023

Polishing.

8fa9e3e

Simplify type and interface arrangement. See #1601 Original pull request: #1617

mp911de added this to the 3.2 RC1 (2023.1.0) milestone Sep 26, 2023

mp911de closed this Sep 26, 2023

mp911de deleted the issue/1601-where-clause branch September 26, 2023 12:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Single Query Loading for queries with where clauses #1617

Support Single Query Loading for queries with where clauses #1617

schauder commented Sep 19, 2023

mp911de left a comment

mp911de Sep 20, 2023

schauder Sep 22, 2023

mp911de Sep 26, 2023

schauder Sep 26, 2023

mp911de Sep 26, 2023

mp911de Sep 20, 2023

schauder Sep 22, 2023

mp911de Sep 20, 2023

schauder Sep 25, 2023

mp911de Sep 20, 2023

schauder Sep 21, 2023

mp911de Sep 21, 2023

schauder Sep 25, 2023

schauder commented Sep 22, 2023

mp911de commented Sep 26, 2023

schauder commented Sep 26, 2023

mp911de commented Sep 26, 2023

		@@ -28,5 +34,7 @@ public interface SqlGenerator {

		String findAllById();

		String findAllByCondition(BiFunction<Table, RelationalPersistentEntity, Condition> conditionSource);

Support Single Query Loading for queries with where clauses #1617

Support Single Query Loading for queries with where clauses #1617

Conversation

schauder commented Sep 19, 2023

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

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

schauder commented Sep 22, 2023

mp911de commented Sep 26, 2023

schauder commented Sep 26, 2023

mp911de commented Sep 26, 2023