You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by michaelandrepearce <gi...@git.apache.org> on 2018/08/14 06:06:26 UTC
[GitHub] activemq-artemis pull request #2249: ARTEMIS-2023 Fix NPE
GitHub user michaelandrepearce opened a pull request:
https://github.com/apache/activemq-artemis/pull/2249
ARTEMIS-2023 Fix NPE
Add test to prove NPE and compatibility with 2.6.0
Fix decoding logic to avoid NPE
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/michaelandrepearce/activemq-artemis ARTEMIS-2023-NPE
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/2249.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2249
----
commit c6c5625c53526b5572a2ce494cbbeeca6e2d10c0
Author: Michael André Pearce <mi...@...>
Date: 2018-08-14T06:04:06Z
ARTEMIS-2023 Fix NPE
Add test to prove NPE and compatibility with 2.6.0
Fix decoding logic to avoid NPE
----
---
[GitHub] activemq-artemis pull request #2249: ARTEMIS-2023 Fix NPE
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/2249
---
[GitHub] activemq-artemis pull request #2249: ARTEMIS-2023 Fix NPE
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2249#discussion_r210016477
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/client/StoreConfigTest.java ---
@@ -289,6 +294,107 @@ public void testCreateQueue() throws Exception {
jmsServer.stop();
}
+ @Test
+ public void testCompatibilityWith260() {
+ List<String> transportConfigurations = new ArrayList<>();
+ transportConfigurations.add("tst");
+ ConnectionFactoryConfigurationImpl configuration = (ConnectionFactoryConfigurationImpl) new ConnectionFactoryConfigurationImpl();
+ configuration.setName("np").setConnectorNames(transportConfigurations);
+
+ ByteBuffer buffer = ByteBuffer.allocate(configuration.getEncodeSize());
+ ActiveMQBuffer activeMQBuffer = new ChannelBufferWrapper(Unpooled.wrappedBuffer(buffer));
+ activeMQBuffer.clear();
+ encodeVersion260(activeMQBuffer, configuration);
+ configuration.decode(activeMQBuffer);
+ }
+
+ public void encodeVersion260(final ActiveMQBuffer buffer, final ConnectionFactoryConfigurationImpl connectionFactoryConfiguration) {
--- End diff --
leave it with me.. I'm writing a test.. and I will amend yours manually.
---
[GitHub] activemq-artemis pull request #2249: ARTEMIS-2023 Fix NPE
Posted by jdanekrh <gi...@git.apache.org>.
Github user jdanekrh commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2249#discussion_r210198098
--- Diff: artemis-jms-server/src/main/java/org/apache/activemq/artemis/jms/server/config/impl/ConnectionFactoryConfigurationImpl.java ---
@@ -640,7 +640,7 @@ public void decode(final ActiveMQBuffer buffer) {
deserializationWhiteList = BufferHelper.readNullableSimpleStringAsString(buffer);
- enable1xPrefixes = buffer.readableBytes() > 0 ? buffer.readBoolean() : null;
+ enable1xPrefixes = buffer.readableBytes() > 0 ? buffer.readBoolean() : ActiveMQClient.DEFAULT_ENABLE_1X_PREFIXES;
--- End diff --
Btw, there was an ErrorProne warning for this,
```
[17:16:53][Step 3/3] artemis-jms-server/src/main/java/org/apache/activemq/artemis/jms/server/config/impl/ConnectionFactoryConfigurationImpl.java:643: error: [NullTernary] This conditional expression may evaluate to null, which will result in an NPE when the result is unboxed.
[17:16:53][Step 3/3] enable1xPrefixes = buffer.readableBytes() > 0 ? buffer.readBoolean() : null;
[17:16:53][Step 3/3] ^
[17:16:53][Step 3/3] (see http://errorprone.info/bugpattern/NullTernary)
```
---
[GitHub] activemq-artemis pull request #2249: ARTEMIS-2023 Fix NPE
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2249#discussion_r210022817
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/client/StoreConfigTest.java ---
@@ -289,6 +294,107 @@ public void testCreateQueue() throws Exception {
jmsServer.stop();
}
+ @Test
+ public void testCompatibilityWith260() {
+ List<String> transportConfigurations = new ArrayList<>();
+ transportConfigurations.add("tst");
+ ConnectionFactoryConfigurationImpl configuration = (ConnectionFactoryConfigurationImpl) new ConnectionFactoryConfigurationImpl();
+ configuration.setName("np").setConnectorNames(transportConfigurations);
+
+ ByteBuffer buffer = ByteBuffer.allocate(configuration.getEncodeSize());
+ ActiveMQBuffer activeMQBuffer = new ChannelBufferWrapper(Unpooled.wrappedBuffer(buffer));
+ activeMQBuffer.clear();
+ encodeVersion260(activeMQBuffer, configuration);
+ configuration.decode(activeMQBuffer);
+ }
+
+ public void encodeVersion260(final ActiveMQBuffer buffer, final ConnectionFactoryConfigurationImpl connectionFactoryConfiguration) {
--- End diff --
@michaelandrepearce merged... removed your test.. added mine.. take a look please.
---
[GitHub] activemq-artemis pull request #2249: ARTEMIS-2023 Fix NPE
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2249#discussion_r210023905
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/client/StoreConfigTest.java ---
@@ -289,6 +294,107 @@ public void testCreateQueue() throws Exception {
jmsServer.stop();
}
+ @Test
+ public void testCompatibilityWith260() {
+ List<String> transportConfigurations = new ArrayList<>();
+ transportConfigurations.add("tst");
+ ConnectionFactoryConfigurationImpl configuration = (ConnectionFactoryConfigurationImpl) new ConnectionFactoryConfigurationImpl();
+ configuration.setName("np").setConnectorNames(transportConfigurations);
+
+ ByteBuffer buffer = ByteBuffer.allocate(configuration.getEncodeSize());
+ ActiveMQBuffer activeMQBuffer = new ChannelBufferWrapper(Unpooled.wrappedBuffer(buffer));
+ activeMQBuffer.clear();
+ encodeVersion260(activeMQBuffer, configuration);
+ configuration.decode(activeMQBuffer);
+ }
+
+ public void encodeVersion260(final ActiveMQBuffer buffer, final ConnectionFactoryConfigurationImpl connectionFactoryConfiguration) {
--- End diff --
Already noticed, Looks fine and dandy. Probably worth adding another version at some point maybe after 2.6.3 to the versions to be compatibility checked, just as lots of recent serialisation changes and additions
---
[GitHub] activemq-artemis pull request #2249: ARTEMIS-2023 Fix NPE
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2249#discussion_r210016740
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/client/StoreConfigTest.java ---
@@ -289,6 +294,107 @@ public void testCreateQueue() throws Exception {
jmsServer.stop();
}
+ @Test
+ public void testCompatibilityWith260() {
+ List<String> transportConfigurations = new ArrayList<>();
+ transportConfigurations.add("tst");
+ ConnectionFactoryConfigurationImpl configuration = (ConnectionFactoryConfigurationImpl) new ConnectionFactoryConfigurationImpl();
+ configuration.setName("np").setConnectorNames(transportConfigurations);
+
+ ByteBuffer buffer = ByteBuffer.allocate(configuration.getEncodeSize());
+ ActiveMQBuffer activeMQBuffer = new ChannelBufferWrapper(Unpooled.wrappedBuffer(buffer));
+ activeMQBuffer.clear();
+ encodeVersion260(activeMQBuffer, configuration);
+ configuration.decode(activeMQBuffer);
+ }
+
+ public void encodeVersion260(final ActiveMQBuffer buffer, final ConnectionFactoryConfigurationImpl connectionFactoryConfiguration) {
--- End diff --
BTW: I didn't say this. I thought it went without saying.. thanks a lot for pointing it out!
---
[GitHub] activemq-artemis pull request #2249: ARTEMIS-2023 Fix NPE
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2249#discussion_r210009695
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/client/StoreConfigTest.java ---
@@ -289,6 +294,107 @@ public void testCreateQueue() throws Exception {
jmsServer.stop();
}
+ @Test
+ public void testCompatibilityWith260() {
+ List<String> transportConfigurations = new ArrayList<>();
+ transportConfigurations.add("tst");
+ ConnectionFactoryConfigurationImpl configuration = (ConnectionFactoryConfigurationImpl) new ConnectionFactoryConfigurationImpl();
+ configuration.setName("np").setConnectorNames(transportConfigurations);
+
+ ByteBuffer buffer = ByteBuffer.allocate(configuration.getEncodeSize());
+ ActiveMQBuffer activeMQBuffer = new ChannelBufferWrapper(Unpooled.wrappedBuffer(buffer));
+ activeMQBuffer.clear();
+ encodeVersion260(activeMQBuffer, configuration);
+ configuration.decode(activeMQBuffer);
+ }
+
+ public void encodeVersion260(final ActiveMQBuffer buffer, final ConnectionFactoryConfigurationImpl connectionFactoryConfiguration) {
--- End diff --
I think you should use the compatibility tests for this instead. disagree with the test itself here.
---
[GitHub] activemq-artemis pull request #2249: ARTEMIS-2023 Fix NPE
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2249#discussion_r210014188
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/client/StoreConfigTest.java ---
@@ -289,6 +294,107 @@ public void testCreateQueue() throws Exception {
jmsServer.stop();
}
+ @Test
+ public void testCompatibilityWith260() {
+ List<String> transportConfigurations = new ArrayList<>();
+ transportConfigurations.add("tst");
+ ConnectionFactoryConfigurationImpl configuration = (ConnectionFactoryConfigurationImpl) new ConnectionFactoryConfigurationImpl();
+ configuration.setName("np").setConnectorNames(transportConfigurations);
+
+ ByteBuffer buffer = ByteBuffer.allocate(configuration.getEncodeSize());
+ ActiveMQBuffer activeMQBuffer = new ChannelBufferWrapper(Unpooled.wrappedBuffer(buffer));
+ activeMQBuffer.clear();
+ encodeVersion260(activeMQBuffer, configuration);
+ configuration.decode(activeMQBuffer);
+ }
+
+ public void encodeVersion260(final ActiveMQBuffer buffer, final ConnectionFactoryConfigurationImpl connectionFactoryConfiguration) {
--- End diff --
It was to just demo the introduced bug by recent merged pr. I can simply remove and leave the fix
---