GeoTools

SLDParser needs to set namespace context for Geometry/PropertyName

Details

  • Type: Bug Bug
  • Status: Resolved Resolved
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 8.0-M4
  • Fix Version/s: 8.0-RC1
  • Component/s: main
  • Labels:
    None

Description

When a complex path is specified for Geometry in the SLD file:

<sld:PointSymbolizer>
<sld:Geometry>
<ogc:PropertyName>gsml:collarLocation/gsml:BoreholeCollar/gsml:location</ogc:PropertyName>
</sld:Geometry>

It will throw ServiceException:
"The requested Style can not be used with this layer. The style specifies an attribute of gsml:collarLocation/gsml:BoreholeCollar/gsml:location and the layer is: gsml:Borehole"

This is because the namespace context doesn't contain the one from the mapping file.
SLDParser needs to set the namespaces when creating PropertyName in parseGeometry(just like it does in parseFilter).

  1. File
    patch_geoserver_geot4006.diff
    19/Mar/12 10:35 AM
    1 kB
    Niels Charlier
  2. File
    patch_geotools_geot4006.diff
    27/Mar/12 5:48 PM
    14 kB
    Niels Charlier
  3. File
    patch_geotools_geot4006.diff
    21/Mar/12 10:25 AM
    13 kB
    Niels Charlier
  4. File
    patch_geotools_geot4006.diff
    19/Mar/12 10:35 AM
    11 kB
    Niels Charlier

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Niels Charlier added a comment - - edited
There were three things that needed to change to make this work.

1. In SLDParser, namespaces contexted needed to be inserted in the PropertyName created in parseGeometry.
In practice, I accomplished this by moving the namespace process from the FIlterParser to the ExpressionParser, which makes more sense. Now namespaces will be encoded in all expressions of the SLD file, instead of just expressions occuring in filter tags.

Affected package: main.

2. In the StreamingRenderer, the findGeometryCS method needed to be made compatible with using xpaths for property names. This involved the same changes as I have done last year on many places of the streaming renderer: getting a descriptor or property value by evaluating the propertyname against the type or feature rather than just getting it directly by a string name.

Affected package: renderer

3. When evaluating a propertyname against a feature, a GeometryAttribute needs to be converted (unwrapped) to Geometry. I was surprised to find out none of the converters already support this. Simple features seem to return the bare Geometry usually so it is only an issue for complex features. So I made the conversion possible in the ComplexAttributeConverter

Affected package: app-schema
Show
Niels Charlier added a comment - - edited There were three things that needed to change to make this work. 1. In SLDParser, namespaces contexted needed to be inserted in the PropertyName created in parseGeometry. In practice, I accomplished this by moving the namespace process from the FIlterParser to the ExpressionParser, which makes more sense. Now namespaces will be encoded in all expressions of the SLD file, instead of just expressions occuring in filter tags. Affected package: main. 2. In the StreamingRenderer, the findGeometryCS method needed to be made compatible with using xpaths for property names. This involved the same changes as I have done last year on many places of the streaming renderer: getting a descriptor or property value by evaluating the propertyname against the type or feature rather than just getting it directly by a string name. Affected package: renderer 3. When evaluating a propertyname against a feature, a GeometryAttribute needs to be converted (unwrapped) to Geometry. I was surprised to find out none of the converters already support this. Simple features seem to return the bare Geometry usually so it is only an issue for complex features. So I made the conversion possible in the ComplexAttributeConverter Affected package: app-schema
Hide
Permalink
Niels Charlier added a comment - - edited
The patch is ready but having problem with my geoserver build so I can't test it completely b4 putting in review
Show
Niels Charlier added a comment - - edited The patch is ready but having problem with my geoserver build so I can't test it completely b4 putting in review
Hide
Permalink
Niels Charlier added a comment -
Jody, could you please review the changes in main (geotools patch).
SLDParserTest was extended to extend new functionality.
Show
Niels Charlier added a comment - Jody, could you please review the changes in main (geotools patch). SLDParserTest was extended to extend new functionality.
Hide
Permalink
Niels Charlier added a comment -
new version with complexattconverterfactorytest
Show
Niels Charlier added a comment - new version with complexattconverterfactorytest
Hide
Permalink
Andrea Aime added a comment -
Niels, I'll review this one, Jody has not reviewed patches in months (I doubt he's looking much at jira at all).
Observations:
- please avoid commenting out lines or blocks of code, if they are not supposed to be there remove them altogheter
- don't prefix comments with NC, it's a old habit that some coders had but it adds nothing informative to the understanding of the sources,
  if you want attribution (which is totally fair) add your name among the class authors instead
