jamesfredley commented on code in PR #16505:
URL: https://github.com/apache/grails-core/pull/16505#discussion_r4178107728
##########
grails-data-hibernate7/core/src/main/groovy/grails/orm/HibernateCriteriaBuilder.java:
##########
@@ -1112,6 +1113,32 @@ public Criteria sizeLt(String propertyName, int size) {
return this;
}
+ /**
+ * Restricts the results with a native SQL condition. {@code {alias}} in
the SQL stands for the table alias of
+ * the queried entity.
+ *
+ * @param sqlRestriction the SQL condition
+ * @return this criteria
+ */
+ public org.grails.datastore.mapping.query.api.Criteria
sqlRestriction(String sqlRestriction) {
+ return sqlRestriction(sqlRestriction, Collections.emptyList());
+ }
+
+ /**
+ * Restricts the results with a native SQL condition whose {@code ?}
placeholders are bound to the given values.
+ * {@code {alias}} in the SQL stands for the table alias of the queried
entity.
+ *
+ * @param sqlRestriction the SQL condition
+ * @param values the values of the {@code ?} placeholders, in order, none
of them {@code null}
+ * @return this criteria
+ * @throws IllegalArgumentException if the number of {@code ?}
placeholders differs from the number of values,
+ * or a value is {@code null}
+ */
+ public org.grails.datastore.mapping.query.api.Criteria
sqlRestriction(String sqlRestriction, List<?> values) {
+ hibernateQuery.add(new SqlRestriction(sqlRestriction, values));
Review Comment:
This always adds the restriction to the builder's current `hibernateQuery`.
An association nested inside a junction does not update that query.
`HibernateQuery.or` / `not` / `and` delegate to `DetachedCriteria`, and that
association `methodMissing` changes closure delegates without changing
`hibernateQuery.detachedCriteria`. Detached criteria have no `sqlRestriction`,
so the call falls back here and lands on the enclosing criteria.
For `or { chapters { sqlRestriction('{alias}.title = ?', ['Epilogue']) };
eq('title', 'Dune') }` the SQL criterion targets the book, and the chapter
operand stays empty. Route the restriction to the active association criteria,
and add public DSL regressions for association blocks inside `or`, `not`, and
nested associations.
##########
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:
`getIdentity()` is null when the associated entity itself has a composite
identifier, so this throws. `CompositeIdWithDeepOneToManyMappingSpec` is
exactly this mapping: `Child` starts with `parent`, and `Parent` has a
composite id. `aliasColumn` is also called when the SQL has no `{alias}`, so a
constant restriction fails the same way.
Resolve an owning-table column without assuming the associated identifier is
singular. Cover that mapping through the public DSL, with and without `{alias}`.
##########
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();
Review Comment:
Every `?` is counted, including quoted text and comments.
`sqlRestriction("{alias}.title like '%?%'")` is valid SQL with zero parameters
and is rejected as missing a value. Supplying a dummy value instead makes the
renderer at `GrailsSqlRestrictionFunction` bind inside the quoted literal.
Hibernate 5's GORM path passes the SQL and values through to
`Restrictions.sqlRestriction` without this character count.
Use one SQL-aware placeholder scan in both validation and rendering, and
test a literal question mark next to a real parameter.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/GrailsSqlRestrictionFunction.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * 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.List;
+
+import org.hibernate.metamodel.model.domain.ReturnableType;
+import
org.hibernate.query.sqm.function.AbstractSqmSelfRenderingFunctionDescriptor;
+import org.hibernate.query.sqm.produce.function.StandardArgumentsValidators;
+import
org.hibernate.query.sqm.produce.function.StandardFunctionReturnTypeResolvers;
+import org.hibernate.sql.ast.SqlAstTranslator;
+import org.hibernate.sql.ast.spi.SqlAppender;
+import org.hibernate.sql.ast.tree.SqlAstNode;
+import org.hibernate.sql.ast.tree.expression.ColumnReference;
+import org.hibernate.sql.ast.tree.expression.Expression;
+import org.hibernate.sql.ast.tree.expression.Literal;
+import org.hibernate.sql.ast.tree.expression.SqlTuple;
+import org.hibernate.type.StandardBasicTypes;
+import org.hibernate.type.spi.TypeConfiguration;
+
+/**
+ * Renders a {@link SqlRestriction} as a predicate. The arguments are the SQL
condition as a literal, a column of
+ * the queried entity whose table alias replaces {@code {alias}}, and the
value of each {@code ?} placeholder.
+ *
+ * @since 8.0.0
+ */
+public class GrailsSqlRestrictionFunction extends
AbstractSqmSelfRenderingFunctionDescriptor {
+
+ public static final String NAME = "grails_sql_restriction";
+
+ static final String ALIAS_PLACEHOLDER = "{alias}";
+
+ public GrailsSqlRestrictionFunction(TypeConfiguration typeConfiguration) {
+ super(
+ NAME,
+ StandardArgumentsValidators.min(2),
+ StandardFunctionReturnTypeResolvers.invariant(
+
typeConfiguration.getBasicTypeRegistry().resolve(StandardBasicTypes.BOOLEAN)),
+ null);
+ }
+
+ @Override
+ public boolean isPredicate() {
+ return true;
+ }
+
+ @Override
+ public void render(
+ SqlAppender sqlAppender,
+ List<? extends SqlAstNode> arguments,
+ ReturnableType<?> returnType,
+ SqlAstTranslator<?> walker) {
+ if (!(arguments.get(0) instanceof Literal literal) ||
!(literal.getLiteralValue() instanceof String sql)) {
+ throw new IllegalArgumentException("The first argument of " + NAME
+ " must be the SQL as a literal");
+ }
+ if (sql.contains(ALIAS_PLACEHOLDER)) {
+ sql = sql.replace(ALIAS_PLACEHOLDER, tableAlias(arguments.get(1)));
+ }
+ int index = 0;
+ for (int i = 2; i < arguments.size(); i++) {
+ int placeholder = sql.indexOf('?', index);
+ if (placeholder == -1) {
+ throw new IllegalArgumentException("The SQL restriction has
fewer ? placeholders than values: " + sql);
+ }
+ sqlAppender.append(sql, index, placeholder);
+ arguments.get(i).accept(walker);
+ index = placeholder + 1;
+ }
+ if (sql.indexOf('?', index) != -1) {
+ throw new IllegalArgumentException("The SQL restriction has more ?
placeholders than values: " + sql);
+ }
+ sqlAppender.append(sql, index, sql.length());
Review Comment:
The fragment is appended with no parentheses. Combined with another
criterion, `A or B` becomes `A or B and C` rather than `(A or B) and C`.
Hibernate treats the function as an opaque predicate and does not add the
grouping. Reproduced on Hibernate 7.4.10: a restriction matching either Dune or
Emma, combined with an id criterion for Emma, returned both books.
Parenthesize the whole fragment. The `indexOf('?')` scan above has the same
quoted-placeholder bug as `SqlRestriction`.
--
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]
