Fix QgsFeature default ctor set fid to 0

The QgsFeature default constructors initialized feature id to 0, which is a valid feature id instead of initializing it fo FID_NULL. This was just wrong and broke the validator for UNIQUE constraints in case a feature with fid 0 existed in the data provider. Fixes #36962
qgis · Jun 22, 2020 · 0a42144 · 0a42144
1 parent e4718d7
commit 0a42144
Show file tree

Hide file tree

Showing 5 changed files with 39 additions and 8 deletions.
diff --git a/python/core/auto_generated/qgsfeature.sip.in b/python/core/auto_generated/qgsfeature.sip.in
@@ -136,14 +136,14 @@ geometry and a list of field/values attributes.
       sipCpp->deleteAttribute( fieldIdx );
 %End
 
-    QgsFeature( qint64 id = 0 );
+    QgsFeature( qint64 id = FID_NULL );
 %Docstring
 Constructor for QgsFeature
 
 :param id: feature id
 %End
 
-    QgsFeature( const QgsFields &fields, qint64 id = 0 );
+    QgsFeature( const QgsFields &fields, qint64 id = FID_NULL );
 %Docstring
 Constructor for QgsFeature
 

diff --git a/src/core/qgsfeature.h b/src/core/qgsfeature.h
@@ -176,9 +176,9 @@ class CORE_EXPORT QgsFeature
      * \param id feature id
      */
 #ifndef SIP_RUN
-    QgsFeature( QgsFeatureId id = QgsFeatureId() );
+    QgsFeature( QgsFeatureId id = FID_NULL );
 #else
-    QgsFeature( qint64 id = 0 );
+    QgsFeature( qint64 id = FID_NULL );
 #endif
 
     /**
@@ -187,9 +187,9 @@ class CORE_EXPORT QgsFeature
      * \param id feature id
      */
 #ifndef SIP_RUN
-    QgsFeature( const QgsFields &fields, QgsFeatureId id = QgsFeatureId() );
+    QgsFeature( const QgsFields &fields, QgsFeatureId id = FID_NULL );
 #else
-    QgsFeature( const QgsFields &fields, qint64 id = 0 );
+    QgsFeature( const QgsFields &fields, qint64 id = FID_NULL );
 #endif
 
     /**

diff --git a/src/gui/qgsmaptooldigitizefeature.cpp b/src/gui/qgsmaptooldigitizefeature.cpp
@@ -178,7 +178,7 @@ void QgsMapToolDigitizeFeature::cadCanvasReleaseEvent( QgsMapMouseEvent *e )
     //grass provider has its own mechanism of feature addition
     if ( provider->capabilities() & QgsVectorDataProvider::AddFeatures )
     {
-      QgsFeature f( vlayer->fields(), 0 );
+      QgsFeature f( vlayer->fields() );
 
       QgsGeometry g;
       if ( layerWKBType == QgsWkbTypes::Point )

diff --git a/tests/src/core/testqgsfeature.cpp b/tests/src/core/testqgsfeature.cpp
@@ -32,6 +32,7 @@ class TestQgsFeature: public QObject
     void init();// will be called before each testfunction is executed.
     void cleanup();// will be called after every testfunction.
     void attributesTest(); //test QgsAttributes
+    void constructorTest(); //test default constructors
     void attributesToMap();
     void create();//test creating a feature
     void copy();// test cpy destruction (double delete)
@@ -120,6 +121,18 @@ void TestQgsFeature::attributesTest()
   QCOMPARE( attr7.size(), 5 );
 }
 
+void TestQgsFeature::constructorTest()
+{
+  QgsFeature f;
+  QVERIFY( FID_IS_NULL( f.id() ) );
+  QgsFeature f2 { QgsFields() };
+  QVERIFY( FID_IS_NULL( f2.id() ) );
+  QgsFeature f3 { 1234 };
+  QVERIFY( ! FID_IS_NULL( f3.id() ) );
+  QgsFeature f4 { QgsFields(), 1234 };
+  QVERIFY( ! FID_IS_NULL( f4.id() ) );
+}
+
 void TestQgsFeature::attributesToMap()
 {
   QgsAttributes attr1;

diff --git a/tests/src/python/test_qgsfeature.py b/tests/src/python/test_qgsfeature.py
@@ -30,7 +30,7 @@
 class TestQgsFeature(unittest.TestCase):
 
     def test_CreateFeature(self):
-        feat = QgsFeature()
+        feat = QgsFeature(0)
         feat.initAttributes(1)
         feat.setAttribute(0, "text")
         feat.setGeometry(QgsGeometry.fromPointXY(QgsPointXY(123, 456)))
@@ -39,6 +39,24 @@ def test_CreateFeature(self):
         myMessage = '\nExpected: %s\nGot: %s' % (myExpectedId, myId)
         assert myId == myExpectedId, myMessage
 
+    def test_FeatureDefaultConstructor(self):
+        """Test for FID_IS_NULL default constructors See: https://github.com/qgis/QGIS/issues/36962"""
+        feat = QgsFeature()
+        # it should be FID_NULL std::numeric_limits<QgsFeatureId>::min(),
+        # not sure if I can test the exact value in python
+        self.assertNotEqual(feat.id(), 0)
+        self.assertTrue(feat.id() < 0)
+
+        feat = QgsFeature(QgsFields())
+        self.assertNotEqual(feat.id(), 0)
+        self.assertTrue(feat.id() < 0)
+
+        feat = QgsFeature(1234)
+        self.assertEqual(feat.id(), 1234)
+
+        feat = QgsFeature(QgsFields(), 1234)
+        self.assertEqual(feat.id(), 1234)
+
     def test_ValidFeature(self):
         myPath = os.path.join(unitTestDataPath(), 'points.shp')
         myLayer = QgsVectorLayer(myPath, 'Points', 'ogr')