activemq-artemis git commit: ARTEMIS-2085 - Improve validation of MDB activation config properties values

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

activemq-artemis git commit: ARTEMIS-2085 - Improve validation of MDB activation config properties values

jbertram
Repository: activemq-artemis
Updated Branches:
  refs/heads/master 14c41ecfc -> 39b177d02


ARTEMIS-2085 - Improve validation of MDB activation config properties values


Project: http://git-wip-us.apache.org/repos/asf/activemq-artemis/repo
Commit: http://git-wip-us.apache.org/repos/asf/activemq-artemis/commit/39b177d0
Tree: http://git-wip-us.apache.org/repos/asf/activemq-artemis/tree/39b177d0
Diff: http://git-wip-us.apache.org/repos/asf/activemq-artemis/diff/39b177d0

Branch: refs/heads/master
Commit: 39b177d0274886159c6006b83de52d7af6436ee4
Parents: 14c41ec
Author: Romain Pelisse <[hidden email]>
Authored: Thu Sep 13 17:02:58 2018 +0200
Committer: Justin Bertram <[hidden email]>
Committed: Thu Dec 13 11:14:37 2018 -0600

----------------------------------------------------------------------
 .../activemq/artemis/ra/ActiveMQRALogger.java   | 12 +++
 .../artemis/ra/inflow/ActiveMQActivation.java   |  2 +-
 .../ra/inflow/ActiveMQActivationSpec.java       | 59 +++----------
 .../ActiveMQActivationValidationUtils.java      | 90 ++++++++++++++++++++
 .../unit/ra/ActiveMQActivationsSpecTest.java    | 61 +++++++++++++
 .../tests/unit/ra/ResourceAdapterTest.java      |  9 +-
 6 files changed, 183 insertions(+), 50 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/activemq-artemis/blob/39b177d0/artemis-ra/src/main/java/org/apache/activemq/artemis/ra/ActiveMQRALogger.java
----------------------------------------------------------------------
diff --git a/artemis-ra/src/main/java/org/apache/activemq/artemis/ra/ActiveMQRALogger.java b/artemis-ra/src/main/java/org/apache/activemq/artemis/ra/ActiveMQRALogger.java
index 99fb031..720c1fd 100644
--- a/artemis-ra/src/main/java/org/apache/activemq/artemis/ra/ActiveMQRALogger.java
+++ b/artemis-ra/src/main/java/org/apache/activemq/artemis/ra/ActiveMQRALogger.java
@@ -149,4 +149,16 @@ public interface ActiveMQRALogger extends BasicLogger {
 
    @Message(id = 153002, value = "Cannot create a subscriber on the durable subscription since it already has subscriber(s)")
    IllegalStateException canNotCreatedNonSharedSubscriber();
+
+   @LogMessage(level = Logger.Level.WARN)
+   @Message(id = 153003, value = "Unsupported acknowledgement mode {0}", format = Message.Format.MESSAGE_FORMAT)
+   void invalidAcknowledgementMode(String mode);
+
+   @LogMessage(level = Logger.Level.WARN)
+   @Message(id = 153004, value = "Invalid number of session (negative) '{0}', defaulting to '${1}'.", format = Message.Format.MESSAGE_FORMAT)
+   void invalidNumberOfMaxSession(int value, int defaultValue);
+
+   @LogMessage(level = Logger.Level.WARN)
+   @Message(id = 153005, value =  "Unable to retrieve '${0}' from JNDI. Creating a new '${1}' named '${2}' to be used by the MDB.", format = Message.Format.MESSAGE_FORMAT)
+   void unableToRetrieveDestinationName(String destinationName, String name, String calculatedDestinationName);
 }

