jamesfredley commented on code in PR #16784: URL: https://github.com/apache/grails-core/pull/16784#discussion_r4420903015
########## 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: A closed outer session remains in a bound `SessionHolder`, with no datasource holder and no synchronization. `withNewSession` selects the no-op strategy and leaves that holder bound. Binding the new holder throws `IllegalStateException` before the closure runs, and cleanup unbinds the old holder without restoring it. Treat a holder whose session is closed as a holder that still has to be unbound. ########## 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: With `FLUSH_EAGER`, a callback that queues an insert or update whose constraint fails in `flushIfNecessary` throws the Hibernate exception. The new catch covers the callback only, so the flush at the next line is outside it. Callers catching `DataAccessException` no longer see that failure, including a template execution that is not `withNewSession`. Translate the flush with the callback. -- 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]
