This is an automated email from the ASF dual-hosted git repository. robbie pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/activemq-artemis.git
The following commit(s) were added to refs/heads/main by this push: new 1a83220711 ARTEMIS-4910 fix divert config encoding 1a83220711 is described below commit 1a8322071168e6d33053cc47dd8f657280c0ed64 Author: Justin Bertram <jbert...@apache.org> AuthorDate: Tue Jul 9 16:28:44 2024 -0500 ARTEMIS-4910 fix divert config encoding This commit fixes 4 distinct issues with divert configuration encoding: - The encoding size was calculated incorrectly in three ways: - Using 0 instead of `DataConstants.SIZE_NULL` when no transformer is defined resulting in a calculated size discrepancy of -1. - Using `DataConstants.INT` instead of `DataConstants.SIZE_INT` for the number of transformer properties resulting in a calculated size discrepancy of +2 when using a configuration with a transformer. - Using `BufferHelper.sizeOfNullableBoolean` instead of `DataConstants.SIZE_BOOLEAN` for the `exclusive` property resulting in a calculated discrepancy of +1. - Encoding was using `writeString` instead of `writeNullableString` for the name of the transformer class resulting in an actual buffer size discrepancy of -1. Aside from these fixes this commit also: - Updates the divert storage test to force a reload from disk which will also trigger this problem. - Adds new tests specifically for encoding & decoding include a test to verify known bytes during encoding. Lastly, it's worth noting that this *won't fix* bad data that was already stored to disk by an older broker or bad data that comes over the wire from an older broker (e.g. if a older primary broker paired with a newer backup). --- .../artemis/core/config/DivertConfiguration.java | 16 +- .../config/DivertConfigurationEncodingTest.java | 217 +++++++++++++++++++++ .../DivertConfigurationStorageTest.java | 33 +--- .../persistence/StorageManagerTestBase.java | 11 +- 4 files changed, 242 insertions(+), 35 deletions(-) diff --git a/artemis-server/src/main/java/org/apache/activemq/artemis/core/config/DivertConfiguration.java b/artemis-server/src/main/java/org/apache/activemq/artemis/core/config/DivertConfiguration.java index a10e97774f..64127082c9 100644 --- a/artemis-server/src/main/java/org/apache/activemq/artemis/core/config/DivertConfiguration.java +++ b/artemis-server/src/main/java/org/apache/activemq/artemis/core/config/DivertConfiguration.java @@ -217,21 +217,22 @@ public class DivertConfiguration implements Serializable, EncodingSupport { @Override public int getEncodeSize() { - int transformerSize = 0; + int transformerSize; if (transformerConfiguration != null) { - transformerSize += BufferHelper.sizeOfNullableString(transformerConfiguration.getClassName()); - transformerSize += DataConstants.INT; - Map<String, String> properties = transformerConfiguration.getProperties(); - for (Map.Entry<String, String> entry : properties.entrySet()) { + transformerSize = BufferHelper.sizeOfNullableString(transformerConfiguration.getClassName()); + transformerSize += DataConstants.SIZE_INT; + for (Map.Entry<String, String> entry : transformerConfiguration.getProperties().entrySet()) { transformerSize += BufferHelper.sizeOfNullableString(entry.getKey()); transformerSize += BufferHelper.sizeOfNullableString(entry.getValue()); } + } else { + transformerSize = DataConstants.SIZE_NULL; } int size = BufferHelper.sizeOfNullableString(name) + BufferHelper.sizeOfNullableString(address) + BufferHelper.sizeOfNullableString(forwardingAddress) + BufferHelper.sizeOfNullableString(routingName) + - BufferHelper.sizeOfNullableBoolean(exclusive) + + DataConstants.SIZE_BOOLEAN + BufferHelper.sizeOfNullableString(filterString) + DataConstants.SIZE_BYTE + transformerSize; return size; @@ -247,7 +248,7 @@ public class DivertConfiguration implements Serializable, EncodingSupport { buffer.writeNullableString(filterString); buffer.writeByte(routingType != null ? routingType.getType() : ComponentConfigurationRoutingType.valueOf(ActiveMQDefaultConfiguration.getDefaultDivertRoutingType()).getType()); if (transformerConfiguration != null) { - buffer.writeString(transformerConfiguration.getClassName()); + buffer.writeNullableString(transformerConfiguration.getClassName()); Map<String, String> properties = transformerConfiguration.getProperties(); buffer.writeInt(properties.size()); for (Map.Entry<String, String> entry : properties.entrySet()) { @@ -280,7 +281,6 @@ public class DivertConfiguration implements Serializable, EncodingSupport { for (int i = 0; i < propsSize; i++) { transformerConfiguration.getProperties().put(buffer.readNullableString(), buffer.readNullableString()); } - } } } diff --git a/artemis-server/src/test/java/org/apache/activemq/artemis/core/config/DivertConfigurationEncodingTest.java b/artemis-server/src/test/java/org/apache/activemq/artemis/core/config/DivertConfigurationEncodingTest.java new file mode 100644 index 0000000000..d7e58fceb2 --- /dev/null +++ b/artemis-server/src/test/java/org/apache/activemq/artemis/core/config/DivertConfigurationEncodingTest.java @@ -0,0 +1,217 @@ +/* + * 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 + * + * http://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.apache.activemq.artemis.core.config; + +import java.util.Map; + +import org.apache.activemq.artemis.api.core.ActiveMQBuffer; +import org.apache.activemq.artemis.api.core.ActiveMQBuffers; +import org.apache.activemq.artemis.core.server.ComponentConfigurationRoutingType; +import org.apache.activemq.artemis.utils.DataConstants; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; + +public class DivertConfigurationEncodingTest { + + @Test + public void testEncodedBytes() { + final String name = "aa"; + final String address = "bb"; + final String forwardingAddress = "cc"; + final String routingName = "dd"; + final boolean exclusive = true; + final String filterString = "ee"; + final ComponentConfigurationRoutingType routingType = ComponentConfigurationRoutingType.ANYCAST; + final String transformerClass = "ff"; + final String transformerKey = "gg"; + final String transformerValue = "hh"; + final TransformerConfiguration myDivertTransformer = new TransformerConfiguration(transformerClass); + + myDivertTransformer.getProperties().put(transformerKey, transformerValue); + + DivertConfiguration configuration = new DivertConfiguration() + .setName(name) + .setAddress(address) + .setForwardingAddress(forwardingAddress) + .setFilterString(filterString) + .setRoutingName(routingName) + .setExclusive(exclusive) + .setRoutingType(routingType) + .setTransformerConfiguration(myDivertTransformer); + + int encodeSize = configuration.getEncodeSize(); + ActiveMQBuffer data = ActiveMQBuffers.fixedBuffer(encodeSize); + configuration.encode(data); + assertEquals(encodeSize, data.writerIndex()); + + // name + byte[] read = new byte[9]; + data.readBytes(read, 0, 9); + assertArrayEquals(new byte[] {DataConstants.NOT_NULL, 0, 0, 0, 2, 0, 97, 0, 97}, read); + + // address + read = new byte[9]; + data.readBytes(read, 0, 9); + assertArrayEquals(new byte[] {DataConstants.NOT_NULL, 0, 0, 0, 2, 0, 98, 0, 98}, read); + + // forwardingAddress + read = new byte[9]; + data.readBytes(read, 0, 9); + assertArrayEquals(new byte[] {DataConstants.NOT_NULL, 0, 0, 0, 2, 0, 99, 0, 99}, read); + + // routingName + read = new byte[9]; + data.readBytes(read, 0, 9); + assertArrayEquals(new byte[] {DataConstants.NOT_NULL, 0, 0, 0, 2, 0, 100, 0, 100}, read); + + // exclusive + assertEquals((byte) -1, data.readByte()); + + // filterString + read = new byte[9]; + data.readBytes(read, 0, 9); + assertArrayEquals(new byte[] {DataConstants.NOT_NULL, 0, 0, 0, 2, 0, 101, 0, 101}, read); + + // routingType + assertEquals(ComponentConfigurationRoutingType.ANYCAST.getType(), data.readByte()); + + // transformerClassName + read = new byte[9]; + data.readBytes(read, 0, 9); + assertArrayEquals(new byte[] {DataConstants.NOT_NULL, 0, 0, 0, 2, 0, 102, 0, 102}, read); + + // number of transformer properties + read = new byte[4]; + data.readBytes(read, 0, 4); + assertArrayEquals(new byte[] {0, 0, 0, 1}, read); + + // transformer key name + read = new byte[9]; + data.readBytes(read, 0, 9); + assertArrayEquals(new byte[] {DataConstants.NOT_NULL, 0, 0, 0, 2, 0, 103, 0, 103}, read); + + // transformer key value + read = new byte[9]; + data.readBytes(read, 0, 9); + assertArrayEquals(new byte[] {DataConstants.NOT_NULL, 0, 0, 0, 2, 0, 104, 0, 104}, read); + + assertEquals(0, data.readableBytes()); + } + + @Test + public void testEncodeDecodeWithTransformer() { + TransformerConfiguration mytransformer = new TransformerConfiguration("myDivertTransformer"); + mytransformer.getProperties().put("key1", "prop1"); + mytransformer.getProperties().put("key2", "prop2"); + mytransformer.getProperties().put("key3", "prop3"); + + DivertConfiguration configuration = new DivertConfiguration() + .setName("name") + .setAddress("address") + .setForwardingAddress("forward") + .setFilterString("foo='foo'") + .setRoutingName("myDivertRoutingName") + .setExclusive(true) + .setRoutingType(ComponentConfigurationRoutingType.STRIP) + .setTransformerConfiguration(mytransformer); + + int encodeSize = configuration.getEncodeSize(); + ActiveMQBuffer data = ActiveMQBuffers.fixedBuffer(encodeSize); + configuration.encode(data); + DivertConfiguration persistedDivertConfiguration = new DivertConfiguration(); + persistedDivertConfiguration.decode(data); + + assertEquals(configuration.getName(), persistedDivertConfiguration.getName()); + assertEquals(configuration.getAddress(), persistedDivertConfiguration.getAddress()); + assertEquals(configuration.getForwardingAddress(), persistedDivertConfiguration.getForwardingAddress()); + assertEquals(configuration.getFilterString(), persistedDivertConfiguration.getFilterString()); + assertEquals(configuration.getRoutingName(), persistedDivertConfiguration.getRoutingName()); + assertEquals(configuration.isExclusive(), persistedDivertConfiguration.isExclusive()); + assertEquals(configuration.getRoutingType(), persistedDivertConfiguration.getRoutingType()); + assertNotNull(persistedDivertConfiguration.getTransformerConfiguration()); + assertEquals("myDivertTransformer", persistedDivertConfiguration.getTransformerConfiguration().getClassName()); + Map<String, String> properties = persistedDivertConfiguration.getTransformerConfiguration().getProperties(); + assertEquals(3, properties.size()); + assertEquals("prop1", properties.get("key1")); + assertEquals("prop2", properties.get("key2")); + assertEquals("prop3", properties.get("key3")); + } + + @Test + public void testEncodeDecodeWithTransformerWithNoProperties() { + TransformerConfiguration mytransformer = new TransformerConfiguration("myDivertTransformer"); + + DivertConfiguration configuration = new DivertConfiguration() + .setName("name") + .setAddress("address") + .setForwardingAddress("forward") + .setFilterString("foo='foo'") + .setRoutingName("myDivertRoutingName") + .setExclusive(true) + .setRoutingType(ComponentConfigurationRoutingType.MULTICAST) + .setTransformerConfiguration(mytransformer); + + int encodeSize = configuration.getEncodeSize(); + ActiveMQBuffer data = ActiveMQBuffers.fixedBuffer(encodeSize); + configuration.encode(data); + DivertConfiguration persistedDivertConfiguration = new DivertConfiguration(); + persistedDivertConfiguration.decode(data); + + assertEquals(configuration.getName(), persistedDivertConfiguration.getName()); + assertEquals(configuration.getAddress(), persistedDivertConfiguration.getAddress()); + assertEquals(configuration.getForwardingAddress(), persistedDivertConfiguration.getForwardingAddress()); + assertEquals(configuration.getFilterString(), persistedDivertConfiguration.getFilterString()); + assertEquals(configuration.getRoutingName(), persistedDivertConfiguration.getRoutingName()); + assertEquals(configuration.isExclusive(), persistedDivertConfiguration.isExclusive()); + assertEquals(configuration.getRoutingType(), persistedDivertConfiguration.getRoutingType()); + assertNotNull(persistedDivertConfiguration.getTransformerConfiguration()); + assertEquals("myDivertTransformer", persistedDivertConfiguration.getTransformerConfiguration().getClassName()); + Map<String, String> properties = persistedDivertConfiguration.getTransformerConfiguration().getProperties(); + assertEquals(0, properties.size()); + } + + @Test + public void testEncodeDecodeWithoutTransformer() { + DivertConfiguration configuration = new DivertConfiguration() + .setName("name") + .setAddress("address") + .setForwardingAddress("forward") + .setFilterString("foo='foo'") + .setExclusive(true) + .setRoutingName("myDivertRoutingName") + .setRoutingType(ComponentConfigurationRoutingType.PASS); + + int encodeSize = configuration.getEncodeSize(); + ActiveMQBuffer data = ActiveMQBuffers.fixedBuffer(encodeSize); + configuration.encode(data); + DivertConfiguration decodedDivertConfiguration = new DivertConfiguration(); + decodedDivertConfiguration.decode(data); + + assertEquals(configuration.getName(), decodedDivertConfiguration.getName()); + assertEquals(configuration.getAddress(), decodedDivertConfiguration.getAddress()); + assertEquals(configuration.getForwardingAddress(), decodedDivertConfiguration.getForwardingAddress()); + assertEquals(configuration.getFilterString(), decodedDivertConfiguration.getFilterString()); + assertEquals(configuration.getRoutingName(), decodedDivertConfiguration.getRoutingName()); + assertEquals(configuration.isExclusive(), decodedDivertConfiguration.isExclusive()); + assertEquals(configuration.getRoutingType(), decodedDivertConfiguration.getRoutingType()); + assertNull(decodedDivertConfiguration.getTransformerConfiguration()); + } +} diff --git a/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/persistence/DivertConfigurationStorageTest.java b/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/persistence/DivertConfigurationStorageTest.java index 6cc697a9b6..0c4d751e8e 100644 --- a/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/persistence/DivertConfigurationStorageTest.java +++ b/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/persistence/DivertConfigurationStorageTest.java @@ -16,21 +16,20 @@ */ package org.apache.activemq.artemis.tests.integration.persistence; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertNull; +import java.util.List; +import java.util.Map; import org.apache.activemq.artemis.core.config.DivertConfiguration; import org.apache.activemq.artemis.core.config.StoreConfiguration; import org.apache.activemq.artemis.core.config.TransformerConfiguration; import org.apache.activemq.artemis.core.persistence.config.PersistedDivertConfiguration; import org.apache.activemq.artemis.tests.extensions.parameterized.ParameterizedTestExtension; -import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.TestTemplate; import org.junit.jupiter.api.extension.ExtendWith; -import java.util.List; -import java.util.Map; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; //Parameters set in super class @ExtendWith(ParameterizedTestExtension.class) @@ -40,12 +39,6 @@ public class DivertConfigurationStorageTest extends StorageManagerTestBase { super(storeType); } - @Override - @BeforeEach - public void setUp() throws Exception { - super.setUp(); - } - @TestTemplate public void testStoreDivertConfiguration() throws Exception { createStorage(); @@ -64,9 +57,7 @@ public class DivertConfigurationStorageTest extends StorageManagerTestBase { journal.storeDivertConfiguration(new PersistedDivertConfiguration(configuration)); - journal.stop(); - - journal.start(); + rebootStorage(); List<PersistedDivertConfiguration> divertConfigurations = journal.recoverDivertConfigurations(); @@ -85,10 +76,6 @@ public class DivertConfigurationStorageTest extends StorageManagerTestBase { assertEquals("prop1", properties.get("key1")); assertEquals("prop2", properties.get("key2")); assertEquals("prop3", properties.get("key3")); - journal.stop(); - - journal = null; - } @TestTemplate @@ -104,9 +91,7 @@ public class DivertConfigurationStorageTest extends StorageManagerTestBase { journal.storeDivertConfiguration(new PersistedDivertConfiguration(configuration)); - journal.stop(); - - journal.start(); + rebootStorage(); List<PersistedDivertConfiguration> divertConfigurations = journal.recoverDivertConfigurations(); @@ -119,9 +104,5 @@ public class DivertConfigurationStorageTest extends StorageManagerTestBase { assertEquals(configuration.getForwardingAddress(), persistedDivertConfiguration.getDivertConfiguration().getForwardingAddress()); assertEquals(configuration.getRoutingName(), persistedDivertConfiguration.getDivertConfiguration().getRoutingName()); assertNull(persistedDivertConfiguration.getDivertConfiguration().getTransformerConfiguration()); - journal.stop(); - - journal = null; - } } diff --git a/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/persistence/StorageManagerTestBase.java b/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/persistence/StorageManagerTestBase.java index a30f378109..a781fcaeca 100644 --- a/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/persistence/StorageManagerTestBase.java +++ b/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/persistence/StorageManagerTestBase.java @@ -112,6 +112,16 @@ public abstract class StorageManagerTestBase extends ActiveMQTestBase { journal.loadMessageJournal(new FakePostOffice(), null, null, null, null, null, null, null, new FakeJournalLoader()); } + /** + * This forces a reload of the journal from disk + * + * @throws Exception + */ + protected void rebootStorage() throws Exception { + journal.stop(); + createStorage(); + } + /** * @param configuration */ @@ -129,5 +139,4 @@ public abstract class StorageManagerTestBase extends ActiveMQTestBase { addActiveMQComponent(jsm); return jsm; } - } --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@activemq.apache.org For additional commands, e-mail: commits-h...@activemq.apache.org For further information, visit: https://activemq.apache.org/contact