From f86aadcc2e4bbb04d1163b98df06d32a2c3e46b1 Mon Sep 17 00:00:00 2001 From: Laird Nelson Date: Fri, 6 Oct 2023 12:13:48 -0700 Subject: [PATCH] Fixes an issue where autoCommit is not restored properly in certain edge cases; ensures related proper Hibernate JTA settings are set by default (#7741) Signed-off-by: Laird Nelson --- .../cdi/hibernate/CDISEJtaPlatform.java | 176 +++++++++++------- .../src/main/java/module-info.java | 14 +- .../cdi/jpa/PersistenceExtension.java | 36 +++- .../integrations/jta/jdbc/JtaConnection.java | 34 +++- .../jta/jdbc/LocalXAResource.java | 44 +++-- 5 files changed, 203 insertions(+), 101 deletions(-) diff --git a/integrations/cdi/hibernate-cdi/src/main/java/io/helidon/integrations/cdi/hibernate/CDISEJtaPlatform.java b/integrations/cdi/hibernate-cdi/src/main/java/io/helidon/integrations/cdi/hibernate/CDISEJtaPlatform.java index b7281067411..62288da5681 100644 --- a/integrations/cdi/hibernate-cdi/src/main/java/io/helidon/integrations/cdi/hibernate/CDISEJtaPlatform.java +++ b/integrations/cdi/hibernate-cdi/src/main/java/io/helidon/integrations/cdi/hibernate/CDISEJtaPlatform.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2019, 2021 Oracle and/or its affiliates. + * Copyright (c) 2019, 2023 Oracle and/or its affiliates. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -15,15 +15,23 @@ */ package io.helidon.integrations.cdi.hibernate; +import java.lang.System.Logger; import java.util.Objects; +import java.util.Properties; import jakarta.enterprise.context.ApplicationScoped; +import jakarta.enterprise.event.Observes; import jakarta.inject.Inject; +import jakarta.persistence.spi.PersistenceUnitInfo; import jakarta.transaction.TransactionManager; import jakarta.transaction.UserTransaction; import org.hibernate.engine.jndi.spi.JndiService; import org.hibernate.engine.transaction.jta.platform.internal.AbstractJtaPlatform; +import static java.lang.System.Logger.Level.DEBUG; +import static org.hibernate.cfg.AvailableSettings.CONNECTION_HANDLING; +import static org.hibernate.resource.jdbc.spi.PhysicalConnectionHandlingMode.DELAYED_ACQUISITION_AND_RELEASE_AFTER_TRANSACTION; + /** * An {@link AbstractJtaPlatform} that is an {@link ApplicationScoped} * CDI managed bean that supplies {@link TransactionManager} and @@ -35,74 +43,114 @@ */ @ApplicationScoped public class CDISEJtaPlatform extends AbstractJtaPlatform { - private final TransactionManager transactionManager; - private final UserTransaction userTransaction; + private static final Logger LOGGER = System.getLogger(CDISEJtaPlatform.class.getName()); + + private static final long serialVersionUID = 1L; + + private transient TransactionManager transactionManager; + + private transient UserTransaction userTransaction; + + /** + * Creates a new {@link CDISEJtaPlatform}. + * + * @deprecated Required by the + * CDI specification and not intended for end-user use. + */ + @Deprecated + CDISEJtaPlatform() { + super(); + this.transactionManager = null; + this.userTransaction = null; + } - private static final long serialVersionUID = 1L; + /** + * Creates a new {@link CDISEJtaPlatform}. + * + * @param transactionManager the {@link TransactionManager} to use; + * must not be {@code null} + * + * @param userTransaction the {@link UserTransaction} to use; must + * not be {@code null} + * + * @exception NullPointerException if either {@code + * transactionManager} or {@code userTransaction} is {@code null} + */ + @Inject + public CDISEJtaPlatform(TransactionManager transactionManager, + UserTransaction userTransaction) { + super(); + this.transactionManager = Objects.requireNonNull(transactionManager); + this.userTransaction = Objects.requireNonNull(userTransaction); + } - /** - * Creates a new {@link CDISEJtaPlatform}. - * - * @param transactionManager the {@link TransactionManager} to use; - * must not be {@code null} - * - * @param userTransaction the {@link UserTransaction} to use; must - * not be {@code null} - * - * @exception NullPointerException if either {@code - * transactionManager} or {@code userTransaction} is {@code null} - */ - @Inject - public CDISEJtaPlatform(final TransactionManager transactionManager, - final UserTransaction userTransaction) { - super(); - this.transactionManager = Objects.requireNonNull(transactionManager); - this.userTransaction = Objects.requireNonNull(userTransaction); - } + /** + * Throws an {@link UnsupportedOperationException} when invoked. + * + * @return (not applicable) + * + * @exception UnsupportedOperationException when invoked + */ + @Override + protected JndiService jndiService() { + throw new UnsupportedOperationException(); + } - /** - * Throws an {@link UnsupportedOperationException} when invoked. - * - * @return (not applicable) - * - * @exception UnsupportedOperationException when invoked - */ - @Override - protected JndiService jndiService() { - throw new UnsupportedOperationException(); - } + /** + * Returns the {@link UserTransaction} instance supplied at + * {@linkplain #CDISEJtaPlatform(TransactionManager, + * UserTransaction) construction time}. + * + *

This method never returns {@code null}.

+ * + * @return a non-{@code null} {@link UserTransaction} + * + * @see #CDISEJtaPlatform(TransactionManager, UserTransaction) + */ + @Override + protected UserTransaction locateUserTransaction() { + return this.userTransaction; + } - /** - * Returns the {@link UserTransaction} instance supplied at - * {@linkplain #CDISEJtaPlatform(TransactionManager, - * UserTransaction) construction time}. - * - *

This method never returns {@code null}.

- * - * @return a non-{@code null} {@link UserTransaction} - * - * @see #CDISEJtaPlatform(TransactionManager, UserTransaction) - */ - @Override - protected UserTransaction locateUserTransaction() { - return this.userTransaction; - } + /** + * Returns the {@link TransactionManager} instance supplied at + * {@linkplain #CDISEJtaPlatform(TransactionManager, + * UserTransaction) construction time}. + * + *

This method never returns {@code null}.

+ * + * @return a non-{@code null} {@link TransactionManager} + * + * @see #CDISEJtaPlatform(TransactionManager, UserTransaction) + */ + @Override + protected TransactionManager locateTransactionManager() { + return this.transactionManager; + } - /** - * Returns the {@link TransactionManager} instance supplied at - * {@linkplain #CDISEJtaPlatform(TransactionManager, - * UserTransaction) construction time}. - * - *

This method never returns {@code null}.

- * - * @return a non-{@code null} {@link TransactionManager} - * - * @see #CDISEJtaPlatform(TransactionManager, UserTransaction) - */ - @Override - protected TransactionManager locateTransactionManager() { - return this.transactionManager; - } + /** + * Customizes the supplied {@link PersistenceUnitInfo}, when it is fired as a CDI event by, for example, the {@code + * io.helidon.integrations.cdi.jpa.PersistenceExtension} portable extension, by ensuring that certain important + * Hibernate properties are always set on the persistence unit. + * + * @param pui the {@link PersistenceUnitInfo} to customize; must not be {@code null} + * + * @exception NullPointerException if {@code pui} is {@code null} + * + * @see + * org.hibernate.resource.jdbc.spi.PhysicalConnectionHandlingMode#DELAYED_ACQUISITION_AND_RELEASE_AFTER_TRANSACTION + */ + private static void customizePersistenceUnitInfo(@Observes PersistenceUnitInfo pui) { + Properties p = pui.getProperties(); + if (p != null && p.getProperty(CONNECTION_HANDLING) == null && p.get(CONNECTION_HANDLING) == null) { + if (LOGGER.isLoggable(DEBUG)) { + LOGGER.log(DEBUG, "Setting " + CONNECTION_HANDLING + " property to " + + DELAYED_ACQUISITION_AND_RELEASE_AFTER_TRANSACTION + + " on persistence unit " + pui.getPersistenceUnitName()); + } + p.setProperty(CONNECTION_HANDLING, DELAYED_ACQUISITION_AND_RELEASE_AFTER_TRANSACTION.toString()); + } + } } diff --git a/integrations/cdi/hibernate-cdi/src/main/java/module-info.java b/integrations/cdi/hibernate-cdi/src/main/java/module-info.java index f3a6b9b7753..d0b0ac43d11 100644 --- a/integrations/cdi/hibernate-cdi/src/main/java/module-info.java +++ b/integrations/cdi/hibernate-cdi/src/main/java/module-info.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2020, 2021 Oracle and/or its affiliates. + * Copyright (c) 2020, 2023 Oracle and/or its affiliates. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -23,12 +23,14 @@ * * @see io.helidon.integrations.cdi.hibernate.CDISEJtaPlatform */ +@SuppressWarnings({"requires-automatic", "requires-transitive-automatic"}) module io.helidon.integrations.cdi.hibernate { - requires jakarta.transaction; - requires java.sql; - requires jakarta.inject; - requires jakarta.cdi; - requires org.hibernate.orm.core; + + requires transitive jakarta.cdi; + requires transitive jakarta.inject; + requires jakarta.persistence; + requires transitive jakarta.transaction; + requires transitive org.hibernate.orm.core; exports io.helidon.integrations.cdi.hibernate; diff --git a/integrations/cdi/jpa-cdi/src/main/java/io/helidon/integrations/cdi/jpa/PersistenceExtension.java b/integrations/cdi/jpa-cdi/src/main/java/io/helidon/integrations/cdi/jpa/PersistenceExtension.java index 31767003a1d..33944c708e3 100644 --- a/integrations/cdi/jpa-cdi/src/main/java/io/helidon/integrations/cdi/jpa/PersistenceExtension.java +++ b/integrations/cdi/jpa-cdi/src/main/java/io/helidon/integrations/cdi/jpa/PersistenceExtension.java @@ -55,6 +55,7 @@ import jakarta.annotation.Priority; import jakarta.enterprise.context.Dependent; import jakarta.enterprise.context.spi.CreationalContext; +import jakarta.enterprise.event.Event; import jakarta.enterprise.event.Observes; import jakarta.enterprise.inject.Any; import jakarta.enterprise.inject.CreationException; @@ -151,6 +152,9 @@ public final class PersistenceExtension implements Extension { private static final Annotation[] EMPTY_ANNOTATION_ARRAY = new Annotation[0]; + private static final TypeLiteral> EVENT_PERSISTENCEUNITINFOBEAN_TYPELITERAL = + new TypeLiteral<>() {}; + private static final Logger LOGGER = Logger.getLogger(PersistenceExtension.class.getName()); @@ -997,7 +1001,7 @@ private void processPersistenceXmls(AfterBeanDiscovery event, } Supplier dataSourceProviderSupplier = () -> bm.createInstance().select(DataSourceProvider.class).get(); - PersistenceUnitInfo solePui = null; + PersistenceUnitInfoBean solePui = null; Supplier tempClassLoaderSupplier = classLoader instanceof URLClassLoader ucl ? () -> new URLClassLoader(ucl.getURLs()) : () -> classLoader; for (int puCount = 0; persistenceXmlUrls.hasMoreElements();) { @@ -1032,6 +1036,7 @@ private void processPersistenceXmls(AfterBeanDiscovery event, if (unitName == null || unitName.isBlank()) { unitName = DEFAULT_PERSISTENCE_UNIT_NAME; } + Named qualifier = NamedLiteral.of(unitName); // Provide support for, e.g.: // @Inject // @Named("test") @@ -1040,8 +1045,8 @@ private void processPersistenceXmls(AfterBeanDiscovery event, .beanClass(PersistenceUnitInfoBean.class) .addTransitiveTypeClosure(PersistenceUnitInfoBean.class) .scope(Singleton.class) - .qualifiers(NamedLiteral.of(unitName)) - .createWith(cc -> pui); + .qualifiers(qualifier) + .produceWith(i -> producePersistenceUnitInfoBean(i, pui, qualifier)); addPersistenceProviderBeanIfAbsent(event, pui, providers); if (puCount == 0) { solePui = pui; @@ -1056,17 +1061,18 @@ private void processPersistenceXmls(AfterBeanDiscovery event, assert soleUnitName != null; assert !soleUnitName.isBlank(); if (!soleUnitName.equals(DEFAULT_PERSISTENCE_UNIT_NAME)) { - PersistenceUnitInfo pui = solePui; + PersistenceUnitInfoBean pui = solePui; // Provide support for, e.g.: // @Inject // @Named("__DEFAULT__")) // private PersistenceUnitInfo persistenceUnitInfo; + Named qualifier = NamedLiteral.of(DEFAULT_PERSISTENCE_UNIT_NAME); event.addBean() .beanClass(PersistenceUnitInfoBean.class) .addTransitiveTypeClosure(PersistenceUnitInfoBean.class) .scope(Singleton.class) - .qualifiers(NamedLiteral.of(DEFAULT_PERSISTENCE_UNIT_NAME)) - .createWith(cc -> pui); + .qualifiers(qualifier) + .produceWith(i -> producePersistenceUnitInfoBean(i, pui, qualifier)); } } } @@ -1087,6 +1093,7 @@ private void processImplicitPersistenceUnits(AfterBeanDiscovery event, Iterable< } } } + Named qualifier = NamedLiteral.of(unitName); // Provide support for, e.g.: // @Inject // @Named("test") @@ -1095,8 +1102,8 @@ private void processImplicitPersistenceUnits(AfterBeanDiscovery event, Iterable< .beanClass(PersistenceUnitInfoBean.class) .addTransitiveTypeClosure(PersistenceUnitInfoBean.class) .scope(Singleton.class) - .qualifiers(NamedLiteral.of(unitName)) - .createWith(cc -> pui); + .qualifiers(qualifier) + .produceWith(i -> producePersistenceUnitInfoBean(i, pui, qualifier)); addPersistenceProviderBeanIfAbsent(event, pui, providers); if (puCount == 0) { solePui = pui; @@ -1112,6 +1119,7 @@ private void processImplicitPersistenceUnits(AfterBeanDiscovery event, Iterable< assert !soleUnitName.isBlank(); if (!soleUnitName.equals(DEFAULT_PERSISTENCE_UNIT_NAME)) { PersistenceUnitInfoBean pui = solePui; + Named qualifier = NamedLiteral.of(DEFAULT_PERSISTENCE_UNIT_NAME); // Provide support for, e.g.: // @Inject // @Named("__DEFAULT__") @@ -1120,8 +1128,8 @@ private void processImplicitPersistenceUnits(AfterBeanDiscovery event, Iterable< .beanClass(PersistenceUnitInfoBean.class) .addTransitiveTypeClosure(PersistenceUnitInfoBean.class) .scope(Singleton.class) - .qualifiers(NamedLiteral.of(DEFAULT_PERSISTENCE_UNIT_NAME)) - .createWith(cc -> pui); + .qualifiers(qualifier) + .produceWith(i -> producePersistenceUnitInfoBean(i, pui, qualifier)); } } } @@ -1316,6 +1324,14 @@ private static void disposeJtaExtendedEntityManager(JtaExtendedEntityManager em, containerManagedSelectionQualifiers); } + private static PersistenceUnitInfoBean producePersistenceUnitInfoBean(Instance instance, + PersistenceUnitInfoBean pui, + Annotation... qualifiers) { + // Permit arbitrary customization of the PersistenceUnitInfoBean right before it is produced. + instance.select(EVENT_PERSISTENCEUNITINFOBEAN_TYPELITERAL, qualifiers).get().fire(pui); + return pui; + } + private static EntityManagerFactory produceEntityManagerFactory(Instance instance) { BeanAttributes ba = instance.select(BEAN_ENTITYMANAGERFACTORY_TYPELITERAL).get(); Set selectionQualifiers = new HashSet<>(); diff --git a/integrations/jta/jdbc/src/main/java/io/helidon/integrations/jta/jdbc/JtaConnection.java b/integrations/jta/jdbc/src/main/java/io/helidon/integrations/jta/jdbc/JtaConnection.java index 875ff67eef7..6e04d3a2cd1 100644 --- a/integrations/jta/jdbc/src/main/java/io/helidon/integrations/jta/jdbc/JtaConnection.java +++ b/integrations/jta/jdbc/src/main/java/io/helidon/integrations/jta/jdbc/JtaConnection.java @@ -47,6 +47,7 @@ import javax.transaction.xa.Xid; import io.helidon.integrations.jdbc.ConditionallyCloseableConnection; +import io.helidon.integrations.jdbc.DelegatingConnection; import io.helidon.integrations.jdbc.SQLSupplier; import io.helidon.integrations.jdbc.UncheckedSQLException; @@ -304,6 +305,10 @@ class JtaConnection extends ConditionallyCloseableConnection { ? () -> new LocalXAResource(this::connectionFunction, exceptionConverter) : xaResourceSupplier; this.xidConsumer = xidConsumer == null ? JtaConnection::sink : xidConsumer; + if (LOGGER.isLoggable(Level.FINE)) { + LOGGER.logp(Level.FINE, this.getClass().getName(), "", + "Creating {0} using delegate {1} on thread {2}", new Object[] {this, delegate, Thread.currentThread()}); + } if (immediateEnlistment) { this.enlist(); } @@ -317,10 +322,12 @@ class JtaConnection extends ConditionallyCloseableConnection { @Override // ConditionallyCloseableConnection public final void setCloseable(boolean closeable) { - super.setCloseable(closeable); if (LOGGER.isLoggable(Level.FINER)) { LOGGER.entering(this.getClass().getName(), "setCloseable", closeable); + super.setCloseable(closeable); LOGGER.exiting(this.getClass().getName(), "setCloseable"); + } else { + super.setCloseable(closeable); } } @@ -355,6 +362,10 @@ public final String nativeSQL(String sql) throws SQLException { @Override // ConditionallyCloseableConnection public final void setAutoCommit(boolean autoCommit) throws SQLException { this.failWhenClosed(); + if (LOGGER.isLoggable(Level.FINE)) { + LOGGER.logp(Level.FINE, this.getClass().getName(), "setAutoCommit", + "Setting autoCommit on {0} to {1}", new Object[] {this, autoCommit}); + } this.enlist(); if (autoCommit && this.enlisted()) { // "SQLException...if...setAutoCommit(true) is called while participating in a distributed transaction" @@ -367,7 +378,12 @@ public final void setAutoCommit(boolean autoCommit) throws SQLException { public final boolean getAutoCommit() throws SQLException { this.failWhenClosed(); this.enlist(); - return super.getAutoCommit(); + boolean ac = super.getAutoCommit(); + if (LOGGER.isLoggable(Level.FINE)) { + LOGGER.logp(Level.FINE, this.getClass().getName(), "getAutoCommit", + "Getting autoCommit ({0}) on {1}", new Object[] {ac, this}); + } + return ac; } @Override // ConditionallyCloseableConnection @@ -1116,12 +1132,22 @@ private void enlist() throws SQLException { // Transaction#enlistResource(XAResource) in enlist() above.) private Connection connectionFunction(Xid xid) { this.xidConsumer.accept(xid); - return this.delegate(); + return new DelegatingConnection(this.delegate()) { + @Override + public void setAutoCommit(boolean autoCommit) throws SQLException { + if (LOGGER.isLoggable(Level.FINE)) { + LOGGER.logp(Level.FINE, "", "setAutoCommit", + "Setting autoCommit on anonymous DelegatingConnection {0} to {1}", + new Object[] {this, autoCommit}); + } + super.setAutoCommit(autoCommit); + } + }; } // (Used only by reference in enlist() above. Remember, this callback may be called by the TransactionManager on // any thread at any time for any reason.) - private void transactionCompleted(int commitedOrRolledBack) { + private void transactionCompleted(int committedOrRolledBack) { this.enlistment = null; // volatile write try { boolean closeWasPending = this.isClosePending(); diff --git a/integrations/jta/jdbc/src/main/java/io/helidon/integrations/jta/jdbc/LocalXAResource.java b/integrations/jta/jdbc/src/main/java/io/helidon/integrations/jta/jdbc/LocalXAResource.java index 3350700eab1..185fafceb81 100644 --- a/integrations/jta/jdbc/src/main/java/io/helidon/integrations/jta/jdbc/LocalXAResource.java +++ b/integrations/jta/jdbc/src/main/java/io/helidon/integrations/jta/jdbc/LocalXAResource.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2022 Oracle and/or its affiliates. + * Copyright (c) 2022, 2023 Oracle and/or its affiliates. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -644,7 +644,7 @@ private static Association prepare(Association a) { throw new UncheckedSQLException(e); } if (LOGGER.isLoggable(Level.FINER)) { - LOGGER.exiting(Association.class.getName(), "preapre", a); + LOGGER.exiting(Association.class.getName(), "prepare", a); } return a; } @@ -725,34 +725,40 @@ static record Association(BranchState branchState, // S5: Heuristically Completed Association(BranchState branchState, Xid xid, Connection connection) { - this(branchState, xid, false, connection); - } - - Association(BranchState branchState, Xid xid, boolean suspended, Connection connection) { - this(branchState, xid, suspended, connection, true /* JDBC default; will be set from connection anyway */); + this(branchState, xid, false, connection, autoCommit(connection)); } Association { Objects.requireNonNull(xid, "xid"); + boolean autoCommit = false; switch (branchState) { case IDLE: break; case ACTIVE: case HEURISTICALLY_COMPLETED: - case NON_EXISTENT_TRANSACTION: case PREPARED: case ROLLBACK_ONLY: if (suspended) { throw new IllegalArgumentException("suspended"); } break; + case NON_EXISTENT_TRANSACTION: + if (suspended) { + throw new IllegalArgumentException("suspended"); + } + autoCommit = priorAutoCommit; + break; default: throw new IllegalArgumentException("branchState: " + branchState); } try { - priorAutoCommit = connection.getAutoCommit(); - if (priorAutoCommit) { - connection.setAutoCommit(false); + if (connection.getAutoCommit() != autoCommit) { + if (LOGGER.isLoggable(Level.FINE)) { + LOGGER.logp(Level.FINE, Association.class.getName(), "", + "Setting autoCommit to {0} on connection {1}", + new Object[] {autoCommit, connection}); + } + connection.setAutoCommit(autoCommit); } } catch (SQLException sqlException) { throw new UncheckedSQLException(sqlException); @@ -984,14 +990,10 @@ private Association forgetAndReset() throws SQLException { } private Association reset() throws SQLException { - Connection connection = this.connection(); - connection.setAutoCommit(this.priorAutoCommit()); if (LOGGER.isLoggable(Level.FINE)) { LOGGER.logp(Level.FINE, this.getClass().getName(), "reset", - "Resetting; restored autoCommit to {0} on connection {1}", - new Object[] {this.priorAutoCommit(), connection}); - LOGGER.logp(Level.FINE, this.getClass().getName(), "reset", - "Transitioning Association {0} to NON_EXISTENT_TRANSACTION", this); + "Transitioning Association {0} from state {1} to state NON_EXISTENT_TRANSACTION", + new Object[] {this, this.branchState()}); } return new Association(BranchState.NON_EXISTENT_TRANSACTION, this.xid(), @@ -1000,6 +1002,14 @@ private Association reset() throws SQLException { this.priorAutoCommit()); } + private static boolean autoCommit(Connection c) { + try { + return c.getAutoCommit(); + } catch (SQLException e) { + throw new UncheckedSQLException(e); + } + } + // Transaction Branch States (XA specification, table 6-4): // S0: Non-existent Transaction // S1: Active