Skip to content

Commit

Permalink
[bugfix][wfs] Expand support for 2.0.0 TYPENAMES
Browse files Browse the repository at this point in the history
Fixes #17872 - WFS 2.0.0 DescribeFeatureType : TypeNames vs TypeName

This PR introduces the plural form of TYPENAME for
2.0.0 WFS servers, with some additional logic to
allow for 2.0.0 servers that only support TYPENAME
for DescribeFeatureType: in this case, the singular
form is also tried in case the plural one fails.

There is still some work to do for transactional
support but at least the read-only client part
should now be ok.

Tests have been added to check that:

- TYPENAME form still works with old 2.0.0
- TYPENAMES form works with compliant 2.0.0
- choice geometry types are handled by calling
  GetFeature and examining the result
  • Loading branch information
elpaso committed Jan 23, 2018
1 parent 3b0afb2 commit ccb4c80
Show file tree
Hide file tree
Showing 9 changed files with 424 additions and 35 deletions.
2 changes: 2 additions & 0 deletions src/providers/wfs/qgswfsconnection.cpp
Expand Up @@ -28,12 +28,14 @@ QgsWfsConnection::QgsWfsConnection( const QString &connName )
const QString &version = settings.value( key + "/" + QgsWFSConstants::SETTINGS_VERSION ).toString();
if ( !version.isEmpty() )
{
mUri.removeParam( QgsWFSConstants::URI_PARAM_VERSION ); // setParam allow for duplicates!
mUri.setParam( QgsWFSConstants::URI_PARAM_VERSION, version );
}

const QString &maxnumfeatures = settings.value( key + "/" + QgsWFSConstants::SETTINGS_MAXNUMFEATURES ).toString();
if ( !maxnumfeatures.isEmpty() )
{
mUri.removeParam( QgsWFSConstants::URI_PARAM_MAXNUMFEATURES ); // setParam allow for duplicates!
mUri.setParam( QgsWFSConstants::URI_PARAM_MAXNUMFEATURES, maxnumfeatures );
}

Expand Down
11 changes: 8 additions & 3 deletions src/providers/wfs/qgswfsdescribefeaturetype.cpp
Expand Up @@ -14,20 +14,25 @@
***************************************************************************/

#include "qgswfsdescribefeaturetype.h"
#include "qgswfsutils.h"

QgsWFSDescribeFeatureType::QgsWFSDescribeFeatureType( const QString &uri )
: QgsWfsRequest( uri )
{
}

bool QgsWFSDescribeFeatureType::requestFeatureType( const QString &WFSVersion,
const QString &typeName )
const QString &typeName, bool forceSingularTypeName )
{
QUrl url( baseURL() );
url.addQueryItem( QStringLiteral( "REQUEST" ), QStringLiteral( "DescribeFeatureType" ) );
url.addQueryItem( QStringLiteral( "VERSION" ), WFSVersion );
url.addQueryItem( QStringLiteral( "TYPENAME" ), typeName );

// The specs are not consistent: is it singular in 1.0.x and plural in 2.0.0?
// see http://docs.opengeospatial.org/is/09-025r2/09-025r2.html#147
if ( ! forceSingularTypeName )
url.addQueryItem( QgsWFSUtils::typeNameParameterForVersion( WFSVersion ).toUpper( ), typeName );
else
url.addQueryItem( QStringLiteral( "TYPENAME" ), typeName );
return sendGET( url, true, false );
}

Expand Down
3 changes: 2 additions & 1 deletion src/providers/wfs/qgswfsdescribefeaturetype.h
Expand Up @@ -25,7 +25,8 @@ class QgsWFSDescribeFeatureType : public QgsWfsRequest
explicit QgsWFSDescribeFeatureType( const QString &uri );

//! Issue the request
bool requestFeatureType( const QString &WFSVersion, const QString &typeName );
bool requestFeatureType( const QString &WFSVersion, const QString &typeName,
bool forceSingularTypeName = false );

protected:
QString errorMessageWithReason( const QString &reason ) override;
Expand Down
62 changes: 46 additions & 16 deletions src/providers/wfs/qgswfsprovider.cpp
Expand Up @@ -919,7 +919,7 @@ bool QgsWFSProvider::deleteFeatures( const QgsFeatureIds &id )
transactionDoc.appendChild( transactionElem );
//delete element
QDomElement deleteElem = transactionDoc.createElementNS( QgsWFSConstants::WFS_NAMESPACE, QStringLiteral( "Delete" ) );
deleteElem.setAttribute( QStringLiteral( "typeName" ), tname );
deleteElem.setAttribute( QgsWFSUtils::typeNameParameterForVersion( mShared->mWFSVersion ), tname );
QDomElement filterElem = transactionDoc.createElementNS( QgsWFSConstants::OGC_NAMESPACE, QStringLiteral( "Filter" ) );


