From baceab0a73707f753cc05848f3124571d92d3f5b Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Tue, 9 Jul 2024 10:12:03 +1000 Subject: [PATCH] [processing] When writing a layer to a temporary GPKG file for compatibilty with provider's supported formats, rename the existing FID column if we can't write the FID values into the GPKG Eg when the source layer has string/duplicate fids, we warn the user and then automatically move the existing fids to a "OLD_FID" field --- src/core/processing/qgsprocessingutils.cpp | 26 +++++++++++++++++----- tests/src/analysis/testqgsprocessing.cpp | 15 +++++++++---- 2 files changed, 32 insertions(+), 9 deletions(-) diff --git a/src/core/processing/qgsprocessingutils.cpp b/src/core/processing/qgsprocessingutils.cpp index 646c6ebf60de..54069c50d238 100644 --- a/src/core/processing/qgsprocessingutils.cpp +++ b/src/core/processing/qgsprocessingutils.cpp @@ -1350,7 +1350,7 @@ QString QgsProcessingUtils::formatHelpMapAsHtml( const QVariantMap &map, const Q } QString convertToCompatibleFormatInternal( const QgsVectorLayer *vl, bool selectedFeaturesOnly, const QString &baseName, const QStringList &compatibleFormats, const QString &preferredFormat, QgsProcessingContext &context, QgsProcessingFeedback *feedback, QString *layerName, - long long featureLimit, const QString &filterExpression ) + long long featureLimit, const QString &filterExpression, bool renameFid ) { bool requiresTranslation = false; @@ -1412,7 +1412,14 @@ QString convertToCompatibleFormatInternal( const QgsVectorLayer *vl, bool select QgsVectorFileWriter::SaveVectorOptions saveOptions; saveOptions.fileEncoding = context.defaultEncoding(); saveOptions.driverName = QgsVectorFileWriter::driverForExtension( preferredFormat ); - std::unique_ptr< QgsVectorFileWriter > writer( QgsVectorFileWriter::create( temp, vl->fields(), vl->wkbType(), vl->crs(), context.transformContext(), saveOptions ) ); + QgsFields fields = vl->fields(); + if ( renameFid ) + { + const int fidIndex = fields.lookupField( QStringLiteral( "fid" ) ); + if ( fidIndex >= 0 ) + fields.rename( fidIndex, QStringLiteral( "OLD_FID" ) ); + } + std::unique_ptr< QgsVectorFileWriter > writer( QgsVectorFileWriter::create( temp, fields, vl->wkbType(), vl->crs(), context.transformContext(), saveOptions ) ); QgsFeature f; QgsFeatureIterator it; QgsFeatureRequest request; @@ -1439,10 +1446,19 @@ QString convertToCompatibleFormatInternal( const QgsVectorLayer *vl, bool select if ( !writer->addFeature( f, QgsFeatureSink::FastInsert ) && feedback ) { + const QString errorMessage = writer->errorMessage(); + if ( !renameFid && saveOptions.driverName == QLatin1String( "GPKG" ) && errorMessage.contains( "fid", Qt::CaseInsensitive ) ) + { + // try again, dropping the FID field + feedback->reportError( QObject::tr( "Cannot store existing FID values in temporary GeoPackage layer, these will be moved to \"OLD_FID\" instead." ), false ); + return convertToCompatibleFormatInternal( vl, selectedFeaturesOnly, baseName, compatibleFormats, preferredFormat, context, feedback, layerName, + featureLimit, filterExpression, true ); + } + QString errorText; if ( errorCounter++ < maxErrors ) { - errorText = QObject::tr( "Error writing feature # %1 to output layer: %2" ).arg( QString::number( f.id() ), writer->errorMessage() ); + errorText = QObject::tr( "Error writing feature # %1 to output layer: %2" ).arg( QString::number( f.id() ), errorMessage ); feedback->reportError( errorText ); } @@ -1462,13 +1478,13 @@ QString convertToCompatibleFormatInternal( const QgsVectorLayer *vl, bool select QString QgsProcessingUtils::convertToCompatibleFormat( const QgsVectorLayer *vl, bool selectedFeaturesOnly, const QString &baseName, const QStringList &compatibleFormats, const QString &preferredFormat, QgsProcessingContext &context, QgsProcessingFeedback *feedback, long long featureLimit, const QString &filterExpression ) { - return convertToCompatibleFormatInternal( vl, selectedFeaturesOnly, baseName, compatibleFormats, preferredFormat, context, feedback, nullptr, featureLimit, filterExpression ); + return convertToCompatibleFormatInternal( vl, selectedFeaturesOnly, baseName, compatibleFormats, preferredFormat, context, feedback, nullptr, featureLimit, filterExpression, false ); } QString QgsProcessingUtils::convertToCompatibleFormatAndLayerName( const QgsVectorLayer *layer, bool selectedFeaturesOnly, const QString &baseName, const QStringList &compatibleFormats, const QString &preferredFormat, QgsProcessingContext &context, QgsProcessingFeedback *feedback, QString &layerName, long long featureLimit, const QString &filterExpression ) { layerName.clear(); - return convertToCompatibleFormatInternal( layer, selectedFeaturesOnly, baseName, compatibleFormats, preferredFormat, context, feedback, &layerName, featureLimit, filterExpression ); + return convertToCompatibleFormatInternal( layer, selectedFeaturesOnly, baseName, compatibleFormats, preferredFormat, context, feedback, &layerName, featureLimit, filterExpression, false ); } QgsFields QgsProcessingUtils::combineFields( const QgsFields &fieldsA, const QgsFields &fieldsB, const QString &fieldsBPrefix ) diff --git a/tests/src/analysis/testqgsprocessing.cpp b/tests/src/analysis/testqgsprocessing.cpp index f4ae5a24a023..698543904bbd 100644 --- a/tests/src/analysis/testqgsprocessing.cpp +++ b/tests/src/analysis/testqgsprocessing.cpp @@ -12672,11 +12672,18 @@ void TestQgsProcessing::convertCompatibleDuplicateFids() QVariantMap params; params.insert( QStringLiteral( "source" ), QgsProcessingFeatureSourceDefinition( layer->id(), false ) ); - QgsProcessingParameters::parameterAsCompatibleSourceLayerPath( def.get(), params, context, QStringList() << "gpkg", QString( "gpkg" ), &feedback ); + const QString temporaryFile = QgsProcessingParameters::parameterAsCompatibleSourceLayerPath( def.get(), params, context, QStringList() << "gpkg", QString( "gpkg" ), &feedback ); const QStringList logs( feedback.textLog().split( '\n', Qt::SplitBehaviorFlags::SkipEmptyParts ) ); - QCOMPARE( logs.size(), 11 ); - QVERIFY( logs.first().startsWith( QLatin1String( "Error writing feature # 2" ) ) ); - QVERIFY( logs.last().startsWith( QLatin1String( "There were 11 errors writing features" ) ) ); + QCOMPARE( logs.size(), 1 ); + QCOMPARE( logs.at( 0 ), QStringLiteral( "Cannot store existing FID values in temporary GeoPackage layer, these will be moved to \"OLD_FID\" instead." ) ); + + QgsVectorLayer vl( temporaryFile ); + QVERIFY( vl.isValid() ); + + QCOMPARE( vl.featureCount(), 12 ); + QCOMPARE( vl.fields().at( 0 ).name(), QStringLiteral( "fid" ) ); + QCOMPARE( vl.fields().at( 1 ).name(), QStringLiteral( "OLD_FID" ) ); + QCOMPARE( vl.fields().at( 2 ).name(), QStringLiteral( "name" ) ); } void TestQgsProcessing::create()