[GEOS-9154] Env variables not always expanded when classifying layer

src/extension/sldService/src/main/java/org/geoserver/sldservice/rest/ClassifierController.java

@@ -250,6 +250,12 @@ public Object classify(
             throw new RestException(e.getMessage(), HttpStatus.BAD_REQUEST, e);
         }
 
+        if (rules == null || rules.isEmpty()) {
+            throw new RestException(
+                    "Could not generate any rule, there is likely no data matching the request (layer is empty, of filtered down to no matching features/pixels)",
+                    HttpStatus.NOT_FOUND);
+        }
+
         if (fullSLD) {
             StyledLayerDescriptor sld = SF.createStyledLayerDescriptor();
             NamedLayer namedLayer = SF.createNamedLayer();

src/extension/sldService/src/main/java/org/geoserver/sldservice/rest/ImageReader.java

@@ -18,8 +18,14 @@
 import org.geotools.coverage.grid.GridEnvelope2D;
 import org.geotools.coverage.grid.GridGeometry2D;
 import org.geotools.coverage.grid.io.AbstractGridFormat;
+import org.geotools.coverage.grid.io.GranuleSource;
 import org.geotools.coverage.grid.io.GridCoverage2DReader;
+import org.geotools.coverage.grid.io.StructuredGridCoverage2DReader;
 import org.geotools.coverage.processing.CoverageProcessor;
+import org.geotools.data.Query;
+import org.geotools.data.simple.SimpleFeatureCollection;
+import org.geotools.filter.visitor.SimplifyingFilterVisitor;
+import org.geotools.gce.imagemosaic.ImageMosaicFormat;
 import org.geotools.geometry.Envelope2D;
 import org.geotools.geometry.GeneralEnvelope;
 import org.geotools.geometry.jts.JTS;
@@ -30,8 +36,11 @@
 import org.locationtech.jts.geom.Geometry;
 import org.locationtech.jts.geom.Polygon;
 import org.opengis.coverage.grid.GridEnvelope;
+import org.opengis.coverage.grid.GridGeometry;
+import org.opengis.filter.Filter;
 import org.opengis.parameter.GeneralParameterDescriptor;
 import org.opengis.parameter.GeneralParameterValue;
+import org.opengis.parameter.ParameterValue;
 import org.opengis.parameter.ParameterValueGroup;
 import org.opengis.referencing.FactoryException;
 import org.opengis.referencing.crs.CoordinateReferenceSystem;
@@ -89,12 +98,11 @@ public ImageReader invoke() throws IOException, TransformException, FactoryExcep
                         AbstractGridFormat.USE_JAI_IMAGEREAD.getName().getCode());
 
         // grab the original grid geometry
-        GridEnvelope originalGridrange = reader.getOriginalGridRange();
-        GeneralEnvelope originalEnvelope = reader.getOriginalEnvelope();
-        MathTransform g2w = reader.getOriginalGridToWorld(PixelInCell.CELL_CORNER);
+        Filter readFilter = getReadFilter(readParameters);
+        GridGeometry originalGeometry = getOriginalGridGeometry(reader, readFilter);
+        Envelope2D originalEnvelope = ((GridGeometry2D) originalGeometry).getEnvelope2D();
         CoordinateReferenceSystem crs = originalEnvelope.getCoordinateReferenceSystem();
-        GridGeometry2D originalGeometry =
-                new GridGeometry2D(originalGridrange, PixelInCell.CELL_CORNER, g2w, crs, null);
+        MathTransform g2w = reader.getOriginalGridToWorld(PixelInCell.CELL_CORNER);
         GridGeometry2D readGeometry = new GridGeometry2D(originalGeometry);
 
         // do we need to restrict to a specific bounding box?
@@ -134,6 +142,17 @@ public ImageReader invoke() throws IOException, TransformException, FactoryExcep
             readGeometry = new GridGeometry2D(reducedRange, readGeometry.getEnvelope());
         }
 
