[OGR provider] Update layer extent for GPKG layers

When moving or deleting a geometry that previously touched the layer extent, the layer extent was never shrinked. This fix requires GDAL 2.1.2 or above as well. Fixes #15273
qgis · Oct 18, 2016 · 9d316b0 · 9d316b0
1 parent e22b195
commit 9d316b0
Show file tree

Hide file tree

Showing 3 changed files with 67 additions and 4 deletions.
diff --git a/src/providers/ogr/qgsogrprovider.cpp b/src/providers/ogr/qgsogrprovider.cpp
@@ -287,6 +287,7 @@ QgsOgrProvider::QgsOgrProvider( QString const & uri )
     , mFirstFieldIsFid( false )
     , ogrDataSource( nullptr )
     , mExtent( nullptr )
+    , mForceRecomputeExtent( false )
     , ogrLayer( nullptr )
     , ogrOrigLayer( nullptr )
     , mLayerIndex( 0 )
@@ -483,7 +484,7 @@ bool QgsOgrProvider::setSubsetString( const QString& theSQL, bool updateFeatureC
   loadFields();
   QgsDebugMsg( "Done checking validity" );
 
-  updateExtents();
+  invalidateCachedExtent( false );
 
   emit dataChanged();
 
@@ -978,6 +979,17 @@ QgsRectangle QgsOgrProvider::extent()
     // get the extent_ (envelope) of the layer
     QgsDebugMsg( "Starting get extent" );
 
+#if defined(GDAL_COMPUTE_VERSION) && GDAL_VERSION_NUM >= GDAL_COMPUTE_VERSION(2,1,2)
+    if ( mForceRecomputeExtent && mValid && ogrDriverName == "GPKG" && ogrDataSource && ogrOrigLayer )
+    {
+      QByteArray layerName = OGR_FD_GetName( OGR_L_GetLayerDefn( ogrOrigLayer ) );
+      // works with unquoted layerName
+      QByteArray sql = QByteArray( "RECOMPUTE EXTENT ON " ) + layerName;
+      QgsDebugMsg( QString( "SQL: %1" ).arg( FROM8( sql ) ) );
+      OGR_DS_ExecuteSQL( ogrDataSource, sql.constData(), nullptr, nullptr );
+    }
+#endif
+
     // TODO: This can be expensive, do we really need it!
     if ( ogrLayer == ogrOrigLayer )
     {
@@ -1021,6 +1033,12 @@ QgsRectangle QgsOgrProvider::extent()
 
 void QgsOgrProvider::updateExtents()
 {
+  invalidateCachedExtent( true );
+}
+
+void QgsOgrProvider::invalidateCachedExtent( bool bForceRecomputeExtent )
+{
+  mForceRecomputeExtent = bForceRecomputeExtent;
   delete mExtent;
   mExtent = nullptr;
 }
@@ -1665,6 +1683,8 @@ bool QgsOgrProvider::changeGeometryValues( const QgsGeometryMap &geometry_map )
     }
     mShapefileMayBeCorrupted = true;
 
+    invalidateCachedExtent( true );
+
     OGR_F_Destroy( theOGRFeature );
   }
   QgsOgrConnPool::instance()->invalidateConnections( dataSourceUri() );
@@ -1734,7 +1754,7 @@ bool QgsOgrProvider::deleteFeatures( const QgsFeatureIds & id )
 
   clearMinMaxCache();
 
-  updateExtents();
+  invalidateCachedExtent( true );
 
   return returnvalue;
 }
@@ -3442,7 +3462,7 @@ void QgsOgrProvider::close()
   mValid = false;
   setProperty( "_debug_open_mode", "invalid" );
 
-  updateExtents();
+  invalidateCachedExtent( false );
 }
 
 void QgsOgrProvider::reloadData()

diff --git a/src/providers/ogr/qgsogrprovider.h b/src/providers/ogr/qgsogrprovider.h
@@ -286,6 +286,9 @@ class QgsOgrProvider : public QgsVectorDataProvider
     /** Clean shapefile from features which are marked as deleted */
     void repack();
 
