-
Notifications
You must be signed in to change notification settings - Fork 355
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.
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
base: main
Are you sure you want to change the base?
Conversation
Thanks for the contribution. We need a Developer Certificate of Origin (DCO) for new PRs. For details about how and why see https://spring.io/blog/2025/01/06/hello-dco-goodbye-cla-simplifying-contributions-to-spring |
74172af
to
6692442
Compare
Hey @schauder, yeah, I am aware of this, I've added the sign off. |
I don't think this is a good design direction. We should have rather some way to express predicates in a way that allows to determine bindings, mapping of values and to generate SQL text. I'll leave some thoughts in the original ticket #1953 |
1f54016
to
295c9f5
Compare
Signed-off-by: mipo256 <[email protected]>
295c9f5
to
ce5f2b9
Compare
I've revisited the solution according to the comment left in here: #1953 (comment) The usage sample for the issue looks like this:
This would render into the following:
I've introduced a new interface - Also, the API, as it is now created, allows users to define their own
|
* | ||
* @author Mikhail Polivakha | ||
*/ | ||
public interface DialectCriteriaCondition { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- 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?
- The Criteria API is a top level API designed for end users, do we want to expose
MappingContext
,PersistentProperty
an other abstraction? They arepublic
, but it does not seem to be desirable to expose them to users. - What is the custom operator is not performed on a single
PersistentProperty
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I tend to disagree.
Combinator
andComparator
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 ofCriteria
is limited totoString()
usage. The actual mapping ofCriteriaDefinition
toCondition
(the SQL DSL) happens inQueryMapper
. - 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 likePostgres.arrayContains("first", "second")
where the resulting value is used inCriteria
so that users don't have to interact with methods exposed by the result object ofPostgres.arrayContains(…)
. We have a similar arrangement in MongoDB withAggregationOperation
.AggregationOperationContext
handles some abstraction for mapping and I think we might end up doing something similar.
- 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.
This PR addresses the following issue #1953.
The core of the problem, I think, is that we had the
Comparator
as a Javaenum
, which is not extendable. I thought that, from the future perspective, as @schauder pointed out, we cannot really foresee all the possible conditions that can be used. Arrays in PostgreSQL is just a very concrete case.So, the issue that is pointed out by the author, I think, should be solved in the following way, from the API's perspective:
and that would render to the SQL that we expect:
This approach allows users to customize the condition in any possible way for any given column. This is achieved by introducing the
Comparator
interface, rather than the Comparatorenum
.P.S: I've tried to minimize the backwards incompatibility as much as possible, but still, some migration would be needed here. Therefore, I guess, we need to assign a
breaking-change
tag