Skip to content

Commit

Permalink
[GEOT-7607] Error using function Interpolate in COLOR mode from SLD (g…
Browse files Browse the repository at this point in the history 
…eotools#4823)

* [GEOT-7607] Error using function Interpolate in COLOR mode from SLD

Make `InterpolateFunction.evaluate(Object, Class)` adhere to the interface
contract by returning a value converted to the requested "context" type.

This was not true for the case when the `method` parameter is `COLOR`,
and hence the function would have failed instead of returning a
converted value or `null`, according to `Converters.covert()`.

* revise based on feedback

---------

Co-authored-by: Jody Garnett <[email protected]>
  • Loading branch information
@groldan @jodygarnett
groldan and jodygarnett authored Jun 24, 2024
1 parent 2534428 commit b072b03
Show file tree
Hide file tree
Showing 2 changed files with 112 additions and 26 deletions.
30 changes: 15 additions & 15 deletions modules/library/main/src/main/java/org/geotools/filter/function/InterpolateFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -308,9 +308,8 @@ public Object evaluate(Object object) {
* derived from the {@code method} parameter, as {@code java.awt.Color.class} when {@code method
* == COLOR}, and as {@code java.lang.Double} when {@code method == NUMERIC}.
*
* @throws IllegalArgumentException if the {@code method} parameter is {@code NUMERIC} and
* {@code context == java.awt.Color.class}, or {@code method} is {@code COLOR} and {@code
* context} is specified and is not {@code context == java.awt.Color.class}
* @throws IllegalArgumentException if {@code context == java.awt.Color.class} and {@code method
* != COLOR}
*/
@Override
public <T> T evaluate(Object object, Class<T> context) {
Expand Down Expand Up @@ -372,29 +371,31 @@ public <T> T evaluate(Object object, Class<T> context) {
}
}

/**
* Returns the default context based on method when it's not specified (e.g. null or
* Object.class), for the Converters to return the default object type: Color if method ==
* COLOR, Double of method == NUMERIC. If a specific "context" is asked, returns it as-is.
*/
@SuppressWarnings("unchecked")
private <T> Class<T> sanitizeContext(Class<T> context) {
if (null == context || Object.class.equals(context)) {
final boolean specified = context != null && !Object.class.equals(context);
if (!specified) {
if (method == Method.NUMERIC) {
context = (Class<T>) Double.class;
} else if (method == Method.COLOR) {
context = (Class<T>) Color.class;
} else {
// should never reach here for as long as initialize() is called first
throw new IllegalStateException("Unknown method, expected NUMERIC or COLOR");
}
}
return context;
}

private void validateArguments(Class<?> context) {
if (method == Method.NUMERIC && Color.class.isAssignableFrom(context)) {
if (Color.class.isAssignableFrom(context) && method != Method.COLOR) {
throw new IllegalArgumentException(
"Trying to evaluate the function as Color but the method parameter is set as NUMERIC");
} else if (method == Method.COLOR && !Color.class.isAssignableFrom(context)) {
throw new IllegalArgumentException(
"Trying to evaluate the function as "
+ context.getSimpleName()
+ " but the method parameter is set as COLOR");
"Unable to evaluate Color as the interpolation method is set as " + method);
}
}

Expand Down Expand Up @@ -445,8 +446,7 @@ private <T> T linearInterpolate(
color1.getBlue())),
0,
255);
return context.cast(new Color(r, g, b));

return Converters.convert(new Color(r, g, b), context);
} else { // assume numeric
Double value1 = interpPoints.get(segment).getValue(object);
Double value0 = interpPoints.get(segment - 1).getValue(object);
Expand Down Expand Up @@ -503,7 +503,7 @@ private <T> T cosineInterpolate(
color1.getBlue())),
0,
255);
return context.cast(new Color(r, g, b));
return Converters.convert(new Color(r, g, b), context);

} else { // assume numeric
Double value1 = interpPoints.get(segment).getValue(object);
Expand Down Expand Up @@ -570,7 +570,7 @@ private <T> T cubicInterpolate(
}
int b = (int) clamp(Math.round(doCubic(lookupValue, xi, yi)), 0, 255);

return context.cast(new Color(r, g, b));
return Converters.convert(new Color(r, g, b), context);

} else { // numeric
for (int i = segment - 2, k = 0; k < 4; i++, k++) {
Expand Down
108 changes: 97 additions & 11 deletions modules/library/main/src/test/java/org/geotools/filter/function/InterpolateFunctionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
*/
package org.geotools.filter.function;

import static org.hamcrest.CoreMatchers.instanceOf;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertNull;
Expand All @@ -25,14 +27,18 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Date;
import java.util.List;
import java.util.regex.Pattern;
import org.geotools.api.filter.capability.FunctionName;
import org.geotools.api.filter.expression.Expression;
import org.geotools.api.filter.expression.Function;
import org.geotools.api.filter.expression.Literal;
import org.geotools.api.filter.expression.VolatileFunction;
import org.geotools.filter.FunctionExpressionImpl;
import org.geotools.filter.capability.FunctionNameImpl;
import org.geotools.util.Converter;
import org.geotools.util.Converters;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
Expand Down Expand Up @@ -127,6 +133,48 @@ void testLinearNumericInterpolation(Class<?> context) throws Exception {
assertEquals(values[values.length - 1], result, TOL);
}

/**
* Verify the return value is {@link Converters#convert(Object, Class) converted} to the
* requested context type
*/
@Test
public void testNumericInterpolationStringContext() throws Exception {
double testValue = data[0];
assertThat(
runNumeric(InterpolateFunction.MODE_LINEAR, testValue, String.class),
instanceOf(String.class));
assertThat(
runNumeric(InterpolateFunction.MODE_CUBIC, testValue, String.class),
instanceOf(String.class));
assertThat(
runNumeric(InterpolateFunction.MODE_COSINE, testValue, String.class),
instanceOf(String.class));
}

/**
* Verify the return value is {@link Converters#convert(Object, Class) converted} to the
* requested context type and returns {@code null} if no conversion is possible
*/
@Test
public void testNumericInterpolationUnsupportedContextReturnsNull() throws Exception {
double testValue = data[0];
// preflight, make sure there's no converter for Double -> Date
assertNull(Converters.convert(1d, Date.class));
Class<?> context = Date.class;
assertNull(runNumeric(InterpolateFunction.MODE_LINEAR, testValue, context));
assertNull(runNumeric(InterpolateFunction.MODE_CUBIC, testValue, context));
assertNull(runNumeric(InterpolateFunction.MODE_COSINE, testValue, context));
}

Object runNumeric(String mode, double testValue, Class<?> context) throws Exception {
setupParameters(data, values);
parameters.add(ff2.literal(InterpolateFunction.METHOD_NUMERIC));
parameters.add(ff2.literal(mode));

Function fn = finder.findFunction("interpolate", parameters);
return fn.evaluate(feature(testValue), context);
}

@Test
public void testLinearColorInterpolation() throws Exception {
testLinearColorInterpolation(Color.class);
Expand Down Expand Up @@ -373,6 +421,7 @@ public void testNoModeParameter() throws Exception {
}
}

/** If {@code context == Color.class} and {@code method == NUMERIC} throws an IAE */
@Test
public void testColorValuesNumericMethodMismatch() throws Exception {
/*
Expand All @@ -392,20 +441,57 @@ public void testColorValuesNumericMethodMismatch() throws Exception {
assertTrue(gotEx);
}

/**
* Verify the output is delegated to {@link Converters#convert(Object, Class)} for the provided
* {@code context} {@literal evaluate()} parameter when {@literal method == COLOR}
*/
@Test
public void testNumericValuesColorMethodMismatch() throws Exception {
setupParameters(data, values);
parameters.add(ff2.literal(InterpolateFunction.METHOD_COLOR));
public void testMethodColorWithStringContext() throws Exception {
testMethodColorWithStringContext(InterpolateFunction.MODE_LINEAR);
testMethodColorWithStringContext(InterpolateFunction.MODE_COSINE);
testMethodColorWithStringContext(InterpolateFunction.MODE_CUBIC);
}

Function fn = finder.findFunction("interpolate", parameters);
boolean gotEx = false;
try {
fn.evaluate(feature(data[1]), Double.class);
} catch (IllegalArgumentException ex) {
gotEx = true;
}
/**
* Verify the function returns {@code null} when asked to convert the result to a type for which
* there's no capable {@link Converter}, as specified in the interface contract
*/
@Test
public void testMethodColorWithUnknownConversionContextReturnsNull() throws Exception {
double testValue = (data[1] + data[0]) / 2.0;
Object result;

assertTrue(gotEx);
// preflight, make sure there's no converter for Color -> Date
assertNull(Converters.convert(Color.RED, Date.class));
result = runMethodColor(InterpolateFunction.MODE_LINEAR, testValue, Date.class);
assertNull(result);

// preflight, make sure there's no converter for Color -> Double
assertNull(Converters.convert(Color.RED, Double.class));
result = runMethodColor(InterpolateFunction.MODE_COSINE, testValue, Double.class);
assertNull(result);

// preflight, make sure there's no converter for Color -> Integer
assertNull(Converters.convert(Color.RED, Integer.class));
result = runMethodColor(InterpolateFunction.MODE_CUBIC, testValue, Integer.class);
assertNull(result);
}

void testMethodColorWithStringContext(String interpolationMode) throws Exception {
double testValue = (data[1] + data[0]) / 2.0;
Object result = runMethodColor(interpolationMode, testValue, String.class);
assertThat(result, instanceOf(String.class));
Pattern hexColor = Pattern.compile("^#([a-fA-F0-9]{6}|[a-fA-F0-9]{3})$");
assertTrue(hexColor.matcher((String) result).matches());
}

private Object runMethodColor(String interpolationMode, double testValue, Class<?> context)
throws Exception {
setupParameters(data, colors);
parameters.add(ff2.literal(InterpolateFunction.METHOD_COLOR));
parameters.add(ff2.literal(interpolationMode));
Function fn = finder.findFunction("interpolate", parameters);
return fn.evaluate(feature(testValue), context);
}

/** Set up parameters for the Interpolate function with a set of input data and output values */
Expand Down

0 comments on commit b072b03

Please sign in to comment.