New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Server][FEATURE][needs-docs] Support WFS 1.1.0 #5297

Merged
merged 14 commits into from Oct 17, 2017

Conversation

Projects
None yet
5 participants
@rldhont
Contributor

rldhont commented Oct 4, 2017

Description

QGIS Server already supports WMS 1.3.0 and 1.1.1, WFS 1.0.0 and WCS 1.0.0
WFS 1.1.0 adds parameters like: SrsName, to transform GML geometry and SortBy to order features.

Funded by Ifremer

Checklist

  • Commit messages are descriptive and explain the rationale for changes
  • Commits which add new features are tagged with [FEATURE] in the commit message
  • Commits which change the UI or existing user workflows are tagged with [needs-docs] in the commit message and containt sufficient information in the commit message to be documented
  • I have read the QGIS Coding Standards and this PR complies with them
  • This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
  • New unit tests have been added for core changes
  • I have run the scripts/prepare-commit.sh script before each commit

@rldhont rldhont added this to the QGIS 3 milestone Oct 4, 2017

@rldhont rldhont requested review from elpaso, sbrunner and pblottiere Oct 4, 2017

// Supports WFS 1.0.0
if ( QSTR_COMPARE( versionString, "1.0.0" ) )
{
v1_0_0::writeGetCapabilities( mServerIface, project, versionString, request, response );

This comment has been minimized.

@pblottiere

pblottiere Oct 11, 2017

Member

I'm not a big fan of this notation : v1_0_0::.

Moreover, I've the feeling that most of the code is duplicated between qgswfsgetcapabilities.cpp and qgswfsgetcapabilities_1_0_0.cpp. And it's not homogeneous with other services (for example, we don't have a qgswmsrenderer.cpp and a qgswmsrenderer_1_3_0.cpp).

No? Or am I missing something?

@pblottiere

pblottiere Oct 11, 2017

Member

I'm not a big fan of this notation : v1_0_0::.

Moreover, I've the feeling that most of the code is duplicated between qgswfsgetcapabilities.cpp and qgswfsgetcapabilities_1_0_0.cpp. And it's not homogeneous with other services (for example, we don't have a qgswmsrenderer.cpp and a qgswmsrenderer_1_3_0.cpp).

No? Or am I missing something?

This comment has been minimized.

@rldhont

rldhont Oct 11, 2017

Contributor

The WFS capabilities documents are not the same at all and it will be easiest to enhance or maintain with separate files. For the other request like DescribeFeature or GetFeature, we reused the same code.

@rldhont

rldhont Oct 11, 2017

Contributor

The WFS capabilities documents are not the same at all and it will be easiest to enhance or maintain with separate files. For the other request like DescribeFeature or GetFeature, we reused the same code.

This comment has been minimized.

@pblottiere

pblottiere Oct 11, 2017

Member

It's true that it's not as similar as I thought... However, I think we could factor a little more.

For example, writeGetCapabilities is identical and there's some redundant content in createGetCapabilitiesDocument (WFS_Capabilities, ogc:SpatialOperators) or getFeatureTypeListElement.

That being said, your comment about maintainability is justified...

@nyalldawson @m-kuhn What do you think about adding files for specific service versions (like those introduced in this PR : qgswfsgetcapabilities.cpp and qgswfsgetcapabilities_1_0_0.cpp) ?

@pblottiere

pblottiere Oct 11, 2017

Member

It's true that it's not as similar as I thought... However, I think we could factor a little more.

For example, writeGetCapabilities is identical and there's some redundant content in createGetCapabilitiesDocument (WFS_Capabilities, ogc:SpatialOperators) or getFeatureTypeListElement.

That being said, your comment about maintainability is justified...

@nyalldawson @m-kuhn What do you think about adding files for specific service versions (like those introduced in this PR : qgswfsgetcapabilities.cpp and qgswfsgetcapabilities_1_0_0.cpp) ?

This comment has been minimized.

@pblottiere

pblottiere Oct 13, 2017

Member

FWIW, owslib made the same thing as you (separate versions in files) : https://github.com/geopython/OWSLib/tree/master/owslib/feature

So why not, but in this case, maybe we should have a qgswfsgetcapabilities_common.x, qgswfsgetcapabilities_100.x and qgswfsgetcapabilities_110.x files to be more explicit?

Otherwise (I'm just thinking out loud, so it may probably be a bad idea...), maybe a composite/visitor pattern could be used:

  • 1 node of the tree for 1 node of the document
  • one or several validity versions for each node (the same node may be valid for 1.0.0 or 1.1.0)
  • a visitor collects only nodes for a specific version
  • when we want to add a new version, we just have to add some node and retrieve existing nodes
@pblottiere

pblottiere Oct 13, 2017

Member

FWIW, owslib made the same thing as you (separate versions in files) : https://github.com/geopython/OWSLib/tree/master/owslib/feature

So why not, but in this case, maybe we should have a qgswfsgetcapabilities_common.x, qgswfsgetcapabilities_100.x and qgswfsgetcapabilities_110.x files to be more explicit?

Otherwise (I'm just thinking out loud, so it may probably be a bad idea...), maybe a composite/visitor pattern could be used:

  • 1 node of the tree for 1 node of the document
  • one or several validity versions for each node (the same node may be valid for 1.0.0 or 1.1.0)
  • a visitor collects only nodes for a specific version
  • when we want to add a new version, we just have to add some node and retrieve existing nodes

This comment has been minimized.

@dmarteau

dmarteau Oct 13, 2017

Contributor

Hi @pblottiere

There is several ways to add new versions:

  • Handle several versions in one module (all in one, only one default version registred but assumed to handle everything, or register multiple versions in the same module.)
  • Add new versions with different modules python or c++ (register them with there proper versions)

In the present the case we use a different namespace juste to separates units specific to a given version: I do not care about the name of the namespace as long it is meaningful in the context - IMO, the advantage of using namespace over is that you don't have to rename all your fonctions/objects.

IMHO, we should consider the current implementation as a easy way to implement quickly the 1_1_0 without taking the chance to break actual implementation with more or less heavy refactoring: we will have time to factorize things when all the stuff will be stabilized a little more.

@dmarteau

dmarteau Oct 13, 2017

Contributor

Hi @pblottiere

There is several ways to add new versions:

  • Handle several versions in one module (all in one, only one default version registred but assumed to handle everything, or register multiple versions in the same module.)
  • Add new versions with different modules python or c++ (register them with there proper versions)

In the present the case we use a different namespace juste to separates units specific to a given version: I do not care about the name of the namespace as long it is meaningful in the context - IMO, the advantage of using namespace over is that you don't have to rename all your fonctions/objects.

IMHO, we should consider the current implementation as a easy way to implement quickly the 1_1_0 without taking the chance to break actual implementation with more or less heavy refactoring: we will have time to factorize things when all the stuff will be stabilized a little more.

This comment has been minimized.

@mhugo

mhugo Oct 13, 2017

Contributor

Hi @dmarteau
I don't have a strong opinion against the use of a namespaces here as long as it does not introduce redundancy in the code.
And I would also explicit versions: it is a bit strange to have qgswfsgetcapabilities_1_0_0 and qgswfsgetcapabilities without suffix (which version ?), no ?

@mhugo

mhugo Oct 13, 2017

Contributor

Hi @dmarteau
I don't have a strong opinion against the use of a namespaces here as long as it does not introduce redundancy in the code.
And I would also explicit versions: it is a bit strange to have qgswfsgetcapabilities_1_0_0 and qgswfsgetcapabilities without suffix (which version ?), no ?

This comment has been minimized.

@rldhont

rldhont Oct 13, 2017

Contributor

@mhugo it's because 1.1.0 will be the default version instead of 1.0.0 which become a special version.

@rldhont

rldhont Oct 13, 2017

Contributor

@mhugo it's because 1.1.0 will be the default version instead of 1.0.0 which become a special version.

This comment has been minimized.

@dmarteau

dmarteau Oct 13, 2017

Contributor

@mhugo, @rldhont there is no points to make a debat about it, I really do not care about it as long as naming is not ambiguous.

@dmarteau

dmarteau Oct 13, 2017

Contributor

@mhugo, @rldhont there is no points to make a debat about it, I really do not care about it as long as naming is not ambiguous.

@@ -71,7 +72,15 @@ namespace QgsWfs
if ( QSTR_COMPARE( req, "GetCapabilities" ) )
{
writeGetCapabilities( mServerIface, project, versionString, request, response );
// Supports WFS 1.0.0
if ( QSTR_COMPARE( versionString, "1.0.0" ) )

This comment has been minimized.

@pblottiere

pblottiere Oct 11, 2017

Member

For now there's only 2 versions to manage. But something like QgsProjectVersion (for minor versions and so on) could be used instead of comparing string. What do you think?

@pblottiere

pblottiere Oct 11, 2017

Member

For now there's only 2 versions to manage. But something like QgsProjectVersion (for minor versions and so on) could be used instead of comparing string. What do you think?

This comment has been minimized.

@rldhont

rldhont Oct 13, 2017

Contributor

Not need to use a QgsProjectVersion, here the versionSTring has to be "1.0.0", if it's not the default version has to be returned.

@rldhont

rldhont Oct 13, 2017

Contributor

Not need to use a QgsProjectVersion, here the versionSTring has to be "1.0.0", if it's not the default version has to be returned.

geom = transformed;
crs = params.outputCrs;
if ( crs.isGeographic() && !params.crs.isGeographic() )
prec = std::min( params.precision + 3, 6 );

This comment has been minimized.

@pblottiere

pblottiere Oct 11, 2017

Member

Is it standard to manage precision this way?

@pblottiere

pblottiere Oct 11, 2017

Member

Is it standard to manage precision this way?

This comment has been minimized.

@rldhont

rldhont Oct 11, 2017

Contributor

In the ISO GeoJSON standard, the max precision has been fixed to 6 for EPSG:4326, so I think I can homogenize the precision to 6 for all the Geographic requested outputCrs.

@rldhont

rldhont Oct 11, 2017

Contributor

In the ISO GeoJSON standard, the max precision has been fixed to 6 for EPSG:4326, so I think I can homogenize the precision to 6 for all the Geographic requested outputCrs.

This comment has been minimized.

@haubourg

haubourg Oct 13, 2017

Contributor

Mm, after checking the standard, 6 digit is only a recommandation to avoid heavy files with 15 digits.
As there is a per layer precision configuration windows, I would reuse the precision used for GML output. If really you need a different precision between GML an GeoJSON, we could add another column there, but I don't think it's worth it.
Also we could discuss if setting by default 6 digits there woudn't be a better behavior than the current digit default

@haubourg

haubourg Oct 13, 2017

Contributor

Mm, after checking the standard, 6 digit is only a recommandation to avoid heavy files with 15 digits.
As there is a per layer precision configuration windows, I would reuse the precision used for GML output. If really you need a different precision between GML an GeoJSON, we could add another column there, but I don't think it's worth it.
Also we could discuss if setting by default 6 digits there woudn't be a better behavior than the current digit default

This comment has been minimized.

@haubourg

haubourg Oct 13, 2017

Contributor

Sorry, last sentence was "the current 8 digit default"

@haubourg

haubourg Oct 13, 2017

Contributor

Sorry, last sentence was "the current 8 digit default"

This comment has been minimized.

@rldhont

rldhont Oct 13, 2017

Contributor

@haubourg the problem is not between GML or GeoJSON, is between meters and degrees. In WFS 1.1.0, the GML can be requested in an other CRS than the source. So if the features ares requested in a Geographic CRS, I can't used directly the precision, for exemple 1 for a CRS in meters. The test here is only for this type of transform.
The other point is that I have updated the current default precision to 6 digits to avoid heavy files.

@rldhont

rldhont Oct 13, 2017

Contributor

@haubourg the problem is not between GML or GeoJSON, is between meters and degrees. In WFS 1.1.0, the GML can be requested in an other CRS than the source. So if the features ares requested in a Geographic CRS, I can't used directly the precision, for exemple 1 for a CRS in meters. The test here is only for this type of transform.
The other point is that I have updated the current default precision to 6 digits to avoid heavy files.

This comment has been minimized.

@rldhont

rldhont Oct 13, 2017

Contributor

I can add the inverse, from Geographic to Planar, I can limit to 3 digits.

@rldhont

rldhont Oct 13, 2017

Contributor

I can add the inverse, from Geographic to Planar, I can limit to 3 digits.

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Oct 11, 2017

Member

@rldhont Thank you for this PR, it's good to see WFS 1.1.0 landing in server!

Just some comments:

  • Do you think that QgsWmsParameters and QgsWfsParameters classes could inherit to avoid some duplication (like load or toXXX, and so on)?
  • Just by curiosity, did you try to run OGC tests on WFS services?
  • Check const on variables
Member

pblottiere commented Oct 11, 2017

@rldhont Thank you for this PR, it's good to see WFS 1.1.0 landing in server!

Just some comments:

  • Do you think that QgsWmsParameters and QgsWfsParameters classes could inherit to avoid some duplication (like load or toXXX, and so on)?
  • Just by curiosity, did you try to run OGC tests on WFS services?
  • Check const on variables
@rldhont

This comment has been minimized.

Show comment
Hide comment
@rldhont

rldhont Oct 11, 2017

Contributor

I think that QgsWmsParameters and QgsWfsParameters classes could inherit to avoid some duplication (like load or toXXX, and so on), but I'm not sure how to do it with namespace.
I did not try to run OGC tests on WFS services, because I firstly like to finish it and test it with QGIS.
I think it will be great to check const in all the QGIS Server Capabilities writer.

Contributor

rldhont commented Oct 11, 2017

I think that QgsWmsParameters and QgsWfsParameters classes could inherit to avoid some duplication (like load or toXXX, and so on), but I'm not sure how to do it with namespace.
I did not try to run OGC tests on WFS services, because I firstly like to finish it and test it with QGIS.
I think it will be great to check const in all the QGIS Server Capabilities writer.

@rldhont

This comment has been minimized.

Show comment
Hide comment
@rldhont

rldhont Oct 11, 2017

Contributor

@pblottiere have you some comment on tests/src/python/test_qgsserver.py changes ?

Contributor

rldhont commented Oct 11, 2017

@pblottiere have you some comment on tests/src/python/test_qgsserver.py changes ?

@rldhont

This comment has been minimized.

Show comment
Hide comment
@rldhont

rldhont Oct 14, 2017

Contributor

@pblottiere I'd like to merge it, we can enhance the code after the feature freeze.

Contributor

rldhont commented Oct 14, 2017

@pblottiere I'd like to merge it, we can enhance the code after the feature freeze.

@rldhont rldhont merged commit ab107d0 into qgis:master Oct 17, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment