Make Protocol.WFS work with just featureType and featurePrefix

[ahocevar]

The OpenLayers.Protocol.WFS.fromWMSLayer convenience function can easily fail, because it does not know the featureNS and geometryName. The WFST format's Query writer only adds the featurePrefix to the typeName attribute when featureNS is provided. And to successfully make a first query with the commonly used BBOX filter (e.g. when a BBOX strategy is used), the BBOX filter should only add the geometryName if we know it for sure. Omission of the PropertyName of a BBOX filter is compliant with the WFS 1.1.0 spec, and also works in GeoServer with WFS 1.0.0. The result (i.e. GetFeature response) of the successful query can then be parsed with the GML format, which can configure the geometryName and featureNS for subsequent transactions or queries.

[ahocevar]

no tests yet

[ahocevar]

Note to reviewers: if reviewed before #3367 is in (or patch applied), the 2nd assertion of the new tests in Protocol.WFS will fail.

Tests pass in Safari5. Thanks for any review.

[ahocevar]

Please ignore the note to reviewers above - all tests pass regardless of whether they are run before or after #3367 (the featureNS autodetection was in Protocol.WFS.v1 previously).

Again, thanks for any review.

[bartvde]

Looks good to go, relevant tests also pass in FF4, with a few remarks/questions:

• A question about the change you made in lib/OpenLayers/Format/WFST/v1_0_0.js (and 1_1_0.js as well), are you 100% that in all cases both prefix and featureNS will be set? Imagine the probably hypothetical case where only prefix is set, we would get a prefixed node, but no xmlns declaration leading to invalid XML. Is it not safer to keep the current code or check for both prefix and featureNS?

• Maybe add a comment to the change in lib/OpenLayers/Format/Filter/v1_0_0.js stating this will only happen if you configure geometryName with null, so it's not default behaviour to create invalid WFS 1.0 requests.

• lib/OpenLayers/Protocol/WFS/v1.js: the comment about default prefix is this not implementation (i.e. GeoServer) specific? I don't think e.g. Mapserver has something equivalent to default namespace/workspace. Why not just state if the prefix matches the prefix of the geometry node's "featureNode"?

• GML/Base talks about a default value of "geometry", but Protocol WFS mentions "the_geom". So isn't the default value geometry?

• Put var format = this.readFormat || this.format; before the read call to simplify.

• Trailing comma in tests/Format/WFST/v1_0_0.html and tests/Format/WFST/v1_1_0.html

• tests/Format/Filter/v1_0_0.html: typo in test function name test_BBOX_noGeometrName

• tests/Protocol/WFS.html: trailing comma

• was the change in wfs:Delete made to pass the tests? So this mean we cannot write out a request anymore with: <ogc:PropertyName/> ? Or would this require setting geometryName to an empty string?

[ahocevar]

Thanks Bart for the thorough review. Some follow-up questions and explanations:

Replying to bartvde:

* A question about the change you made in lib/OpenLayers/Format/WFST/v1_0_0.js (and 1_1_0.js as well), are you 100% that in all cases both prefix and featureNS will be set? Imagine the probably hypothetical case where only prefix is set, we would get a prefixed node, but no xmlns declaration leading to invalid XML. Is it not safer to keep the current code or check for both prefix and featureNS?

I don't see this problem. The typeName attribute of the Query is not a node, so XML doesn't care what goes in there. Note that the whole point of this change is to make things work when only the featurePrefix and no featureNS is set.

* Maybe add a comment to the change in lib/OpenLayers/Format/Filter/v1_0_0.js stating this will only happen if you configure geometryName with null, so it's not default behaviour to create invalid WFS 1.0 requests.

Will do.

* lib/OpenLayers/Protocol/WFS/v1.js: the comment about default prefix is this not implementation (i.e. GeoServer) specific? I don't think e.g. Mapserver has something equivalent to default namespace/workspace. Why not just state if the prefix matches the prefix of the geometry node's "featureNode"?

Good point. But it's not just the prefix of the geometry node's "featureNode", it's "the feature prefix used by the server".

* GML/Base talks about a default value of "geometry", but Protocol WFS mentions "the_geom". So isn't the default value geometry?

This really depends on the inheritance chain and the mixins used. I just used what the docs already said before my change. If you can confirm that it is "geometry" in this case (which I doubt, because otherwise we wouldn't have been able to use this with GeoServer ever without setting the geometryName), I'll change it.

* Put var format = this.readFormat || this.format; before the read call to simplify.

I had this already, but changed it back because one read call is with options and the other without, and I didn't investigate if the options do harm in the case where we currently don't use them. If you can confirm that they don't, I'll change it.

* Trailing comma in tests/Format/WFST/v1_0_0.html and tests/Format/WFST/v1_1_0.html * tests/Format/Filter/v1_0_0.html: typo in test function name test_BBOX_noGeometrName * tests/Protocol/WFS.html: trailing comma

Thanks, will change them.

* was the change in wfs:Delete made to pass the tests? So this mean we cannot write out a request anymore with: <ogc:PropertyName/> ? Or would this require setting geometryName to an empty string?

Yes, this change was made to make the test pass. Do you see any side effects of this? Would it be desirable being able to serialize a Query that has an empty PropertyName element? What would be the point of doing so?

I'm waiting for your feedback before I commit this.

[bartvde]

Replying to ahocevar:

Thanks Bart for the thorough review. Some follow-up questions and explanations: Replying to bartvde:

* A question about the change you made in lib/OpenLayers/Format/WFST/v1_0_0.js (and 1_1_0.js as well), are you 100% that in all cases both prefix and featureNS will be set? Imagine the probably hypothetical case where only prefix is set, we would get a prefixed node, but no xmlns declaration leading to invalid XML. Is it not safer to keep the current code or check for both prefix and featureNS?

I don't see this problem. The typeName attribute of the Query is not a node, so XML doesn't care what goes in there. Note that the whole point of this change is to make things work when only the featurePrefix and no featureNS is set.

You're right, there is no issue, please ignore my previous comment.

* Maybe add a comment to the change in lib/OpenLayers/Format/Filter/v1_0_0.js stating this will only happen if you configure geometryName with null, so it's not default behaviour to create invalid WFS 1.0 requests.

Will do.

* lib/OpenLayers/Protocol/WFS/v1.js: the comment about default prefix is this not implementation (i.e. GeoServer) specific? I don't think e.g. Mapserver has something equivalent to default namespace/workspace. Why not just state if the prefix matches the prefix of the geometry node's "featureNode"?

Good point. But it's not just the prefix of the geometry node's "featureNode", it's "the feature prefix used by the server".

* GML/Base talks about a default value of "geometry", but Protocol WFS mentions "the_geom". So isn't the default value geometry?

This really depends on the inheritance chain and the mixins used. I just used what the docs already said before my change. If you can confirm that it is "geometry" in this case (which I doubt, because otherwise we wouldn't have been able to use this with GeoServer ever without setting the geometryName), I'll change it.

You're right, I had missed that. Though it is kind of confusing to have different defaults at different levels, at least it confused me for a minute :-)

* Put var format = this.readFormat || this.format; before the read call to simplify.

I had this already, but changed it back because one read call is with options and the other without, and I didn't investigate if the options do harm in the case where we currently don't use them. If you can confirm that they don't, I'll change it.

I had missed that, I think they can potentially harm (e.g. GeoJSON), so leave it as you had it.

* Trailing comma in tests/Format/WFST/v1_0_0.html and tests/Format/WFST/v1_1_0.html * tests/Format/Filter/v1_0_0.html: typo in test function name test_BBOX_noGeometrName * tests/Protocol/WFS.html: trailing comma

Thanks, will change them.

* was the change in wfs:Delete made to pass the tests? So this mean we cannot write out a request anymore with: <ogc:PropertyName/> ? Or would this require setting geometryName to an empty string?

Yes, this change was made to make the test pass. Do you see any side effects of this? Would it be desirable being able to serialize a Query that has an empty PropertyName element? What would be the point of doing so?

Maybe there is a WFS out there that needs PropertyName, but ignores its value for WFS 1.0. But I think we can safely ignore until someone files a bug with a case that fails.

I'm waiting for your feedback before I commit this.

Thanks for the follow-up questions, seems most of my concerns are not valid.

[ahocevar]

with changes suggested by bartvde

[ahocevar]

(In [12129]) making Protocol.WFS work with just featureType and featurePrefix configured. r=bartvde (closes #3368)

[bartvde]

This change probably killed a testcase in OpenLayers.Format.SLD:

test_writeSpatialFilter fail 1 ok 0

fail rule with spatial filter correctly written: Bad child 1 for element sld:Rule: Bad child 0 for element ogc:Filter: Children length mismatch for ogc:BBOX: got '1' but expected '2'

[bartvde]

Fixed with r12141

Replying to bartvde:

This change probably killed a testcase in OpenLayers.Format.SLD: {{{ test_writeSpatialFilter fail 1 ok 0 fail rule with spatial filter correctly written: Bad child 1 for element sld:Rule: Bad child 0 for element ogc:Filter: Children length mismatch for ogc:BBOX: got '1' but expected '2' }}}