+    /** Invalidate extent and optionnaly force its low level recomputation */
+    void invalidateCachedExtent( bool bForceRecomputeExtent );
+
     enum OpenMode
     {
       OpenModeInitial,
@@ -305,6 +308,7 @@ class QgsOgrProvider : public QgsVectorDataProvider
     bool mFirstFieldIsFid;
     OGRDataSourceH ogrDataSource;
     OGREnvelope* mExtent;
+    bool mForceRecomputeExtent;
 
     /** This member variable receives the same value as extent_
      in the method QgsOgrProvider::extent(). The purpose is to prevent a memory leak*/

diff --git a/tests/src/python/test_provider_ogr_gpkg.py b/tests/src/python/test_provider_ogr_gpkg.py
@@ -20,7 +20,7 @@
 import glob
 from osgeo import gdal, ogr
 
-from qgis.core import QgsVectorLayer, QgsFeature, QgsGeometry, QgsFeatureRequest
+from qgis.core import QgsVectorLayer, QgsFeature, QgsGeometry, QgsFeatureRequest, QgsRectangle
 from qgis.testing import start_app, unittest
 from utilities import unitTestDataPath
 
@@ -156,5 +156,44 @@ def testBug15351_closeIter_commit_closeProvider(self):
     def testBug15351_commit_closeIter_closeProvider(self):
         self.internalTestBug15351('commit_closeIter_closeProvider')
 
+    @unittest.expectedFailure(int(gdal.VersionInfo('VERSION_NUM')) < GDAL_COMPUTE_VERSION(2, 1, 2))
+    def testGeopackageExtentUpdate(self):
+        ''' test http://hub.qgis.org/issues/15273 '''
+        tmpfile = os.path.join(self.basetestpath, 'testGeopackageExtentUpdate.gpkg')
+        ds = ogr.GetDriverByName('GPKG').CreateDataSource(tmpfile)
+        lyr = ds.CreateLayer('test', geom_type=ogr.wkbPoint)
+        f = ogr.Feature(lyr.GetLayerDefn())
+        f.SetGeometry(ogr.CreateGeometryFromWkt('POINT(0 0)'))
+        lyr.CreateFeature(f)
+        f = ogr.Feature(lyr.GetLayerDefn())
+        f.SetGeometry(ogr.CreateGeometryFromWkt('POINT(1 1)'))
+        lyr.CreateFeature(f)
+        f = None
+        f = ogr.Feature(lyr.GetLayerDefn())
+        f.SetGeometry(ogr.CreateGeometryFromWkt('POINT(1 0.5)'))
+        lyr.CreateFeature(f)
+        f = None
+        ds = None
+
+        vl = QgsVectorLayer(u'{}'.format(tmpfile), u'test', u'ogr')
+
+        # Test moving a geometry that touches the bbox
+        self.assertTrue(vl.startEditing())
+        self.assertTrue(vl.changeGeometry(1, QgsGeometry.fromWkt('Point (0.5 0)')))
+        self.assertTrue(vl.commitChanges())
+        reference = QgsGeometry.fromRect(QgsRectangle(0.5, 0.0, 1.0, 1.0))
+        provider_extent = QgsGeometry.fromRect(vl.extent())
+        self.assertTrue(QgsGeometry.compare(provider_extent.asPolygon()[0], reference.asPolygon()[0], 0.00001),
+                        provider_extent.asPolygon()[0])
+
+        # Test deleting a geometry that touches the bbox
+        self.assertTrue(vl.startEditing())
+        self.assertTrue(vl.deleteFeature(2))
+        self.assertTrue(vl.commitChanges())
+        reference = QgsGeometry.fromRect(QgsRectangle(0.5, 0.0, 1.0, 0.5))
+        provider_extent = QgsGeometry.fromRect(vl.extent())
+        self.assertTrue(QgsGeometry.compare(provider_extent.asPolygon()[0], reference.asPolygon()[0], 0.00001),
+                        provider_extent.asPolygon()[0])
+
 if __name__ == '__main__':
     unittest.main()