jdaugherty commented on code in PR #15779:
URL: https://github.com/apache/grails-core/pull/15779#discussion_r3670053180
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/AbstractDatastore.java:
##########
@@ -55,12 +62,47 @@
@SuppressWarnings({"rawtypes", "unchecked"})
public abstract class AbstractDatastore implements Datastore,
StatelessDatastore, ServiceRegistry {
protected static final Logger LOG =
LoggerFactory.getLogger(AbstractDatastore.class);
+
+ /**
+ * A minimal {@link ApplicationEventPublisher} that composes a {@link
SimpleApplicationEventMulticaster}
+ * rather than hand-rolling dispatch, so listener type/source filtering,
ordering, and thread-safe
+ * listener management are Spring's, not a partial reimplementation.
+ */
+ private static final class MulticasterApplicationEventPublisher implements
ApplicationEventPublisher {
Review Comment:
This is a third implementation of an abstraction we already ship.
`org.grails.datastore.gorm.events.ConfigurableApplicationEventPublisher` is
exactly `ApplicationEventPublisher + addApplicationListener`, and
`DefaultApplicationEventPublisher` is exactly this class (it just filters via
`SmartApplicationListener` instead of `SimpleApplicationEventMulticaster`).
The only reason you can't reuse it is module direction — it lives in
`grails-datamapping-core`, which has an `api` dependency on
`grails-datastore-core`. That's a five-minute fix: move the one-method
interface (and optionally the default impl) down into `grails-datastore-core`,
leave a deprecated subinterface behind in the old package for source
compatibility. Then this private class becomes
`DefaultApplicationEventPublisher` and the reflective fallback at line 218
collapses into an `instanceof` check.
As it stands we'd have three publisher implementations with three different
listener-selection behaviours (`SimpleApplicationEventMulticaster` here,
hand-rolled `SmartApplicationListener` filtering in
`DefaultApplicationEventPublisher`, full Spring semantics in
`ConfigurableApplicationContextEventPublisher`), which is worse than the
situation this change set out to fix.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/AbstractDatastore.java:
##########
@@ -80,8 +122,9 @@ public AbstractDatastore(MappingContext mappingContext,
PropertyResolver connect
ConfigurableApplicationContext ctx,
TPCacheAdapterRepository cacheAdapterRepository) {
this.mappingContext = mappingContext;
this.connectionDetails = connectionDetails;
- setApplicationContext(ctx);
this.cacheAdapterRepository = cacheAdapterRepository;
+ this.sessionResolver = new ThreadLocalSessionResolver<>(this);
Review Comment:
`this` escapes the constructor here — `ThreadLocalSessionResolver` captures
a partially-constructed `AbstractDatastore`, and the very next line calls the
overridable `setApplicationContext(ctx)`. It happens to be safe today because
the resolver only stores the reference and never dereferences it during
construction, but it is a fragile invariant to rely on in a base class every
datastore extends. Constructing the resolver lazily in `getSessionResolver()`
(or storing the resolver's datastore reference as a supplier) would remove the
hazard.
Also: the field is declared as a raw `SessionResolver` (line 100) and
`getSessionResolver()` returns it raw, so the `<S extends Session>` type
parameter on the interface buys nothing and every consumer has to cast. Either
drop the type parameter or return `SessionResolver<? extends Session>` — see my
note on `SessionResolver.groovy`.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/AbstractDatastore.java:
##########
@@ -106,8 +149,28 @@ public <T extends Service> Iterable<T> getServices() {
return serviceRegistry.getServices();
}
+ /**
+ * Closes every session held by the current thread's {@link
SessionHolder}, if any. Since
+ * {@link TransactionSynchronizationManager} is thread-local, and {@link
#sessionResolver} is
+ * just a view over the same state, this only reaches the thread invoking
{@code @PreDestroy} -
+ * sessions bound on other threads are not visible here and must be closed
by their own owning
+ * thread.
+ */
@PreDestroy
public void destroy() {
+ if (TransactionSynchronizationManager.hasResource(this)) {
+ Object resource =
TransactionSynchronizationManager.unbindResource(this);
+ if (resource instanceof SessionHolder) {
+ for (Session session : new ArrayList<>(((SessionHolder)
resource).getSessions())) {
+ try {
+ session.disconnect();
Review Comment:
This calls `session.disconnect()` directly rather than going through
`DatastoreUtils.closeSession(session)`, which already does exactly this — null
guard, debug log, and swallow-with-log on failure. Using it removes the
hand-rolled try/catch and keeps shutdown consistent with every other close path
in the module. (`ThreadLocalSessionResolver.unbind()` in this same PR correctly
uses `closeSessionOrRegisterDeferredClose`; `destroy()` should not diverge.)
Separately: this unbinds and closes unconditionally, with no check of
`SessionHolder.isSynchronizedWithTransaction()`. If `@PreDestroy` fires on a
thread that is inside a Spring-managed transaction,
`DatastoreTransactionManager`/`SpringSessionSynchronization` still hold that
same `SessionHolder` and will operate on a closed session during
commit/rollback. At minimum this should skip holders that are synchronized with
a transaction rather than yanking them.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/AbstractDatastore.java:
##########
@@ -122,6 +185,46 @@ public void destroy() {
public void setApplicationContext(ApplicationContext ctx) {
applicationContext = ctx;
+ if (ctx instanceof ApplicationEventPublisher) {
Review Comment:
The `applicationEventPublisherExplicitlySet` guard is applied
asymmetrically: it protects the `ctx == null` branch but not this one. So a
caller that does
```java
datastore.setApplicationEventPublisher(myPublisher);
datastore.setApplicationContext(ctx); // myPublisher silently discarded
```
loses their publisher and every listener registered on it, while
`setApplicationContext(null)` preserves it. The new spec only covers the null
case (`"setApplicationContext(null) does not discard an explicitly-installed
custom publisher"`), so the inconsistent half is untested.
Either honour the flag in both branches, or drop the flag and document that
`setApplicationContext` always wins. Whichever you pick, please add the
non-null counterpart test — as written the flag reads like it protects the
caller but only does so half the time.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/AbstractDatastore.java:
##########
@@ -223,7 +326,7 @@ public ConfigurableApplicationContext
getApplicationContext() {
}
public ApplicationEventPublisher getApplicationEventPublisher() {
- return getApplicationContext();
+ return applicationEventPublisher;
Review Comment:
Worth calling out explicitly in the PR description: this is a behavioural
change for datastores constructed with a null `ApplicationContext`. Previously
`getApplicationEventPublisher()` returned `getApplicationContext()`, i.e.
`null`; now `setApplicationContext(null)` installs a default multicaster so it
is always non-null.
Every `if (publisher != null)` guard in the codebase —
`Query.doList()`/`doCount()`, `HibernateQuery` — now takes the publish branch,
allocating a `PreQueryEvent` and running a multicast per query for datastores
that previously published nothing. Functionally harmless with zero listeners,
but it is a per-query allocation added in a PR about reducing allocation, and
it changes what `Datastore.getApplicationEventPublisher()` promises. If the
intent is "never null", say so in the `Datastore` javadoc and drop the now-dead
null checks; if not, keep returning null when nothing is configured.
(Genuinely good that this no longer routes through the `@Deprecated`
`getApplicationContext()`.)
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/SessionResolver.groovy:
##########
@@ -0,0 +1,45 @@
+/*
+ * 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 the current session bound to a datastore in the current
context (thread).
+ * Implementations compose over {@link
org.grails.datastore.mapping.transactions.SessionHolder}/
+ * {@link
org.springframework.transaction.support.TransactionSynchronizationManager}
rather than
+ * maintaining independent state, so this never disagrees with Spring's own
transactional session
+ * bookkeeping.
+ *
+ * @author borinquenkid
+ * @since 8.0
+ */
+@CompileStatic
+interface SessionResolver<S extends Session> {
+
+ /** Resolves the current session based on current context (thread) */
+ S resolve()
+
+ /** Binds a session to the current context */
+ void bind(S session)
+
+ /** Unbinds the current session */
+ void unbind()
Review Comment:
The doc comment says "Unbinds the current session", but the only
implementation also **closes** it (`closeSessionOrRegisterDeferredClose`).
That's a meaningful difference for anyone implementing this interface or
calling it with a session they don't own — `bind(session); unbind()` destroys a
session the caller may still be using, which is not what "unbind" implies
anywhere else in this codebase
(`TransactionSynchronizationManager.unbindResource` doesn't close, and
`DatastoreUtils` deliberately has separate `bindSession`/`closeSession`
operations).
Either document it as `unbindAndClose()`/rename it, or make `unbind()`
purely detach and leave closing to the caller. As the contract for a brand-new
public interface, this needs to be unambiguous before implementations start
appearing in the adapter PRs.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/SessionResolver.groovy:
##########
@@ -0,0 +1,45 @@
+/*
+ * 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 the current session bound to a datastore in the current
context (thread).
+ * Implementations compose over {@link
org.grails.datastore.mapping.transactions.SessionHolder}/
+ * {@link
org.springframework.transaction.support.TransactionSynchronizationManager}
rather than
+ * maintaining independent state, so this never disagrees with Spring's own
transactional session
+ * bookkeeping.
+ *
+ * @author borinquenkid
+ * @since 8.0
+ */
+@CompileStatic
+interface SessionResolver<S extends Session> {
Review Comment:
Two API notes on the new interface:
1. `<S extends Session>` is never used by any consumer —
`Datastore.getSessionResolver()` and `AbstractDatastore.sessionResolver` are
both raw, so the parameter only forces unchecked casts (see
`ThreadLocalSessionResolver.resolve()`'s `(S)` cast). Drop it, or thread it
through `Datastore` properly.
2. `@author borinquenkid` — the convention in this module is a real name
(`@author Graeme Rocher`); GitHub handles don't appear in any existing
`@author` tag here.
Minor, but this is permanent public API in a core package.
##########
grails-datastore-core/src/test/groovy/org/grails/datastore/mapping/services/DefaultServiceRegistrySpec.groovy:
##########
@@ -53,6 +53,8 @@ class DefaultServiceRegistrySpec extends Specification {
}
}
-class TestService implements Service, ITestService {}
+class TestService implements Service, ITestService {
+ Datastore datastore
Review Comment:
This is a coverage regression, and I think it's leftover from the trait
change that got reverted (`Service.groovy`'s diff is now a single blank line).
Declaring `Datastore datastore` on `TestService` generates
`getDatastore()`/`setDatastore()` on the class itself, which take precedence
over the trait's implementations. So after this change:
- `'test load services into service registry'` no longer exercises
`Service`'s own accessors or its private `datastore` field — it exercises a
plain Groovy property.
- `'test that all Service trait methods are marked as Generated'` now
asserts `@Generated` on Groovy's property accessors rather than on the
trait-contributed methods, which is precisely what that test exists to verify.
Both specs still pass, which is what makes this worth catching. Please
revert `TestService` to `class TestService implements Service, ITestService {}`.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/connections/AbstractConnectionSourceFactory.java:
##########
@@ -88,6 +101,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
+ */
+ @SuppressWarnings("unchecked")
+ public S createSettings(PropertyResolver configuration) {
+ ConnectionSourceSettings fallbackSettings =
buildFallbackSettings(configuration);
+ return (S) buildSettings(ConnectionSource.DEFAULT, configuration,
fallbackSettings, true);
Review Comment:
Three things here:
1. `buildSettings` is already declared to return `S` (line 120), so the
`(S)` cast and the `@SuppressWarnings("unchecked")` are both no-ops. Remove
them.
2. It hard-codes `ConnectionSource.DEFAULT` / `isDefaultDataSource = true`,
so it can only ever produce settings for the default connection. `create(name,
configuration)` derives that flag from the name; this method can't. If
`GormRegistry` needs per-connection settings this signature won't serve, and
`AbstractConnectionSourceFactorySpec` doesn't cover a named connection.
3. Nothing in this PR calls it. Given (1) and (2), the shape hasn't been
validated against a real consumer — I'd rather this landed with the code that
uses it.
Extracting `buildFallbackSettings` is a genuine improvement, though; that
part is fine to keep.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/ThreadLocalSessionResolver.groovy:
##########
@@ -0,0 +1,77 @@
+/*
+ * 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()} closes and pops only the top
session, restoring the outer
+ * binding rather than discarding the whole holder.
+ *
+ * @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() {
+ SessionHolder holder = (SessionHolder)
TransactionSynchronizationManager.getResource(datastore)
Review Comment:
This is the third copy of the unbind sequence in this PR
(`DatastoreUtils.unbindSession`, `DatastoreUtils.executeWithNewSession`'s
`finally`, and here). Once the key mismatch above is fixed, this whole method
is:
```groovy
void unbind() {
S session = resolve()
if (session != null) {
DatastoreUtils.unbindSession(session)
}
}
```
which also picks up `unbindSession`'s `containsSession` guard and its
warn-logging for the "nothing bound" case.
Separately — now that the independent `ThreadLocal` is gone (good change),
the class name is misleading: it holds no `ThreadLocal` and is a view over
`TransactionSynchronizationManager`.
`TransactionSynchronizationSessionResolver` or `SessionHolderSessionResolver`
would describe what it actually is. Worth renaming now, before the adapter PRs
reference it.
##########
grails-datastore-core/src/test/groovy/org/grails/datastore/mapping/model/AbstractPersistentEntityGetTenantIdSpec.groovy:
##########
@@ -0,0 +1,122 @@
+/*
+ * 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.model
+
+import org.grails.datastore.mapping.core.connections.ConnectionSourceSettings
+import org.grails.datastore.mapping.multitenancy.MultiTenancySettings
+import spock.lang.Specification
+
+class AbstractPersistentEntityGetTenantIdSpec extends Specification {
+
+ def "getTenantId resolves the TenantId property in DISCRIMINATOR mode"() {
+ given:
+ TestMappingContext context = new TestMappingContext()
+ context.initialize(discriminatorSettings())
Review Comment:
`AbstractMappingContext.initialize(ConnectionSourceSettings)` is
`protected`; this only compiles because Groovy ignores that. Per AGENTS.md
("Test via public APIs — never invoke internal implementations, package-private
methods, or bypass the public surface directly") this isn't a valid way to set
up the spec.
More importantly, it's diagnostic: the reason the setup needs a protected
lifecycle hook is that the production fallback it exercises
(`AbstractPersistentEntity.getTenantId()`'s lazy scan) can't be reached through
public API at all — see my comment on `AbstractPersistentEntity.java:101`. If
you can't drive the scenario through a public entry point, that's a strong
signal the production code shouldn't be handling it yet.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/transactions/CustomizableRollbackTransactionAttribute.java:
##########
@@ -51,38 +53,51 @@ 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) {
+ TransactionAttribute otherAttribute = (TransactionAttribute) other;
+ setQualifier(otherAttribute.getQualifier());
+ setLabels(otherAttribute.getLabels());
Review Comment:
This reintroduces, for labels, exactly the aliasing defect that was just
fixed for rollback rules. Spring's
`DefaultTransactionAttribute.setLabels(Collection<String>)` stores the
reference (`this.labels = labels;` — no copy), so the copy and the source share
one mutable collection.
`new LinkedHashSet<>(otherAttribute.getLabels())` (or `List.copyOf`) fixes
it.
The new spec doesn't catch this: `"copy constructor preserves qualifier,
labels, and connection metadata"` asserts label *values*, not independence. The
rollback-rules test asserts independence — labels needs the same treatment.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/transactions/CustomizableRollbackTransactionAttribute.java:
##########
@@ -51,38 +53,51 @@ 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) {
+ TransactionAttribute otherAttribute = (TransactionAttribute) other;
+ setQualifier(otherAttribute.getQualifier());
+ setLabels(otherAttribute.getLabels());
+ }
+ if (other instanceof RuleBasedTransactionAttribute) {
+ List<RollbackRuleAttribute> otherRules =
((RuleBasedTransactionAttribute) other).getRollbackRules();
Review Comment:
Two problems, both avoidable by reusing Spring's own copy constructor:
1. `RuleBasedTransactionAttribute.getRollbackRules()` **never returns null**
— it lazily assigns `new ArrayList<>()` and returns it. So the `otherRules !=
null` branch is dead, and worse, calling the getter here *mutates* `other` by
installing a fresh list on it. Use the field via the copy ctor instead of the
lazy getter.
2. `copyFrom` silently drops `DefaultTransactionAttribute.descriptor` and
`timeoutString`. `timeoutString` matters: `resolveAttributeStrings()` turns it
into the effective timeout, so a copy of an attribute built from
`@Transactional(timeoutString = "...")` loses its timeout and falls back to
`TIMEOUT_DEFAULT`.
Spring already provides
`RuleBasedTransactionAttribute(RuleBasedTransactionAttribute other)`, which
does `super(other)` plus `new ArrayList<>(other.rollbackRules)`. For the
`RuleBasedTransactionAttribute`/`CustomizableRollbackTransactionAttribute`
overloads this reduces to:
```java
public
CustomizableRollbackTransactionAttribute(RuleBasedTransactionAttribute other) {
super(other);
if (other instanceof CustomizableRollbackTransactionAttribute crta) {
this.inheritRollbackOnly = crta.inheritRollbackOnly;
this.connection = crta.connection;
}
}
```
which stays correct automatically as Spring adds fields.
##########
grails-datastore-core/src/test/groovy/org/grails/datastore/mapping/core/AbstractDatastoreSpec.groovy:
##########
@@ -0,0 +1,319 @@
+/*
+ * 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 org.grails.datastore.mapping.model.MappingContext
+import org.grails.datastore.mapping.model.PersistentEntity
Review Comment:
`PersistentEntity` is imported but never used.
Also, the import ordering in all the new specs doesn't follow the convention
the rest of the module uses (`java` → `groovy` → `jakarta` → other →
`org.springframework`/`io.spring` → `org.grails`, groups blank-line separated —
see the checkstyle `ImportOrderCheck` config and e.g. `DatastoreUtilsSpec`).
Here `org.grails` comes first; in `ConnectionSourceSettingsBuilderSpec`
`jakarta.persistence.FlushModeType` comes after `org.grails`. Checkstyle
doesn't run on Groovy or on test source sets so nothing enforces it, but
consistency with the surrounding code is worth having.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/DatastoreUtils.java:
##########
@@ -361,6 +361,42 @@ 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 {
Review Comment:
This `finally` block is `DatastoreUtils.unbindSession(Session)` re-inlined
(it's ~40 lines up in this same file — `removeSession` / unbind-if-last /
`closeSessionOrRegisterDeferredClose`, plus a `containsSession` guard this
version lacks). `ThreadLocalSessionResolver.unbind()` in this PR is a *third*
copy. Please delegate:
```java
public static <T> T executeWithNewSession(Datastore datastore,
SessionCallback<T> callback) {
Session session = bindNewSession(datastore.connect());
try {
return callback.doInSession(session);
}
finally {
unbindSession(session);
}
}
```
There's also a key inconsistency: `bindNewSession(session)` binds against
`session.getDatastore()`, but the `finally` reads `getResource(datastore)`. For
`ChildHibernateDatastore` and other cases where `datastore.connect()` returns a
session owned by a different `Datastore` instance, the bind and the cleanup
target different TSM keys — the holder is never cleaned up and the session
leaks. `unbindSession(session)` keys off the session consistently and avoids
this entirely.
Both overloads are also unused anywhere in this PR, so nothing exercises the
divergent-datastore case.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/Datastore.java:
##########
@@ -39,6 +39,17 @@
*/
public interface Datastore extends ServiceRegistry {
+ /**
+ * @return The session resolver for this datastore. The default
implementation is a stateless
+ * view over the same {@link
org.grails.datastore.mapping.transactions.SessionHolder}/
+ * {@link
org.springframework.transaction.support.TransactionSynchronizationManager}
state used
+ * elsewhere for this datastore, so implementations only need to override
this if they resolve
+ * sessions some other way.
+ */
Review Comment:
The `default` method is the right compatibility call, but it allocates a
fresh `ThreadLocalSessionResolver` on every invocation. Two consequences:
1. `datastore.getSessionResolver() != datastore.getSessionResolver()` for
any implementation that doesn't override it, so callers can never cache/compare
resolvers, and `AbstractDatastore` (which does hold a stable instance) and a
bare implementor behave differently. That's an easy source of downstream bugs
in (2/3) if `GormRegistry` ever keys anything by resolver identity.
2. In a PR whose stated purpose is turning O(M×N) allocation into O(M+N), a
public accessor that allocates per call is the wrong default.
Given `Datastore` is an interface you can't hold state in, I'd rather this
method not be on `Datastore` at all in this PR — `AbstractDatastore` already
exposes a stable `getSessionResolver()`, and nothing here or in (2/3) consumes
the interface-level method. If it must stay, please document that the returned
resolver is stateless and may be a new instance per call.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/AbstractDatastore.java:
##########
@@ -122,6 +185,46 @@ 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
+ * @throws IllegalStateException if the configured publisher exposes no
way to register a listener - silently
+ * dropping the listener would violate this method's contract that the
listener receives future events
+ */
+ 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) {
+ try {
+ Method method =
publisher.getClass().getMethod("addApplicationListener",
ApplicationListener.class);
Review Comment:
This is duck-typing `ConfigurableApplicationEventPublisher`. Note this is
not an exotic fallback — it is the *primary* path for every real datastore:
`MongoDatastore`, `SimpleMapDatastore`, `HibernateDatastore` and
`Neo4jDatastore` all return a `ConfigurableApplicationEventPublisher` from
`getApplicationEventPublisher()`, which is neither a
`ConfigurableApplicationContext` nor the private multicaster above. So in
production this method reaches every listener registration through reflection.
Move `ConfigurableApplicationEventPublisher` into `grails-datastore-core`
(see my comment on line 71) and this whole branch becomes:
```java
else if (publisher instanceof ConfigurableApplicationEventPublisher cfg) {
cfg.addApplicationListener(listener);
}
else if (publisher != null) {
throw new IllegalStateException(...);
}
```
which is checked at compile time, has no per-call reflection, and gives the
same fail-fast guarantee you added for the unsupported case.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/AbstractDatastore.java:
##########
@@ -171,7 +274,7 @@ public Session getCurrentSession() throws
ConnectionNotFoundException {
}
public boolean hasCurrentSession() {
- return TransactionSynchronizationManager.hasResource(this);
+ return sessionResolver.resolve() != null;
Review Comment:
This changes the meaning of a `Datastore` public-API method that every
adapter inherits, and it does not actually achieve the alignment it was changed
for.
- Old: `hasCurrentSession()` == "a `SessionHolder` is bound for this
datastore".
- New: "the holder's top session is non-null".
Those differ for a bound-but-empty holder, which is reachable
(`SessionHolder.removeSession` is called from
`AbstractAttributeStoringSession.disconnect()`, `DatastoreUtils.unbindSession`,
`executeWithNewSession`, and now `ThreadLocalSessionResolver.unbind()`), and
`DatastoreUtils.execute`/`doWithSession` branch on this value and then call
`bindSession(...)`, which throws from
`TransactionSynchronizationManager.bindResource` if a resource is already bound
for the key.
More importantly, the stated goal was that `hasCurrentSession()` and
`getCurrentSession()` agree — they still don't. `getCurrentSession()` →
`DatastoreUtils.doGetSession` uses `sessionHolder.getValidatedSession()`, which
evicts and returns `null` for a disconnected session; `resolve()` uses the
unvalidated `getSession()`. So a holder whose top session has been disconnected
reports `hasCurrentSession() == true` while `getCurrentSession()` opens a brand
new session.
If the resolver is meant to be the single authoritative lookup, `resolve()`
should use `getValidatedSession()`. Either way this needs a test that exercises
the disconnected-session and empty-holder cases, otherwise I'd rather leave
`hasCurrentSession()` as `hasResource(this)` in this PR.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/ThreadLocalSessionResolver.groovy:
##########
@@ -0,0 +1,77 @@
+/*
+ * 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()} closes and pops only the top
session, restoring the outer
+ * binding rather than discarding the whole holder.
+ *
+ * @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)
Review Comment:
`bind()` binds against `session.getDatastore()` (via `bindNewSession`),
while `resolve()` (line 51) and `unbind()` (line 62) both read
`this.datastore`. When those differ — a session obtained from a
`ChildHibernateDatastore`, or any resolver handed a session it doesn't own —
`bind(session)` silently writes to one TSM key and `resolve()` reads another,
so the bind appears to succeed and then vanishes, and `unbind()` operates on
the wrong holder.
`ThreadLocalSessionResolverSpec` stubs `session.getDatastore() >> datastore`
in every feature, so this case is never exercised. Please either key everything
off `session.getDatastore()`, or reject a mismatched session up front:
```groovy
void bind(S session) {
assert session.datastore.is(datastore) : "session belongs to a different
datastore"
DatastoreUtils.bindNewSession(session)
}
```
and add a spec for the mismatched case.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/query/Query.java:
##########
@@ -646,7 +646,7 @@ private List doList() {
ApplicationEventPublisher publisher =
session.getDatastore().getApplicationEventPublisher();
if (publisher != null) {
- publisher.publishEvent(new PreQueryEvent(this));
+ publisher.publishEvent(new PreQueryEvent(session.getDatastore(),
this));
Review Comment:
This is a no-op. `PreQueryEvent(Query)` delegates to
`AbstractQueryEvent(Query)`, which already does
`super(query.getSession().getDatastore())` — the same `session.getDatastore()`
you're now passing explicitly (`session` here is `this.session`, and
`query.getSession()` returns it). Identical event source before and after.
Unrelated churn in a `SessionResolver` PR; please revert. Same for the
`(Map<String, Object>[])` cast added in `DatastoreUtils.createPropertyResolver`
— the array assignment was already unchecked, the cast changes nothing.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/transactions/CustomizableRollbackTransactionAttribute.java:
##########
@@ -51,38 +53,51 @@ 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);
Review Comment:
This is a user-visible transaction-semantics change and should not ride
along in a `SessionResolver` infrastructure PR.
Before,
`CustomizableRollbackTransactionAttribute(RuleBasedTransactionAttribute)` — and
`(CustomizableRollbackTransactionAttribute)`, which delegated to it — copied
**only** `inheritRollbackOnly`. Nothing else. Now they copy propagation,
isolation, timeout, readOnly, name, qualifier, labels **and rollback rules**.
That reaches production paths.
`DefaultTransactionService.withNewTransaction(TransactionDefinition, Closure)`
does `new CustomizableRollbackTransactionAttribute(definition)`;
`GrailsTransactionTemplate` does the same for non-`Customizable` attributes.
Rollback rules now propagate into the derived attribute where they previously
didn't — so a `NoRollbackRuleAttribute` on the source will now suppress a
rollback that used to happen (`rollbackOn` returns `true` when no rule matches,
`false` when a `NoRollbackRuleAttribute` wins). That is a silent change to
whether user transactions roll back.
It may well be the correct fix for a latent bug, but it needs: its own PR, a
note in the description, tests through
`GrailsTransactionTemplate`/`DefaultTransactionService` (not just the attribute
in isolation), and a look at whether it warrants a docs/upgrade note. Please
split it out.
(The `"...on $ex"` → string-concat fixes further down are a genuine catch —
GString syntax in a `.java` file was printing literal `$ex`. Happy to see those
land, ideally in the same split-out PR.)
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/services/Service.groovy:
##########
@@ -46,4 +46,5 @@ trait Service<T> {
void setDatastore(Datastore datastore) {
this.datastore = datastore
}
+
Review Comment:
This file's entire diff is one added blank line. Please revert — the
`Service` trait shouldn't appear in this PR at all.
Relatedly: the `DefaultServiceRegistrySpec` change looks like leftover from
the reverted trait rework (commit "restore concrete Service trait methods…").
See my comment there.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/model/AbstractPersistentEntity.java:
##########
@@ -98,6 +98,15 @@ public PersistentProperty[] getCompositeIdentity() {
}
public TenantId getTenantId() {
+ if (this.tenantId == null && persistentProperties != null &&
Review Comment:
This fallback is unreachable through any public API, and it has real costs.
**Unreachable:** it only fires when `context.getMultiTenancyMode()` becomes
`DISCRIMINATOR` *after* `initialize()` already ran. The only way to do that is
`AbstractMappingContext.initialize(ConnectionSourceSettings)`, which is
`protected` — and the `MappingContext.initialize`/`setMultiTenancyMode`
promotions that would have made it reachable were (correctly) reverted out of
this PR. `AbstractPersistentEntityGetTenantIdSpec` can only reach it by calling
that protected hook from a test. So this is dead defensive code guarding a
state no caller can produce.
**Cost 1 — mutation from a getter, unsynchronized:** `tenantId` is a
private, non-`volatile` field on an object shared across every request thread,
and `getTenantId()` is called on hot paths (`AbstractGormMappingFactory`,
`MultiTenantEventListener`, `HibernateGormStaticApi`). Writing to it from a
getter with no memory barrier is exactly the kind of shared-mutable-state issue
this module works hard to avoid.
**Cost 2 — no negative caching:** for a multi-tenant entity with no
`TenantId` property, `this.tenantId` stays null, so the full
`persistentProperties` scan re-runs on *every* call, forever. In a change set
justified by O(M+N) scaling, adding an unbounded repeated O(properties) scan to
a hot getter is the wrong direction.
If downstream work genuinely needs mode-independent tenant-property lookup,
please add it as an explicit method (e.g. `findTenantIdProperty()`) in the PR
that needs it, and leave this getter's DISCRIMINATOR-only contract intact.
The `isMultiTenant` → `isMultiTenant()` swap at line 165 is also unrelated:
`isMultiTenant` is a `final` field and the accessor just returns it, so it is a
no-op today — and a silent behaviour change the moment any subclass overrides
`isMultiTenant()`. Please revert it.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/transactions/CustomizableRollbackTransactionAttribute.java:
##########
@@ -51,38 +53,51 @@ 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) {
Review Comment:
`copyFrom` is `protected` and is invoked from four constructors. A subclass
overriding it would see its own fields uninitialised — the classic
overridable-call-from-constructor hazard (SpotBugs
`MC_OVERRIDABLE_METHOD_CALL_IN_CONSTRUCTOR`). Since nothing needs to extend it,
make it `private` (or `final`).
##########
grails-datastore-core/src/test/groovy/org/grails/datastore/mapping/core/AbstractDatastoreSpec.groovy:
##########
@@ -0,0 +1,319 @@
+/*
+ * 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 org.grails.datastore.mapping.model.MappingContext
+import org.grails.datastore.mapping.model.PersistentEntity
+import org.springframework.context.ApplicationEvent
+import org.springframework.context.ApplicationEventPublisher
+import org.springframework.context.ApplicationListener
+import org.springframework.context.ConfigurableApplicationContext
+import org.springframework.context.PayloadApplicationEvent
+import org.springframework.context.support.GenericApplicationContext
+import org.springframework.core.env.PropertyResolver
+import spock.lang.Specification
+
+class AbstractDatastoreSpec extends Specification {
+
+ void "test that getApplicationEventPublisher returns the application
context if set"() {
+ given:
+ def mappingContext = Mock(MappingContext)
+ def ctx = new GenericApplicationContext()
+ ctx.refresh()
+ def datastore = new TestDatastore(mappingContext,
(PropertyResolver)null, ctx)
+
+ expect:
+ datastore.applicationEventPublisher == ctx
+ datastore.applicationContext == ctx
+ }
+
+ void "test that SessionCreationEvent is published when connect is
called"() {
+ given:
+ def mappingContext = Mock(MappingContext)
+ def events = []
+ def publisher = [
Review Comment:
`publisher` is assigned here and never used — this feature registers its
listener via `ctx.addApplicationListener` instead. Same dead local in `"test
that getApplicationEventPublisher returns the standalone publisher if set"` at
line 78 (there it *is* used, so only this one is dead). There's also trailing
whitespace on the blank lines around it.
CodeNarc/Checkstyle are disabled for test source sets so nothing will flag
these, which is exactly why they're worth cleaning up by hand.
##########
grails-datastore-core/src/test/groovy/org/grails/datastore/mapping/core/connections/ConnectionSourceSettingsBuilderSpec.groovy:
##########
@@ -0,0 +1,40 @@
+/*
+ * 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.connections
+
+import org.grails.datastore.mapping.config.Settings
+import org.grails.datastore.mapping.core.DatastoreUtils
+import jakarta.persistence.FlushModeType
+import spock.lang.Specification
+
+class ConnectionSourceSettingsBuilderSpec extends Specification {
+
+ void "the 3-arg constructor applies a fallBackConfiguration when the
property source has no override"() {
+ given:
+ def config = DatastoreUtils.createPropertyResolver([:])
+ def fallback = new
ConnectionSourceSettings().flushMode(FlushModeType.COMMIT)
Review Comment:
This spec exists to cover a constructor that no production code calls — the
3-arg `ConnectionSourceSettingsBuilder(PropertyResolver, String, Object)` added
in this PR has zero callers in the repo. A test whose only purpose is to
justify otherwise-unused API isn't really coverage.
If `GormRegistry` needs it in (2/3), add the constructor there alongside its
caller. (Note `ConfigurationBuilder` already exposes the 3-arg form via Groovy
default parameters; the subclass ctor is only needed because Groovy doesn't
inherit constructors — so this is purely plumbing for a future consumer.)
##########
grails-datastore-core/src/test/groovy/org/grails/datastore/mapping/model/AbstractPersistentEntityGetTenantIdSpec.groovy:
##########
@@ -0,0 +1,122 @@
+/*
+ * 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.model
+
+import org.grails.datastore.mapping.core.connections.ConnectionSourceSettings
+import org.grails.datastore.mapping.multitenancy.MultiTenancySettings
+import spock.lang.Specification
+
+class AbstractPersistentEntityGetTenantIdSpec extends Specification {
+
+ def "getTenantId resolves the TenantId property in DISCRIMINATOR mode"() {
+ given:
+ TestMappingContext context = new TestMappingContext()
+ context.initialize(discriminatorSettings())
+ def entity = context.addPersistentEntity(TenantScopedEntity)
+
+ expect:
+ entity.getTenantId() != null
+ entity.getTenantId().name == 'tenantId'
+ }
+
+ def "getTenantId returns null when multi-tenancy mode is not
DISCRIMINATOR"() {
+ given:
+ TestMappingContext context = new TestMappingContext()
+ def entity = context.addPersistentEntity(TenantScopedEntity)
+
+ expect:
+ context.multiTenancyMode == MultiTenancySettings.MultiTenancyMode.NONE
+ entity.getTenantId() == null
+ }
+
+ def "getTenantId does not throw and returns null when entity
initialization is deferred"() {
+ given:
+ TestMappingContext context = new TestMappingContext()
+ context.initialize(discriminatorSettings())
+ context.setCanInitializeEntities(false)
+ def entity = context.addPersistentEntity(TenantScopedEntity)
+
+ expect:
+ entity.getTenantId() == null
+ }
+
+ def "getTenantId lazily resolves the TenantId property when the context
switches to DISCRIMINATOR mode after the entity was already initialized"() {
+ given: "an entity initialized while the context is still in NONE mode,
so initialize()'s eager loop never assigns tenantId"
+ TestMappingContext context = new TestMappingContext()
+ def entity = context.addPersistentEntity(TenantScopedEntity)
+
+ expect:
+ context.multiTenancyMode == MultiTenancySettings.MultiTenancyMode.NONE
+ entity.getTenantId() == null
+
+ when: "the context is reconfigured into DISCRIMINATOR mode afterwards"
+ context.initialize(discriminatorSettings())
+
+ then: "getTenantId() falls back to a lazy scan of the
already-populated persistentProperties instead of staying stuck at null"
+ entity.getTenantId() != null
+ entity.getTenantId().name == 'tenantId'
+ }
+
+ def "getTenantId's lazy scan completes without a match and returns null
for a multi-tenant entity with no tenantId property"() {
+ given: "initialized in NONE mode - DISCRIMINATOR mode at initialize()
time would reject this class for lacking a tenant identifier property"
+ TestMappingContext context = new TestMappingContext()
+ def entity =
context.addPersistentEntity(TenantScopedEntityWithoutTenantIdProperty)
+
+ when: "the context is reconfigured into DISCRIMINATOR mode afterwards,
same as the successful-lookup case above"
+ context.initialize(discriminatorSettings())
+
+ then: "the lazy scan runs to completion without finding a TenantId
property, rather than throwing or looping forever"
+ entity.getTenantId() == null
+ }
+
+ def "getTenantId's lazy fallback short-circuits on isMultiTenant() for a
non-multi-tenant entity, even in DISCRIMINATOR mode"() {
+ given:
+ TestMappingContext context = new TestMappingContext()
+ context.initialize(discriminatorSettings())
+ def entity = context.addPersistentEntity(NonTenantScopedEntity)
+
+ expect:
+ !entity.isMultiTenant()
+ entity.getTenantId() == null
+ }
+
+ private static ConnectionSourceSettings discriminatorSettings() {
+ ConnectionSourceSettings settings = new ConnectionSourceSettings()
+ settings.multiTenancy.mode =
MultiTenancySettings.MultiTenancyMode.DISCRIMINATOR
+ return settings
+ }
+}
+
+interface MultiTenant {
Review Comment:
This declares a **public top-level
`org.grails.datastore.mapping.model.MultiTenant`** interface, in the same
package as production classes, from a test file — along with three public
top-level entity classes. Any production or test code in that package that
references `MultiTenant` unqualified now silently resolves to this one instead
of `org.grails.datastore.mapping.multitenancy.MultiTenant`. That is a landmine
for whoever touches this package next.
It also only works because `ClassUtils.isMultiTenant(Class)` matches on
`getSimpleName().equals("MultiTenant")` rather than on the actual type — so the
test is passing by exploiting an implementation detail of the very mechanism
it's supposed to be verifying, and would keep passing if that detail were
tightened.
Please implement the real
`org.grails.datastore.mapping.multitenancy.MultiTenant` and make the fixture
classes static nested classes of the spec.
--
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]
