Merge pull request #8186 from rouault/fix_20104

[GDAL provider] Make sure that setEditable(true) invalides cached GDAL handles to get proper refresh (fixes #20104)

Author: rouault

src/providers/gdal/qgsgdalprovider.cpp

@@ -392,7 +392,7 @@ bool QgsGdalProvider::cacheGdalHandlesForLaterReuse( QgsGdalProvider *provider,
       {
         mgDatasetCacheSize --;
         DatasetPair pair = mgDatasetCache[ candidateProvider ].takeLast();
-        if ( pair.mGdalBaseDataset )
+        if ( pair.mGdalBaseDataset != pair.mGdalDataset )
         {
           GDALDereferenceDataset( pair.mGdalBaseDataset );
         }
@@ -435,7 +435,7 @@ void QgsGdalProvider::closeCachedGdalHandlesFor( QgsGdalProvider *provider )
     {
       mgDatasetCacheSize --;
       DatasetPair pair = iter.value().takeLast();
-      if ( pair.mGdalBaseDataset )
+      if ( pair.mGdalBaseDataset != pair.mGdalDataset )
       {
         GDALDereferenceDataset( pair.mGdalBaseDataset );
       }
@@ -464,7 +464,7 @@ QgsGdalProvider::~QgsGdalProvider()
     }
     else
     {
-      if ( mGdalBaseDataset )
+      if ( mGdalBaseDataset != mGdalDataset )
       {
         GDALDereferenceDataset( mGdalBaseDataset );
       }
@@ -490,7 +490,6 @@ QgsGdalProvider::~QgsGdalProvider()
 }
 
 
-// This was used by raster layer to reload data
 void QgsGdalProvider::closeDataset()
 {
   if ( !mValid )
@@ -499,11 +498,16 @@ void QgsGdalProvider::closeDataset()
   }
   mValid = false;
 
-  GDALDereferenceDataset( mGdalBaseDataset );
+  if ( mGdalBaseDataset != mGdalDataset )
+  {
+    GDALDereferenceDataset( mGdalBaseDataset );
+  }
   mGdalBaseDataset = nullptr;
 
   GDALClose( mGdalDataset );
   mGdalDataset = nullptr;
+
+  closeCachedGdalHandlesFor( this );
 }
 
 QString QgsGdalProvider::htmlMetadata()
@@ -2605,7 +2609,6 @@ void QgsGdalProvider::initBaseDataset()
     {
       QgsLogger::warning( QStringLiteral( "Warped VRT Creation failed." ) );
       mGdalDataset = mGdalBaseDataset;
-      GDALReferenceDataset( mGdalDataset );
     }
     else
     {
@@ -2615,7 +2618,6 @@ void QgsGdalProvider::initBaseDataset()
   else
   {
     mGdalDataset = mGdalBaseDataset;
-    GDALReferenceDataset( mGdalDataset );
   }
 
   if ( !hasGeoTransform )
@@ -2644,12 +2646,7 @@ void QgsGdalProvider::initBaseDataset()
     {
       appendError( ERRMSG( tr( "Cannot get GDAL raster band: %1" ).arg( msg ) ) );
 
-      GDALDereferenceDataset( mGdalBaseDataset );
-      mGdalBaseDataset = nullptr;
-
-      GDALClose( mGdalDataset );
-      mGdalDataset = nullptr;
-      mValid = false;
+      closeDataset();
       return;
     }
     // if there are subdatasets, leave the dataset open for subsequent queries
@@ -2956,8 +2953,7 @@ bool QgsGdalProvider::remove()
   if ( mGdalDataset )
   {
     GDALDriverH driver = GDALGetDatasetDriver( mGdalDataset );
-    GDALClose( mGdalDataset );
-    mGdalDataset = nullptr;
+    closeDataset();
 
     CPLErrorReset();
     CPLErr err = GDALDeleteDataset( driver, dataSourceUri( true ).toUtf8().constData() );

tests/src/providers/testqgsgdalprovider.cpp

@@ -54,6 +54,7 @@ class TestQgsGdalProvider : public QObject
     void bandName(); // test band name based on `gtiff` tags (#7317)
     void bandNameNoDescription(); // test band name for when no description or tags available (#16047)
     void bandNameWithDescription(); // test band name for when description available (#16047)
+    void interactionBetweenRasterChangeAndCache(); // test that updading a raster invalidates the GDAL dataset cache (#20104)
 
   private:
     QString mTestDataDir;
@@ -275,5 +276,50 @@ void TestQgsGdalProvider::bandNameWithDescription()
   delete provider;
 }
 
+void TestQgsGdalProvider::interactionBetweenRasterChangeAndCache()
+{
+  double geoTransform[6] = { 0, 2, 0, 0, 0, -2};
+  QgsCoordinateReferenceSystem crs;
+  QString filename = QStringLiteral( "/vsimem/temp.tif" );
+
+  // Create a all-0 dataset
+  auto provider = QgsRasterDataProvider::create(
+                    QStringLiteral( "gdal" ), filename, "GTiff", 1, Qgis::Byte, 1, 1, geoTransform, crs );
+  delete provider;
+
+  // Open it
+  provider = dynamic_cast< QgsRasterDataProvider * >(
+               QgsProviderRegistry::instance()->createProvider(
+                 QStringLiteral( "gdal" ), filename, QgsDataProvider::ProviderOptions() ) );
+  QVERIFY( provider );
+  auto rp = dynamic_cast< QgsRasterDataProvider * >( provider );
+  QVERIFY( rp );
+
+  // Create a first clone, and destroys it
+  auto rpClone = dynamic_cast< QgsRasterDataProvider *>( rp->clone() );
+  QVERIFY( rpClone );
+  QCOMPARE( rpClone->sample( QgsPointXY( 0.5, -0.5 ), 1 ), 0.0 );
+  delete rpClone;
+  // Now the main provider should have a cached GDAL dataset corresponding
+  // to the one that was used by rpClone
+
+  // Modify the raster
+  rp->setEditable( true );
+  auto rblock = new QgsRasterBlock( Qgis::Byte, 1, 1 );
+  rblock->setValue( 0, 0, 255 );
+  rp->writeBlock( rblock, 1, 0, 0 );
+  delete rblock;
+  rp->setEditable( false );
+
+  // Creates a new clone, and check that we get an updated sample value
+  rpClone = dynamic_cast< QgsRasterDataProvider *>( rp->clone() );
+  QVERIFY( rpClone );
+  QCOMPARE( rpClone->sample( QgsPointXY( 0.5, -0.5 ), 1 ), 255.0 );
+  delete rpClone;
+
+  provider->remove();
+  delete provider;
+}
+
 QGSTEST_MAIN( TestQgsGdalProvider )
 #include "testqgsgdalprovider.moc"