-
Notifications
You must be signed in to change notification settings - Fork 351
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.
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