Merge pull request #36967 from elpaso/bugfix-gh36962-fid-is-null

QgsFeature default constructor set fid to invalid
qgis · Jul 6, 2020 · 992b1f2 · 992b1f2
2 parents 01b345c + a8d78b7
commit 992b1f2
Show file tree

Hide file tree

Showing 12 changed files with 66 additions and 25 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/expression/qgsexpressioncontextutils.cpp b/src/core/expression/qgsexpressioncontextutils.cpp
@@ -29,6 +29,7 @@
 #include "qgslayoutpagecollection.h"
 #include "qgslayoutatlas.h"
 #include "qgslayoutmultiframe.h"
+#include "qgsfeatureid.h"
 
 QgsExpressionContextScope *QgsExpressionContextUtils::globalScope()
 {
@@ -525,7 +526,7 @@ QgsExpressionContextScope *QgsExpressionContextUtils::layoutScope( const QgsLayo
     QgsFeature atlasFeature = layout->reportContext().feature();
     scope->setFeature( atlasFeature );
     scope->addVariable( QgsExpressionContextScope::StaticVariable( QStringLiteral( "atlas_feature" ), QVariant::fromValue( atlasFeature ), true ) );
-    scope->addVariable( QgsExpressionContextScope::StaticVariable( QStringLiteral( "atlas_featureid" ), atlasFeature.id(), true ) );
+    scope->addVariable( QgsExpressionContextScope::StaticVariable( QStringLiteral( "atlas_featureid" ), FID_IS_NULL( atlasFeature.id() ) ? QVariant() : atlasFeature.id(), true ) );
     scope->addVariable( QgsExpressionContextScope::StaticVariable( QStringLiteral( "atlas_geometry" ), QVariant::fromValue( atlasFeature.geometry() ), true ) );
   }
 
@@ -576,7 +577,7 @@ QgsExpressionContextScope *QgsExpressionContextUtils::atlasScope( const QgsLayou
     //users that these variables are available even if they have no current value
     scope->addVariable( QgsExpressionContextScope::StaticVariable( QStringLiteral( "atlas_pagename" ), QString(), true ) );
     scope->addVariable( QgsExpressionContextScope::StaticVariable( QStringLiteral( "atlas_feature" ), QVariant::fromValue( QgsFeature() ), true ) );
-    scope->addVariable( QgsExpressionContextScope::StaticVariable( QStringLiteral( "atlas_featureid" ), 0, true ) );
+    scope->addVariable( QgsExpressionContextScope::StaticVariable( QStringLiteral( "atlas_featureid" ), QVariant(), true ) );
     scope->addVariable( QgsExpressionContextScope::StaticVariable( QStringLiteral( "atlas_geometry" ), QVariant::fromValue( QgsGeometry() ), true ) );
     return scope;
   }
@@ -599,7 +600,7 @@ QgsExpressionContextScope *QgsExpressionContextUtils::atlasScope( const QgsLayou
     QgsFeature atlasFeature = atlas->layout()->reportContext().feature();
     scope->setFeature( atlasFeature );
     scope->addVariable( QgsExpressionContextScope::StaticVariable( QStringLiteral( "atlas_feature" ), QVariant::fromValue( atlasFeature ), true ) );
-    scope->addVariable( QgsExpressionContextScope::StaticVariable( QStringLiteral( "atlas_featureid" ), atlasFeature.id(), true ) );
+    scope->addVariable( QgsExpressionContextScope::StaticVariable( QStringLiteral( "atlas_featureid" ), FID_IS_NULL( atlasFeature.id() ) ? QVariant() : atlasFeature.id(), true ) );
     scope->addVariable( QgsExpressionContextScope::StaticVariable( QStringLiteral( "atlas_geometry" ), QVariant::fromValue( atlasFeature.geometry() ), true ) );
   }
 

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/core/qgsjsonutils.cpp b/src/core/qgsjsonutils.cpp
@@ -26,6 +26,7 @@
 #include "qgsfieldformatterregistry.h"
 #include "qgsfieldformatter.h"
 #include "qgsapplication.h"
+#include "qgsfeatureid.h"
 
 #include <QJsonDocument>
 #include <QJsonArray>
@@ -94,6 +95,10 @@ json QgsJsonExporter::exportFeatureToJsonObject( const QgsFeature &feature, cons
       featureJson["id"] = id.toString().toStdString();
     }
   }
