Improve support for encoding complex features

Justin Deoliveira added a comment - 06/May/09 4:53 PM A couple of things: 1) I am not crazy about the reference to ComplexAttribute from ElementEncodeExecutor. The core of the encoder should ideally remain independent of any external dependencies. I realize that in this case the dependency is not "external" but it is a dependency on a particular class of a specific object model which should probably remain confined to GML. So if we can avoid it i would like to. So it seems the problem is that when encoding the binding being executed declares its type to be String, but the object it is getting is a ComplexAttribute. This indicates to me a problem more with the binding. If you can explain the issue to me perhaps you can convince me it is necessary, i just need more context. 2) I would need to do some hand testing but it seems the mods to GML2EncodingUtils kill the simple feature case. The changes to getProperty will basically return null for all cases except for boundedBy? it looks like this is counter balanced with the mods to getProperties() which removes the gml filter check, and only returns attributes for which the value is non-null... hmmm I can't be sure by inspection if this will not break anything. Off the top of my head i can't think of a reason why it would not work, but once again... the cite tests here will relieve any doubt. Has this patch been put through cite QA? For both wfs 1.0 and wfs 1.1? The rest of the patch looks ok at a first glance.

Ben Caradoc-Davies added a comment - 07/May/09 11:55 PM Justin, I got CITE WFS 1.1 test failures caused by boundedBy being encoded too early in sf:PrimitiveGeoFeature (before name or description). The encoder bindings assume that all gml:AbstractFeatureType properties are encoded by getProperty. Leaving only boundedBy in getProperty causes it to be encoded first, which is schema-invalid. I have moved boundedBy handling to getProperties to ensure schema order is respected. If tests pass, I will submit an updated patch.

Ben Caradoc-Davies added a comment - 08/May/09 12:12 AM To address your questions: ElementEncodeExecutor does too much: it calls the encode method of a binding, but also provides automatic conversion services for simple features. This causes encoding of any complexType with simpleContent to fail if the input data is a GeoAPI object. ElementEncodeExecutor was written to support encoding of, for example, CodeType from a String. However, CodeType is most fully represented by a ComplexAttribute (it can have a codeSpace attribute). The instanceof check retains the old behaviour so ElementEncodeExecutor can still automagically convert simple features into XML. The instanceof is necessary because ElementEncodeExecutor will otherwise use Converters.convert to turn any ComplexAttribute bound to a String into a String. Because every Java object has a toString method, this will always "succeed", and garbage will be encoded. This breaks XML well-formedness, not to mention being generally useless. We can avoid the instanceof by getting rid of the autoconversion, and no longer supporting simple features. I thought you might prefer the instanceof. :-) As for the getProperty change, all the properties are now supported by getProperties, even for simple features. See my earlier remark about CITE tests.

Ben Caradoc-Davies added a comment - 08/May/09 2:37 AM Justin attached patch moves all gml:AbstractFeatureType property encoding into getProperties. Unused getProperty methods are removed from the gml2 and gml3 bindings.

Ben Caradoc-Davies added a comment - 08/May/09 2:42 AM - edited Justin, with the updated v2 patch, all GT and GS unit tests pass, except GS wms GetMapTest (remote service is offline: see GEOS-3012 ). All CITE WFS 1.0 tests pass. Only one CITE WFS 1.1 fails, wfs:wfs-1.1.0-Basic-GetFeature-tc42.4, which is expected because I am using PostgreSQL 8.3 ( GEOS-1854 ). I also patch GeoServerLoader and the web2 pom to make CITE WFS 1.1 work with old postgis ( GEOS-2922 ), same as for the last time I ran the CITE tests. Please let me know if this is acceptable.

Ben Caradoc-Davies added a comment - 08/May/09 3:03 AM All this testing is on GeoTools trunk r32955 and GeoServer trunk r12370.

Justin Deoliveira added a comment - 11/May/09 6:31 PM Hi Ben, if CITE is good, the patch looks good to me. But can you make a bit fat comment around the check for ComplexAttribute, basically state why we hardcode the check, and eventually hopefully we can find a better way to solve that later. As for the GeoServerLoader issue ( GEOS-2922 ), I don't see a patch anywhere.,

Ben Caradoc-Davies added a comment - 11/May/09 10:21 PM - edited Thanks very much, Justin. Patch plus big fat ElementEncodeExecutor comment (sans apsirations) committed as r32989.

Ben Caradoc-Davies added a comment - 11/May/09 10:39 PM Patch applied.

Andrea Aime added a comment - 28/Nov/10 2:57 AM Mass closing all issues that have been in "resolved" state for 2 months or more without any feedback or update