jdaugherty commented on code in PR #16784:
URL: https://github.com/apache/grails-core/pull/16784#discussion_r4423856699
##########
grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/GrailsHibernateTemplate.java:
##########
@@ -352,11 +330,30 @@ protected <T> T doExecute(HibernateCallback<T> action,
boolean enforceNativeSess
}
}
+ protected <T> T executeInSession(HibernateCallback<T> action, Session
session) throws DataAccessException {
+ try {
+ return action.doInHibernate(session);
+ } catch (HibernateException ex) {
+ throw convertHibernateAccessException(ex);
+ } catch (PersistenceException ex) {
+ if (ex.getCause() instanceof HibernateException) {
+ throw
SessionFactoryUtils.convertHibernateAccessException((HibernateException)
ex.getCause());
+ }
+ throw ex;
+ } catch (SQLException ex) {
+ throw jdbcExceptionTranslator.translate("Hibernate-related JDBC
operation", null, ex);
+ }
+ }
+
protected boolean isSessionTransactional(Session session) {
- SessionHolder sessionHolder = (SessionHolder)
TransactionSynchronizationManager.getResource(sessionFactory);
+ SessionHolder sessionHolder = (SessionHolder)
txResources.getResource(sessionFactory);
return sessionHolder != null && sessionHolder.getSession() == session;
}
+ public boolean isLazyConnection() {
Review Comment:
Nothing reads this, in main or in test. It is new public API on a patch
line; please drop it.
##########
grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/OuterSessionLifecycleStrategy.java:
##########
@@ -0,0 +1,105 @@
+/*
+ * 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.orm.hibernate;
+
+import javax.sql.DataSource;
+
+import org.hibernate.SessionFactory;
+
+import org.springframework.orm.hibernate5.SessionHolder;
+
+import org.grails.orm.hibernate.support.TransactionResources;
+
+/**
+ * Strategy interface governing outer session lifecycle preparation,
suspension, and restoration
+ * across nested {@code withNewSession} invocations.
+ */
+interface OuterSessionLifecycleStrategy {
+
+ /**
+ * Prepares the outer session before unbinding and opening the new session.
+ */
+ void prepare();
+
+ /**
+ * Suspends any active Spring synchronizations and unbinds outer session
and connection holders.
+ */
+ void suspendAndUnbind();
+
+ /**
+ * Restores previously suspended synchronizations and rebinds outer
session and connection holders.
+ */
+ void restore();
+
+ /**
+ * Factory method creating the appropriate strategy based on the outer
session, connection, and transaction context.
+ *
+ * @param previousHolder the session holder bound to the outer scope, or
null
+ * @param sessionFactory the current Hibernate SessionFactory
+ * @param dataSource the underlying DataSource
+ * @param txResources the transaction synchronization and resource manager
+ * @return the appropriate {@link OuterSessionLifecycleStrategy}
+ */
+ static OuterSessionLifecycleStrategy from(
+ SessionHolder previousHolder,
+ SessionFactory sessionFactory,
+ DataSource dataSource,
+ TransactionResources txResources) {
+ return from(previousHolder, sessionFactory, dataSource, txResources,
false);
Review Comment:
Only `OuterSessionLifecycleStrategySpec` calls this overload and the
4-argument constructors behind it; the production path always passes the flag.
Pass it from the spec and drop the overloads.
##########
grails-data-hibernate5/core/src/test/groovy/org/grails/orm/hibernate/OuterSessionLifecycleStrategySpec.groovy:
##########
@@ -0,0 +1,487 @@
+/*
+ * 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.orm.hibernate
+
+import java.sql.Connection
+import java.sql.SQLException
+import javax.sql.DataSource
+
+import org.hibernate.Session
+import org.hibernate.SessionFactory
+import org.hibernate.engine.jdbc.spi.JdbcCoordinator
+import org.hibernate.engine.spi.SessionImplementor
+import org.hibernate.resource.jdbc.spi.LogicalConnectionImplementor
+import org.springframework.jdbc.datasource.ConnectionHolder
+import org.springframework.orm.hibernate5.SessionFactoryUtils
+import org.springframework.orm.hibernate5.SessionHolder
+import org.springframework.transaction.support.TransactionSynchronization
+import spock.lang.Specification
+
+import org.grails.orm.hibernate.support.TransactionResources
+
+class OuterSessionLifecycleStrategySpec extends Specification {
+
+ SessionFactory sessionFactory = Mock(SessionFactory)
+ DataSource dataSource = Mock(DataSource)
+ TransactionResources txResources = Mock(TransactionResources)
+ Session session = Mock(Session)
+ SessionHolder sessionHolder = new SessionHolder(session)
+
+ //
-------------------------------------------------------------------------
+ // Strategy selection via factory
+ //
-------------------------------------------------------------------------
+
+ void "from returns NoOpOuterSessionStrategy when previousHolder is null"()
{
+ when:
+ OuterSessionLifecycleStrategy strategy =
OuterSessionLifecycleStrategy.from(
+ null, sessionFactory, dataSource, txResources)
+
+ then:
+ strategy instanceof NoOpOuterSessionStrategy
+ }
+
+ void "from returns NoOpOuterSessionStrategy when session is null"() {
+ given:
+ SessionHolder emptyHolder = Mock(SessionHolder)
+ emptyHolder.getSession() >> null
+
+ when:
+ OuterSessionLifecycleStrategy strategy =
OuterSessionLifecycleStrategy.from(
+ emptyHolder, sessionFactory, dataSource, txResources)
+
+ then:
+ strategy instanceof NoOpOuterSessionStrategy
+ }
+
+ void "from returns NoOpOuterSessionStrategy when session is closed"() {
Review Comment:
This pins the regression James found: a closed session in a bound holder has
to go through the default strategy so the holder is unbound and rebound, which
is what 7.0.x does. When `from` changes, this feature flips, which is why these
cases are better written as `withNewSession` calls against a
`HibernateDatastore`, as `WithNewSessionNonLazyPoolSpec` does.
##########
grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/OuterSessionLifecycleStrategy.java:
##########
@@ -0,0 +1,105 @@
+/*
+ * 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.orm.hibernate;
+
+import javax.sql.DataSource;
+
+import org.hibernate.SessionFactory;
+
+import org.springframework.orm.hibernate5.SessionHolder;
+
+import org.grails.orm.hibernate.support.TransactionResources;
+
+/**
+ * Strategy interface governing outer session lifecycle preparation,
suspension, and restoration
+ * across nested {@code withNewSession} invocations.
+ */
+interface OuterSessionLifecycleStrategy {
+
+ /**
+ * Prepares the outer session before unbinding and opening the new session.
+ */
+ void prepare();
+
+ /**
+ * Suspends any active Spring synchronizations and unbinds outer session
and connection holders.
+ */
+ void suspendAndUnbind();
+
+ /**
+ * Restores previously suspended synchronizations and rebinds outer
session and connection holders.
+ */
+ void restore();
+
+ /**
+ * Factory method creating the appropriate strategy based on the outer
session, connection, and transaction context.
+ *
+ * @param previousHolder the session holder bound to the outer scope, or
null
+ * @param sessionFactory the current Hibernate SessionFactory
+ * @param dataSource the underlying DataSource
+ * @param txResources the transaction synchronization and resource manager
+ * @return the appropriate {@link OuterSessionLifecycleStrategy}
+ */
+ static OuterSessionLifecycleStrategy from(
+ SessionHolder previousHolder,
+ SessionFactory sessionFactory,
+ DataSource dataSource,
+ TransactionResources txResources) {
+ return from(previousHolder, sessionFactory, dataSource, txResources,
false);
+ }
+
+ /**
+ * Factory method creating the appropriate strategy based on the outer
session, connection, and transaction context.
+ *
+ * @param previousHolder the session holder bound to the outer scope, or
null
+ * @param sessionFactory the current Hibernate SessionFactory
+ * @param dataSource the underlying DataSource
+ * @param txResources the transaction synchronization and resource manager
+ * @param lazyConnection whether the datasource is configured with a lazy
connection proxy
+ * @return the appropriate {@link OuterSessionLifecycleStrategy}
+ */
+ static OuterSessionLifecycleStrategy from(
+ SessionHolder previousHolder,
+ SessionFactory sessionFactory,
+ DataSource dataSource,
+ TransactionResources txResources,
+ boolean lazyConnection) {
+
+ boolean hasOpenSession = previousHolder != null &&
previousHolder.getSession() != null && previousHolder.getSession().isOpen();
+ boolean hasConnectionHolder = txResources.getResource(dataSource) !=
null;
+ boolean hasSynchronization = txResources.isSynchronizationActive();
+
+ if (!hasOpenSession && !hasConnectionHolder && !hasSynchronization) {
Review Comment:
Reproduced against `upstream/7.0.x` with the same probe: a bound holder
whose session is closed, then `Author.withNewSession { Author.count() }`. 7.0.x
runs the closure and rebinds the same holder; this head throws
`IllegalStateException: Already value [SessionHolder@…] for key
[SessionFactoryImpl@…] bound to thread` and leaves nothing bound afterwards.
`previousHolder != null` in place of `hasOpenSession` on this line is enough:
`getLogicalConnection()` already returns null for a closed session, so the
default strategy only unbinds and rebinds.
##########
grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/GrailsHibernateTemplate.java:
##########
@@ -323,23 +315,9 @@ protected <T> T doExecute(HibernateCallback<T> action,
boolean enforceNativeSess
session.setDefaultReadOnly(true);
}
Session sessionToExpose = (enforceNativeSession ||
exposeNativeSession ? session : createSessionProxy(session));
- T result = action.doInHibernate(sessionToExpose);
+ T result = executeInSession(action, sessionToExpose);
Review Comment:
Reproduced against `upstream/7.0.x`: a `FLUSH_EAGER` template, `execute { s
-> ... }` inside `withTransaction`, with a callback that sets a column mapped
`length: 5` to a longer value so the UPDATE fails at this flush. 7.0.x throws
`DataIntegrityViolationException`; this head throws
`jakarta.persistence.PersistenceException` wrapping
`org.hibernate.exception.DataException`. Moving `flushIfNecessary` into
`executeInSession` restores it.
--
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]