+        // if there is a filter, set it back as it might have been simplified (e.g., env var
+        // expansion)
+        if (readFilter != null) {
+            readParameters =
+                    CoverageUtils.mergeParameter(
+                            parameterDescriptors,
+                            readParameters,
+                            readFilter,
+                            ImageMosaicFormat.FILTER.getName().getCode());
+        }
+
         if (!readGeometry.equals(originalGeometry)) {
             readParameters =
                     CoverageUtils.mergeParameter(
@@ -169,6 +188,11 @@ public ImageReader invoke() throws IOException, TransformException, FactoryExcep
 
         // finally perform the read
         coverage = reader.read(readParameters);
+        if (coverage == null) {
+            throw new RestException(
+                    "Could not generate any rule, there is likely no data matching the request (layer is empty, of filtered down to no matching features/pixels)",
+                    HttpStatus.NOT_FOUND);
+        }
 
         // the reader can do a best-effort on grid geometry, might not have cut it
         if (readEnvelope != null && isUncut(coverage, readGeometry)) {
@@ -202,6 +226,46 @@ public ImageReader invoke() throws IOException, TransformException, FactoryExcep
         return this;
     }
 
+    private GridGeometry2D getOriginalGridGeometry(GridCoverage2DReader reader, Filter readFilter)
+            throws IOException {
+        MathTransform g2w = reader.getOriginalGridToWorld(PixelInCell.CELL_CORNER);
+        CoordinateReferenceSystem crs = reader.getCoordinateReferenceSystem();
+
+        if (readFilter != null && reader instanceof StructuredGridCoverage2DReader) {
+            StructuredGridCoverage2DReader sr = (StructuredGridCoverage2DReader) reader;
+            String coverageName = reader.getGridCoverageNames()[0];
+            GranuleSource granules = sr.getGranules(coverageName, true);
+            SimpleFeatureCollection filteredGranules =
+                    granules.getGranules(new Query(null, readFilter));
+            ReferencedEnvelope bounds = filteredGranules.getBounds();
+            if (bounds == null || bounds.isEmpty()) {
+                throw new RestException(
+                        "Could not generate any rule, there is likely no data matching the request (layer is empty, of filtered down to no matching features/pixels)",
+                        HttpStatus.NOT_FOUND);
+            }
+
+            return new GridGeometry2D(PixelInCell.CELL_CORNER, g2w, bounds, null);
+        } else {
+            GridEnvelope originalGridRange = reader.getOriginalGridRange();
+            return new GridGeometry2D(originalGridRange, PixelInCell.CELL_CORNER, g2w, crs, null);
+        }
+    }
+
+    private Filter getReadFilter(GeneralParameterValue[] readParameters) {
+        for (GeneralParameterValue readParameter : readParameters) {
+            if (readParameter instanceof ParameterValue
+                    && ImageMosaicFormat.FILTER
+                            .getName()
+                            .equals(readParameter.getDescriptor().getName())) {
+                ParameterValue pv = (ParameterValue) readParameter;
+                Filter filter = (Filter) pv.getValue();
+                return (Filter) filter.accept(new SimplifyingFilterVisitor(), null);
+            }
+        }
+
+        return null;
+    }
+
     private boolean isUncut(GridCoverage2D coverage, GridGeometry2D targetGridGeometry) {
         Envelope2D actual = coverage.getEnvelope2D();
         Envelope2D expected = targetGridGeometry.getEnvelope2D();

src/extension/sldService/src/test/java/org/geoserver/sldservice/rest/ClassifierTest.java

@@ -29,6 +29,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
+import java.util.Set;
 import java.util.TreeSet;
 import java.util.stream.Collectors;
 import javax.media.jai.PlanarImage;
@@ -50,10 +51,12 @@
 import org.geotools.coverage.grid.GridGeometry2D;
 import org.geotools.coverage.grid.io.AbstractGridFormat;
 import org.geotools.factory.CommonFactoryFinder;
+import org.geotools.filter.function.EnvFunction;
 import org.geotools.filter.function.FilterFunction_parseDouble;
 import org.geotools.filter.text.cql2.CQL;
 import org.geotools.filter.text.cql2.CQLException;
 import org.geotools.filter.text.ecql.ECQL;
+import org.geotools.gce.imagemosaic.ImageMosaicFormat;
 import org.geotools.geometry.jts.ReferencedEnvelope;
 import org.geotools.image.util.ImageUtilities;
 import org.geotools.referencing.CRS;
@@ -757,6 +760,19 @@ public void testEnvVectorGroup0() throws Exception {
         assertNull(CQL.toExpression("env('group')").evaluate(null));
     }
 
+    @Test
+    public void testEnvVectorNotThere() throws Exception {
+        final String restPath =
+                RestBaseController.ROOT_PATH
+                        + "/sldservice/cite:FilteredPoints/"
+                        + getServiceUrl()
+                        + ".xml?"
+                        + "attribute=name&method=uniqueInterval&fullSLD=true&env=group:NotAGroup";
+        MockHttpServletResponse response = getAsServletResponse(restPath);
+        assertEquals(404, response.getStatus());
+        assertNull(CQL.toExpression("env('group')").evaluate(null));
+    }
+
     @Test
     public void testBlueRamp() throws Exception {
         final String restPath =
@@ -1587,7 +1603,7 @@ public void testRasterEnv() throws Exception {
         checkRasterEnv("SE", 1214, 1735);
     }
 
-    public void checkRasterEnv(String direction, double low, double high) throws Exception {
+    private void checkRasterEnv(String direction, double low, double high) throws Exception {
         final String restPath =
                 RestBaseController.ROOT_PATH
                         + "/sldservice/cite:sfdem_mosaic/"
@@ -1604,6 +1620,18 @@ public void checkRasterEnv(String direction, double low, double high) throws Exc
         assertEquals(high, entries[1].getQuantity().evaluate(null, Double.class), 0.1);
     }
 
+    @Test
+    public void testRasterEnvNotFound() throws Exception {
+        final String restPath =
+                RestBaseController.ROOT_PATH
+                        + "/sldservice/cite:sfdem_mosaic/"
+                        + getServiceUrl()
+                        + ".xml?"
+                        + "method=equalInterval&intervals=1&ramp=jet&fullSLD=true&env=direction:IDontExist";
+        MockHttpServletResponse servletResponse = getAsServletResponse(restPath);
+        assertEquals(404, servletResponse.getStatus());
+    }
+
     private Map<GeneralParameterDescriptor, Object> getParametersMap(
             List<GeneralParameterValue> readParameters) {
         return readParameters
@@ -1631,4 +1659,32 @@ private void assertBoundsEquals2D(Envelope env1, Envelope env2, double eps) {
             }
         }
     }
+
+    @Test
+    public void testImageReaderEnv() throws Exception {
+        // the backing reader supports native selection
+        CoverageInfo coverage = getCatalog().getCoverageByName(getLayerId(SFDEM_MOSAIC));
+        try {
+            EnvFunction.setLocalValue("direction", "NE");
+            ImageReader reader = new ImageReader(coverage, 1, DEFAULT_MAX_PIXELS, null).invoke();
+
+            List<GeneralParameterValue> readParameters = reader.getReadParameters();
+            Map<GeneralParameterDescriptor, Object> parameterValues =
+                    getParametersMap(readParameters);
+
+            // check no duplicates
+            Set<String> parameterCodes =
+                    readParameters
+                            .stream()
+                            .map(rp -> rp.getDescriptor().getName().getCode())
+                            .collect(Collectors.toSet());
+            assertEquals(readParameters.size(), parameterCodes.size());
+
+            // the filter has been set with env vars expanded
+            Filter filter = (Filter) parameterValues.get(ImageMosaicFormat.FILTER);
+            assertEquals(ECQL.toFilter("direction = 'NE'"), filter);
+        } finally {
+            EnvFunction.clearLocalValues();
+        }
+    }
 }

src/main/src/main/java/org/geoserver/data/util/CoverageUtils.java

@@ -5,7 +5,7 @@
  */
 package org.geoserver.data.util;
 
-import java.awt.Color;
+import java.awt.*;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.HashMap;
@@ -418,12 +418,30 @@ public static GeneralParameterValue[] mergeParameter(
                 final ParameterValue pv = (ParameterValue) pd.createValue();
                 pv.setValue(value);
 
-                // add to the list
-                GeneralParameterValue[] readParametersClone =
-                        new GeneralParameterValue[readParameters.length + 1];
-                System.arraycopy(readParameters, 0, readParametersClone, 0, readParameters.length);
-                readParametersClone[readParameters.length] = pv;
-                readParameters = readParametersClone;
+                // if it's in the list already, override
+                int existingPvIndex = -1;
+                for (int i = 0; i < readParameters.length && existingPvIndex < 0; i++) {
+                    GeneralParameterValue oldPv = readParameters[i];
+                    if (aliases.contains(oldPv.getDescriptor().getName().getCode())) {
+                        existingPvIndex = i;
+                    }
+                }
+
+                if (existingPvIndex >= 0) {
+                    GeneralParameterValue[] clone =
+                            new GeneralParameterValue[readParameters.length];
+                    System.arraycopy(readParameters, 0, clone, 0, readParameters.length);
+                    clone[existingPvIndex] = pv;
+                    readParameters = clone;
+
+                } else {
+                    // add to the list
+                    GeneralParameterValue[] clone =
+                            new GeneralParameterValue[readParameters.length + 1];
+                    System.arraycopy(readParameters, 0, clone, 0, readParameters.length);
+                    clone[readParameters.length] = pv;
+                    readParameters = clone;
+                }
 
                 // leave
                 break;

src/main/src/main/java/org/vfny/geoserver/global/GeoServerFeatureSource.java

@@ -36,6 +36,7 @@
 import org.geotools.feature.collection.SortedSimpleFeatureCollection;
 import org.geotools.filter.spatial.DefaultCRSFilterVisitor;
 import org.geotools.filter.spatial.ReprojectingFilterVisitor;
+import org.geotools.filter.visitor.SimplifyingFilterVisitor;
 import org.geotools.geometry.jts.ReferencedEnvelope;
 import org.geotools.referencing.CRS;
 import org.geotools.util.factory.Hints;
@@ -228,7 +229,7 @@ public static GeoServerFeatureSource create(
      * @throws DataSourceException If query could not meet the restrictions of definitionQuery
      */
     protected Query makeDefinitionQuery(Query query, SimpleFeatureType schema) throws IOException {
-        if ((query == Query.ALL) || query.equals(Query.ALL)) {
+        if (definitionQuery == null && linearizationTolerance == null) {
             return query;
         }
 
@@ -327,8 +328,16 @@ protected Filter makeDefinitionFilter(Filter filter) throws DataSourceException
         Filter newFilter = filter;
 
         try {
-            if (definitionQuery != Filter.INCLUDE) {
-                newFilter = ff.and(definitionQuery, filter);
+            if (definitionQuery == Filter.INCLUDE) {
+                return filter;
+            }
+            SimplifyingFilterVisitor visitor = new SimplifyingFilterVisitor();
+            Filter simplifiedDefinitionQuery = (Filter) definitionQuery.accept(visitor, null);
+            if (filter == Filter.INCLUDE) {
+                newFilter = simplifiedDefinitionQuery;
+            } else if (simplifiedDefinitionQuery != Filter.INCLUDE) {
+                // expand eventual env vars before hitting the store machinery
+                newFilter = ff.and(simplifiedDefinitionQuery, filter);
             }
         } catch (Exception ex) {
             throw new DataSourceException("Can't create the definition filter", ex);

src/main/src/test/java/org/geoserver/data/util/CoverageUtilsTest.java

@@ -5,13 +5,20 @@
  */
 package org.geoserver.data.util;
 
-import static org.junit.Assert.*;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
 
-import java.awt.Color;
+import java.awt.*;
 import java.util.Collections;
+import java.util.List;
 import java.util.Map;
+import org.geotools.filter.text.cql2.CQL;
+import org.geotools.filter.text.cql2.CQLException;
 import org.geotools.gce.imagemosaic.ImageMosaicFormat;
 import org.junit.Test;
+import org.opengis.filter.Filter;
+import org.opengis.parameter.GeneralParameterDescriptor;
+import org.opengis.parameter.GeneralParameterValue;
 import org.opengis.parameter.ParameterDescriptor;
 import org.opengis.parameter.ParameterValue;
 
@@ -38,4 +45,21 @@ public void testMaxTiles() {
         assertTrue(value instanceof Integer);
         assertEquals(Integer.valueOf(1), value);
     }
+
+    @Test
+    public void testMergeExisting() throws CQLException {
+        List<GeneralParameterDescriptor> descriptors =
+                new ImageMosaicFormat().getReadParameters().getDescriptor().descriptors();
+        GeneralParameterValue[] oldParams = new GeneralParameterValue[1];
+        oldParams[0] = ImageMosaicFormat.FILTER.createValue();
+        Filter filter = CQL.toFilter("a = 6");
+        GeneralParameterValue[] newParams =
+                CoverageUtils.mergeParameter(
+                        descriptors,
+                        oldParams,
+                        filter,
+                        ImageMosaicFormat.FILTER.getName().getCode());
+        assertEquals(1, newParams.length);
+        assertEquals(filter, ((ParameterValue) newParams[0]).getValue());
+    }
 }