http://git-wip-us.apache.org/repos/asf/activemq-artemis/blob/39b177d0/artemis-ra/src/main/java/org/apache/activemq/artemis/ra/inflow/ActiveMQActivation.java
----------------------------------------------------------------------
diff --git a/artemis-ra/src/main/java/org/apache/activemq/artemis/ra/inflow/ActiveMQActivation.java b/artemis-ra/src/main/java/org/apache/activemq/artemis/ra/inflow/ActiveMQActivation.java
index 1bb25b6..d9b308c 100644
--- a/artemis-ra/src/main/java/org/apache/activemq/artemis/ra/inflow/ActiveMQActivation.java
+++ b/artemis-ra/src/main/java/org/apache/activemq/artemis/ra/inflow/ActiveMQActivation.java
@@ -562,7 +562,7 @@ public class ActiveMQActivation {
                   calculatedDestinationName = spec.getQueuePrefix() + calculatedDestinationName;
                }
 
-               logger.debug("Unable to retrieve " + destinationName + " from JNDI. Creating a new " + destinationType.getName() + " named " + calculatedDestinationName + " to be used by the MDB.");
+               ActiveMQRALogger.LOGGER.unableToRetrieveDestinationName(destinationName, destinationType.getName(), calculatedDestinationName);
 
                // If there is no binding on naming, we will just create a new instance
                if (isTopic) {

http://git-wip-us.apache.org/repos/asf/activemq-artemis/blob/39b177d0/artemis-ra/src/main/java/org/apache/activemq/artemis/ra/inflow/ActiveMQActivationSpec.java
----------------------------------------------------------------------
diff --git a/artemis-ra/src/main/java/org/apache/activemq/artemis/ra/inflow/ActiveMQActivationSpec.java b/artemis-ra/src/main/java/org/apache/activemq/artemis/ra/inflow/ActiveMQActivationSpec.java
index 1252486..135b249 100644
--- a/artemis-ra/src/main/java/org/apache/activemq/artemis/ra/inflow/ActiveMQActivationSpec.java
+++ b/artemis-ra/src/main/java/org/apache/activemq/artemis/ra/inflow/ActiveMQActivationSpec.java
@@ -16,19 +16,13 @@
  */
 package org.apache.activemq.artemis.ra.inflow;
 
-import javax.jms.Queue;
 import javax.jms.Session;
-import javax.jms.Topic;
 import javax.resource.ResourceException;
 import javax.resource.spi.ActivationSpec;
 import javax.resource.spi.InvalidPropertyException;
 import javax.resource.spi.ResourceAdapter;
-import java.beans.IntrospectionException;
-import java.beans.PropertyDescriptor;
 import java.io.Serializable;
-import java.util.ArrayList;
 import java.util.Hashtable;
-import java.util.List;
 
 import org.apache.activemq.artemis.ra.ActiveMQRALogger;
 import org.apache.activemq.artemis.ra.ActiveMQRaUtils;
@@ -401,12 +395,11 @@ public class ActiveMQActivationSpec extends ConnectionFactoryProperties implemen
          logger.trace("setAcknowledgeMode(" + value + ")");
       }
 
-      if ("DUPS_OK_ACKNOWLEDGE".equalsIgnoreCase(value) || "Dups-ok-acknowledge".equalsIgnoreCase(value)) {
-         acknowledgeMode = Session.DUPS_OK_ACKNOWLEDGE;
-      } else if ("AUTO_ACKNOWLEDGE".equalsIgnoreCase(value) || "Auto-acknowledge".equalsIgnoreCase(value)) {
-         acknowledgeMode = Session.AUTO_ACKNOWLEDGE;
-      } else {
-         throw new IllegalArgumentException("Unsupported acknowledgement mode " + value);
+      try {
+         this.acknowledgeMode = ActiveMQActivationValidationUtils.validateAcknowledgeMode(value);
+      } catch ( IllegalArgumentException e ) {
+         ActiveMQRALogger.LOGGER.invalidAcknowledgementMode(value);
+         throw e;
       }
    }
 
@@ -603,7 +596,11 @@ public class ActiveMQActivationSpec extends ConnectionFactoryProperties implemen
          logger.trace("setMaxSession(" + value + ")");
       }
 
