Skip to content
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

DATAJDBC-1953 Introduced DialectCriteriaCondition interface #1981

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

DATAJDBC-1953 Introduced DialectCriteriaCondition interface #1981

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions spring-data-r2dbc/src/test/java/org/springframework/data/r2dbc/query/CriteriaUnitTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.assertj.core.api.SoftAssertions;
import org.junit.jupiter.api.Test;

import org.springframework.data.relational.core.dialect.condition.Postgres;
import org.springframework.data.relational.core.query.Criteria;
import org.springframework.data.relational.core.sql.SqlIdentifier;

Expand Down
2 changes: 1 addition & 1 deletion ...ational/src/main/java/org/springframework/data/relational/core/dialect/OracleDialect.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
* An SQL dialect for Oracle.
*
* @author Jens Schauder
* @author Mikahil Polivakha
* @author Mikhail Polivakha
* @since 2.1
*/
public class OracleDialect extends AnsiDialect {
Expand Down
18 changes: 18 additions & 0 deletions .../org/springframework/data/relational/core/dialect/condition/DialectCriteriaCondition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package org.springframework.data.relational.core.dialect.condition;

import org.springframework.data.relational.core.query.Criteria;

/**
* This interface represents dialect specific conditions used in WHERE causes built by {@link Criteria}.
*
* @author Mikhail Polivakha
*/
public interface DialectCriteriaCondition {
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit too little, especially given the later variant of concatenating values into SQL as it opens loopholes for SQL injection.

An operator should have a method accepting a MappingContext and likely a PersistentProperty that applies to it and an abstraction like R2DBC's BindMarkers. In return, we need a tuple of bind markers associated with their value and the SQL fragment. Maybe even a slimmer variant of BindTarget (without the binding identifier) would suit fit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mp911de Am I correct that the desired interface signature should look like this:

public interface DialectCriteriaCondition {

   BindTarget render(MappingContext context, PersistentProperty property);
}

If so, then I have a couple of questions here:

  1. Criteria API by itself is an open loophole to SQL injection, like it is how it is designed, since we're build query literally by concatinating values into SQL. Maybe if we're going to address this problem, then we can do it in a separate issue, what do you think?
  2. The Criteria API is a top level API designed for end users, do we want to expose MappingContext, PersistentProperty an other abstraction? They are public, but it does not seem to be desirable to expose them to users.
  3. What is the custom operator is not performed on a single PersistentProperty?

Copy link
Member

Choose a reason for hiding this comment

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

  1. I tend to disagree. Combinator and Comparator enums are closed types that do not allow for extensions. In return, Spring Data Relational controls what enum values are available to ensure we only specify safe operators. At some point, we have to translate criteria elements into a String. Specifically referring to the linked method, rendering of criteria elements inside of Criteria is limited to toString() usage. The actual mapping of CriteriaDefinition to Condition (the SQL DSL) happens in QueryMapper.
  2. It depends on how close we want to bring MappingContext towards users. Ideally, we have an API that allows building comparators/query elements without leading users into Spring Data's Mapping infrastructure. Something like Postgres.arrayContains("first", "second") where the resulting value is used in Criteria so that users don't have to interact with methods exposed by the result object of Postgres.arrayContains(…). We have a similar arrangement in MongoDB with AggregationOperation. AggregationOperationContext handles some abstraction for mapping and I think we might end up doing something similar.
  1. What is the custom operator is not performed on a single PersistentProperty?

This also crossed my mind. There could be variants where two functions compare against each other or the property is specified on the right side. So far, we have to consider this in some way. Let's however start from a simpler side first so that we can implement a first proof of concept before digging into the more complex things. It is typically easier to build something (starting from the side how things should be expressed in code) and then we can iterate on it towards a variant that handles an increasing number of usecases.


/**
* Render a vendor-specific part of the SQL condition.
*
* @return the rendered part of the SQL statement
*/
String render();
}
67 changes: 67 additions & 0 deletions ...al/src/main/java/org/springframework/data/relational/core/dialect/condition/Postgres.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
package org.springframework.data.relational.core.dialect.condition;

import org.springframework.data.relational.core.dialect.PostgresDialect;

/**
* {@link DialectCriteriaCondition DialectCriteriaConditions} that are specific to {@link PostgresDialect PostgreSQL Dialect}
*
* @author Mikhail Polivakha
*/
public class Postgres {

/**
* Creates a condition that checks that the assumed column of an {@link java.sql.Array} type
* contains an array of any values
*
* @param values array that assumed column should contain
* @return crafted {@link DialectCriteriaCondition}
*/
public static DialectCriteriaCondition arrayContains(Object... values) {
return () -> "@> ARRAY[%s]".formatted(toLiterals(false, values));
}

/**
* Creates a condition that checks that the assumed column of an {@link java.sql.Array} type
* contains an array of {@link String} values.
*
* @param values array of {@link String String} that assumed column should contain
* @return crafted {@link DialectCriteriaCondition}
*/
public static DialectCriteriaCondition arrayContains(String... values) {
return () -> "@> ARRAY[%s]::text[]".formatted(toLiterals(true, values));
}

/**
* Creates a condition that checks that the assumed column of an {@link java.sql.Array} type
* contains an array of a single {@link String} value.
*
* @param value array of {@link String String} that assumed value should contain
* @return crafted {@link DialectCriteriaCondition}
*/
public static DialectCriteriaCondition arrayContains(String value) {
return arrayContains(new String[]{value});
}

@SafeVarargs
private static <T> String toLiterals(boolean quoted, T... values) {
StringBuilder result = new StringBuilder();
for (int i = 0; i < values.length; i++) {
T value = values[i];

if (value != null) {
if (quoted) {
result.append('\'').append(value).append('\'');
} else {
result.append(value);
}
} else {
result.append("NULL");
}

if (i != values.length - 1) {
result.append(",");
}
}
return result.toString();
}
}
Loading
Loading