Expand Down Expand Up @@ -984,7 +984,7 @@ bool QgsWFSProvider::changeGeometryValues( const QgsGeometryMap &geometry_map )
continue;
}
QDomElement updateElem = transactionDoc.createElementNS( QgsWFSConstants::WFS_NAMESPACE, QStringLiteral( "Update" ) );
updateElem.setAttribute( QStringLiteral( "typeName" ), tname );
updateElem.setAttribute( QgsWFSUtils::typeNameParameterForVersion( mShared->mWFSVersion ), tname );
//Property
QDomElement propertyElem = transactionDoc.createElementNS( QgsWFSConstants::WFS_NAMESPACE, QStringLiteral( "Property" ) );
QDomElement nameElem = transactionDoc.createElementNS( QgsWFSConstants::WFS_NAMESPACE, QStringLiteral( "Name" ) );
Expand Down Expand Up @@ -1040,6 +1040,7 @@ QString QgsWFSProvider::convertToXML( const QVariant &value )
return valueStr;
}


bool QgsWFSProvider::changeAttributeValues( const QgsChangedAttributesMap &attr_map )
{
//find out typename from uri and strip namespace prefix
Expand All @@ -1065,7 +1066,7 @@ bool QgsWFSProvider::changeAttributeValues( const QgsChangedAttributesMap &attr_
}

QDomElement updateElem = transactionDoc.createElementNS( QgsWFSConstants::WFS_NAMESPACE, QStringLiteral( "Update" ) );
updateElem.setAttribute( QStringLiteral( "typeName" ), tname );
updateElem.setAttribute( QgsWFSUtils::typeNameParameterForVersion( mShared->mWFSVersion ), tname );

QgsAttributeMap::const_iterator attMapIt = attIt.value().constBegin();
for ( ; attMapIt != attIt.value().constEnd(); ++attMapIt )
Expand Down Expand Up @@ -1158,17 +1159,20 @@ QString QgsWFSProvider::translateMetadataValue( const QString &mdKey, const QVar
{
return value.toString();
}
};
}

bool QgsWFSProvider::describeFeatureType( QString &geometryAttribute, QgsFields &fields, QgsWkbTypes::Type &geomType )
bool QgsWFSProvider::describeFeatureType( QString &geometryAttribute,
QgsFields &fields,
QgsWkbTypes::Type &geomType,
bool forceSingularTypeNames )
{
fields.clear();

QgsWFSDescribeFeatureType describeFeatureType( mShared->mURI.uri() );
if ( !describeFeatureType.requestFeatureType( mShared->mWFSVersion,
mShared->mURI.typeName() ) )
mShared->mURI.typeName(), forceSingularTypeNames ) )
{
QgsMessageLog::logMessage( tr( "DescribeFeatureType failed for url %1: %2" ).
QgsMessageLog::logMessage( tr( "DescribeFeatureType network request failed for url %1: %2" ).
arg( dataSourceUri(), describeFeatureType.errorMessage() ), tr( "WFS" ) );
return false;
}
Expand All @@ -1180,7 +1184,7 @@ bool QgsWFSProvider::describeFeatureType( QString &geometryAttribute, QgsFields
if ( !describeFeatureDocument.setContent( response, true, &errorMsg ) )
{
QgsDebugMsg( response );
QgsMessageLog::logMessage( tr( "DescribeFeatureType failed for url %1: %2" ).
QgsMessageLog::logMessage( tr( "DescribeFeatureType XML parse failed for url %1: %2" ).
arg( dataSourceUri(), errorMsg ), tr( "WFS" ) );
return false;
}
Expand All @@ -1189,11 +1193,19 @@ bool QgsWFSProvider::describeFeatureType( QString &geometryAttribute, QgsFields
mShared->mURI.typeName(),
geometryAttribute, fields, geomType, errorMsg ) )
{
// If 2.0.0, let's assume it was a server that only accepted TYPENAME singular form
// and try with that ...
if ( ! forceSingularTypeNames && mShared->mWFSVersion.startsWith( '2' ) )
{
return QgsWFSProvider::describeFeatureType( geometryAttribute,
fields,
geomType,
true );
}
QgsMessageLog::logMessage( tr( "Analysis of DescribeFeatureType response failed for url %1: %2" ).
arg( dataSourceUri(), errorMsg ), tr( "WFS" ) );
return false;
}

return true;
}

Expand Down Expand Up @@ -1241,6 +1253,20 @@ bool QgsWFSProvider::readAttributesFromSchema( QDomDocument &schemaDoc,
}
elementElement = elementElement.nextSiblingElement( QStringLiteral( "element" ) );
}
// Try to get a complex type whose name contains the unprefixed typename
if ( elementTypeString.isEmpty() && complexTypeElement.isNull() )
{
const QDomNodeList complexElements = schemaElement.elementsByTagName( QStringLiteral( "complexType" ) );
for ( int i = 0; i < complexElements.size(); i++ )
{
if ( complexElements.at( i ).toElement().attribute( QStringLiteral( "name" ) ).contains( unprefixedTypename ) )
{
complexTypeElement = complexElements.at( i ).toElement();
break;
}
}
}
// Give up :(
if ( elementTypeString.isEmpty() && complexTypeElement.isNull() )
{
// "http://demo.deegree.org/inspire-workspace/services/wfs?SERVICE=WFS&REQUEST=DescribeFeatureType&VERSION=2.0.0&TYPENAME=ad:Address"
Expand All @@ -1266,7 +1292,6 @@ bool QgsWFSProvider::readAttributesFromSchema( QDomDocument &schemaDoc,
{
errorMsg = tr( "Cannot find element '%1'" ).arg( unprefixedTypename );
}

return false;
}

Expand Down Expand Up @@ -1337,29 +1362,33 @@ bool QgsWFSProvider::readAttributesFromSchema( QDomDocument &schemaDoc,
QRegExp gmlRefProperty( "gml:(.*)Property" );

// gmgml: is Geomedia Web Server
if ( type == QLatin1String( "gmgml:Polygon_Surface_MultiSurface_CompositeSurfacePropertyType" ) )
if ( ! foundGeometryAttribute && type == QLatin1String( "gmgml:Polygon_Surface_MultiSurface_CompositeSurfacePropertyType" ) )
{
foundGeometryAttribute = true;
geometryAttribute = name;
geomType = QgsWkbTypes::MultiPolygon;
}
else if ( type == QLatin1String( "gmgml:LineString_Curve_MultiCurve_CompositeCurvePropertyType" ) )
else if ( ! foundGeometryAttribute && type == QLatin1String( "gmgml:LineString_Curve_MultiCurve_CompositeCurvePropertyType" ) )
{
foundGeometryAttribute = true;
geometryAttribute = name;
geomType = QgsWkbTypes::MultiLineString;
}
//is it a geometry attribute?
// the GeometryAssociationType has been seen in #11785
else if ( type.indexOf( gmlPT ) == 0 || type == QLatin1String( "gml:GeometryAssociationType" ) )
else if ( ! foundGeometryAttribute && ( type.indexOf( gmlPT ) == 0 || type == QLatin1String( "gml:GeometryAssociationType" ) ) )
{
foundGeometryAttribute = true;
geometryAttribute = name;
geomType = geomTypeFromPropertyType( geometryAttribute, gmlPT.cap( 1 ) );
// We have a choice parent element we cannot assume any valid information over the geometry type
if ( attributeElement.parentNode().nodeName() == QStringLiteral( "choice" ) && ! attributeElement.nextSibling().isNull() )
geomType = QgsWkbTypes::Unknown;
else
geomType = geomTypeFromPropertyType( geometryAttribute, gmlPT.cap( 1 ) );
}
//MH 090428: sometimes the <element> tags for geometry attributes have only attribute ref="gml:polygonProperty"
//Note: this was deprecated with GML3.
else if ( ref.indexOf( gmlRefProperty ) == 0 )
else if ( ! foundGeometryAttribute && ref.indexOf( gmlRefProperty ) == 0 )
{
foundGeometryAttribute = true;
geometryAttribute = ref.mid( 4 ); // Strip gml: prefix
Expand Down Expand Up @@ -1441,7 +1470,8 @@ QDomElement QgsWFSProvider::createTransactionElement( QDomDocument &doc ) const
describeFeatureTypeURL = QUrl( QStringLiteral( "http://fake_qgis_http_endpoint" ) );
describeFeatureTypeURL.addQueryItem( QStringLiteral( "REQUEST" ), QStringLiteral( "DescribeFeatureType" ) );
describeFeatureTypeURL.addQueryItem( QStringLiteral( "VERSION" ), QStringLiteral( "1.0.0" ) );
describeFeatureTypeURL.addQueryItem( QStringLiteral( "TYPENAME" ), mShared->mURI.typeName() );
//TODO: proper support of 2.0.0, for now hardcoded
describeFeatureTypeURL.addQueryItem( QgsWFSUtils::typeNameParameterForVersion( WfsVersion ).toUpper(), mShared->mURI.typeName() );

transactionElem.setAttribute( QStringLiteral( "xsi:schemaLocation" ), mApplicationNamespace + ' '
+ describeFeatureTypeURL.toEncoded() );
Expand Down
2 changes: 1 addition & 1 deletion src/providers/wfs/qgswfsprovider.h
Expand Up @@ -151,7 +151,7 @@ class QgsWFSProvider : public QgsVectorDataProvider
The method gives back the name of
the geometry attribute and the thematic attributes with their types*/
bool describeFeatureType( QString &geometryAttribute,
QgsFields &fields, QgsWkbTypes::Type &geomType );
QgsFields &fields, QgsWkbTypes::Type &geomType, bool forceSingularTypeNames = false );

/**
* For a given typename, reads the name of the geometry attribute, the
Expand Down
4 changes: 3 additions & 1 deletion src/providers/wfs/qgswfsrequest.cpp
Expand Up @@ -96,10 +96,12 @@ bool QgsWfsRequest::sendGET( const QUrl &url, bool synchronous, bool forceRefres
}
#endif
modifiedUrlString = modifiedUrlString.mid( 0, modifiedUrlString.indexOf( '?' ) ) + args;
QgsDebugMsg( QString( "Get %1 (after laundering)" ).arg( modifiedUrlString ) );
QgsDebugMsg( QStringLiteral( "Get %1 (after laundering)" ).arg( modifiedUrlString ) );
modifiedUrl = QUrl::fromLocalFile( modifiedUrlString );
}

QgsDebugMsgLevel( QStringLiteral( "Calling: %1" ).arg( modifiedUrl.toDisplayString( ) ), 4 );

QNetworkRequest request( modifiedUrl );
if ( !mUri.auth().setAuthorization( request ) )
{
Expand Down
13 changes: 13 additions & 0 deletions src/providers/wfs/qgswfsutils.cpp
Expand Up @@ -138,6 +138,19 @@ bool QgsWFSUtils::removeDir( const QString &dirName )
return dir.rmdir( dirName );
}

QString QgsWFSUtils::typeNameParameterForVersion( const QString &WfsVersion )
{
// WFS 2.0 uses the plural form TYPENAMES
if ( WfsVersion.startsWith( '2' ) )
{
return QString( "typeNames" );
}
else
{
return QString( "typeName" );
}
}


// We use a keep alive mechanism where every KEEP_ALIVE_DELAY ms we update
// a shared memory segment with the current timestamp. This way, other QGIS
Expand Down
4 changes: 4 additions & 0 deletions src/providers/wfs/qgswfsutils.h
Expand Up @@ -45,6 +45,9 @@ class QgsWFSUtils
//! Return a unique identifier made from feature content
static QString getMD5( const QgsFeature &f );

//! Return the correct form of typeName(s) according to the specified \a WfsVersion
static QString typeNameParameterForVersion( const QString &WfsVersion );

protected:
friend class QgsWFSUtilsKeepAlive;
static QSharedMemory *createAndAttachSHM();
Expand All @@ -62,6 +65,7 @@ class QgsWFSUtils

//! Remove (recursively) a directory.
static bool removeDir( const QString &dirName );

};

//! For internal use of QgsWFSUtils
Expand Down

6 comments on commit ccb4c80

@palmerj
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elpaso Did you test this with geoserver instances? See https://issues.qgis.org/issues/18882

@elpaso
Copy link
Contributor Author

@elpaso elpaso commented on ccb4c80 Apr 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes: I think I did.

@palmerj
Copy link
Contributor

@palmerj palmerj commented on ccb4c80 Apr 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems that on the Geoserver instances we are running TYPENAMES doesn't work for the DescribeFeatureType request. Instead it just ignores TYPENAMES and returns schemas for the entire catalogue. Our catalogue is very big.

@rouault Do you have any thoughts about this?

@rouault
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the commit message "TYPENAME form still works with old 2.0.0". From what I see in http://docs.opengeospatial.org/is/09-025r2/09-025r2.html#142 , TYPENAME is the official parameter name for DescribeFeatureType. So servers requiring TYPENAMES are "buggy" (even if the OGC folks got crazy in changing in GetFeatureType from TYPENAME to TYPENAMES and not doing the same for DescribeFeatureType)

So the logic should be trying first TYPENAME and then TYPENAMES. But as @palmerj underlines a server that ignores a parameter could send the whole catalog. So basically there's no good solution except asking servers to fix their implementation to accept TYPENAME. Or providing a manual switch that knowleadgable users could enable.

@palmerj
Copy link
Contributor

@palmerj palmerj commented on ccb4c80 Apr 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rouault thanks great information.

So we need to revert this change, then possibly implementing a new advanced switch that provides an override for buggy servers.

@palmerj
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elpaso are you able to comment?

Please sign in to comment.