jdaugherty commented on code in PR #15779: URL: https://github.com/apache/grails-core/pull/15779#discussion_r3555784361
########## grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/ThreadLocalSessionResolver.groovy: ########## @@ -0,0 +1,65 @@ +/* + * 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 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 ThreadLocal<Map<String, S>> qualifiedSessions = ThreadLocal.<Map<String, S>> withInitial { new HashMap<String, S>() } + + @Override + S resolve() { + return currentSession.get() + } + + @Override + S resolve(String qualifier) { + return qualifiedSessions.get().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. Review Comment: This placeholder comment ("In a production scenario, we’d need to...") signals the design is incomplete and should not ship as the default core implementation. A single value also cannot support nested scopes: `bind(A); bind(B); unbind()` leaves no current session instead of restoring A, while the existing `SessionHolder` already has push/pop semantics. Qualified bindings have the same replacement problem, and plain `unbind()` unexpectedly clears every qualified binding as well. Prefer composing the default resolver over TSM/`SessionHolder`; if an independent context is genuinely required, it needs scoped stack semantics and automatic cleanup tied to session close. ########## grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/SessionResolver.groovy: ########## @@ -0,0 +1,44 @@ +/* + * 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 groovy.transform.CompileStatic + +/** + * Resolver for sessions in the current context (thread, tenant, etc) + * + * @author borinquenkid + * @since 8.0 + */ +@CompileStatic +interface SessionResolver<S extends Session> { + + /** Resolves the current session based on current context (thread, tenant, etc) */ + S resolve() + + /** Resolves a session for a specific qualifier/tenant */ + S resolve(String qualifier) Review Comment: Asymmetric abstraction: `resolve(String qualifier)` is on the interface, but the corresponding `bind(String, S)` / `unbind(String)` exist only on `ThreadLocalSessionResolver`. Any code that manages qualified sessions must downcast to the concrete class, which defeats the point of the interface. Either the full qualified lifecycle belongs on `SessionResolver`, or qualified resolution should be dropped from it until PR 2/3 actually needs it. ########## grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/DatastoreUtils.java: ########## @@ -116,11 +116,15 @@ public static Session doGetSession(Datastore datastore, boolean allowCreate) { Assert.notNull(datastore, "No Datastore specified"); + Session session = datastore.getSessionResolver().resolve(); Review Comment: This creates two independent sources of truth and gives the new one precedence over Spring transaction state. If the resolver holds session R while `DatastoreTransactionManager` has bound transactional session T, every GORM lookup returns R while commit/rollback still operates on T. The same precedence defeats `executeWithNewSession`: that method pushes its new session only into `SessionHolder`, so ambient lookups inside the callback still return R rather than the callback session. Nothing in this PR or its downstream PR currently binds the resolver, so these interactions are also untested. Please keep one authoritative session stack and make the resolver compose/delegate to it rather than bypassing transaction synchronization, validation, and `allowCreate` semantics. ########## grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/Datastore.java: ########## @@ -39,6 +39,11 @@ */ public interface Datastore extends ServiceRegistry { + /** + * @return The session resolver for this datastore + */ + SessionResolver getSessionResolver(); Review Comment: Adding an abstract method to the `Datastore` interface breaks every external implementation. Since the goal is decoupling session lookup from a concrete `Datastore`, a `default` method returning a TSM-backed resolver would preserve compatibility *and* give all implementations correct behavior for free — only datastores that need different resolution would override it. As written, any `Datastore` impl that returns `null` here causes an NPE in `DatastoreUtils.doGetSession`. ########## grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/DatastoreUtils.java: ########## @@ -361,13 +365,65 @@ public static void execute(final Datastore datastore, final VoidSessionCallback } } + /** + * Execute the given callback with a new session, regardless of whether an existing session is present + * @param datastore The datastore + * @param callback The callback + * @param <T> The return type + * @return The result of the callback + */ + public static <T> T executeWithNewSession(Datastore datastore, SessionCallback<T> callback) { + Session session = bindNewSession(datastore.connect()); + try { + return callback.doInSession(session); + } + finally { + SessionHolder sessionHolder = (SessionHolder) TransactionSynchronizationManager.getResource(datastore); + if (sessionHolder != null) { + sessionHolder.removeSession(session); + if (sessionHolder.isEmpty()) { + TransactionSynchronizationManager.unbindResource(datastore); + } + } + closeSessionOrRegisterDeferredClose(session, datastore); + } + } + + /** + * Execute the given callback with a new session, regardless of whether an existing session is present + * @param datastore The datastore + * @param callback The callback + */ + public static void executeWithNewSession(Datastore datastore, VoidSessionCallback callback) { Review Comment: This is a copy-paste of the generic overload above, including the whole `finally` block. The void variant can delegate: ```java public static void executeWithNewSession(Datastore datastore, VoidSessionCallback callback) { executeWithNewSession(datastore, session -> { callback.doInSession(session); return null; }); } ``` One body to maintain when the unbind/close logic inevitably changes. ########## grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/DatastoreUtils.java: ########## @@ -361,13 +365,65 @@ public static void execute(final Datastore datastore, final VoidSessionCallback } } + /** + * Execute the given callback with a new session, regardless of whether an existing session is present + * @param datastore The datastore + * @param callback The callback + * @param <T> The return type + * @return The result of the callback + */ + public static <T> T executeWithNewSession(Datastore datastore, SessionCallback<T> callback) { + Session session = bindNewSession(datastore.connect()); + try { + return callback.doInSession(session); + } + finally { + SessionHolder sessionHolder = (SessionHolder) TransactionSynchronizationManager.getResource(datastore); + if (sessionHolder != null) { + sessionHolder.removeSession(session); + if (sessionHolder.isEmpty()) { + TransactionSynchronizationManager.unbindResource(datastore); + } + } + closeSessionOrRegisterDeferredClose(session, datastore); + } + } + + /** + * Execute the given callback with a new session, regardless of whether an existing session is present + * @param datastore The datastore + * @param callback The callback + */ + public static void executeWithNewSession(Datastore datastore, VoidSessionCallback callback) { + Session session = bindNewSession(datastore.connect()); + try { + callback.doInSession(session); + } + finally { + SessionHolder sessionHolder = (SessionHolder) TransactionSynchronizationManager.getResource(datastore); + if (sessionHolder != null) { + sessionHolder.removeSession(session); + if (sessionHolder.isEmpty()) { + TransactionSynchronizationManager.unbindResource(datastore); + } + } + closeSessionOrRegisterDeferredClose(session, datastore); + } + } + /** * Bind the session to the thread with a SessionHolder keyed by its Datastore. * @param session the session * @return the session (for method chaining) */ public static Session bindSession(final Session session) { - TransactionSynchronizationManager.bindResource(session.getDatastore(), new SessionHolder(session)); + SessionHolder sessionHolder = (SessionHolder) TransactionSynchronizationManager.getResource(session.getDatastore()); + if (sessionHolder == null) { + TransactionSynchronizationManager.bindResource(session.getDatastore(), new SessionHolder(session)); + } + else { + sessionHolder.addSession(session); Review Comment: Semantic change worth calling out in the PR description: previously a second `bindSession` for the same datastore threw `IllegalStateException` from `TransactionSynchronizationManager.bindResource`, which was fail-fast leak detection. Now double-binds silently stack sessions, and in the creator overload below the second creator is silently dropped (the new spec even asserts this). If stacking is required for the registry work, consider a distinctly named method (e.g. reuse the existing `bindNewSession`) so existing callers keep their fail-fast behavior. ########## grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/AbstractDatastore.java: ########## @@ -55,12 +63,51 @@ @SuppressWarnings({"rawtypes", "unchecked"}) public abstract class AbstractDatastore implements Datastore, StatelessDatastore, ServiceRegistry { protected static final Logger LOG = LoggerFactory.getLogger(AbstractDatastore.class); + + private static final class DefaultApplicationEventPublisher implements ApplicationEventPublisher { + private final List<ApplicationListener> listeners = new CopyOnWriteArrayList<>(); + + @Override + public void publishEvent(ApplicationEvent event) { + publishEvent((Object) event); + } + + @Override + public void publishEvent(Object event) { + ApplicationEvent applicationEvent = (event instanceof ApplicationEvent) ? + (ApplicationEvent) event : + new PayloadApplicationEvent(this, event); + ResolvableType eventType = ResolvableType.forInstance(applicationEvent); + for (ApplicationListener listener : listeners) { + GenericApplicationListenerAdapter adapter = new GenericApplicationListenerAdapter(listener); Review Comment: This hand-rolled publisher still does not implement Spring listener-selection semantics: it checks `supportsEventType` but not `supportsSourceType`, and it ignores Spring ordering. Existing datastore listeners use source filtering, so the fallback can invoke a listener for a source it explicitly rejects. It also allocates a new adapter for every listener on every event. Rather than partially reproducing Spring dispatch, compose with `SimpleApplicationEventMulticaster`, which already provides type/source filtering, ordering, caching, and thread-safe listener management. ########## grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/AbstractDatastore.java: ########## @@ -122,6 +174,36 @@ public void destroy() { public void setApplicationContext(ApplicationContext ctx) { applicationContext = ctx; + if (ctx instanceof ApplicationEventPublisher) { + this.applicationEventPublisher = (ApplicationEventPublisher) ctx; + } + else if (ctx == null && !(this.applicationEventPublisher instanceof DefaultApplicationEventPublisher)) { + this.applicationEventPublisher = new DefaultApplicationEventPublisher(); + } + } + + public void setApplicationEventPublisher(ApplicationEventPublisher applicationEventPublisher) { + this.applicationEventPublisher = applicationEventPublisher; + } + + /** + * Adds an application listener to the datastore + * @param listener The listener + */ + public void addApplicationListener(ApplicationListener<?> listener) { + if (applicationEventPublisher instanceof ConfigurableApplicationContext) { + ((ConfigurableApplicationContext) applicationEventPublisher).addApplicationListener(listener); + } else if (applicationEventPublisher instanceof DefaultApplicationEventPublisher) { + ((DefaultApplicationEventPublisher) applicationEventPublisher).addApplicationListener(listener); + } + else { + try { + Method method = applicationEventPublisher.getClass().getMethod("addApplicationListener", ApplicationListener.class); Review Comment: This listener path is disconnected from the publisher real datastores use. SimpleMap, Mongo, Neo4j, and Hibernate keep their own `eventPublisher` and override `getApplicationEventPublisher()`, so events publish through the subclass field while this inherited method registers against the unused superclass field. The reflective fallback also silently drops registration for any valid `ApplicationEventPublisher` without an unrelated `addApplicationListener` method. Please establish one composed publisher path: datastore-owned listener state/multicaster plus an optional outbound publisher delegate, rather than parallel fields and reflective capability detection. ########## grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingSupport.groovy: ########## @@ -78,8 +78,17 @@ class DirtyCheckingSupport { PersistentCollection coll = (PersistentCollection) value if (coll.isInitialized()) { if (coll.isDirty()) return true + for (Object item in (Collection) coll) { Review Comment: This turns association dirty-checking from O(collections) into O(total elements) — a deep scan of every initialized collection on every check — and changes semantics (a parent now reports dirty children transitively). Both may be intended, but this is unrelated to the SessionResolver infrastructure this PR describes, and it ships with no tests. Suggest splitting it out with its own tests and perf justification. ########## grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/transactions/CustomizableRollbackTransactionAttribute.java: ########## @@ -51,38 +52,48 @@ public CustomizableRollbackTransactionAttribute(int propagationBehavior, List<Ro super(propagationBehavior, rollbackRules); } - public CustomizableRollbackTransactionAttribute(org.springframework.transaction.interceptor.TransactionAttribute other) { + public CustomizableRollbackTransactionAttribute(TransactionAttribute other) { super(); - setPropagationBehavior(other.getPropagationBehavior()); - setIsolationLevel(other.getIsolationLevel()); - setTimeout(other.getTimeout()); - setReadOnly(other.isReadOnly()); - setName(other.getName()); + copyFrom(other); } public CustomizableRollbackTransactionAttribute(TransactionDefinition other) { super(); - setPropagationBehavior(other.getPropagationBehavior()); - setIsolationLevel(other.getIsolationLevel()); - setTimeout(other.getTimeout()); - setReadOnly(other.isReadOnly()); - setName(other.getName()); + copyFrom(other); } public CustomizableRollbackTransactionAttribute(CustomizableRollbackTransactionAttribute other) { - this((RuleBasedTransactionAttribute) other); + super(); + copyFrom(other); } public CustomizableRollbackTransactionAttribute(RuleBasedTransactionAttribute other) { + super(); + copyFrom(other); + } + + protected void copyFrom(TransactionDefinition other) { + setPropagationBehavior(other.getPropagationBehavior()); + setIsolationLevel(other.getIsolationLevel()); + setTimeout(other.getTimeout()); + setReadOnly(other.isReadOnly()); + setName(other.getName()); + if (other instanceof TransactionAttribute) { + setQualifier(((TransactionAttribute) other).getQualifier()); + } + if (other instanceof RuleBasedTransactionAttribute) { + setRollbackRules(((RuleBasedTransactionAttribute) other).getRollbackRules()); Review Comment: This aliases the source attribute’s mutable rollback-rule list, so modifying either the source or copy changes both. Spring’s own `RuleBasedTransactionAttribute` copy constructor creates a new `ArrayList` specifically to preserve copy independence. The common helper is useful consolidation, but it also omits transaction labels and other `DefaultTransactionAttribute` metadata; please preserve full copy semantics and add constructor-copy tests before treating this as a safe replacement. ########## grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/reflect/AstUtils.groovy: ########## @@ -366,8 +366,10 @@ class AstUtils { String annotationClassName = node.getClassNode().getName() if ((excluded == null || !excluded.contains(annotationClassName)) && (included == null || included.contains(annotationClassName))) { Review Comment: Deduplicating solely by annotation class silently discards later annotations of the same type, including repeatable annotations represented as multiple `AnnotationNode`s, and suppresses a source annotation whenever the target has a same-type annotation with different members. This is an unrelated AST behavior change with no coverage. Please split it out and define/test the intended merge semantics rather than globally treating annotation type as identity. ########## grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/connections/AbstractConnectionSourceFactory.java: ########## @@ -88,6 +88,17 @@ public ConnectionSource<T, S> createRuntime(String name, PropertyResolver config S settings = buildRuntimeSettings(name, configuration, fallbackSettings); return create(name, settings); } + + /** + * Creates the settings for the given configuration + * @param configuration The configuration + * @return The settings + */ + public S createSettings(PropertyResolver configuration) { Review Comment: This duplicates only part of the existing `create(...)` settings path: it does not apply the injected `TenantResolver` or custom type marshallers before calling `buildSettings`. The settings returned here can therefore differ materially from those used to create the default connection. Please compose this from the existing settings-building path (or extract one shared helper) and add a test covering both injected collaborators. ########## grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/AbstractDatastore.java: ########## @@ -108,6 +157,9 @@ public <T extends Service> Iterable<T> getServices() { @PreDestroy public void destroy() { + if (TransactionSynchronizationManager.hasResource(this)) { + TransactionSynchronizationManager.unbindResource(this); Review Comment: `TransactionSynchronizationManager` state is per-thread, so this only sees the thread that happens to invoke `@PreDestroy`. More importantly, it drops the entire `SessionHolder` without closing any held sessions, and the holder can now contain multiple sessions; resolver-bound state is also untouched. That can leak native resources while presenting as cleanup. If datastore destruction owns this lifecycle, close every owned session and clear every contextual store; otherwise do not silently detach a live holder here. ########## grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/AbstractDatastore.java: ########## @@ -80,8 +127,10 @@ public AbstractDatastore(MappingContext mappingContext, PropertyResolver connect ConfigurableApplicationContext ctx, TPCacheAdapterRepository cacheAdapterRepository) { this.mappingContext = mappingContext; this.connectionDetails = connectionDetails; - setApplicationContext(ctx); this.cacheAdapterRepository = cacheAdapterRepository; + this.applicationEventPublisher = ctx != null ? ctx : new DefaultApplicationEventPublisher(); Review Comment: `applicationEventPublisher` is now assigned in three places: the field initializer (line 95), this ternary, and `setApplicationContext(ctx)` on the next-plus-one line, which re-implements the same logic. One assignment path would do — drop the field initializer and this ternary, and let `setApplicationContext` own it. Also note a surprising interaction: if a caller installs a custom standalone publisher via `setApplicationEventPublisher(...)`, a later `setApplicationContext(null)` silently replaces it with a fresh default publisher, discarding the caller's listeners. ########## grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/model/MappingContext.java: ########## @@ -51,11 +52,24 @@ @SuppressWarnings("rawtypes") public interface MappingContext { + /** + * Initialize the mapping context with the given settings + * @param settings The settings + */ + void initialize(ConnectionSourceSettings settings); Review Comment: Promoting `initialize(...)` from a protected `AbstractMappingContext` lifecycle hook to the public `MappingContext` interface (and adding `setMultiTenancyMode` below) leaks implementation lifecycle into core API and breaks every external `MappingContext` implementer. It also forces `MongoMappingContext`/`DocumentMappingContext` to widen their overrides to public. If `GormRegistry` needs these, interface segregation fits better: a small `InitializableMappingContext` (or keeping the dependency on `AbstractMappingContext`) gives the registry what it needs without widening the contract for everyone. Separately, a mutable `setMultiTenancyMode` is risky: `AbstractPersistentEntity.initialize()` caches `tenantId` based on the mode at init time, so flipping the mode afterwards leaves entities in an inconsistent state. ########## grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/keyvalue/mapping/config/KeyValueMappingContext.java: ########## @@ -54,7 +54,7 @@ public KeyValueMappingContext(String keyspace) { Assert.notNull(keyspace, "Argument [keyspace] cannot be null"); this.keyspace = keyspace; initializeDefaultMappingFactory(keyspace); - syntaxStrategy = new JpaMappingConfigurationStrategy(mappingFactory); + syntaxStrategy = new GormMappingConfigurationStrategy(mappingFactory); Review Comment: This is not an equivalent strategy substitution. `JpaMappingConfigurationStrategy` extends the GORM strategy and additionally handles `@Id`, `@EmbeddedId`, `@Transient`, `@Embedded`, generated values, and JPA relationship annotations. Replacing it with the superclass means JPA-annotated entities accepted by this context can now receive different identity, persistence, and association metadata. I could not find a KeyValue JPA regression test or a registry-related reason for this change; please revert it or split it into a separately justified and tested behavioral PR. ########## grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/model/AbstractPersistentEntity.java: ########## @@ -98,6 +98,14 @@ public PersistentProperty[] getCompositeIdentity() { } public TenantId getTenantId() { + if (this.tenantId == null && isMultiTenant()) { Review Comment: Two behavioral regressions here: initialization intentionally records a `TenantId` only in DISCRIMINATOR mode, but this fallback ignores the context mode and can now return one in NONE, DATABASE, or SCHEMA mode. It also iterates `persistentProperties` without checking entity initialization; with deferred initialization (`setCanInitializeEntities(false)`), that field is null and this getter now throws instead of returning null. If mode-independent lookup is genuinely required, it needs a separate explicit API; please preserve this getter’s discriminator contract and add deferred/non-discriminator coverage. -- 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]