-      maxSession = value;
+      if ( value < 1 ) {
+         maxSession = 1;
+         ActiveMQRALogger.LOGGER.invalidNumberOfMaxSession(value, maxSession);
+      } else
+         maxSession = value;
    }
 
    /**
@@ -707,41 +704,7 @@ public class ActiveMQActivationSpec extends ConnectionFactoryProperties implemen
       if (logger.isTraceEnabled()) {
          logger.trace("validate()");
       }
-
-      List<String> errorMessages = new ArrayList<>();
-      List<PropertyDescriptor> propsNotSet = new ArrayList<>();
-
-      try {
-         if (destination == null || destination.trim().equals("")) {
-            propsNotSet.add(new PropertyDescriptor("destination", ActiveMQActivationSpec.class));
-            errorMessages.add("Destination is mandatory.");
-         }
-
-         if (destinationType != null && !Topic.class.getName().equals(destinationType) && !Queue.class.getName().equals(destinationType)) {
-            propsNotSet.add(new PropertyDescriptor("destinationType", ActiveMQActivationSpec.class));
-            errorMessages.add("If set, the destinationType must be either 'javax.jms.Topic' or 'javax.jms.Queue'.");
-         }
-
-         if ((destinationType == null || destinationType.length() == 0 || Topic.class.getName().equals(destinationType)) && isSubscriptionDurable() && (subscriptionName == null || subscriptionName.length() == 0)) {
-            propsNotSet.add(new PropertyDescriptor("subscriptionName", ActiveMQActivationSpec.class));
-            errorMessages.add("If subscription is durable then subscription name must be specified.");
-         }
-      } catch (IntrospectionException e) {
-         ActiveMQRALogger.LOGGER.unableToValidateProperties(e);
-      }
-
-      if (propsNotSet.size() > 0) {
-         StringBuffer b = new StringBuffer();
-         b.append("Invalid settings:");
-         for (String errorMessage : errorMessages) {
-            b.append(" ");
-            b.append(errorMessage);
-         }
-         InvalidPropertyException e = new InvalidPropertyException(b.toString());
-         final PropertyDescriptor[] descriptors = propsNotSet.toArray(new PropertyDescriptor[propsNotSet.size()]);
-         e.setInvalidPropertyDescriptors(descriptors);
-         throw e;
-      }
+      ActiveMQActivationValidationUtils.validate(destination, destinationType, isSubscriptionDurable(), subscriptionName);
    }
 
    public String getConnectorClassName() {

http://git-wip-us.apache.org/repos/asf/activemq-artemis/blob/39b177d0/artemis-ra/src/main/java/org/apache/activemq/artemis/ra/inflow/ActiveMQActivationValidationUtils.java
----------------------------------------------------------------------
diff --git a/artemis-ra/src/main/java/org/apache/activemq/artemis/ra/inflow/ActiveMQActivationValidationUtils.java b/artemis-ra/src/main/java/org/apache/activemq/artemis/ra/inflow/ActiveMQActivationValidationUtils.java
new file mode 100644
index 0000000..42973b5
--- /dev/null
+++ b/artemis-ra/src/main/java/org/apache/activemq/artemis/ra/inflow/ActiveMQActivationValidationUtils.java
@@ -0,0 +1,90 @@
+/*
+ * 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.ra.inflow;
+
+import java.beans.IntrospectionException;
+import java.beans.PropertyDescriptor;
+import java.util.ArrayList;
+import java.util.List;
+
+import javax.jms.Queue;
+import javax.jms.Session;
+import javax.jms.Topic;
+import javax.resource.spi.InvalidPropertyException;
+
+import org.apache.activemq.artemis.ra.ActiveMQRALogger;
+
+public final class ActiveMQActivationValidationUtils {
+
+   private ActiveMQActivationValidationUtils() {
+   }
+
+   public static int validateAcknowledgeMode(final String value) {
+      if ("DUPS_OK_ACKNOWLEDGE".equalsIgnoreCase(value) || "Dups-ok-acknowledge".equalsIgnoreCase(value)) {
+         return Session.DUPS_OK_ACKNOWLEDGE;
+      } else if ("AUTO_ACKNOWLEDGE".equalsIgnoreCase(value) || "Auto-acknowledge".equalsIgnoreCase(value)) {
+         return Session.AUTO_ACKNOWLEDGE;
+      } else {
+         throw new IllegalArgumentException(value);
+      }
+   }
+
+   public static void validate(String destination, String destinationType, Boolean subscriptionDurability,
+         String subscriptionName) throws InvalidPropertyException {
+      List<String> errorMessages = new ArrayList<>();
+      List<PropertyDescriptor> propsNotSet = new ArrayList<>();
+
+      try {
+         if (destination == null || destination.trim().equals("")) {
+            propsNotSet.add(new PropertyDescriptor("destination", ActiveMQActivationSpec.class));
+            errorMessages.add("Destination is mandatory.");
+         }
+
+         if (destinationType != null && !Topic.class.getName().equals(destinationType)
+               && !Queue.class.getName().equals(destinationType)) {
+            propsNotSet.add(new PropertyDescriptor("destinationType", ActiveMQActivationSpec.class));
+            errorMessages.add("If set, the destinationType must be either 'javax.jms.Topic' or 'javax.jms.Queue'.");
+         }
+
+         if ((destinationType == null || destinationType.length() == 0 || Topic.class.getName().equals(destinationType))
+               && subscriptionDurability && (subscriptionName == null || subscriptionName.length() == 0)) {
+            propsNotSet.add(new PropertyDescriptor("subscriptionName", ActiveMQActivationSpec.class));
+            errorMessages.add("If subscription is durable then subscription name must be specified.");
+         }
+      } catch (IntrospectionException e) {
+         ActiveMQRALogger.LOGGER.unableToValidateProperties(e);
+      }
+
+      ActiveMQActivationValidationUtils.buildAndThrowExceptionIfNeeded(propsNotSet, errorMessages);
+   }
+
+   private static void buildAndThrowExceptionIfNeeded(List<PropertyDescriptor> propsNotSet, List<String> errorMessages)
+         throws InvalidPropertyException {
+      if (propsNotSet.size() > 0) {
+         StringBuffer b = new StringBuffer();
+         b.append("Invalid settings:");
+         for (String errorMessage : errorMessages) {
+            b.append(" ");
+            b.append(errorMessage);
+         }
+         InvalidPropertyException e = new InvalidPropertyException(b.toString());
+         final PropertyDescriptor[] descriptors = propsNotSet.toArray(new PropertyDescriptor[propsNotSet.size()]);
+         e.setInvalidPropertyDescriptors(descriptors);
+         throw e;
+      }
+   }
+}

http://git-wip-us.apache.org/repos/asf/activemq-artemis/blob/39b177d0/tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/ra/ActiveMQActivationsSpecTest.java
----------------------------------------------------------------------
diff --git a/tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/ra/ActiveMQActivationsSpecTest.java b/tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/ra/ActiveMQActivationsSpecTest.java
new file mode 100644
index 0000000..640bad1
--- /dev/null
+++ b/tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/ra/ActiveMQActivationsSpecTest.java
@@ -0,0 +1,61 @@
+/*
+ * 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.tests.unit.ra;
+
+import static org.junit.Assert.assertEquals;
+
+import javax.jms.Session;
+import javax.resource.spi.InvalidPropertyException;
+
+import org.apache.activemq.artemis.ra.inflow.ActiveMQActivationValidationUtils;
+import org.junit.Test;
+
+public class ActiveMQActivationsSpecTest {
+
+   @Test(expected = InvalidPropertyException.class)
+   public void nullDestinationName() throws InvalidPropertyException {
+      ActiveMQActivationValidationUtils.validate(null, "destinationType", false, "subscriptionName");
+   }
+
+   @Test(expected = InvalidPropertyException.class)
+   public void emptyDestinationName() throws InvalidPropertyException {
+      ActiveMQActivationValidationUtils.validate(null, "destinationType", false, "subscriptionName");
+   }
+
+   public void nullDestinationType() throws InvalidPropertyException {
+      ActiveMQActivationValidationUtils.validate("destinationName", null, false, "subscriptionName");
+   }
+
+   @Test(expected = InvalidPropertyException.class)
+   public void emptyDestinationType() throws InvalidPropertyException {
+      ActiveMQActivationValidationUtils.validate("destinationName", "", false, "subscriptionName");
+   }
+
+   @Test(expected = InvalidPropertyException.class)
+   public void subscriptionDurableButNoName() throws InvalidPropertyException {
+      ActiveMQActivationValidationUtils.validate("", "", true, "subscriptionName");
+   }
+
+   @Test(expected = IllegalArgumentException.class)
+   public void validateAcknowledgeMode() {
+      assertEquals(ActiveMQActivationValidationUtils.validateAcknowledgeMode("DUPS_OK_ACKNOWLEDGE"), Session.DUPS_OK_ACKNOWLEDGE);
+      assertEquals(ActiveMQActivationValidationUtils.validateAcknowledgeMode("Dups-ok-acknowledge"), Session.DUPS_OK_ACKNOWLEDGE);
+      assertEquals(ActiveMQActivationValidationUtils.validateAcknowledgeMode("AUTO_ACKNOWLEDGE"), Session.AUTO_ACKNOWLEDGE);
+      assertEquals(ActiveMQActivationValidationUtils.validateAcknowledgeMode("Auto-acknowledge"), Session.AUTO_ACKNOWLEDGE);
+      ActiveMQActivationValidationUtils.validateAcknowledgeMode("Invalid Acknowledge Mode");
+   }
+}

http://git-wip-us.apache.org/repos/asf/activemq-artemis/blob/39b177d0/tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/ra/ResourceAdapterTest.java
----------------------------------------------------------------------
diff --git a/tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/ra/ResourceAdapterTest.java b/tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/ra/ResourceAdapterTest.java
index 0ca9a28..1cb6798 100644
--- a/tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/ra/ResourceAdapterTest.java
+++ b/tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/ra/ResourceAdapterTest.java
@@ -383,7 +383,7 @@ public class ResourceAdapterTest extends ActiveMQTestBase {
    public void testValidateProperties() throws Exception {
       validateGettersAndSetters(new ActiveMQResourceAdapter(), "backupTransportConfiguration", "connectionParameters", "jndiParams");
       validateGettersAndSetters(new ActiveMQRAManagedConnectionFactory(), "connectionParameters", "sessionDefaultType", "backupConnectionParameters", "jndiParams");
-      validateGettersAndSetters(new ActiveMQActivationSpec(), "connectionParameters", "acknowledgeMode", "subscriptionDurability", "jndiParams");
+      validateGettersAndSetters(new ActiveMQActivationSpec(), "connectionParameters", "acknowledgeMode", "subscriptionDurability", "jndiParams", "maxSession");
 
       ActiveMQActivationSpec spec = new ActiveMQActivationSpec();
 
@@ -396,6 +396,13 @@ public class ResourceAdapterTest extends ActiveMQTestBase {
       spec.setSubscriptionDurability("NonDurable");
       Assert.assertEquals("NonDurable", spec.getSubscriptionDurability());
 
+      final int validMaxSessionValue = 110;
+      spec.setMaxSession(validMaxSessionValue);
+      Assert.assertTrue(validMaxSessionValue == spec.getMaxSession());
+
+      spec.setMaxSession(-3);
+      Assert.assertTrue(spec.getMaxSession() == 1);
+
       spec = new ActiveMQActivationSpec();
       ActiveMQResourceAdapter adapter = new ActiveMQResourceAdapter();