[GEOS-8034] Fix ysld color conversion master

[hwbllmnn]

From #1648

This fixes GEOS-8034. I'm unsure why the literals were using java.awt.Color objects before, even the tests were checking for that. I tested with Geoserver 2.11 and ysld styles work just fine with this fix. I'll also provide a fix for the master branch shortly.

Bug report https://osgeo-org.atlassian.net/browse/GEOS-8034

[jodygarnett]

Okay what is this logic trying to do? Is there a bug report? There is a bunch of color converters that cover these cases, would be good to avoid duplication.

Okay updated the description with details from #1648

[jodygarnett]

@smithkm do you want to have a look at this?

Since this is a bug fix we can merge it after the code freeze

[jodygarnett]

We have a tension between design (using converters configured with Hints) and YSLD expected functionality.

Functionality to check:

• three digit hex {{'#00F'}}
• octal colors {{'0x007'}}

[jodygarnett]

We need to make sure that litearls with color objects are still supported, please do not remove the above tests, but you can add to them :)

[hwbllmnn]

Please see my comment on #1648 , I ported the fix here as well.

[tbarsballe]

Looks like you have found the true source of the error, although I still have some concerns about your approach. See comments inline.

[tbarsballe]

This seems quite brittle.
Tracing through the existing YSLD logic, it looks like the only time this gets called with a Color an an argument is when it is called by toColorOrNull (line 170 of this class). Rather than risk breaking the behaviour of toObjOrNull, it would be better to modify that method to return the Color object instead.

A simple check prior to calling this method should be sufficient (replace line 170 with the following):

Object obj;
if (expr instanceof Literal) {
  obj = ((Literal)expr).getValue();
  if (obj instanceof Color) {
    return obj;
  }
}
obj = toObjOrNull(expr, false);

(Additionally, this change violates the api defined in the javadocs for this method - it should return either a String or a Number; if you modify toColorOrNull instead you wont have that issue, as that method is expected to return a Color)

[tbarsballe]

With the change suggested above, this test will no longer be valid. Something that tests a full style would probably be better anyway; something like this should work (using the style example reported in the ticket):

    @Test
    public void testEncodeColorMapEntry() throws IOException {
        StyledLayerDescriptor style = new YsldParser(new ByteArrayInputStream(
                    ("name:  Test\n" +
                    "title: Test Style title\n" +
                    "abstract: Styling of Test layer\n" +
                    "feature-styles:\n" +
                    "- rules:\n" +
                    "  - title: raster\n" +
                    "    symbolizers:\n" +
                    "      - raster:\n" +
                    "          opacity: 1.0\n" +
                    "          color-map:\n" +
                    "            type: values\n" +
                    "            entries:\n" +
                    "            - ['#e20374', 1.0, 1, Lorem Ipsum (magenta = covered)]").getBytes())).parse();

        RasterSymbolizer symbolizer = (RasterSymbolizer)((NamedLayer)style.getStyledLayers()[0]).styles().get(0).featureTypeStyles().get(0).rules().get(0).symbolizers().get(0);

        ColorMap colorMap = symbolizer.getColorMap();
        RasterSymbolizerEncoder.ColorMapEntryIterator iterator = new RasterSymbolizerEncoder(symbolizer).new ColorMapEntryIterator(colorMap);
        Tuple map = iterator.next();
        assertEquals("('#E20374',1.0,1,Lorem Ipsum (magenta = covered))", map.toString());
    }

[hwbllmnn]

Ok, I've incorporated your changes, will backport to 17.x now.

[tbarsballe]

I've squashed and merged, and will backport manually, so that the changes are encapsulated within a single commit.