From fdfb6974be4b28005082465786397aa95089ab1f Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Wed, 17 Apr 2024 14:47:08 +0100 Subject: [PATCH] Polish "Add Spring Pulsar transaction support" See gh- --- .../pulsar/PulsarProperties.java | 50 ++----------------- .../pulsar/PulsarPropertiesMapper.java | 19 +++++-- .../pulsar/PulsarAutoConfigurationTests.java | 14 +++--- .../pulsar/PulsarPropertiesTests.java | 40 --------------- 4 files changed, 26 insertions(+), 97 deletions(-) diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/pulsar/PulsarProperties.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/pulsar/PulsarProperties.java index f0974e92353a..97021d31e24d 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/pulsar/PulsarProperties.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/pulsar/PulsarProperties.java @@ -36,7 +36,6 @@ import org.springframework.boot.context.properties.ConfigurationProperties; import org.springframework.boot.context.properties.NestedConfigurationProperty; -import org.springframework.boot.context.properties.source.InvalidConfigurationPropertyValueException; import org.springframework.util.Assert; /** @@ -104,14 +103,6 @@ public Template getTemplate() { return this.template; } - /** - * Whether transactions are enabled for either the template or the listener. - * @return whether transactions are enabled for either the template or the listener - */ - boolean isTransactionEnabled() { - return this.template.getTransaction().isEnabled() || this.listener.getTransaction().isEnabled(); - } - public static class Client { /** @@ -775,7 +766,7 @@ public static class Listener { /** * Transaction settings. */ - private final Transaction transaction = new ListenerTransaction(); + private final Transaction transaction = new Transaction(); public SchemaType getSchemaType() { return this.schemaType; @@ -879,7 +870,7 @@ public static class Template { /** * Transaction settings. */ - private final Transaction transaction = new TemplateTransaction(); + private final Transaction transaction = new Transaction(); public boolean isObservationsEnabled() { return this.observationsEnabled; @@ -895,7 +886,7 @@ public Transaction getTransaction() { } - public abstract static class Transaction { + public static class Transaction { /** * Whether transactions are enabled for the component. @@ -937,41 +928,6 @@ public void setTimeout(Duration timeout) { this.timeout = timeout; } - void validate() { - if (this.required && !this.enabled) { - String requiredProp = "%s.required".formatted(this.propertyPath()); - String enabledProp = "%s.enabled".formatted(this.propertyPath()); - throw new InvalidConfigurationPropertyValueException(requiredProp, this.required, - "Transactions must be enabled in order to be required. " - + "Either set %s to 'true' or make transactions optional by setting %s to 'false'" - .formatted(enabledProp, requiredProp)); - } - } - - /** - * Gets the property path that the transaction properties are mapped to. - * @return the property path that the transaction properties are mapped to - */ - protected abstract String propertyPath(); - - } - - static class TemplateTransaction extends Transaction { - - @Override - protected String propertyPath() { - return "spring.pulsar.template.transaction"; - } - - } - - static class ListenerTransaction extends Transaction { - - @Override - protected String propertyPath() { - return "spring.pulsar.listener.transaction"; - } - } public static class Authentication { diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/pulsar/PulsarPropertiesMapper.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/pulsar/PulsarPropertiesMapper.java index 90590c670e81..fb8ac02253b4 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/pulsar/PulsarPropertiesMapper.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/pulsar/PulsarPropertiesMapper.java @@ -39,6 +39,7 @@ import org.apache.pulsar.client.impl.AutoClusterFailover.AutoClusterFailoverBuilderImpl; import org.springframework.boot.context.properties.PropertyMapper; +import org.springframework.boot.context.properties.source.InvalidConfigurationPropertyValueException; import org.springframework.pulsar.core.PulsarTemplate; import org.springframework.pulsar.listener.PulsarContainerProperties; import org.springframework.pulsar.reader.PulsarReaderContainerProperties; @@ -65,7 +66,8 @@ void customizeClientBuilder(ClientBuilder clientBuilder, PulsarConnectionDetails map.from(properties::getConnectionTimeout).to(timeoutProperty(clientBuilder::connectionTimeout)); map.from(properties::getOperationTimeout).to(timeoutProperty(clientBuilder::operationTimeout)); map.from(properties::getLookupTimeout).to(timeoutProperty(clientBuilder::lookupTimeout)); - if (this.properties.isTransactionEnabled()) { + if (this.properties.getTemplate().getTransaction().isEnabled() + || this.properties.getListener().getTransaction().isEnabled()) { clientBuilder.enableTransaction(true); } customizeAuthentication(properties.getAuthentication(), clientBuilder::authentication); @@ -163,7 +165,7 @@ void customizeProducerBuilder(ProducerBuilder producerBuilder) { void customizeTemplate(PulsarTemplate template) { PulsarProperties.Transaction properties = this.properties.getTemplate().getTransaction(); - properties.validate(); + validate(properties, "spring.pulsar.template.transaction"); PropertyMapper map = PropertyMapper.get().alwaysApplyingWhenNonNull(); map.from(properties::isEnabled).to(template.transactions()::setEnabled); map.from(properties::isRequired).to(template.transactions()::setRequired); @@ -214,13 +216,24 @@ private void customizePulsarContainerListenerProperties(PulsarContainerPropertie private void customizePulsarContainerTransactionProperties(PulsarContainerProperties containerProperties) { PulsarProperties.Transaction properties = this.properties.getListener().getTransaction(); - properties.validate(); + validate(properties, "spring.pulsar.listener.transaction"); PropertyMapper map = PropertyMapper.get().alwaysApplyingWhenNonNull(); map.from(properties::isEnabled).to(containerProperties.transactions()::setEnabled); map.from(properties::isRequired).to(containerProperties.transactions()::setRequired); map.from(properties::getTimeout).to(containerProperties.transactions()::setTimeout); } + private void validate(PulsarProperties.Transaction properties, String prefix) { + if (properties.isRequired() && !properties.isEnabled()) { + String requiredProp = "%s.required".formatted(prefix); + String enabledProp = "%s.enabled".formatted(prefix); + throw new InvalidConfigurationPropertyValueException(requiredProp, properties.isRequired(), + "When transactions are required they must also be enabled. " + + "Either set %s to 'true' or make transactions optional by setting %s to 'false'" + .formatted(enabledProp, requiredProp)); + } + } + void customizeReaderBuilder(ReaderBuilder readerBuilder) { PulsarProperties.Reader properties = this.properties.getReader(); PropertyMapper map = PropertyMapper.get().alwaysApplyingWhenNonNull(); diff --git a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/pulsar/PulsarAutoConfigurationTests.java b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/pulsar/PulsarAutoConfigurationTests.java index 9c54941c6ebb..19da431643b5 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/pulsar/PulsarAutoConfigurationTests.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/pulsar/PulsarAutoConfigurationTests.java @@ -629,20 +629,20 @@ class TransactionManagerTests { @Test @SuppressWarnings("unchecked") - void whenHasUserDefinedBeanDoesNotAutoConfigureBean() { + void whenUserHasDefinedATransactionManagerTheAutoConfigurationBacksOff() { PulsarAwareTransactionManager txnMgr = mock(PulsarAwareTransactionManager.class); this.contextRunner.withBean("customTransactionManager", PulsarAwareTransactionManager.class, () -> txnMgr) .run((context) -> assertThat(context).getBean(PulsarAwareTransactionManager.class).isSameAs(txnMgr)); } @Test - void whenNoPropertiesSetDoesNotAutoconfigureBean() { + void whenNoPropertiesAreSetTransactionManagerShouldNotBeDefined() { this.contextRunner .run((context) -> assertThat(context).doesNotHaveBean(PulsarAwareTransactionManager.class)); } @Test - void whenListenerAndTemplateDisablesTransactionsDoesNotAutoconfigureBean() { + void whenListenerAndTemplateDisableTransactionsTransactionManagerIsNotAutoConfigured() { this.contextRunner .withPropertyValues("spring.pulsar.listener.transaction.enabled=false", "spring.pulsar.template.transaction.enabled=false") @@ -650,7 +650,7 @@ void whenListenerAndTemplateDisablesTransactionsDoesNotAutoconfigureBean() { } @Test - void whenListenerEnablesTransactionsAutoconfiguresBean() { + void whenListenerEnablesTransactionsTransactionManagerIsAutoConfigured() { this.contextRunner .withPropertyValues("spring.pulsar.listener.transaction.enabled=true", "spring.pulsar.template.transaction.enabled=false") @@ -658,7 +658,7 @@ void whenListenerEnablesTransactionsAutoconfiguresBean() { } @Test - void whenTemplateEnablesTransactionsAutoconfiguresBean() { + void whenTemplateEnablesTransactionsTransactionMAnagerIsAutoConfigured() { this.contextRunner .withPropertyValues("spring.pulsar.listener.transaction.enabled=false", "spring.pulsar.template.transaction.enabled=true") @@ -674,7 +674,7 @@ void whenTemplateRequiresTransactionsThenTransactionsMustBeEnabled() { .getFailure() .hasMessageEndingWith( "Property spring.pulsar.template.transaction.required with value 'true' is invalid: " - + "Transactions must be enabled in order to be required. Either set " + + "When transactions are required they must also be enabled. Either set " + "spring.pulsar.template.transaction.enabled to 'true' or make transactions " + "optional by setting spring.pulsar.template.transaction.required to 'false'")); } @@ -688,7 +688,7 @@ void whenListenerRequiresTransactionsThenTransactionsMustBeEnabled() { .getFailure() .hasMessageEndingWith( "Property spring.pulsar.listener.transaction.required with value 'true' is invalid: " - + "Transactions must be enabled in order to be required. Either set " + + "When transactions are required they must also be enabled. Either set " + "spring.pulsar.listener.transaction.enabled to 'true' or make transactions " + "optional by setting spring.pulsar.listener.transaction.required to 'false'")); } diff --git a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/pulsar/PulsarPropertiesTests.java b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/pulsar/PulsarPropertiesTests.java index 456325d2f15e..6be3326de70c 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/pulsar/PulsarPropertiesTests.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/pulsar/PulsarPropertiesTests.java @@ -408,44 +408,4 @@ void bind() { } - @Nested - class TransactionProperties { - - @Test - void transactionsEnabledWhenListenerAndTemplateBothEnabled() { - PulsarProperties properties = new PulsarProperties(); - properties.getListener().getTransaction().setEnabled(true); - properties.getTemplate().getTransaction().setEnabled(true); - assertThat(properties.isTransactionEnabled()).isTrue(); - - } - - @Test - void transactionsEnabledWhenListenerEnabledAndTemplateDisabled() { - PulsarProperties properties = new PulsarProperties(); - properties.getListener().getTransaction().setEnabled(true); - properties.getTemplate().getTransaction().setEnabled(false); - assertThat(properties.isTransactionEnabled()).isTrue(); - - } - - @Test - void transactionsEnabledWhenListenerDisabledAndTemplateEnabled() { - PulsarProperties properties = new PulsarProperties(); - properties.getListener().getTransaction().setEnabled(false); - properties.getTemplate().getTransaction().setEnabled(true); - assertThat(properties.isTransactionEnabled()).isTrue(); - - } - - void transactionsDisabledWhenListenerAndTemplateBothDisabled() { - PulsarProperties properties = new PulsarProperties(); - properties.getListener().getTransaction().setEnabled(false); - properties.getTemplate().getTransaction().setEnabled(false); - assertThat(properties.isTransactionEnabled()).isFalse(); - - } - - } - }