Copilot commented on code in PR #15772:
URL: https://github.com/apache/grails-core/pull/15772#discussion_r3484189047
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/validation/jakarta/GormValidatorAdapter.groovy:
##########
@@ -46,6 +50,17 @@ class GormValidatorAdapter extends SpringValidatorAdapter {
thisValidator = targetValidator
}
+ @Override
+ void validate(Object obj, Errors errors, boolean cascade) {
+ println "GormValidatorAdapter.validate called with cascade=${cascade}"
+ CASCADE_VALIDATION.set(cascade)
Review Comment:
`validate(Object, Errors, boolean)` currently prints to stdout on every
validation call. This will be noisy in production and in test output, and it
bypasses the existing logging system.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/jdbc/schema/DefaultSchemaHandler.groovy:
##########
@@ -56,6 +56,7 @@ class DefaultSchemaHandler implements SchemaHandler {
@Override
void useSchema(Connection connection, String name) {
String useStatement = String.format(useSchemaStatement,
quoteName(connection, name))
+ System.err.println "Executing SQL: ${useStatement}"
log.debug('Executing SQL Set Schema Statement: {}', useStatement)
Review Comment:
Avoid writing SQL statements directly to stderr. This is very noisy during
tests and production usage, and bypasses the logger already present in this
class.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/jdbc/schema/DefaultSchemaHandler.groovy:
##########
@@ -64,12 +65,14 @@ class DefaultSchemaHandler implements SchemaHandler {
@Override
void useDefaultSchema(Connection connection) {
+ System.err.println "Executing SQL: useDefaultSchema
(${defaultSchemaName})"
useSchema(connection, defaultSchemaName)
}
@Override
void createSchema(Connection connection, String name) {
String schemaCreateStatement = String.format(createSchemaStatement,
quoteName(connection, name))
+ System.err.println "Executing SQL: ${schemaCreateStatement}"
log.debug('Executing SQL Create Schema Statement: {}',
schemaCreateStatement)
Review Comment:
Avoid writing SQL statements directly to stderr in `createSchema`. This
bypasses the existing structured logger and will spam test/prod output.
##########
grails-testing-support-datamapping/src/main/groovy/org/grails/testing/gorm/spock/DataTestCleanupInterceptor.groovy:
##########
@@ -38,11 +43,38 @@ class DataTestCleanupInterceptor implements
IMethodInterceptor {
}
void cleanupDataTest(DataTest testInstance) {
+ SimpleMapDatastore simpleDatastore =
testInstance.applicationContext.getBean(SimpleMapDatastore)
+ unbindNonDefaultConnectionSessions(simpleDatastore)
if (testInstance.currentSession != null) {
testInstance.currentSession.disconnect()
DatastoreUtils.unbindSession(testInstance.currentSession)
}
- SimpleMapDatastore simpleDatastore =
testInstance.applicationContext.getBean(SimpleMapDatastore)
simpleDatastore.clearData()
}
+
+ /**
+ * Symmetric to {@code
DataTestSetupInterceptor.bindNonDefaultConnectionSessions}: disconnect and
+ * unbind the per-connection sessions bound for non-default datasources so
they do not leak into
+ * the next feature method on the same thread.
+ */
+ private static void unbindNonDefaultConnectionSessions(SimpleMapDatastore
datastore) {
+ for (ConnectionSource connectionSource :
datastore.connectionSources.allConnectionSources) {
+ String name = connectionSource.name
+ if (ConnectionSource.DEFAULT == name) {
+ continue
+ }
+ Datastore connectionDatastore =
datastore.getDatastoreForConnection(name)
+ if (connectionDatastore == null) {
+ continue
+ }
+ SessionHolder holder = (SessionHolder)
TransactionSynchronizationManager.getResource(connectionDatastore)
+ if (holder != null) {
+ Session session = holder.session
+ if (session != null) {
+ session.disconnect()
+ DatastoreUtils.unbindSession(session)
+ }
+ }
Review Comment:
In `unbindNonDefaultConnectionSessions`, calling `session.disconnect()`
before `DatastoreUtils.unbindSession(session)` is redundant and can make
cleanup brittle if `disconnect()` throws (since `unbindSession` already handles
close/disconnect and logs exceptions). It also results in a double-disconnect
(manual + DatastoreUtils.closeSession).
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/jdbc/schema/DefaultSchemaHandler.groovy:
##########
@@ -64,12 +65,14 @@ class DefaultSchemaHandler implements SchemaHandler {
@Override
void useDefaultSchema(Connection connection) {
+ System.err.println "Executing SQL: useDefaultSchema
(${defaultSchemaName})"
useSchema(connection, defaultSchemaName)
}
Review Comment:
Avoid writing to stderr in `useDefaultSchema`. The `useSchema` method
already logs via SLF4J, and stderr output can overwhelm test logs.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/ThreadLocalSessionResolver.groovy:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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.mapping.core
+
+import java.util.concurrent.ConcurrentHashMap
+
+import groovy.transform.CompileStatic
+
+/**
+ * A default thread-bound SessionResolver
+ *
+ * @author borinquenkid
+ * @since 8.0
+ */
+@CompileStatic
+class ThreadLocalSessionResolver<S extends Session> implements
SessionResolver<S> {
+
+ private final ThreadLocal<S> currentSession = new ThreadLocal<>()
+ private final Map<String, S> qualifiedSessions = new ConcurrentHashMap<>()
+
+ @Override
+ S resolve() {
+ return currentSession.get()
+ }
+
+ @Override
+ S resolve(String qualifier) {
+ return qualifiedSessions.get(qualifier)
+ }
+
+ @Override
+ void bind(S session) {
+ currentSession.set(session)
+ // Note: In a production scenario, we'd need to link the session's
datastore qualifier here.
+ }
+
+ void bind(String qualifier, S session) {
+ qualifiedSessions.put(qualifier, session)
+ }
+
+ @Override
+ void unbind() {
+ currentSession.remove()
+ }
+
+ void unbind(String qualifier) {
+ qualifiedSessions.remove(qualifier)
+ }
Review Comment:
`ThreadLocalSessionResolver` is described as "thread-bound", but
`qualifiedSessions` is a shared `ConcurrentHashMap`, which can leak sessions
across threads. Also, `unbind()` currently doesn't clear any qualified state,
so any qualified bindings would persist beyond the thread lifecycle.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/jdbc/MultiTenantConnection.groovy:
##########
@@ -45,12 +45,6 @@ class MultiTenantConnection implements Connection {
@Override
void close() throws SQLException {
- try {
- if (!isClosed()) {
- schemaHandler.useDefaultSchema(this)
- }
- } finally {
- target.close()
- }
+ target.close()
}
Review Comment:
`MultiTenantConnection`'s class-level documentation says it restores the
default schema before returning the connection to the pool, but `close()` now
just closes the target connection. Without resetting the schema, pooled
connections can leak a tenant schema into subsequent borrowers.
--
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]
