From: <svn...@os...> - 2010-11-28 08:50:11
|
Author: aaime Date: 2010-11-28 00:50:01 -0800 (Sun, 28 Nov 2010) New Revision: 36360 Modified: trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/AbstractCalcResult.java trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/AverageVisitor.java trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/BoundsVisitor.java trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/CalcResult.java trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/CountVisitor.java trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/MaxVisitor.java trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/MedianVisitor.java trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/MinVisitor.java trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/QuantileListVisitor.java trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/StandardDeviationVisitor.java trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/SumVisitor.java trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/UniqueVisitor.java trunk/modules/library/main/src/test/java/org/geotools/feature/visitor/VisitorCalculationTest.java Log: [GEOT-3323] Common visitors break with an NPE when they visit an empty collection Modified: trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/AbstractCalcResult.java =================================================================== --- trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/AbstractCalcResult.java 2010-11-27 17:05:48 UTC (rev 36359) +++ trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/AbstractCalcResult.java 2010-11-28 08:50:01 UTC (rev 36360) @@ -40,11 +40,22 @@ */ public class AbstractCalcResult implements CalcResult { public boolean isCompatible(CalcResult targetResults) { - return false; + return targetResults == CalcResult.NULL_RESULT; } public CalcResult merge(CalcResult resultsToAdd) { - return null; + if(resultsToAdd == CalcResult.NULL_RESULT) { + return this; + } else { + if (!isCompatible(resultsToAdd)) { + throw new IllegalArgumentException( + "Parameter is not a compatible type"); + } else { + throw new IllegalArgumentException( + "The CalcResults claim to be compatible, but the appropriate merge " + + "method has not been implemented."); + } + } } public Object getValue() { Modified: trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/AverageVisitor.java =================================================================== --- trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/AverageVisitor.java 2010-11-27 17:05:48 UTC (rev 36359) +++ trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/AverageVisitor.java 2010-11-28 08:50:01 UTC (rev 36360) @@ -154,6 +154,9 @@ * */ public CalcResult getResult() { + if(strategy == null) { + return CalcResult.NULL_RESULT; + } return new AverageResult(strategy, isOptimized); } @@ -374,7 +377,7 @@ * @return boolean */ public boolean isCompatible(CalcResult targetResults) { - if (targetResults instanceof AverageResult) { + if (targetResults instanceof AverageResult || targetResults == CalcResult.NULL_RESULT) { return true; } else { return false; @@ -398,6 +401,10 @@ throw new IllegalArgumentException( "Parameter is not a compatible type"); } + + if(resultsToAdd == CalcResult.NULL_RESULT) { + return this; + } if (resultsToAdd instanceof AverageResult) { AverageResult moreResults = (AverageResult) resultsToAdd; Modified: trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/BoundsVisitor.java =================================================================== --- trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/BoundsVisitor.java 2010-11-27 17:05:48 UTC (rev 36359) +++ trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/BoundsVisitor.java 2010-11-28 08:50:01 UTC (rev 36360) @@ -45,6 +45,9 @@ } public CalcResult getResult() { + if(bounds == null || bounds.isEmpty()) { + return CalcResult.NULL_RESULT; + } return new BoundsResult(bounds); } @@ -61,7 +64,7 @@ public boolean isCompatible(CalcResult targetResults) { //list each calculation result which can merge with this type of result - if (targetResults instanceof BoundsResult) { + if (targetResults instanceof BoundsResult || targetResults == CalcResult.NULL_RESULT) { return true; } @@ -73,6 +76,10 @@ throw new IllegalArgumentException( "Parameter is not a compatible type"); } + + if(resultsToAdd == CalcResult.NULL_RESULT) { + return this; + } if (resultsToAdd instanceof BoundsResult) { BoundsResult boundsToAdd = (BoundsResult) resultsToAdd; Modified: trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/CalcResult.java =================================================================== --- trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/CalcResult.java 2010-11-27 17:05:48 UTC (rev 36359) +++ trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/CalcResult.java 2010-11-28 08:50:01 UTC (rev 36360) @@ -37,6 +37,27 @@ * @source $URL$ */ public interface CalcResult { + /** + * The result obtained when a FeatureCalc found no features to visit. + * It lets itself merge with any other result by just returning the other result + * as the output of the merge + */ + public static final CalcResult NULL_RESULT = new AbstractCalcResult() { + /** + * Always compatible + */ + public boolean isCompatible(CalcResult targetResults) { + return true; + }; + + /** + * Just returns the other result + */ + public CalcResult merge(CalcResult resultsToAdd) { + return resultsToAdd; + }; + }; + /** * Returns true if the target results is a compatible type with the current * results, with compatible meaning that the two results may be merged. Modified: trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/CountVisitor.java =================================================================== --- trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/CountVisitor.java 2010-11-27 17:05:48 UTC (rev 36359) +++ trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/CountVisitor.java 2010-11-28 08:50:01 UTC (rev 36360) @@ -30,7 +30,7 @@ * @source $URL$ */ public class CountVisitor implements FeatureCalc { - int count = 0; + Integer count = null; public void init(SimpleFeatureCollection collection) { //do nothing @@ -40,10 +40,16 @@ } public void visit(org.opengis.feature.Feature feature) { + if(count == null) { + count = 0; + } count++; } public int getCount() { + if(count == null) { + return 0; + } return count; } @@ -52,10 +58,13 @@ } public void reset() { - this.count = 0; + this.count = null; } public CalcResult getResult() { + if(count == null) { + return CalcResult.NULL_RESULT; + } return new CountResult(count); } @@ -71,6 +80,7 @@ } public boolean isCompatible(CalcResult targetResults) { + if (targetResults == CalcResult.NULL_RESULT) return true; if (targetResults instanceof CountResult) return true; if (targetResults instanceof SumResult) return true; return false; @@ -81,6 +91,10 @@ throw new IllegalArgumentException( "Parameter is not a compatible type"); } + + if(resultsToAdd == CalcResult.NULL_RESULT) { + return this; + } if (resultsToAdd instanceof CountResult) { //add the two counts Modified: trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/MaxVisitor.java =================================================================== --- trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/MaxVisitor.java 2010-11-27 17:05:48 UTC (rev 36359) +++ trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/MaxVisitor.java 2010-11-28 08:50:01 UTC (rev 36360) @@ -145,8 +145,7 @@ public CalcResult getResult() { if (!visited) { - throw new IllegalStateException( - "Must visit before max value is ready!"); + return CalcResult.NULL_RESULT; } return new MaxResult(maxvalue); @@ -182,7 +181,7 @@ public boolean isCompatible(CalcResult targetResults) { //list each calculation result which can merge with this type of result - if (targetResults instanceof MaxResult) { + if (targetResults instanceof MaxResult || targetResults == CalcResult.NULL_RESULT) { return true; } @@ -194,6 +193,10 @@ throw new IllegalArgumentException( "Parameter is not a compatible type"); } + + if(resultsToAdd == CalcResult.NULL_RESULT) { + return this; + } if (resultsToAdd instanceof MaxResult) { //take the smaller of the 2 values Modified: trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/MedianVisitor.java =================================================================== --- trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/MedianVisitor.java 2010-11-27 17:05:48 UTC (rev 36359) +++ trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/MedianVisitor.java 2010-11-28 08:50:01 UTC (rev 36360) @@ -125,7 +125,7 @@ return new MedianResult(median); } else if (list.size() < 1) { // no items in the list - return null; + return CalcResult.NULL_RESULT; } else { // we have a list; create a CalcResult containing the list return new MedianResult(list); @@ -174,7 +174,7 @@ public boolean isCompatible(CalcResult targetResults) { //list each calculation result which can merge with this type of result - if (targetResults instanceof MedianResult) return true; + if (targetResults instanceof MedianResult || targetResults == CalcResult.NULL_RESULT) return true; return false; } @@ -188,6 +188,11 @@ throw new IllegalArgumentException( "Parameter is not a compatible type"); } + + if(resultsToAdd == CalcResult.NULL_RESULT) { + return this; + } + if (resultsToAdd instanceof MedianResult) { MedianResult moreResults = (MedianResult) resultsToAdd; //ensure both MedianResults are NOT optimized Modified: trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/MinVisitor.java =================================================================== --- trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/MinVisitor.java 2010-11-27 17:05:48 UTC (rev 36359) +++ trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/MinVisitor.java 2010-11-28 08:50:01 UTC (rev 36360) @@ -113,8 +113,7 @@ public CalcResult getResult() { if (!visited) { - throw new IllegalStateException( - "Must visit before min value is ready!"); + return CalcResult.NULL_RESULT; } return new MinResult(minvalue); @@ -154,7 +153,7 @@ public boolean isCompatible(CalcResult targetResults) { //list each calculation result which can merge with this type of result - if (targetResults instanceof MinResult) { + if (targetResults instanceof MinResult || targetResults == CalcResult.NULL_RESULT) { return true; } @@ -166,6 +165,10 @@ throw new IllegalArgumentException( "Parameter is not a compatible type"); } + + if(resultsToAdd == CalcResult.NULL_RESULT) { + return this; + } if (resultsToAdd instanceof MinResult) { //take the smaller of the 2 values Modified: trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/QuantileListVisitor.java =================================================================== --- trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/QuantileListVisitor.java 2010-11-27 17:05:48 UTC (rev 36359) +++ trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/QuantileListVisitor.java 2010-11-28 08:50:01 UTC (rev 36360) @@ -55,7 +55,9 @@ } public CalcResult getResult() { - if (bins == 0 || count == 0) return null; + if (bins == 0 || count == 0) { + return CalcResult.NULL_RESULT; + } // sort the list Collections.sort(items); Modified: trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/StandardDeviationVisitor.java =================================================================== --- trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/StandardDeviationVisitor.java 2010-11-27 17:05:48 UTC (rev 36359) +++ trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/StandardDeviationVisitor.java 2010-11-28 08:50:01 UTC (rev 36360) @@ -57,6 +57,9 @@ } public CalcResult getResult() { + if(count == 0) { + return CalcResult.NULL_RESULT; + } return new AbstractCalcResult() { public Object getValue() { if (count == 0) return null; Modified: trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/SumVisitor.java =================================================================== --- trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/SumVisitor.java 2010-11-27 17:05:48 UTC (rev 36359) +++ trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/SumVisitor.java 2010-11-28 08:50:01 UTC (rev 36360) @@ -117,6 +117,9 @@ } public CalcResult getResult() { + if(strategy == null) { + return CalcResult.NULL_RESULT; + } return new SumResult(strategy); } @@ -186,6 +189,7 @@ } public boolean isCompatible(CalcResult targetResults) { + if (targetResults == CalcResult.NULL_RESULT) return true; if (targetResults instanceof SumResult) return true; if (targetResults instanceof CountResult) return true; return false; @@ -196,6 +200,10 @@ throw new IllegalArgumentException( "Parameter is not a compatible type"); } + + if(resultsToAdd == CalcResult.NULL_RESULT) { + return this; + } if (resultsToAdd instanceof SumResult) { //create a new strategy object of the correct dataType Modified: trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/UniqueVisitor.java =================================================================== --- trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/UniqueVisitor.java 2010-11-27 17:05:48 UTC (rev 36359) +++ trunk/modules/library/main/src/main/java/org/geotools/feature/visitor/UniqueVisitor.java 2010-11-28 08:50:01 UTC (rev 36360) @@ -113,7 +113,7 @@ public CalcResult getResult() { if (set.size() < 1) { - return null; + return CalcResult.NULL_RESULT; } return new UniqueResult(set); } @@ -131,7 +131,7 @@ public boolean isCompatible(CalcResult targetResults) { //list each calculation result which can merge with this type of result - if (targetResults instanceof UniqueResult) return true; + if (targetResults instanceof UniqueResult || targetResults == CalcResult.NULL_RESULT) return true; return false; } @@ -140,6 +140,10 @@ throw new IllegalArgumentException( "Parameter is not a compatible type"); } + + if(resultsToAdd == CalcResult.NULL_RESULT) { + return this; + } if (resultsToAdd instanceof UniqueResult) { //add one set to the other (to create one big unique list) Modified: trunk/modules/library/main/src/test/java/org/geotools/feature/visitor/VisitorCalculationTest.java =================================================================== --- trunk/modules/library/main/src/test/java/org/geotools/feature/visitor/VisitorCalculationTest.java 2010-11-27 17:05:48 UTC (rev 36359) +++ trunk/modules/library/main/src/test/java/org/geotools/feature/visitor/VisitorCalculationTest.java 2010-11-28 08:50:01 UTC (rev 36360) @@ -26,6 +26,7 @@ import org.geotools.data.DataUtilities; import org.geotools.data.simple.SimpleFeatureCollection; import org.geotools.factory.CommonFactoryFinder; +import org.geotools.feature.FeatureCollections; import org.geotools.feature.simple.SimpleFeatureBuilder; import org.geotools.feature.visitor.MaxVisitor.MaxResult; import org.geotools.feature.visitor.MedianVisitor.MedianResult; @@ -44,6 +45,7 @@ * @source $URL$ */ public class VisitorCalculationTest extends DataTestCase { + SimpleFeatureCollection empty; SimpleFeatureCollection fc; SimpleFeatureType ft; SimpleFeatureCollection fc2; @@ -57,6 +59,7 @@ protected void setUp() throws Exception { super.setUp(); + empty = FeatureCollections.newCollection(); fc = DataUtilities.collection(roadFeatures); fc2 = DataUtilities.collection(riverFeatures); ft = roadType; @@ -112,6 +115,13 @@ minResult7 = minResult7.merge(minResult1); assertEquals(-100.0, minResult7.toDouble(), 0); assertEquals(-100, minResult7.toInt()); + //test empty collection + minVisitor.reset(); + empty.accepts(minVisitor, null); + assertEquals(CalcResult.NULL_RESULT, minVisitor.getResult()); + // test merge + assertSame(minResult2, minVisitor.getResult().merge(minResult2)); + assertSame(minResult2, minResult2.merge(minVisitor.getResult())); } public void testMax() throws IllegalFilterException, IOException { @@ -152,6 +162,13 @@ maxResult7 = maxResult7.merge(maxResult1); assertEquals(6453, maxResult7.toDouble(), 0); assertEquals(6453, maxResult7.toInt()); + //test empty collection + maxVisitor.reset(); + empty.accepts(maxVisitor, null); + assertEquals(CalcResult.NULL_RESULT, maxVisitor.getResult()); + // test merge + assertSame(maxResult2, maxVisitor.getResult().merge(maxResult2)); + assertSame(maxResult2, maxResult2.merge(maxVisitor.getResult())); } public void testMedian() throws IllegalFilterException, IOException { @@ -183,6 +200,13 @@ } catch (Exception e) { assertEquals("Optimized median results cannot be merged.", e.getMessage()); } + //test empty collection + medianVisitor1.reset(); + empty.accepts(medianVisitor1, null); + assertEquals(CalcResult.NULL_RESULT, medianVisitor1.getResult()); + // test merge + assertSame(medianResult2, medianVisitor1.getResult().merge(medianResult2)); + assertSame(medianResult2, medianResult2.merge(medianVisitor1.getResult())); } public void testSum() throws IllegalFilterException, IOException { @@ -209,6 +233,13 @@ //test for destruction during merge assertEquals(13.5, sumResult3.toDouble(), 0); assertEquals(-42.0, sumResult4.toDouble(), 0); + //test empty collection + sumVisitor.reset(); + empty.accepts(sumVisitor, null); + assertEquals(CalcResult.NULL_RESULT, sumVisitor.getResult()); + // test merge + assertSame(sumResult2, sumVisitor.getResult().merge(sumResult2)); + assertSame(sumResult2, sumResult2.merge(sumVisitor.getResult())); } public void testCount() throws IllegalFilterException, IOException { @@ -236,6 +267,13 @@ assertEquals(5, countResult3.toInt()); assertEquals(20, countResult4.toInt()); assertEquals(25, countResult5.toInt()); + //test empty collection + countVisitor.reset(); + empty.accepts(countVisitor, null); + assertEquals(CalcResult.NULL_RESULT, countVisitor.getResult()); + // test merge + assertSame(countResult2, countVisitor.getResult().merge(countResult2)); + assertSame(countResult2, countResult2.merge(countVisitor.getResult())); } public void testAverage() throws IllegalFilterException, IOException { @@ -278,6 +316,13 @@ averageResult2 = averageVisitor2.getResult(); averageResult3 = averageResult1.merge(averageResult2); //int + double --> double? assertEquals((double) 4.33, averageResult3.toDouble(), 0); + //test empty collection + averageVisitor.reset(); + empty.accepts(averageVisitor, null); + assertEquals(CalcResult.NULL_RESULT, averageVisitor.getResult()); + // test merge + assertSame(averageResult2, averageVisitor.getResult().merge(averageResult2)); + assertSame(averageResult2, averageResult2.merge(averageVisitor.getResult())); } public void testUnique() throws IllegalFilterException, IOException { @@ -318,6 +363,13 @@ assertTrue(set.contains(2)); assertTrue(set.contains(4)); assertFalse(set.contains(6)); + //test empty collection + uniqueVisitor.reset(); + empty.accepts(uniqueVisitor, null); + assertEquals(CalcResult.NULL_RESULT, uniqueVisitor.getResult()); + // test merge + assertSame(uniqueResult2, uniqueVisitor.getResult().merge(uniqueResult2)); + assertSame(uniqueResult2, uniqueResult2.merge(uniqueVisitor.getResult())); } public void testBounds() throws IOException { @@ -334,6 +386,13 @@ CalcResult boundsResult3 = boundsResult2.merge(boundsResult1); Envelope env3 = new Envelope(1,13,0,10); assertEquals(env3, boundsResult3.toEnvelope()); + //test empty collection + boundsVisitor1.reset(null); + empty.accepts(boundsVisitor1, null); + assertEquals(CalcResult.NULL_RESULT, boundsVisitor1.getResult()); + // test merge + assertSame(boundsResult2, boundsVisitor1.getResult().merge(boundsResult2)); + assertSame(boundsResult2, boundsResult2.merge(boundsVisitor1.getResult())); } public void testQuantileList() throws Exception { @@ -341,10 +400,18 @@ Expression expr = factory.property(ft.getDescriptor(0).getLocalName()); QuantileListVisitor visitor = new QuantileListVisitor(expr, 2); fc.accepts(visitor, null); - List[] qResult = (List[]) visitor.getResult().getValue(); + CalcResult result = visitor.getResult(); + List[] qResult = (List[]) result.getValue(); assertEquals(2, qResult.length); assertEquals(2, qResult[0].size()); assertEquals(1, qResult[1].size()); + //test empty collection + QuantileListVisitor emptyVisitor = new QuantileListVisitor(expr, 2); + empty.accepts(emptyVisitor, null); + assertEquals(CalcResult.NULL_RESULT, emptyVisitor.getResult()); + // test merge + assertSame(result, emptyVisitor.getResult().merge(result)); + assertSame(result, result.merge(emptyVisitor.getResult())); } public void testStandardDeviation() throws Exception { @@ -352,11 +419,19 @@ Expression expr = factory.property(ft3.getDescriptor(0).getLocalName()); AverageVisitor visit1 = new AverageVisitor(expr); fc3.accepts(visit1, null); - double average = visit1.getResult().toDouble(); + CalcResult result = visit1.getResult(); + double average = result.toDouble(); System.out.println("AV="+average); StandardDeviationVisitor visit2 = new StandardDeviationVisitor(expr, average); fc3.accepts(visit2, null); assertEquals(28.86, visit2.getResult().toDouble(), 0.01); //TODO: verify std_dev(1..100) =~ 28.86 + //test empty collection + StandardDeviationVisitor emptyVisitor = new StandardDeviationVisitor(expr, average); + empty.accepts(emptyVisitor, null); + assertEquals(CalcResult.NULL_RESULT, emptyVisitor.getResult()); + // test merge + assertSame(result, emptyVisitor.getResult().merge(result)); + assertSame(result, result.merge(emptyVisitor.getResult())); } //try merging a count and sum to get an average, both count+sum and sum+count |