jdaugherty commented on code in PR #16784:
URL: https://github.com/apache/grails-core/pull/16784#discussion_r4426101993
##########
grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/GrailsHibernateTemplate.java:
##########
@@ -352,11 +328,36 @@ protected <T> T doExecute(HibernateCallback<T> action,
boolean enforceNativeSess
}
}
+ protected <T> T executeInSession(HibernateCallback<T> action, Session
session) throws DataAccessException {
Review Comment:
Nothing calls this overload; `doExecute` uses the 4-argument one. Please
drop it, so the patch line does not gain a protected extension point nothing
uses.
##########
grails-data-hibernate5/core/src/test/groovy/org/grails/orm/hibernate/GrailsHibernateTemplateSpec.groovy:
##########
@@ -0,0 +1,96 @@
+/*
+ * 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.SQLException
+
+import org.apache.grails.data.testing.tck.domains.Book
+import org.grails.orm.hibernate.cfg.Settings
+import org.hibernate.HibernateException
+import org.hibernate.Session
+import org.hibernate.exception.GenericJDBCException
+import org.springframework.dao.DataAccessException
+import org.springframework.orm.hibernate5.HibernateJdbcException
+import spock.lang.AutoCleanup
+import spock.lang.Specification
+
+class GrailsHibernateTemplateSpec extends Specification {
+
+ @AutoCleanup
+ HibernateDatastore datastore
+
+ GrailsHibernateTemplate template
+
+ void setup() {
+ datastore = new HibernateDatastore([(Settings.SETTING_DB_CREATE):
'create-drop'], Book)
+ template = new GrailsHibernateTemplate(datastore.sessionFactory,
datastore)
+ }
+
+ void "convertHibernateAccessException falls back to SessionFactoryUtils
when translator returns null"() {
+ given:
+ GenericJDBCException genericEx = new GenericJDBCException('msg', new
SQLException('untranslatable'))
+
+ when:
+ DataAccessException converted =
template.convertHibernateAccessException(genericEx)
+
+ then:
+ converted instanceof HibernateJdbcException
+ converted.cause == genericEx
+ }
+
+ void "doExecute wraps untranslatable GenericJDBCException as
HibernateJdbcException"() {
+ when:
+ Book.withTransaction {
+ template.execute { Session sess -> throw new
GenericJDBCException('msg', new SQLException('untranslatable')) }
+ }
+
+ then:
+ thrown(HibernateJdbcException)
+ }
+
+ void "executeWithNewSession translates raw HibernateException to Spring
DataAccessException"() {
+ when:
+ Book.withNewSession { Session s ->
+ s.get(Book, 'not-a-long')
+ }
+
+ then:
+ thrown(org.springframework.orm.hibernate5.HibernateSystemException)
+ }
+
+ void "doExecute translates HibernateException thrown during
flushIfNecessary to DataAccessException"() {
+ given:
+ GrailsHibernateTemplate customTemplate = new
GrailsHibernateTemplate(datastore.sessionFactory, datastore) {
+ @Override
+ protected void flushIfNecessary(Session session, boolean
existingTransaction) throws HibernateException {
+ throw new HibernateException('Simulated flush failure')
Review Comment:
The override throws a bare `HibernateException`, so this feature goes
through `catch (HibernateException)`. The failure James described arrives as
`jakarta.persistence.PersistenceException` wrapping `DataException` and takes
the `PersistenceException` branch. A `FLUSH_EAGER` template, `execute` inside
`withTransaction`, and an update that overruns a column mapped `length: 5`
fails in `flushIfNecessary` for real: it throws
`DataIntegrityViolationException` at this head and on 7.0.x, and threw
`PersistenceException` at cbbd3a9435. Please test it that way, so the feature
covers the path that regressed.
##########
grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/GrailsHibernateTemplate.java:
##########
@@ -85,6 +86,9 @@ public class GrailsHibernateTemplate implements
IHibernateTemplate {
protected SQLExceptionTranslator jdbcExceptionTranslator;
protected int flushMode = FLUSH_AUTO;
private boolean applyFlushModeOnlyToNonExistingTransactions = false;
+ private boolean transactionAware = false;
+ private boolean lazyConnection = false;
Review Comment:
`lazyConnection` is read now. `transactionAware` is still set in the
constructor and never read.
##########
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:
Verified at d316d92b34: the closed-session holder case runs the closure and
rebinds the same holder, plain, under a SUPPORTS scope, and with an inner
transaction, in lazy and non-lazy pools and with `maxActive=1`; no
synchronization is left active afterwards. Same as 7.0.x.
##########
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:
Verified at d316d92b34: the overrun UPDATE in `flushIfNecessary` throws
`DataIntegrityViolationException` again from `execute` inside `withTransaction`
and inside `withNewSession`, matching 7.0.x.
--
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]
