jdaugherty commented on code in PR #16505:
URL: https://github.com/apache/grails-core/pull/16505#discussion_r4178099393
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/PredicateGenerator.java:
##########
@@ -448,6 +453,38 @@ private Predicate handlePropertyCriterion(
throw new UnsupportedOperationException("Unsupported criterion: " +
pc.getClass().getName());
}
+ /**
+ * Returns a column of the entity's own table, whose table alias replaces
{@code {alias}} in a SQL restriction.
+ */
+ private static Expression<?> aliasColumn(From<?, ?> root,
GrailsHibernatePersistentEntity entity) {
+ PersistentProperty identity = entity.getIdentity();
+ if (identity == null) {
+ HibernatePersistentProperty[] compositeIdentity =
entity.getCompositeIdentity();
+ if (compositeIdentity == null || compositeIdentity.length == 0) {
+ throw new ConfigurationException("Cannot use sqlRestriction on
class [" + entity.getJavaClass().getName() + "] without an identifier");
+ }
+ identity = compositeIdentity[0];
+ }
+ Path<?> path = root.get(identity.getName());
+ if (identity instanceof Association<?> association &&
association.getAssociatedEntity() != null) {
+ path =
path.get(association.getAssociatedEntity().getIdentity().getName());
Review Comment:
**[P2] Handle composite identifiers on the associated entity too.**
`getIdentity()` is null when the associated entity itself has a composite
identifier. For example, with `Printing` identified by `['edition', 'code']`
and `Edition` identified by `['isbn', 'number']`, a saved row can be created
successfully, but `Printing.createCriteria().list {
sqlRestriction('{alias}.place = ?', ['City']) }` throws an NPE on this line. I
also reproduced the same failure with `sqlRestriction('place = ?', ['City'])`,
because alias-column resolution runs unconditionally even without `{alias}`.
Please resolve a usable owning-table column for this mapping, including
composite association targets, and cover both aliased and unaliased
restrictions.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/SqlRestriction.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.grails.orm.hibernate.query;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+import org.grails.datastore.mapping.query.Query;
+
+/**
+ * Criterion that restricts the results with a native SQL condition, created
by {@code sqlRestriction} in a
+ * criteria query. In the SQL, {@code {alias}} stands for the table alias of
the queried entity, and each
+ * {@code ?} for one of the values, which are bound as parameters.
+ *
+ * @param sql the SQL condition
+ * @param values the values of the {@code ?} placeholders, in order
+ * @since 8.0.0
+ */
+public record SqlRestriction(String sql, List<?> values) implements
Query.Criterion {
+
+ public SqlRestriction {
+ if (sql == null) {
+ throw new IllegalArgumentException("The SQL of a sqlRestriction
must not be null");
+ }
+ if (values == null) {
+ values = List.of();
+ }
+ List<Object> copy = new ArrayList<>(values.size());
+ for (Object value : values) {
+ if (value == null) {
+ throw new IllegalArgumentException("The values of a
sqlRestriction must not be null: " + sql);
+ }
+ copy.add(value instanceof CharSequence ? value.toString() : value);
+ }
+ long placeholders = sql.chars().filter(c -> c == '?').count();
+ if (placeholders != copy.size()) {
+ throw new IllegalArgumentException("The SQL of a sqlRestriction
has " + placeholders +
+ " ? placeholders but " + copy.size() + " values: " + sql);
Review Comment:
**[P2] Distinguish JDBC placeholders from literal question marks.** Counting
every `?` rejects valid SQL containing a quoted question mark. I reproduced
this through `createCriteria().list { sqlRestriction("{alias}.title like '%?'
and {alias}.pages > ?", [50]) }`: it throws `has 2 ? placeholders but 1
values`, although the SQL has only one bind parameter. This prevents existing
native restrictions containing such literals from working after migration.
Please use the same quote/comment-aware placeholder scan in both this
validation and `GrailsSqlRestrictionFunction.render()`; fixing only this count
would leave the renderer's `indexOf('?')` consuming the literal as a parameter
slot. Add a public-API regression test with a literal `?` before a real
placeholder.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