+  else if ( FID_IS_NULL( feature.id() ) )
+  {
+    featureJson["id"] = nullptr;
+  }
   else
   {
     featureJson["id"] = feature.id();

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/testqgscompositionconverter.cpp b/tests/src/core/testqgscompositionconverter.cpp
@@ -699,6 +699,7 @@ void TestQgsCompositionConverter::importComposerAtlas()
   QCOMPARE( layout->name(), QStringLiteral( "composer atlas" ) );
 
   QVERIFY( layout->atlas()->enabled() );
+  QVERIFY( layout->atlas()->updateFeatures() > 0 );
 
   checkRenderedImage( layout.get(), QTest::currentTestFunction(), 0 );
 

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/core/testqgsjsonutils.cpp b/tests/src/core/testqgsjsonutils.cpp
@@ -223,7 +223,7 @@ void TestQgsJsonUtils::testExportFeatureJson()
   const auto expectedJson { QStringLiteral( "{\"bbox\":[1.12,1.12,5.45,5.33],\"geometry\":{\"coordinates\":"
                             "[[[1.12,1.34],[5.45,1.12],[5.34,5.33],[1.56,5.2],[1.12,1.34]],"
                             "[[2.0,2.0],[3.0,2.0],[3.0,3.0],[2.0,3.0],[2.0,2.0]]],\"type\":\"Polygon\"}"
-                            ",\"id\":0,\"properties\":{\"flddbl\":2.0,\"fldint\":1,\"fldtxt\":\"a value\"}"
+                            ",\"id\":null,\"properties\":{\"flddbl\":2.0,\"fldint\":1,\"fldtxt\":\"a value\"}"
                             ",\"type\":\"Feature\"}" ) };
 
   const auto j( exporter.exportFeatureToJsonObject( feature ) );
@@ -233,12 +233,14 @@ void TestQgsJsonUtils::testExportFeatureJson()
 
   QgsJsonExporter exporterPrecision { &vl, 1 };
 
+
   const auto expectedJsonPrecision { QStringLiteral( "{\"bbox\":[1.1,1.1,5.5,5.3],\"geometry\":{\"coordinates\":"
                                      "[[[1.1,1.3],[5.5,1.1],[5.3,5.3],[1.6,5.2],[1.1,1.3]],"
                                      "[[2.0,2.0],[3.0,2.0],[3.0,3.0],[2.0,3.0],[2.0,2.0]]],\"type\":\"Polygon\"}"
-                                     ",\"id\":0,\"properties\":{\"flddbl\":2.0,\"fldint\":1,\"fldtxt\":\"a value\"}"
+                                     ",\"id\":123,\"properties\":{\"flddbl\":2.0,\"fldint\":1,\"fldtxt\":\"a value\"}"
                                      ",\"type\":\"Feature\"}" ) };
 
+  feature.setId( 123 );
   const auto jPrecision( exporterPrecision.exportFeatureToJsonObject( feature ) );
   QCOMPARE( QString::fromStdString( jPrecision.dump() ),  expectedJsonPrecision );
   const auto jsonPrecision { exporterPrecision.exportFeature( feature ) };

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')

diff --git a/tests/src/python/test_qgsjsonutils.py b/tests/src/python/test_qgsjsonutils.py
@@ -502,7 +502,7 @@ def testExportFeatureFieldFormatter(self):
         source = QgsVectorLayer("Point?field=fldtxt:string&field=fldint:integer",
                                 "parent", "memory")
         pr = source.dataProvider()
-        pf1 = QgsFeature()
+        pf1 = QgsFeature(123)
         pf1.setFields(source.fields())
         pf1.setAttributes(["test1", 1])
         pf2 = QgsFeature()
@@ -518,7 +518,7 @@ def testExportFeatureFieldFormatter(self):
 
         expected = """{
   "geometry": null,
-  "id": 0,
+  "id": 123,
   "properties": {
     "fldint": "one",
     "fldtxt": "test1"
@@ -578,10 +578,10 @@ def testExportFeatureRelations(self):
         parent = QgsVectorLayer("Point?field=fldtxt:string&field=fldint:integer&field=foreignkey:integer",
                                 "parent", "memory")
         pr = parent.dataProvider()
-        pf1 = QgsFeature()
+        pf1 = QgsFeature(432)
         pf1.setFields(parent.fields())
         pf1.setAttributes(["test1", 67, 123])
-        pf2 = QgsFeature()
+        pf2 = QgsFeature(876)
         pf2.setFields(parent.fields())
         pf2.setAttributes(["test2", 68, 124])
         assert pr.addFeatures([pf1, pf2])
@@ -622,7 +622,7 @@ def testExportFeatureRelations(self):
 
         expected = """{
   "geometry": null,
-  "id": 0,
+  "id": 432,
   "properties": {
     "fldint": 67,
     "fldtxt": "test1",
@@ -646,7 +646,7 @@ def testExportFeatureRelations(self):
 
         expected = """{
   "geometry": null,
-  "id": 0,
+  "id": 876,
   "properties": {
     "fldint": 68,
     "fldtxt": "test2",
@@ -669,7 +669,7 @@ def testExportFeatureRelations(self):
         child.setEditorWidgetSetup(1, setup)
         expected = """{
   "geometry": null,
-  "id": 0,
+  "id": 432,
   "properties": {
     "fldint": 67,
     "fldtxt": "test1",
@@ -697,7 +697,7 @@ def testExportFeatureRelations(self):
 
         expected = """{
   "geometry": null,
-  "id": 0,
+  "id": 876,
   "properties": {
     "fldint": 68,
     "fldtxt": "test2",
@@ -713,7 +713,7 @@ def testExportFeatureRelations(self):
 
         expected = """{
   "geometry": null,
-  "id": 0,
+  "id": 876,
   "properties": {
     "fldint": 68,
     "fldtxt": "test2",
@@ -875,7 +875,8 @@ def testExportFieldAlias(self):
         pf2 = QgsFeature()
         pf2.setFields(source.fields())
         pf2.setAttributes(["test2", 2])
-        assert pr.addFeatures([pf1, pf2])
+        result, features = pr.addFeatures([pf1, pf2])
+        self.assertTrue(result)
 
         source.setFieldAlias(0, "alias_fldtxt")
         source.setFieldAlias(1, "alias_fldint")
@@ -886,14 +887,14 @@ def testExportFieldAlias(self):
 
         expected = """{
   "geometry": null,
-  "id": 0,
+  "id": 1,
   "properties": {
     "alias_fldint": 1,
     "alias_fldtxt": "test1"
   },
   "type": "Feature"
 }"""
-        self.assertEqual(exporter.exportFeature(pf1, indent=2), expected)
+        self.assertEqual(exporter.exportFeature(features[0], indent=2), expected)
 
 
 if __name__ == "__main__":

diff --git a/...tionconverter/expected_importComposerAtlas_0/expected_importComposerAtlas_0.png b/...tionconverter/expected_importComposerAtlas_0/expected_importComposerAtlas_0.png
diff --git a/...onverter/expected_importComposerAtlas_0/expected_importComposerAtlas_0_mask.png b/...onverter/expected_importComposerAtlas_0/expected_importComposerAtlas_0_mask.png