Mercurial > hg > release > thermostat-1.4
changeset 1541:066d45fee15d
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<>();