jdaugherty commented on code in PR #15779:
URL: https://github.com/apache/grails-core/pull/15779#discussion_r3561119492
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/AbstractDatastore.java:
##########
@@ -122,6 +189,44 @@ public void destroy() {
public void setApplicationContext(ApplicationContext ctx) {
applicationContext = ctx;
+ if (ctx instanceof ApplicationEventPublisher) {
+ this.applicationEventPublisher = (ApplicationEventPublisher) ctx;
+ }
+ else if (ctx == null && !applicationEventPublisherExplicitlySet) {
+ this.applicationEventPublisher = new
MulticasterApplicationEventPublisher();
+ }
+ }
+
+ public void setApplicationEventPublisher(ApplicationEventPublisher
applicationEventPublisher) {
+ this.applicationEventPublisher = applicationEventPublisher;
+ this.applicationEventPublisherExplicitlySet = true;
+ }
+
+ /**
+ * Adds an application listener to the datastore. Registers against {@link
#getApplicationEventPublisher()}
+ * rather than the raw field, so the listener reaches whatever publisher
this datastore (or a subclass
+ * overriding {@link #getApplicationEventPublisher()} with its own field)
actually publishes events through.
+ *
+ * @param listener The listener
+ */
+ public void addApplicationListener(ApplicationListener<?> listener) {
+ ApplicationEventPublisher publisher = getApplicationEventPublisher();
+ if (publisher instanceof ConfigurableApplicationContext) {
+ ((ConfigurableApplicationContext)
publisher).addApplicationListener(listener);
+ }
+ else if (publisher instanceof MulticasterApplicationEventPublisher) {
+ ((MulticasterApplicationEventPublisher)
publisher).addApplicationListener(listener);
+ }
+ else if (publisher != null) {
Review Comment:
This still silently loses listeners for any valid
`ApplicationEventPublisher` that does not expose the unrelated
`addApplicationListener` method; logging does not satisfy this API contract.
The new test explicitly accepts the drop with only `noExceptionThrown()`.
Please retain listeners in a datastore-owned multicaster and use the configured
publisher as an outbound delegate, or narrow the accepted publisher type so
registration cannot silently succeed without delivery.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/AbstractDatastore.java:
##########
@@ -171,7 +276,7 @@ public Session getCurrentSession() throws
ConnectionNotFoundException {
}
public boolean hasCurrentSession() {
- return TransactionSynchronizationManager.hasResource(this);
+ return sessionResolver.resolve() != null;
Review Comment:
`hasCurrentSession()` uses the replaceable `sessionResolver`, but
`getCurrentSession()` still delegates to `DatastoreUtils.doGetSession()`, which
reads only TSM/`SessionHolder`. A custom resolver can therefore make this
return true while `getCurrentSession()` throws or returns a different session.
This also breaks `DatastoreUtils.execute()`, which branches here and then calls
`getCurrentSession()`. Either remove the alternate resolver contract/setter or
make both methods use the same authoritative resolution path.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/ThreadLocalSessionResolver.groovy:
##########
@@ -0,0 +1,63 @@
+/*
+ * 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
+
+import
org.springframework.transaction.support.TransactionSynchronizationManager
+
+import org.grails.datastore.mapping.transactions.SessionHolder
+
+/**
+ * The default {@link SessionResolver}, backed by the same {@link
SessionHolder}/
+ * {@link TransactionSynchronizationManager} state as {@link DatastoreUtils}'s
session binding -
+ * a thin, stateless view over the one authoritative session stack for its
owning datastore, rather
+ * than an independent thread-local store that could disagree with it. Nested
bindings are supported
+ * because {@link SessionHolder} itself is a stack: {@link #bind(Session)}
pushes, {@link #resolve()}
+ * returns the top, and {@link #unbind()} clears the whole binding for the
current thread.
+ *
+ * @author borinquenkid
+ * @since 8.0
+ */
+@CompileStatic
+class ThreadLocalSessionResolver<S extends Session> implements
SessionResolver<S> {
+
+ private final Datastore datastore
+
+ ThreadLocalSessionResolver(Datastore datastore) {
+ this.datastore = datastore
+ }
+
+ @Override
+ S resolve() {
+ SessionHolder holder = (SessionHolder)
TransactionSynchronizationManager.getResource(datastore)
+ return holder != null ? (S) holder.getSession() : null
+ }
+
+ @Override
+ void bind(S session) {
+ DatastoreUtils.bindNewSession(session)
+ }
+
+ @Override
+ void unbind() {
+ TransactionSynchronizationManager.unbindResourceIfPossible(datastore)
Review Comment:
`bind()` pushes onto the existing `SessionHolder`, but `unbind()` removes
the entire holder. This still breaks nested scope restoration: after `bind(A);
bind(B); unbind()`, `resolve()` returns null instead of A, and both sessions
are detached without either being closed. The new nested test currently asserts
this destructive behavior. Please remove/close only the top session and leave
the outer binding intact.
--
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]
