From abc96ce2914caa0694071f4c3f173e56a1f4dfc3 Mon Sep 17 00:00:00 2001 From: Alessandro Pasotti Date: Tue, 21 Aug 2018 19:32:00 +0200 Subject: [PATCH 1/4] [bugfix] Vector file writer: also check for layername before giving up while (over)writing gpkg &C from vector file writer With tests Fixes #19611 - Cannot save selected features back to the same GeoPackage --- src/core/qgsvectorfilewriter.cpp | 25 ++++++++++-- tests/src/python/test_qgsvectorfilewriter.py | 41 ++++++++++++++++++++ 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/src/core/qgsvectorfilewriter.cpp b/src/core/qgsvectorfilewriter.cpp index 09f6036a095..904d98f0345 100644 --- a/src/core/qgsvectorfilewriter.cpp +++ b/src/core/qgsvectorfilewriter.cpp @@ -2550,16 +2550,35 @@ QgsVectorFileWriter::WriterError QgsVectorFileWriter::writeAsVectorFormat( Prepa int lastProgressReport = 0; long total = details.featureCount; + // Special rules for OGR layers if ( details.providerType == QLatin1String( "ogr" ) && !details.dataSourceUri.isEmpty() ) { QStringList theURIParts = details.dataSourceUri.split( '|' ); QString srcFileName = theURIParts[0]; + QgsStringMap srcUriParams; + if ( theURIParts.length() > 0 ) + { + for ( int i = 1; i < theURIParts.length(); ++i ) + { + QStringList parts( theURIParts[i].split( '=' ) ); + if ( parts.length() == 2 ) + srcUriParams[parts[0]] = parts[1]; + } + } if ( QFile::exists( srcFileName ) && QFileInfo( fileName ).canonicalFilePath() == QFileInfo( srcFileName ).canonicalFilePath() ) { - if ( errorMessage ) - *errorMessage = QObject::tr( "Cannot overwrite a OGR layer in place" ); - return ErrCreateDataSource; + // Check the layer name too if it's a GPKG/SpatiaLite/SQLite OGR driver + QgsDataSourceUri uri( details.dataSourceUri ); + if ( !( ( options.driverName == QLatin1String( "GPKG" ) || + options.driverName == QLatin1String( "SpatiaLite" ) || + options.driverName == QLatin1String( "SQLite" ) ) && + options.layerName != srcUriParams["layername"] ) ) + { + if ( errorMessage ) + *errorMessage = QObject::tr( "Cannot overwrite a OGR layer in place" ); + return ErrCreateDataSource; + } } // Shapefiles might contain multi types although wkbType() only reports singles diff --git a/tests/src/python/test_qgsvectorfilewriter.py b/tests/src/python/test_qgsvectorfilewriter.py index 22d440b0cbc..27d39199c29 100644 --- a/tests/src/python/test_qgsvectorfilewriter.py +++ b/tests/src/python/test_qgsvectorfilewriter.py @@ -32,6 +32,7 @@ from qgis.core import (QgsVectorLayer, ) from qgis.PyQt.QtCore import QDate, QTime, QDateTime, QVariant, QDir import os +import tempfile import osgeo.gdal # NOQA from osgeo import gdal, ogr from qgis.testing import start_app, unittest @@ -898,6 +899,46 @@ class TestQgsVectorFileWriter(unittest.TestCase): self.assertTrue(QgsVectorFileWriter.supportsFeatureStyles('MapInfo File')) self.assertTrue(QgsVectorFileWriter.supportsFeatureStyles('MapInfo MIF')) + def testOverwriteGPKG(self): + """Test that overwriting the same origin GPKG file works only if the layername is different""" + + # Prepare test data + ml = QgsVectorLayer('Point?field=firstfield:int&field=secondfield:int', 'test', 'memory') + provider = ml.dataProvider() + ft = QgsFeature() + ft.setAttributes([4, -10]) + provider.addFeatures([ft]) + filehandle, filename = tempfile.mkstemp('.gpkg') + + options = QgsVectorFileWriter.SaveVectorOptions() + options.driverName = 'GPKG' + options.layerName = 'test' + write_result, error_message = QgsVectorFileWriter.writeAsVectorFormat( + ml, + filename, + options) + self.assertEqual(write_result, QgsVectorFileWriter.NoError, error_message) + + # Real test + vl = QgsVectorLayer("%s|layername=test" % filename, 'src_test', 'ogr') + self.assertTrue(vl.isValid()) + self.assertEqual(vl.featureCount(), 1) + + # This must fail + write_result, error_message = QgsVectorFileWriter.writeAsVectorFormat( + vl, + filename, + options) + self.assertEqual(write_result, QgsVectorFileWriter.ErrCreateDataSource) + self.assertEqual(error_message, 'Cannot overwrite a OGR layer in place') + + options.layerName = 'test2' + write_result, error_message = QgsVectorFileWriter.writeAsVectorFormat( + vl, + filename, + options) + self.assertEqual(write_result, QgsVectorFileWriter.NoError, error_message) + if __name__ == '__main__': unittest.main() From f4daa8cb60244c6d8f808371d48bc17db0acf4c6 Mon Sep 17 00:00:00 2001 From: Alessandro Pasotti Date: Wed, 22 Aug 2018 08:59:48 +0200 Subject: [PATCH 2/4] Added providerUriParams to QgsVectorFileWriter::PreparedWriterDetails Also added a test for the uri parsing --- src/core/qgsvectorfilewriter.cpp | 19 +++++------- src/core/qgsvectorfilewriter.h | 2 ++ tests/src/core/testqgsvectorfilewriter.cpp | 34 ++++++++++++++++++++++ 3 files changed, 44 insertions(+), 11 deletions(-) diff --git a/src/core/qgsvectorfilewriter.cpp b/src/core/qgsvectorfilewriter.cpp index 904d98f0345..e6aa07d805e 100644 --- a/src/core/qgsvectorfilewriter.cpp +++ b/src/core/qgsvectorfilewriter.cpp @@ -33,6 +33,7 @@ #include "qgsexception.h" #include "qgssettings.h" #include "qgsgeometryengine.h" +#include "qgsproviderregistry.h" #include #include @@ -2422,6 +2423,7 @@ QgsVectorFileWriter::WriterError QgsVectorFileWriter::prepareWriteAsVectorFormat details.dataSourceUri = layer->dataProvider()->dataSourceUri(); details.storageType = layer->storageType(); details.selectedFeatureIds = layer->selectedFeatureIds(); + details.providerUriParams = QgsProviderRegistry::instance()->decodeUri( layer->providerType(), layer->dataProvider()->dataSourceUri() ); if ( details.storageType == QLatin1String( "ESRI Shapefile" ) ) { @@ -2554,26 +2556,21 @@ QgsVectorFileWriter::WriterError QgsVectorFileWriter::writeAsVectorFormat( Prepa if ( details.providerType == QLatin1String( "ogr" ) && !details.dataSourceUri.isEmpty() ) { QStringList theURIParts = details.dataSourceUri.split( '|' ); - QString srcFileName = theURIParts[0]; QgsStringMap srcUriParams; + QString srcFileName; if ( theURIParts.length() > 0 ) { - for ( int i = 1; i < theURIParts.length(); ++i ) - { - QStringList parts( theURIParts[i].split( '=' ) ); - if ( parts.length() == 2 ) - srcUriParams[parts[0]] = parts[1]; - } + srcFileName = theURIParts[0]; } if ( QFile::exists( srcFileName ) && QFileInfo( fileName ).canonicalFilePath() == QFileInfo( srcFileName ).canonicalFilePath() ) { - // Check the layer name too if it's a GPKG/SpatiaLite/SQLite OGR driver + // Check the layer name too if it's a GPKG/SpatiaLite/SQLite OGR driver (pay attention: camel case in layerName) QgsDataSourceUri uri( details.dataSourceUri ); if ( !( ( options.driverName == QLatin1String( "GPKG" ) || options.driverName == QLatin1String( "SpatiaLite" ) || options.driverName == QLatin1String( "SQLite" ) ) && - options.layerName != srcUriParams["layername"] ) ) + options.layerName != details.providerUriParams.value( QLatin1String( "layerName" ) ) ) ) { if ( errorMessage ) *errorMessage = QObject::tr( "Cannot overwrite a OGR layer in place" ); @@ -2596,7 +2593,7 @@ QgsVectorFileWriter::WriterError QgsVectorFileWriter::writeAsVectorFormat( Prepa if ( options.feedback ) { //dedicate first 5% of progress bar to this scan - int newProgress = ( 5.0 * scanned ) / total; + int newProgress = static_cast( ( 5.0 * scanned ) / total ); if ( newProgress != lastProgressReport ) { lastProgressReport = newProgress; @@ -2694,7 +2691,7 @@ QgsVectorFileWriter::WriterError QgsVectorFileWriter::writeAsVectorFormat( Prepa if ( options.feedback ) { //avoid spamming progress reports - int newProgress = initialProgress + ( ( 100.0 - initialProgress ) * saved ) / total; + int newProgress = static_cast( initialProgress + ( ( 100.0 - initialProgress ) * saved ) / total ); if ( newProgress < 100 && newProgress != lastProgressReport ) { lastProgressReport = newProgress; diff --git a/src/core/qgsvectorfilewriter.h b/src/core/qgsvectorfilewriter.h index 4b2559de0bc..d9c3e682b8a 100644 --- a/src/core/qgsvectorfilewriter.h +++ b/src/core/qgsvectorfilewriter.h @@ -760,6 +760,7 @@ class CORE_EXPORT QgsVectorFileWriter : public QgsFeatureSink QgsFeatureIterator sourceFeatureIterator; QgsGeometry filterRectGeometry; std::unique_ptr< QgsGeometryEngine > filterRectEngine; + QVariantMap providerUriParams; }; /** @@ -814,6 +815,7 @@ class CORE_EXPORT QgsVectorFileWriter : public QgsFeatureSink static QStringList concatenateOptions( const QMap &options ); friend class QgsVectorFileWriterTask; + friend class TestQgsVectorFileWriter; }; Q_DECLARE_OPERATORS_FOR_FLAGS( QgsVectorFileWriter::EditionCapabilities ) diff --git a/tests/src/core/testqgsvectorfilewriter.cpp b/tests/src/core/testqgsvectorfilewriter.cpp index 88bae111448..0a22737a9f8 100644 --- a/tests/src/core/testqgsvectorfilewriter.cpp +++ b/tests/src/core/testqgsvectorfilewriter.cpp @@ -80,6 +80,8 @@ class TestQgsVectorFileWriter: public QObject void projectedPlygonGridTest(); //! This is a regression test ticket 1141 (broken Polish characters support since r8592) https://issues.qgis.org/issues/1141 void regression1141(); + //! Test prepareWriteAsVectorFormat + void prepareWriteAsVectorFormat(); private: // a little util fn used by all tests @@ -106,6 +108,7 @@ void TestQgsVectorFileWriter::initTestCase() // init QGIS's paths - true means that all path will be inited from prefix QgsApplication::init(); QgsApplication::showSettings(); + QgsApplication::initQgis(); //create some objects that will be used in all tests... mEncoding = QStringLiteral( "UTF-8" ); @@ -457,5 +460,36 @@ void TestQgsVectorFileWriter::regression1141() QVERIFY( QgsVectorFileWriter::deleteShapeFile( fileName ) ); } +void TestQgsVectorFileWriter::prepareWriteAsVectorFormat() +{ + QgsVectorFileWriter::PreparedWriterDetails details; + QgsVectorFileWriter::SaveVectorOptions options; + QgsVectorLayer ml( "Point?field=firstfield:int&field=secondfield:int", "test", "memory" ); + QgsFeature ft( ml.fields( ) ); + ft.setAttribute( 0, 4 ); + ft.setAttribute( 1, -10 ); + ml.dataProvider()->addFeature( ft ); + QVERIFY( ml.isValid() ); + QTemporaryFile tmpFile( QDir::tempPath() + "/test_qgsvectorfilewriter_XXXXXX.gpkg" ); + tmpFile.open(); + QString fileName( tmpFile.fileName( ) ); + options.driverName = "GPKG"; + options.layerName = "test"; + QString errorMessage; + QgsVectorFileWriter::WriterError error( QgsVectorFileWriter::writeAsVectorFormat( + &ml, + fileName, + options, + &errorMessage ) ); + + QCOMPARE( error, QgsVectorFileWriter::WriterError::NoError ); + QCOMPARE( errorMessage, fileName ); + QgsVectorLayer vl( QStringLiteral( "%1|layername=test" ).arg( fileName ), "src_test", "ogr" ); + QVERIFY( vl.isValid() ); + QgsVectorFileWriter::prepareWriteAsVectorFormat( &vl, options, details ); + QCOMPARE( details.providerUriParams.value( "layerName" ), QStringLiteral( "test" ) ); + QCOMPARE( details.providerUriParams.value( "path" ), fileName ); +} + QGSTEST_MAIN( TestQgsVectorFileWriter ) #include "testqgsvectorfilewriter.moc" From 2da1d6f6ba6b2b6d9011007c5641a1386c2407d4 Mon Sep 17 00:00:00 2001 From: Alessandro Pasotti Date: Wed, 22 Aug 2018 09:24:42 +0200 Subject: [PATCH 3/4] Also get src path from details.providerUriParams --- src/core/qgsvectorfilewriter.cpp | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/core/qgsvectorfilewriter.cpp b/src/core/qgsvectorfilewriter.cpp index e6aa07d805e..4c788820573 100644 --- a/src/core/qgsvectorfilewriter.cpp +++ b/src/core/qgsvectorfilewriter.cpp @@ -2555,14 +2555,7 @@ QgsVectorFileWriter::WriterError QgsVectorFileWriter::writeAsVectorFormat( Prepa // Special rules for OGR layers if ( details.providerType == QLatin1String( "ogr" ) && !details.dataSourceUri.isEmpty() ) { - QStringList theURIParts = details.dataSourceUri.split( '|' ); - QgsStringMap srcUriParams; - QString srcFileName; - if ( theURIParts.length() > 0 ) - { - srcFileName = theURIParts[0]; - } - + QString srcFileName( details.providerUriParams.value( QLatin1String( "path" ) ).toString() ); if ( QFile::exists( srcFileName ) && QFileInfo( fileName ).canonicalFilePath() == QFileInfo( srcFileName ).canonicalFilePath() ) { // Check the layer name too if it's a GPKG/SpatiaLite/SQLite OGR driver (pay attention: camel case in layerName) From fbfb0bc8a5067fef3ee28c8af604bb019e98edd1 Mon Sep 17 00:00:00 2001 From: Alessandro Pasotti Date: Wed, 22 Aug 2018 09:40:46 +0200 Subject: [PATCH 4/4] Fix test --- tests/src/core/testqgsvectorfilewriter.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/src/core/testqgsvectorfilewriter.cpp b/tests/src/core/testqgsvectorfilewriter.cpp index 0a22737a9f8..0c1f4216a43 100644 --- a/tests/src/core/testqgsvectorfilewriter.cpp +++ b/tests/src/core/testqgsvectorfilewriter.cpp @@ -487,8 +487,8 @@ void TestQgsVectorFileWriter::prepareWriteAsVectorFormat() QgsVectorLayer vl( QStringLiteral( "%1|layername=test" ).arg( fileName ), "src_test", "ogr" ); QVERIFY( vl.isValid() ); QgsVectorFileWriter::prepareWriteAsVectorFormat( &vl, options, details ); - QCOMPARE( details.providerUriParams.value( "layerName" ), QStringLiteral( "test" ) ); - QCOMPARE( details.providerUriParams.value( "path" ), fileName ); + QCOMPARE( details.providerUriParams.value( "layerName" ).toString(), QStringLiteral( "test" ) ); + QCOMPARE( details.providerUriParams.value( "path" ).toString(), fileName ); } QGSTEST_MAIN( TestQgsVectorFileWriter )