From: Aron Roberts Date: Fri, 20 Nov 2009 01:19:42 +0000 (+0000) Subject: CSPACE-234,CSPACE-364: Ongoing work on revising interfaces for ID Parts, still in... X-Git-Url: https://git.aero2k.de/?a=commitdiff_plain;h=0d23971c49db392a835c57eb096a2c2bc037d5db;p=tmp%2Fjakarta-migration.git CSPACE-234,CSPACE-364: Ongoing work on revising interfaces for ID Parts, still in progress. Added tests; fixed bug that prevented generation of zero as a starting value in NumericIDPart; added algorithm for more even distribution of pseudorandom numbers. --- diff --git a/services/id/service/src/main/java/org/collectionspace/services/id/part/JavaPrintfIDPartOutputFormatter.java b/services/id/service/src/main/java/org/collectionspace/services/id/part/JavaPrintfIDPartOutputFormatter.java index 5e2f96870..67e1c84ce 100644 --- a/services/id/service/src/main/java/org/collectionspace/services/id/part/JavaPrintfIDPartOutputFormatter.java +++ b/services/id/service/src/main/java/org/collectionspace/services/id/part/JavaPrintfIDPartOutputFormatter.java @@ -55,7 +55,6 @@ public class JavaPrintfIDPartOutputFormatter implements IDPartOutputFormatter { public String format(String id) { String formattedID = id; - String pattern = getFormatPattern(); // If the formatting pattern is empty, just check length. @@ -72,6 +71,7 @@ public class JavaPrintfIDPartOutputFormatter implements IDPartOutputFormatter { // Otherwise, format the ID using the pattern, then check length. } else { + // Clear the StringWriter's buffer from its last usage. StringBuffer buf = stringwriter.getBuffer(); buf.setLength(0); @@ -81,9 +81,19 @@ public class JavaPrintfIDPartOutputFormatter implements IDPartOutputFormatter { printwriter.printf(id, pattern); formattedID = stringwriter.toString(); } catch(IllegalFormatException e) { - // @TODO Log and handle this exception. + logger.error("Error when attempting to format ID '" + + id + "' using formatting pattern '" + pattern); + // @TODO Consider throwing this exception. + } + + if (! isValidLength(formattedID)) { + logger.error( + "Formatted ID '" + formattedID + + "' exceeds maximum length of " + + getMaxOutputLength() + " characters." + + "Returning ID without formatting."); + return id; } - isValidLength(formattedID); } return formattedID; diff --git a/services/id/service/src/main/java/org/collectionspace/services/id/part/JavaRandomNumberIDPartAlgorithm.java b/services/id/service/src/main/java/org/collectionspace/services/id/part/JavaRandomNumberIDPartAlgorithm.java index 782028280..cd72ec135 100644 --- a/services/id/service/src/main/java/org/collectionspace/services/id/part/JavaRandomNumberIDPartAlgorithm.java +++ b/services/id/service/src/main/java/org/collectionspace/services/id/part/JavaRandomNumberIDPartAlgorithm.java @@ -3,6 +3,12 @@ package org.collectionspace.services.id.part; import java.util.Random; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +// Returns a evenly-distributed, pseudorandom number from within a +// default or supplied range of integer values. + public class JavaRandomNumberIDPartAlgorithm implements IDPartAlgorithm { // @TODO Verify whether this simple singleton pattern is @@ -11,36 +17,83 @@ public class JavaRandomNumberIDPartAlgorithm implements IDPartAlgorithm { // @TODO Check whether we might need to store a serialization // of this class, once instantiated, between invocations, and - // load the class from its serialized state. + // load the class from its serialized state, to reduce the + // possibility of // @TODO Look into whether we may have some user stories or use cases // that require the use of java.security.SecureRandom, rather than // java.util.Random. + final Logger logger = + LoggerFactory.getLogger(JavaRandomNumberIDPartAlgorithm.class); + // Starting with Java 5, the default instantiation of Random() // sets the seed "to a value very likely to be distinct from any // other invocation of this constructor." - private Random r = new Random(); + private static Random r = new Random(); + + public final static int DEFAULT_MAX_VALUE = Integer.MAX_VALUE - 1; + public final static int DEFAULT_MIN_VALUE = 0; + + private int maxValue = DEFAULT_MAX_VALUE; + private int minValue = DEFAULT_MIN_VALUE; - private JavaRandomNumberIDPartAlgorithm() { + public JavaRandomNumberIDPartAlgorithm() { } - // See http://en.wikipedia.org/wiki/Singleton_pattern - private static class SingletonHolder { - private static final JavaRandomNumberIDPartAlgorithm INSTANCE = - new JavaRandomNumberIDPartAlgorithm(); + // Throws IllegalArgumentException + public JavaRandomNumberIDPartAlgorithm(int maxVal) { + try { + setMaxValue(maxVal); + } catch (IllegalArgumentException e) { + throw e; + } } - public static JavaRandomNumberIDPartAlgorithm getInstance() { - return SingletonHolder.INSTANCE; + // Throws IllegalArgumentException + public JavaRandomNumberIDPartAlgorithm(int maxVal, int minVal) { + setMaxValue(maxVal); + setMinValue(minVal); + } + + private void setMaxValue(int maxVal) { + if (0 < maxVal && maxVal < DEFAULT_MAX_VALUE) { + this.maxValue = maxVal; + } else { + String msg = + "Invalid maximum value for random number. " + + "Must be between 1 and " + + Integer.toString(DEFAULT_MAX_VALUE - 1) + "."; + logger.error(msg); + throw new IllegalArgumentException(msg); + } + } + + private void setMinValue(int minVal) { + if (DEFAULT_MIN_VALUE <= minVal && minVal < this.maxValue) { + this.minValue = minVal; + } else { + String msg = + "Invalid minimum value for random number. " + + "Must be between 0 and " + + Integer.toString(this.maxValue - 1) + "."; + logger.error(msg); + throw new IllegalArgumentException(msg); + } } - // @TODO Allow setting a maximum value. @Override public String generateID(){ - // Returns a value between 0 (inclusive) and the - // maximum value of an int. - return Integer.toString(r.nextInt(Integer.MAX_VALUE)); + // Returns an evenly distributed random value between 0 + // and the maximum value. + // See http://mindprod.com/jgloss/pseudorandom.html + // + // Note: Random.nextInt() returns a pseudorandom number + // between 0 and n-1 inclusive, not a number between 0 and n. + return + Integer.toString(r.nextInt( + this.maxValue - this.minValue + 1) + this.minValue); + } } diff --git a/services/id/service/src/main/java/org/collectionspace/services/id/part/NumericSequenceIDPart.java b/services/id/service/src/main/java/org/collectionspace/services/id/part/NumericSequenceIDPart.java index ffbe79ccc..b1623f91b 100644 --- a/services/id/service/src/main/java/org/collectionspace/services/id/part/NumericSequenceIDPart.java +++ b/services/id/service/src/main/java/org/collectionspace/services/id/part/NumericSequenceIDPart.java @@ -73,8 +73,9 @@ public class NumericSequenceIDPart extends SequenceIDPart { return Long.toString(this.currentValue); } + // @TODO Consider throwing IllegalArgumentException here. public void setCurrentID(long val) { - if (val <= 0) { + if (val < 0) { logger.error("Current ID value for numeric ID sequences " + "must be positive."); } else { @@ -102,8 +103,9 @@ public class NumericSequenceIDPart extends SequenceIDPart { return Long.toString(this.initialValue); } + // @TODO Consider throwing IllegalArgumentException here. public void setInitialID(long initial) { - if (initial <= 0) { + if (initial < 0) { logger.error("Current ID value for numeric ID sequences " + "must be positive."); } else { diff --git a/services/id/service/src/main/java/org/collectionspace/services/id/part/RandomNumberIDPart.java b/services/id/service/src/main/java/org/collectionspace/services/id/part/RandomNumberIDPart.java index 97634f0a8..676d5280c 100644 --- a/services/id/service/src/main/java/org/collectionspace/services/id/part/RandomNumberIDPart.java +++ b/services/id/service/src/main/java/org/collectionspace/services/id/part/RandomNumberIDPart.java @@ -7,10 +7,27 @@ public class RandomNumberIDPart extends AlgorithmicIDPart { private IDPartOutputFormatter formatter = IDPart.DEFAULT_FORMATTER; private IDPartValidator validator = new NumericIDPartRegexValidator(); - private IDPartAlgorithm algorithm = - JavaRandomNumberIDPartAlgorithm.getInstance(); + private IDPartAlgorithm algorithm; public RandomNumberIDPart(){ + this.algorithm = new JavaRandomNumberIDPartAlgorithm(); + } + + public RandomNumberIDPart(int maxValue) { + try { + this.algorithm = new JavaRandomNumberIDPartAlgorithm(maxValue); + } catch (IllegalArgumentException e) { + throw e; + } + } + + public RandomNumberIDPart(int maxValue, int minValue){ + try { + this.algorithm = + new JavaRandomNumberIDPartAlgorithm(maxValue, minValue); + } catch (IllegalArgumentException e) { + throw e; + } } @Override diff --git a/services/id/service/src/test/java/org/collectionspace/services/id/part/test/NoOpIDPartOutputFormatterTest.java b/services/id/service/src/test/java/org/collectionspace/services/id/part/test/NoOpIDPartOutputFormatterTest.java new file mode 100644 index 000000000..68bc3b9dd --- /dev/null +++ b/services/id/service/src/test/java/org/collectionspace/services/id/part/test/NoOpIDPartOutputFormatterTest.java @@ -0,0 +1,22 @@ +package org.collectionspace.services.id.part.test; + +import org.collectionspace.services.id.part.NoOpIDPartOutputFormatter; + +import org.testng.Assert; +import org.testng.annotations.Test; + +public class NoOpIDPartOutputFormatterTest { + + NoOpIDPartOutputFormatter formatter = new NoOpIDPartOutputFormatter(); + + @Test + public void format() { + Assert.assertEquals("any string", formatter.format("any string")); + } + + @Test(dependsOnMethods = {"format"}) + public void formatWithNullAndEmptyValues() { + Assert.assertEquals(null, formatter.format(null)); + Assert.assertEquals("", formatter.format("")); + } +} diff --git a/services/id/service/src/test/java/org/collectionspace/services/id/part/test/NoOpIDPartValidatorTest.java b/services/id/service/src/test/java/org/collectionspace/services/id/part/test/NoOpIDPartValidatorTest.java index aa8c15151..a6e335b08 100644 --- a/services/id/service/src/test/java/org/collectionspace/services/id/part/test/NoOpIDPartValidatorTest.java +++ b/services/id/service/src/test/java/org/collectionspace/services/id/part/test/NoOpIDPartValidatorTest.java @@ -7,12 +7,16 @@ import org.testng.annotations.Test; public class NoOpIDPartValidatorTest { + NoOpIDPartValidator validator = new NoOpIDPartValidator(); + @Test public void isValid() { - NoOpIDPartValidator validator = new NoOpIDPartValidator(); - Assert.assertTrue(validator.isValid(null)); - Assert.assertTrue(validator.isValid("")); Assert.assertTrue(validator.isValid("any string")); } + @Test(dependsOnMethods = {"isValid"}) + public void isValidWithNullAndEmptyValues() { + Assert.assertTrue(validator.isValid(null)); + Assert.assertTrue(validator.isValid("")); + } } diff --git a/services/id/service/src/test/java/org/collectionspace/services/id/part/test/NumericIDPartRegexValidatorTest.java b/services/id/service/src/test/java/org/collectionspace/services/id/part/test/NumericIDPartRegexValidatorTest.java index 5c02974ff..7a1b28a07 100644 --- a/services/id/service/src/test/java/org/collectionspace/services/id/part/test/NumericIDPartRegexValidatorTest.java +++ b/services/id/service/src/test/java/org/collectionspace/services/id/part/test/NumericIDPartRegexValidatorTest.java @@ -17,16 +17,19 @@ public class NumericIDPartRegexValidatorTest { } @Test(dependsOnMethods = {"isValid"}) - public void isValidWithInvalidValues() { + public void isValidWithNullOrEmptyValues() { Assert.assertFalse(validator.isValid(null)); Assert.assertFalse(validator.isValid("")); - Assert.assertFalse(validator.isValid("non-numeric value")); - Assert.assertFalse(validator.isValid("-1")); } - @Test - public void format() { + @Test(dependsOnMethods = {"isValid"}) + public void isValidWithNonNumericValues() { + Assert.assertFalse(validator.isValid("non-numeric value")); + } + @Test(dependsOnMethods = {"isValid"}) + public void isValidWithNegativeValues() { + Assert.assertFalse(validator.isValid("-1")); } } diff --git a/services/id/service/src/test/java/org/collectionspace/services/id/part/test/NumericSequenceIDPartTest.java b/services/id/service/src/test/java/org/collectionspace/services/id/part/test/NumericSequenceIDPartTest.java index 500d2cd0f..f6217342b 100644 --- a/services/id/service/src/test/java/org/collectionspace/services/id/part/test/NumericSequenceIDPartTest.java +++ b/services/id/service/src/test/java/org/collectionspace/services/id/part/test/NumericSequenceIDPartTest.java @@ -39,6 +39,15 @@ public class NumericSequenceIDPartTest { @Test public void newIDWithSuppliedInitialValue() { + part = new NumericSequenceIDPart(0); + id = part.newID(); + Assert.assertEquals(id, "0"); + part.setCurrentID(id); + + id = part.newID(); + Assert.assertEquals(id, "1"); + part.setCurrentID(id); + part = new NumericSequenceIDPart(100); id = part.newID(); Assert.assertEquals(id, "100"); @@ -79,6 +88,40 @@ public class NumericSequenceIDPartTest { Assert.assertEquals(id, "15"); } + + @Test + public void formatWithZeroValue() { + + // With default format pattern. + part = new NumericSequenceIDPart(0); + id = part.newID(); + Assert.assertEquals(id, Long.toString(0)); + + // With supplied format pattern. + part = new NumericSequenceIDPart("#####", 0, 1); + id = part.newID(); + Assert.assertEquals(id, Long.toString(0)); + part.setCurrentID(id); + + } + + @Test + public void formatWithMaxPossibleValue() { + + // With default format pattern. + part = new NumericSequenceIDPart(Long.MAX_VALUE); + id = part.newID(); + Assert.assertEquals(id, Long.toString(Long.MAX_VALUE)); + part.setCurrentID(id); + + // With supplied format pattern. + part = new NumericSequenceIDPart("#####", Long.MAX_VALUE, 1); + id = part.newID(); + Assert.assertEquals(id, Long.toString(Long.MAX_VALUE)); + part.setCurrentID(id); + + } + @Test public void formatWithLeadingZeros() { @@ -101,7 +144,15 @@ public class NumericSequenceIDPartTest { Assert.assertEquals(id, "025"); part.setCurrentID(id); - // Numerals with more digits than pattern do not receive padding. + // With zero value. + // Pad at left with leading zeros up to width specified. + part = new NumericSequenceIDPart("000", 0, 1); + id = part.newID(); + Assert.assertEquals(id, "000"); + part.setCurrentID(id); + + // Values containing more digits than supplied pattern + // do not receive zero padding. part = new NumericSequenceIDPart("000", 5000, 1); id = part.newID(); Assert.assertEquals(id, "5000"); diff --git a/services/id/service/src/test/java/org/collectionspace/services/id/part/test/RandomNumberIDPartTest.java b/services/id/service/src/test/java/org/collectionspace/services/id/part/test/RandomNumberIDPartTest.java index 1a5b0ebc4..478c85a55 100644 --- a/services/id/service/src/test/java/org/collectionspace/services/id/part/test/RandomNumberIDPartTest.java +++ b/services/id/service/src/test/java/org/collectionspace/services/id/part/test/RandomNumberIDPartTest.java @@ -2,9 +2,10 @@ package org.collectionspace.services.id.part.test; import org.collectionspace.services.id.part.IDPart; import org.collectionspace.services.id.part.RandomNumberIDPart; +import org.collectionspace.services.id.part.JavaRandomNumberIDPartAlgorithm; import org.testng.Assert; -import org.testng.annotations.BeforeTest; +import org.testng.annotations.BeforeSuite; import org.testng.annotations.Test; public class RandomNumberIDPartTest { @@ -14,7 +15,7 @@ public class RandomNumberIDPartTest { String secondID; String thirdID; - @BeforeTest + @BeforeSuite public void setUp() { part = new RandomNumberIDPart(); firstID = part.newID(); @@ -36,4 +37,74 @@ public class RandomNumberIDPartTest { Assert.assertTrue(part.getValidator().isValid(thirdID)); } + @Test(dependsOnMethods = {"newIDGeneratesNonRepeatingIDs"}) + public void newIDGeneratesNonRepeatingIDsWithSuppliedValues() { + part = new RandomNumberIDPart(200, 100); + firstID = part.newID(); + secondID = part.newID(); + thirdID = part.newID(); + Assert.assertTrue(firstID.compareTo(secondID) != 0); + Assert.assertTrue(firstID.compareTo(thirdID) != 0); + Assert.assertTrue(secondID.compareTo(thirdID) != 0); + } + + @Test + public void highMinAndMaxValues() { + int minValue = Integer.MAX_VALUE - 1000; + int maxValue = Integer.MAX_VALUE - 2; + String id; + part = new RandomNumberIDPart(maxValue, minValue); + for (int i=0; i < 20; i++) { + id = part.newID(); + Assert.assertTrue(Integer.parseInt(id) >= minValue); + } + } + + @Test + public void newIDsLessThanOrEqualToSuppliedMaxValue() { + + // With only maximum value specified. + int maxValue = 20; + String id; + // Generate a sufficient number of values that + // there is a high probability of realizing an + // error condition, if any. + part = new RandomNumberIDPart(maxValue); + for (int i=0; i < (maxValue * 5); i++) { + id = part.newID(); + Assert.assertTrue(Integer.parseInt(id) <= maxValue); + } + + // With minimum value also specified. + int minValue = 5; + part = new RandomNumberIDPart(maxValue, minValue); + for (int i=0; i < ((maxValue - minValue) * 5); i++) { + id = part.newID(); + Assert.assertTrue(Integer.parseInt(id) <= maxValue); + } + } + + @Test(dependsOnMethods = {"newIDsLessThanOrEqualToSuppliedMaxValue"}) + public void newIDsHigherThanOrEqualToSuppliedMinValue() { + int minValue = 5; + int maxValue = 20; + String id; + part = new RandomNumberIDPart(maxValue, minValue); + for (int i=0; i <= ((maxValue - minValue) * 5); i++) { + id = part.newID(); + Assert.assertTrue(Integer.parseInt(id) >= minValue); + } + } + + @Test(expectedExceptions = IllegalArgumentException.class) + public void minValueTooLow() { + int minValue = -1; + part = new RandomNumberIDPart( + JavaRandomNumberIDPartAlgorithm.DEFAULT_MAX_VALUE, minValue); + } + + @Test(expectedExceptions = IllegalArgumentException.class) + public void maxValueTooHigh() { + part = new RandomNumberIDPart(Integer.MAX_VALUE); + } }