From e471a0bd1d1de3f61bd54c196ef40f550a5128da Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Mon, 25 Mar 2024 15:43:27 +1000 Subject: [PATCH] Add option to limit the number of expanded features returned This helps reduce the load on backend servers, as feature expansion can quickly balloon out to a huge number of features. Default to a very conservative expansion limit, requiring users to "opt in" to larger limits which may be inappropriate for a service. --- .../sensorthings/qgssensorthingsprovider.cpp | 12 + .../qgssensorthingsshareddata.cpp | 5 + .../sensorthings/qgssensorthingsshareddata.h | 1 + .../sensorthings/qgssensorthingsutils.h | 3 + .../qgssensorthingssourcewidget.cpp | 38 +- src/ui/qgssensorthingssourcewidgetbase.ui | 99 +++-- .../src/python/test_provider_sensorthings.py | 389 +++++++++++++++++- 7 files changed, 501 insertions(+), 46 deletions(-) diff --git a/src/core/providers/sensorthings/qgssensorthingsprovider.cpp b/src/core/providers/sensorthings/qgssensorthingsprovider.cpp index e198c712d5f..b585a4517f3 100644 --- a/src/core/providers/sensorthings/qgssensorthingsprovider.cpp +++ b/src/core/providers/sensorthings/qgssensorthingsprovider.cpp @@ -394,6 +394,12 @@ QVariantMap QgsSensorThingsProviderMetadata::decodeUri( const QString &uri ) con { components.insert( QStringLiteral( "featureLimit" ), featureLimitParam ); } + ok = false; + const int expansionLimitParam = dsUri.param( QStringLiteral( "expansionLimit" ) ).toInt( &ok ); + if ( ok ) + { + components.insert( QStringLiteral( "expansionLimit" ), expansionLimitParam ); + } switch ( QgsWkbTypes::geometryType( dsUri.wkbType() ) ) { @@ -487,6 +493,12 @@ QString QgsSensorThingsProviderMetadata::encodeUri( const QVariantMap &parts ) c { dsUri.setParam( QStringLiteral( "featureLimit" ), QString::number( featureLimitParam ) ); } + ok = false; + const int expansionLimitParam = parts.value( QStringLiteral( "expansionLimit" ) ).toInt( &ok ); + if ( ok ) + { + dsUri.setParam( QStringLiteral( "expansionLimit" ), QString::number( expansionLimitParam ) ); + } const QString geometryType = parts.value( QStringLiteral( "geometryType" ) ).toString(); if ( geometryType.compare( QLatin1String( "point" ), Qt::CaseInsensitive ) == 0 ) diff --git a/src/core/providers/sensorthings/qgssensorthingsshareddata.cpp b/src/core/providers/sensorthings/qgssensorthingsshareddata.cpp index a7b809a517a..a56f9cbf9ed 100644 --- a/src/core/providers/sensorthings/qgssensorthingsshareddata.cpp +++ b/src/core/providers/sensorthings/qgssensorthingsshareddata.cpp @@ -47,8 +47,13 @@ QgsSensorThingsSharedData::QgsSensorThingsSharedData( const QString &uri ) expandQueryParts.append( QgsSensorThingsUtils::entityToSetString( expandToEntityType ) ); } } + mExpansionLimit = uriParts.value( QStringLiteral( "expansionLimit" ) ).toInt(); if ( !expandQueryParts.empty() ) + { mExpandQueryString = QStringLiteral( "$expand=" ) + expandQueryParts.join( '/' ); + if ( mExpansionLimit > 0 ) + mExpandQueryString += QStringLiteral( "($top=%1)" ).arg( mExpansionLimit ); + } mFields = QgsSensorThingsUtils::fieldsForExpandedEntityType( mEntityType, mExpandTo ); diff --git a/src/core/providers/sensorthings/qgssensorthingsshareddata.h b/src/core/providers/sensorthings/qgssensorthingsshareddata.h index aefaf19e057..84feeacf316 100644 --- a/src/core/providers/sensorthings/qgssensorthingsshareddata.h +++ b/src/core/providers/sensorthings/qgssensorthingsshareddata.h @@ -85,6 +85,7 @@ class QgsSensorThingsSharedData QList< Qgis::SensorThingsEntity > mExpandTo; int mFeatureLimit = 0; + int mExpansionLimit = 0; Qgis::WkbType mGeometryType = Qgis::WkbType::Unknown; QString mGeometryField; QgsFields mFields; diff --git a/src/core/providers/sensorthings/qgssensorthingsutils.h b/src/core/providers/sensorthings/qgssensorthingsutils.h index 7ce0b19e3d4..6c73939aa82 100644 --- a/src/core/providers/sensorthings/qgssensorthingsutils.h +++ b/src/core/providers/sensorthings/qgssensorthingsutils.h @@ -40,6 +40,9 @@ class CORE_EXPORT QgsSensorThingsUtils //! Default limit on number of features fetched static constexpr int DEFAULT_FEATURE_LIMIT = 10000; SIP_SKIP + //! Default limit on number of expanded features fetched + static constexpr int DEFAULT_EXPANSION_LIMIT = 100; SIP_SKIP + /** * Converts a string value to a Qgis::SensorThingsEntity type. * diff --git a/src/gui/providers/sensorthings/qgssensorthingssourcewidget.cpp b/src/gui/providers/sensorthings/qgssensorthingssourcewidget.cpp index f7f185a539e..028be3ade50 100644 --- a/src/gui/providers/sensorthings/qgssensorthingssourcewidget.cpp +++ b/src/gui/providers/sensorthings/qgssensorthingssourcewidget.cpp @@ -42,10 +42,16 @@ QgsSensorThingsSourceWidget::QgsSensorThingsSourceWidget( QWidget *parent ) vl->addWidget( mExtentWidget ); mExtentLimitFrame->setLayout( vl ); + mSpinExpansionLimit->setEnabled( false ); + mSpinPageSize->setClearValue( 0, tr( "Default (%1)" ).arg( QgsSensorThingsUtils::DEFAULT_PAGE_SIZE ) ); mSpinFeatureLimit->setClearValue( 0, tr( "No limit" ) ); + mSpinExpansionLimit->setClearValue( 0, tr( "No limit" ) ); + mSpinExpansionLimit->setToolTip( tr( "Limits the maximum number of related features to fetch when expanding child features" ) ); + // set a relatively conservative feature limit by default, to make it so they have to opt-in to shoot themselves in the foot! mSpinFeatureLimit->setValue( QgsSensorThingsUtils::DEFAULT_FEATURE_LIMIT ); + mSpinExpansionLimit->setValue( QgsSensorThingsUtils::DEFAULT_EXPANSION_LIMIT ); for ( Qgis::SensorThingsEntity type : { @@ -72,7 +78,11 @@ QgsSensorThingsSourceWidget::QgsSensorThingsSourceWidget( QWidget *parent ) connect( mRetrieveTypesButton, &QToolButton::clicked, this, &QgsSensorThingsSourceWidget::retrieveTypes ); mRetrieveTypesButton->setEnabled( false ); connect( mExtentWidget, &QgsExtentWidget::extentChanged, this, &QgsSensorThingsSourceWidget::validate ); - connect( mComboExpandTo, qOverload< int >( &QComboBox::currentIndexChanged ), this, &QgsSensorThingsSourceWidget::validate ); + connect( mComboExpandTo, qOverload< int >( &QComboBox::currentIndexChanged ), this, [ = ]( int ) + { + mSpinExpansionLimit->setEnabled( mComboExpandTo->currentData().isValid() ); + validate(); + } ); validate(); } @@ -124,6 +134,23 @@ void QgsSensorThingsSourceWidget::setSourceUri( const QString &uri ) mSpinFeatureLimit->setValue( QgsSensorThingsUtils::DEFAULT_FEATURE_LIMIT ); } + ok = false; + const int expansionLimitParam = mSourceParts.value( QStringLiteral( "expansionLimit" ) ).toInt( &ok ); + if ( ok ) + { + mSpinExpansionLimit->setValue( expansionLimitParam ); + } + else if ( type != Qgis::SensorThingsEntity::Invalid ) + { + // if not setting an initial uri for a new layer, use "no limit" if it's not present in the uri + mSpinExpansionLimit->clear(); + } + else + { + // when setting an initial uri, use the default, not "no limit" + mSpinExpansionLimit->setValue( QgsSensorThingsUtils::DEFAULT_EXPANSION_LIMIT ); + } + const QgsRectangle bounds = mSourceParts.value( QStringLiteral( "bounds" ) ).value< QgsRectangle >(); if ( !bounds.isNull() ) { @@ -226,6 +253,15 @@ QString QgsSensorThingsSourceWidget::updateUriFromGui( const QString &connection parts.remove( QStringLiteral( "featureLimit" ) ); } + if ( mSpinExpansionLimit->value() > 0 ) + { + parts.insert( QStringLiteral( "expansionLimit" ), QString::number( mSpinExpansionLimit->value() ) ); + } + else + { + parts.remove( QStringLiteral( "expansionLimit" ) ); + } + if ( !mComboExpandTo->currentData().isValid() ) { parts.remove( QStringLiteral( "expandTo" ) ); diff --git a/src/ui/qgssensorthingssourcewidgetbase.ui b/src/ui/qgssensorthingssourcewidgetbase.ui index a2555e48e34..9beccd95a99 100644 --- a/src/ui/qgssensorthingssourcewidgetbase.ui +++ b/src/ui/qgssensorthingssourcewidgetbase.ui @@ -7,7 +7,7 @@ 0 0 537 - 180 + 213 @@ -26,6 +26,13 @@ 0 + + + + Geometry type + + + @@ -53,37 +60,30 @@ - + Extent limit - + + + + 9999999 + + + + + + + Qt::StrongFocus - - - - - - - Entity type - - - - - - - Expand to - - - @@ -91,26 +91,9 @@ - - - - 999999999 - - - - + - - - - Geometry type - - - - - - @@ -118,10 +101,41 @@ - - + + + + Expand to + + + + + + + Entity type + + + + + - 9999999 + 999999999 + + + + + + + + + + Expansion limit + + + + + + + 999999999 @@ -139,8 +153,9 @@ mComboGeometryType mRetrieveTypesButton mSpinPageSize - mComboExpandTo mSpinFeatureLimit + mComboExpandTo + mSpinExpansionLimit mExtentLimitFrame diff --git a/tests/src/python/test_provider_sensorthings.py b/tests/src/python/test_provider_sensorthings.py index 0ab0868dacf..0fa7196bc82 100644 --- a/tests/src/python/test_provider_sensorthings.py +++ b/tests/src/python/test_provider_sensorthings.py @@ -4128,6 +4128,387 @@ class TestPyQgsSensorThingsProvider(QgisTestCase): # , ProviderTestCase): 'Polygon ((103 0, 104 0, 104 1, 103 1, 103 0))'], ) + def test_feature_expansion_with_limit(self): + """ + Test a layer using feature expansion with limited child features + """ + with tempfile.TemporaryDirectory() as temp_dir: + base_path = temp_dir.replace("\\", "/") + endpoint = base_path + "/fake_qgis_http_endpoint" + with open(sanitize(endpoint, ""), "wt", encoding="utf8") as f: + f.write( + """ +{ + "value": [ + { + "name": "Locations", + "url": "endpoint/Locations" + } + ], + "serverSettings": { + } +}""".replace( + "endpoint", "http://" + endpoint + ) + ) + + with open( + sanitize(endpoint, + "/Locations?$top=0&$count=true&$filter=location/type eq 'Polygon' or location/geometry/type eq 'Polygon'"), + "wt", + encoding="utf8", + ) as f: + f.write("""{"@iot.count":3,"value":[]}""") + + with open( + sanitize(endpoint, + "/Locations?$top=2&$count=false&$expand=Things/Datastreams($top=1)&$filter=location/type eq 'Polygon' or location/geometry/type eq 'Polygon'"), + "wt", + encoding="utf8", + ) as f: + f.write( + """ +{ + "value": [ + { + "@iot.selfLink": "endpoint/Locations(1)", + "@iot.id": 1, + "name": "Location 1", + "description": "Desc 1", + "unitOfMeasurements": [ + { + "name": "ug.m-3", + "symbol": "ug.m-3", + "definition": "http://dd.eionet.europa.eu/vocabulary/uom/concentration/ug.m-3" + } + ], + "observationType": "http://www.opengis.net/def/observationType/OGC-OM/2.0/OM_Measurement", + "multiObservationDataTypes": ["http://www.opengis.net/def/observationType/OGC-OM/2.0/OM_Measurement"], + "phenomenonTime": "2017-12-31T23:00:00Z/2018-01-12T04:00:00Z", + "resultTime": "2017-12-31T23:30:00Z/2017-12-31T23:31:00Z", + "properties": { + "owner": "owner 1" + }, + "Things": [ + { + "@iot.selfLink": "endpoint/Things(1)", + "@iot.id": 1, + "name": "Thing 1", + "description": "Description Thing 1", + "properties": { + "countryCode": "AT" + }, + "Datastreams": [ + { + "@iot.selfLink": "endpoint/Datastreams(45)", + "@iot.id": 45, + "name": "Datastream 45", + "description": "Description datastream 45", + "observationType": "http://www.opengis.net/def/observationType/OGC-OM/2.0/OM_Measurement", + "unitOfMeasurement": { + "name": "ug.m-3", + "symbol": "ug.m-3", + "definition": "http://dd.eionet.europa.eu/vocabulary/uom/concentration/ug.m-3" + }, + "phenomenonTime": "2017-12-31T23:00:00Z/2024-03-25T04:00:00Z", + "properties": { + "owner": "someone" + } + } + ] + } + ], + "location": { + "type": "Polygon", + "coordinates": [ + [ + [100, 0], [101, 0], [101, 1], [100, 1], [100, 0] + ] + ] + }, + "Things@iot.navigationLink": "endpoint/Locations(1)/Things", + "HistoricalLocations@iot.navigationLink": "endpoint/Locations(1)/HistoricalLocations" + }, + { + "@iot.selfLink": "endpoint/Locations(2)", + "@iot.id": 2, + "name": "Location 2", + "description": "Desc 2", + "unitOfMeasurements": [ + { + "name": "ug.m-3", + "symbol": "ug.m-3", + "definition": "http://dd.eionet.europa.eu/vocabulary/uom/concentration/ug.m-3" + }], + "observationType": "http://www.opengis.net/def/observationType/OGC-OM/2.0/OM_Measurement", + "multiObservationDataTypes": ["http://www.opengis.net/def/observationType/OGC-OM/2.0/OM_Measurement"], + "phenomenonTime": "2018-12-31T23:00:00Z/2019-01-12T04:00:00Z", + "resultTime": "2018-12-31T23:30:00Z/2018-12-31T23:31:00Z", + "properties": { + "owner": "owner 2" + }, + "Things": [ + { + "@iot.selfLink": "endpoint/Things(2)", + "@iot.id": 2, + "name": "Thing 2", + "description": "Description Thing 2", + "properties": { + "countryCode": "AT" + }, + "Datastreams": [ + { + "@iot.selfLink": "endpoint/Datastreams(51)", + "@iot.id": 51, + "name": "Datastream 51", + "description": "Description datastream 51", + "observationType": "http://www.opengis.net/def/observationType/OGC-OM/2.0/OM_Measurement", + "unitOfMeasurement": { + "name": "ug.m-3", + "symbol": "ug.m-3", + "definition": "http://dd.eionet.europa.eu/vocabulary/uom/concentration/ug.m-3" + }, + "phenomenonTime": "2017-12-31T23:00:00Z/2024-03-25T04:00:00Z", + "properties": { + "owner": "someone" + } + } + ] + } + ], + "location": { + "type": "Polygon", + "coordinates": [ + [ + [102, 0], [103, 0], [103, 1], [102, 1], [102, 0] + ] + ] + }, + "Things@iot.navigationLink": "endpoint/Locations(2)/Things", + "HistoricalLocations@iot.navigationLink": "endpoint/Locations(2)/HistoricalLocations" + + } + ], + "@iot.nextLink": "endpoint/Locations?$top=2&$skip=2&$expand=Things/Datastreams($top=1)&$filter=location/type eq 'Polygon' or location/geometry/type eq 'Polygon'" +} + """.replace( + "endpoint", "http://" + endpoint + ) + ) + + with open( + sanitize(endpoint, + "/Locations?$top=2&$skip=2&$expand=Things/Datastreams($top=1)&$filter=location/type eq 'Polygon' or location/geometry/type eq 'Polygon'"), + "wt", + encoding="utf8", + ) as f: + f.write( + """ + { + "value": [ + { + "@iot.selfLink": "endpoint/Locations(3)", + "@iot.id": 3, + "name": "Location 3", + "description": "Desc 3", + "unitOfMeasurements": [{ + "name": "ug.m-3", + "symbol": "ug.m-3", + "definition": "http://dd.eionet.europa.eu/vocabulary/uom/concentration/ug.m-3" + }], + "observationType": "http://www.opengis.net/def/observationType/OGC-OM/2.0/OM_Measurement", + "multiObservationDataTypes": ["http://www.opengis.net/def/observationType/OGC-OM/2.0/OM_Measurement"], + "phenomenonTime": "2020-12-31T23:00:00Z/2021-01-12T04:00:00Z", + "resultTime": "2020-12-31T23:30:00Z/2020-12-31T23:31:00Z", + "properties": { + "owner": "owner 3" + }, + "Things": [ + { + "@iot.selfLink": "endpoint/Things(8)", + "@iot.id": 8, + "name": "Thing 8", + "description": "Description Thing 8", + "properties": { + "countryCode": "AT" + }, + "Datastreams": [ + { + "@iot.selfLink": "endpoint/Datastreams(59)", + "@iot.id": 59, + "name": "Datastream 59", + "description": "Description datastream 59", + "observationType": "http://www.opengis.net/def/observationType/OGC-OM/2.0/OM_Measurement", + "unitOfMeasurement": { + "name": "ug.m-3", + "symbol": "ug.m-3", + "definition": "http://dd.eionet.europa.eu/vocabulary/uom/concentration/ug.m-3" + }, + "phenomenonTime": "2017-12-31T23:00:00Z/2024-03-25T04:00:00Z", + "properties": { + "owner": "someone" + } + } + ] + } + ], + "location": { + "type": "Polygon", + "coordinates": [ + [ + [103, 0], [104, 0], [104, 1], [103, 1], [103, 0] + ] + ] + }, + "Things@iot.navigationLink": "endpoint/Locations(3)/Things", + "HistoricalLocations@iot.navigationLink": "endpoint/Locations(3)/HistoricalLocations" + } + ] + } + """.replace( + "endpoint", "http://" + endpoint + ) + ) + + vl = QgsVectorLayer( + f"url='http://{endpoint}' pageSize=2 type=MultiPolygonZ entity='Location' expansionLimit=1 expandTo='Thing,Datastream'", + "test", + "sensorthings", + ) + self.assertTrue(vl.isValid()) + # basic layer properties tests + self.assertEqual(vl.storageType(), "OGC SensorThings API") + self.assertEqual(vl.wkbType(), Qgis.WkbType.MultiPolygonZ) + + self.assertEqual(vl.featureCount(), -1) + self.assertEqual(vl.crs().authid(), 'EPSG:4326') + self.assertIn("Entity TypeLocation", + vl.htmlMetadata()) + self.assertIn(f'href="http://{endpoint}/Locations"', + vl.htmlMetadata()) + + self.assertEqual( + [f.name() for f in vl.fields()], + ['id', 'selfLink', 'name', 'description', 'properties', + 'Thing_id', 'Thing_selfLink', 'Thing_name', + 'Thing_description', 'Thing_properties', + 'Thing_Datastream_id', 'Thing_Datastream_selfLink', + 'Thing_Datastream_name', 'Thing_Datastream_description', + 'Thing_Datastream_unitOfMeasurement', + 'Thing_Datastream_observationType', + 'Thing_Datastream_properties', + 'Thing_Datastream_phenomenonTimeStart', + 'Thing_Datastream_phenomenonTimeEnd', + 'Thing_Datastream_resultTimeStart', + 'Thing_Datastream_resultTimeEnd'], + ) + self.assertEqual( + [f.type() for f in vl.fields()], + [ + QVariant.String, + QVariant.String, + QVariant.String, + QVariant.String, + QVariant.Map, + QVariant.String, + QVariant.String, + QVariant.String, + QVariant.String, + QVariant.Map, + QVariant.String, + QVariant.String, + QVariant.String, + QVariant.String, + QVariant.Map, + QVariant.String, + QVariant.Map, + QVariant.DateTime, + QVariant.DateTime, + QVariant.DateTime, + QVariant.DateTime, + ], + ) + + # test retrieving all features from layer + features = list(vl.getFeatures()) + self.assertEqual([f.id() for f in features], + [0, 1, 2]) + self.assertEqual([f["id"] for f in features], + ["1", "2", "3"]) + self.assertEqual( + [f["selfLink"][-13:] for f in features], + ["/Locations(1)", "/Locations(2)", + "/Locations(3)"], + ) + self.assertEqual( + [f["name"] for f in features], + ["Location 1", "Location 2", + "Location 3"], + ) + self.assertEqual( + [f["description"] for f in features], + ["Desc 1", "Desc 2", + "Desc 3"] + ) + self.assertEqual( + [f["properties"] for f in features], + [{'owner': 'owner 1'}, + {'owner': 'owner 2'}, + {'owner': 'owner 3'}] + ) + self.assertEqual( + [f["Thing_id"] for f in features], + ['1', '2', '8'] + ) + self.assertEqual( + [f["Thing_selfLink"][-10:] for f in features], + ['/Things(1)', '/Things(2)', '/Things(8)'] + ) + self.assertEqual( + [f["Thing_name"] for f in features], + ['Thing 1', 'Thing 2', 'Thing 8'] + ) + self.assertEqual( + [f["Thing_description"] for f in features], + ['Description Thing 1', 'Description Thing 2', + 'Description Thing 8'] + ) + self.assertEqual( + [f["Thing_properties"] for f in features], + [{'countryCode': 'AT'}, {'countryCode': 'AT'}, + {'countryCode': 'AT'}] + ) + self.assertEqual( + [f["Thing_Datastream_id"] for f in features], + ['45', '51', '59'] + ) + self.assertEqual( + [f["Thing_Datastream_selfLink"][-16:] for f in features], + ['/Datastreams(45)', '/Datastreams(51)', + '/Datastreams(59)'] + ) + self.assertEqual( + [f["Thing_Datastream_name"] for f in features], + ['Datastream 45', 'Datastream 51', 'Datastream 59'] + ) + self.assertEqual( + [f["Thing_Datastream_description"] for f in features], + ['Description datastream 45', 'Description datastream 51', + 'Description datastream 59'] + ) + self.assertEqual( + [f["Thing_Datastream_properties"] for f in features], + [{'owner': 'someone'}, {'owner': 'someone'}, + {'owner': 'someone'}] + ) + + self.assertEqual( + [f.geometry().asWkt() for f in features], + ['Polygon ((100 0, 101 0, 101 1, 100 1, 100 0))', + 'Polygon ((102 0, 103 0, 103 1, 102 1, 102 0))', + 'Polygon ((103 0, 104 0, 104 1, 103 1, 103 0))'], + ) + def testDecodeUri(self): """ Test decoding a SensorThings uri @@ -4222,7 +4603,7 @@ class TestPyQgsSensorThingsProvider(QgisTestCase): # , ProviderTestCase): }, ) - uri = "url='https://sometest.com/api' type=MultiPolygonZ authcfg='abc' expandTo='Thing,Datastream' entity='Location'" + uri = "url='https://sometest.com/api' type=MultiPolygonZ authcfg='abc' expandTo='Thing,Datastream' expansionLimit=30 entity='Location'" parts = QgsProviderRegistry.instance().decodeUri("sensorthings", uri) self.assertEqual( parts, @@ -4231,6 +4612,7 @@ class TestPyQgsSensorThingsProvider(QgisTestCase): # , ProviderTestCase): "entity": "Location", "geometryType": "polygon", "authcfg": "abc", + 'expansionLimit': 30, "expandTo": ['Thing', 'Datastream'] }, ) @@ -4332,12 +4714,13 @@ class TestPyQgsSensorThingsProvider(QgisTestCase): # , ProviderTestCase): "authcfg": "aaaaa", "entity": "location", "geometryType": "polygon", - "expandTo": ["Thing", "Datastream"] + "expandTo": ["Thing", "Datastream"], + 'expansionLimit': 30 } uri = QgsProviderRegistry.instance().encodeUri("sensorthings", parts) self.assertEqual( uri, - "authcfg=aaaaa type=MultiPolygonZ entity='Location' expandTo='Thing,Datastream' url='http://blah.com'", + "authcfg=aaaaa type=MultiPolygonZ entity='Location' expandTo='Thing,Datastream' expansionLimit='30' url='http://blah.com'", )