- the changes in the streaming renderer have no tests... we'd really need some equivalent of the property data store for complex features
  that would allow for easy to setup tests. For the time being, does it make sense to add a rendering test to the complex feature store
  module?
Show
Andrea Aime added a comment - Niels, I'll review this one, Jody has not reviewed patches in months (I doubt he's looking much at jira at all). Observations: - please avoid commenting out lines or blocks of code, if they are not supposed to be there remove them altogheter - don't prefix comments with NC, it's a old habit that some coders had but it adds nothing informative to the understanding of the sources,   if you want attribution (which is totally fair) add your name among the class authors instead - the changes in the streaming renderer have no tests... we'd really need some equivalent of the property data store for complex features   that would allow for easy to setup tests. For the time being, does it make sense to add a rendering test to the complex feature store   module?
Hide
Permalink
Niels Charlier added a comment -
The only way to test complex features and wms compatibility right now is in the geoserver app-schema test module, this test is included in the geoserver patch file.
Well actually, in this case a new test didn't have to be written because I added a geometry to the positional accuracy SLD file (see patch), so that the old WmsGetMapTest does the job. Part of the confirmation of the render code change is also given by the fact that the existing tests don't break on them.
Show
Niels Charlier added a comment - The only way to test complex features and wms compatibility right now is in the geoserver app-schema test module, this test is included in the geoserver patch file. Well actually, in this case a new test didn't have to be written because I added a geometry to the positional accuracy SLD file (see patch), so that the old WmsGetMapTest does the job. Part of the confirmation of the render code change is also given by the fact that the existing tests don't break on them.
Hide
Permalink
Niels Charlier added a comment - - edited
Thank you very much for helping out, Andrea
Show
Niels Charlier added a comment - - edited Thank you very much for helping out, Andrea
Hide
Permalink
Niels Charlier added a comment -
I have improved the patch, please review when possible.

I completely agree about not leaving commented out old code, but I always do that initially while trying and then forget about it afterwards when it works.( I need to review my own patches better before I send them in, I know)
Show
Niels Charlier added a comment - I have improved the patch, please review when possible. I completely agree about not leaving commented out old code, but I always do that initially while trying and then forget about it afterwards when it works.( I need to review my own patches better before I send them in, I know)
Hide
Permalink
Andrea Aime added a comment -
Niels, looks good, please commit it.

Just one minor note: with geomName.getPropertyName().equals("") you are still prone to a NPE if getPropertyName() returns null, "".equals(geomName.getPropertyName() would not be affected instead
Show
Andrea Aime added a comment - Niels, looks good, please commit it. Just one minor note: with geomName.getPropertyName().equals("") you are still prone to a NPE if getPropertyName() returns null, "".equals(geomName.getPropertyName() would not be affected instead
Hide
Permalink
Niels Charlier added a comment -
Thank you for approving, Andrea. I followed your suggestion for the comparison.

Committed r38657 on Geotools
Committed r16883 on Geoserver
Show
Niels Charlier added a comment - Thank you for approving, Andrea. I followed your suggestion for the comparison. Committed r38657 on Geotools Committed r16883 on Geoserver

People

  • Assignee:
    Andrea Aime
    Reporter:
    Rini Angreani
Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: