Mercurial > hg > release > thermostat-1.2

changeset 1541:066d45fee15d

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
Do not throw NPE when performing contains() checks. Reviewed-by: omajid Review-thread: http://icedtea.classpath.org/pipermail/thermostat/2014-November/011497.html
author Severin Gehwolf <sgehwolf@redhat.com>
date Mon, 10 Nov 2014 11:57:17 +0100
parents 874eb4ddfe8f
children f2dcec9f38e1
files web/server/src/main/java/com/redhat/thermostat/web/server/KnownCategoryRegistry.java web/server/src/main/java/com/redhat/thermostat/web/server/KnownDescriptorRegistry.java web/server/src/test/java/com/redhat/thermostat/web/server/KnownCategoryRegistryTest.java web/server/src/test/java/com/redhat/thermostat/web/server/KnownDescriptorRegistryTest.java
diffstat 4 files changed, 95 insertions(+), 13 deletions(-) [+]
line wrap: on
line diff
--- a/web/server/src/main/java/com/redhat/thermostat/web/server/KnownCategoryRegistry.java	Tue Nov 11 14:52:52 2014 -0500
+++ b/web/server/src/main/java/com/redhat/thermostat/web/server/KnownCategoryRegistry.java	Mon Nov 10 11:57:17 2014 +0100
@@ -64,9 +64,18 @@
         trustedCategories = new HashSet<>();
         for (CategoryRegistration reg: registrations) {
             Set<String> currentRegs = reg.getCategoryNames();
-            if (currentRegs.contains(null)) {
-                String msg = "null name not allowed!";
-                throw new IllegalStateException(msg);
+            // Some Set implementations throw NPEs when contains() is called on
+            // a null value. Be sure we catch NPE since those impls can't contain
+            // null values anyway.
+            try {
+                if (currentRegs.contains(null)) {
+                    String msg = "null name not allowed!";
+                    throw new IllegalStateException(msg);
+                }
+                // Pass: Not containing null values.
+            } catch (NullPointerException npe) {
+                // Pass: Set impl does not support contains checks on null
+                //       values.
             }
             trustedCategories.addAll(currentRegs);
         }
--- a/web/server/src/main/java/com/redhat/thermostat/web/server/KnownDescriptorRegistry.java	Tue Nov 11 14:52:52 2014 -0500
+++ b/web/server/src/main/java/com/redhat/thermostat/web/server/KnownDescriptorRegistry.java	Mon Nov 10 11:57:17 2014 +0100
@@ -66,8 +66,17 @@
         trustedSet = new HashSet<>();
         for (StatementDescriptorRegistration reg: trustedDescs) {
             Set<String> newCandidates = reg.getStatementDescriptors();
-            if (newCandidates.contains(null)) {
-                throw new IllegalStateException("null statement descriptor not acceptable!");
+            // Some Set implementations throw NPEs when contains() is called on
+            // a null value. Be sure we catch NPE since those impls can't contain
+            // null values anyway.
+            try {
+                if (newCandidates.contains(null)) {
+                    throw new IllegalStateException("null statement descriptor not acceptable!");
+                }
+                // Pass: Not containing null values.
+            } catch (NullPointerException npe) {
+                // Pass: Set impl does not support contains checks on null
+                //       values.
             }
             // prepare the reverse lookup metadata map
             StatementDescriptorMetadataFactory factory = (StatementDescriptorMetadataFactory) reg;
--- a/web/server/src/test/java/com/redhat/thermostat/web/server/KnownCategoryRegistryTest.java	Tue Nov 11 14:52:52 2014 -0500
+++ b/web/server/src/test/java/com/redhat/thermostat/web/server/KnownCategoryRegistryTest.java	Mon Nov 10 11:57:17 2014 +0100
@@ -44,6 +44,7 @@
 
 import java.util.HashSet;
 import java.util.Set;
+import java.util.TreeSet;
 
 import org.junit.Test;
 
@@ -56,7 +57,7 @@
     @Test
     public void canAddCategories() {
         Set<String> categoryNames = new HashSet<>();
-        categoryNames.add("some-category"); // duplicate
+        categoryNames.add(DUPLICATE_CAT_NAME); // duplicate
         categoryNames.add("some-other-category");
         Iterable<CategoryRegistration> regs = getRegs(categoryNames);
         KnownCategoryRegistry reg = null;
@@ -97,6 +98,36 @@
         }
     }
     
+    /**
+     * Tests that {@code set.contains(null)} does not throw exceptions since
+     * plug-ins may supply any {@link Set} implementation. {@link TreeSet} for
+     * example throws a NPE if such a check is attempted.
+     * 
+     * See: http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=2077
+     */
+    @Test
+    public void testNullCheckWithTreeSet() {
+        String categoryName = "foo-category";
+        Set<String> descs = new TreeSet<>();
+        descs.add(categoryName);
+        try {
+            descs.add(null);
+            fail("Expected NPE when adding null to TreeSet");
+        } catch (NullPointerException e) {
+            // pre-condition passes
+        }
+        Iterable<CategoryRegistration> regs = getRegs(descs);
+        KnownCategoryRegistry registry = null;
+        try {
+            registry = new KnownCategoryRegistry(regs);
+            // pass
+        } catch (NullPointerException npe) {
+            npe.printStackTrace();
+            fail("Should not have thrown NPE");
+        }
+        assertTrue("category should be known", registry.getRegisteredCategoryNames().contains(categoryName));
+    }
+    
     @Test
     public void canLoadRegistrationsInClasspath() {
         KnownCategoryRegistry reg = new KnownCategoryRegistry();
--- a/web/server/src/test/java/com/redhat/thermostat/web/server/KnownDescriptorRegistryTest.java	Tue Nov 11 14:52:52 2014 -0500
+++ b/web/server/src/test/java/com/redhat/thermostat/web/server/KnownDescriptorRegistryTest.java	Mon Nov 10 11:57:17 2014 +0100
@@ -36,19 +36,22 @@
 
 package com.redhat.thermostat.web.server;
 
-import com.redhat.thermostat.storage.core.PreparedParameter;
-import com.redhat.thermostat.storage.core.auth.DescriptorMetadata;
-import com.redhat.thermostat.storage.core.auth.StatementDescriptorRegistration;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
 import java.util.Arrays;
 import java.util.HashSet;
 import java.util.Map;
 import java.util.Set;
+import java.util.TreeSet;
+
 import org.junit.Test;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
+import com.redhat.thermostat.storage.core.PreparedParameter;
+import com.redhat.thermostat.storage.core.auth.DescriptorMetadata;
+import com.redhat.thermostat.storage.core.auth.StatementDescriptorRegistration;
 
 public class KnownDescriptorRegistryTest {
 
@@ -82,6 +85,36 @@
         assertEquals(24, trustedDescs.size());
     }
     
+    /**
+     * Tests that {@code set.contains(null)} does not throw exceptions since
+     * plug-ins may supply any {@link Set} implementation. {@link TreeSet} for
+     * example throws a NPE if such a check is attempted.
+     * 
+     * See: http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=2077
+     */
+    @Test
+    public void testNullCheckWithTreeSet() {
+        String desc = "QUERY test WHERE 'a' = ?s";
+        Set<String> descs = new TreeSet<>();
+        descs.add(desc);
+        try {
+            descs.add(null);
+            fail("Expected NPE when adding null to TreeSet");
+        } catch (NullPointerException e) {
+            // pre-condition passes
+        }
+        Iterable<StatementDescriptorRegistration> regs = getRegs(descs);
+        KnownDescriptorRegistry registry = null;
+        try {
+            registry = new KnownDescriptorRegistry(regs);
+            // pass
+        } catch (NullPointerException npe) {
+            npe.printStackTrace();
+            fail("Should not have thrown NPE");
+        }
+        assertTrue("descriptor should be known", registry.getRegisteredDescriptors().contains(desc));
+    }
+    
     @Test
     public void cannotAddNullDescriptors() {
         Set<String> descs = new HashSet<>();