Copilot commented on code in PR #15968: URL: https://github.com/apache/grails-core/pull/15968#discussion_r3562267292
########## grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/GormQuerySafetyWarnings.groovy: ########## @@ -0,0 +1,67 @@ +/* + * 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.datastore.gorm.query + +import groovy.transform.CompileStatic + +import java.util.concurrent.ConcurrentHashMap + +import org.slf4j.Logger + +@CompileStatic +final class GormQuerySafetyWarnings { + + private static final String GSTRING_VALUE_PLACEHOLDER = '${...}' + private static final Set<String> WARNED_GSTRING_QUERY_SHAPES = Collections.newSetFromMap(new ConcurrentHashMap<String, Boolean>()) + + private GormQuerySafetyWarnings() { + } + + static boolean warnIfGStringQuery(Logger logger, CharSequence query, String operation) { + if (!(query instanceof GString) || ((GString) query).values.length == 0) { + return false + } + + String queryShape = buildQueryShape((GString) query) + if (!logger.warnEnabled) { + return false + } + + String warningKey = "${operation}\n${queryShape}" + if (!WARNED_GSTRING_QUERY_SHAPES.add(warningKey)) { + return false + } Review Comment: WARNED_GSTRING_QUERY_SHAPES is an unbounded static set and `warnIfGStringQuery` builds `queryShape` even when warn logging is disabled. Over time (or in long-running processes), this can retain keys indefinitely and do unnecessary work on hot paths. Consider (a) checking `isWarnEnabled()` before building the shape/key and (b) adding a simple upper bound/eviction strategy for the warned-key set to avoid unbounded growth. ########## grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/HibernateGormStaticApi.groovy: ########## @@ -480,6 +481,8 @@ class HibernateGormStaticApi<D> extends GormStaticApi<D> { if (hints.isEmpty() && querySettings != null) { hints = querySettings.findAll { AvailableHints.getDefinedHints().contains(it.key) } } + String operation = isNative ? 'native SQL query' : (isUpdate ? 'executeUpdate' : 'find/executeQuery') + GormQuerySafetyWarnings.warnIfGStringQuery(log, hql, operation) Map<String, Object> coercedParams = namedParams?.collectEntries { k, v -> [k.toString(), v] } ?: [:] def ctx = HqlQueryContext.prepare(persistentEntity, hql, coercedParams, positionalParams, querySettings, hints, isNative, isUpdate) Review Comment: This change introduces new observable behavior (a one-time warning when a `GString` query is executed) but there is no test in the Hibernate GORM module asserting the warning is emitted once and does not include interpolated values. There are already `HibernateGormStaticApiSpec` tests executing a `GString` query (e.g. the injection-safe test) that could be extended to capture and assert the warning log output. ########## grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/GormQuerySafetyWarnings.groovy: ########## @@ -0,0 +1,67 @@ +/* + * 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.datastore.gorm.query + +import groovy.transform.CompileStatic + +import java.util.concurrent.ConcurrentHashMap + +import org.slf4j.Logger + +@CompileStatic +final class GormQuerySafetyWarnings { + + private static final String GSTRING_VALUE_PLACEHOLDER = '${...}' + private static final Set<String> WARNED_GSTRING_QUERY_SHAPES = Collections.newSetFromMap(new ConcurrentHashMap<String, Boolean>()) + + private GormQuerySafetyWarnings() { + } + + static boolean warnIfGStringQuery(Logger logger, CharSequence query, String operation) { + if (!(query instanceof GString) || ((GString) query).values.length == 0) { + return false + } + + String queryShape = buildQueryShape((GString) query) + if (!logger.warnEnabled) { + return false + } + + String warningKey = "${operation}\n${queryShape}" + if (!WARNED_GSTRING_QUERY_SHAPES.add(warningKey)) { + return false + } + + logger.warn('GString-interpolated HQL passed to [{}]. GORM binds interpolated values as query parameters, but explicit named parameters are recommended for query safety and readability. Query shape: [{}]', operation, queryShape) Review Comment: The warning message hard-codes "HQL", but this helper is also invoked for `isNative` queries (native SQL) via `HibernateGormStaticApi`. Using a neutral term avoids a confusing/incorrect log message for native queries. -- 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]
