Handle bad/null geometries and geometryless

elpaso · nyalldawson · commit e01449f5c5b5 · 2018-09-14T18:23:03.000+10:00
diff --git a/python/plugins/processing/gui/AlgorithmExecutor.py b/python/plugins/processing/gui/AlgorithmExecutor.py
@@ -70,7 +70,7 @@ def execute(alg, parameters, context=None, feedback=None):
         return False, {}
 
 
-def make_features_compatible(new_features, input_layer, context):
+def make_features_compatible(new_features, input_layer):
     """Try to make the new features compatible with old features by:
 
     - converting single to multi part
@@ -83,8 +83,6 @@ def make_features_compatible(new_features, input_layer, context):
     :type new_features: list of QgsFeatures
     :param input_layer: input layer
     :type input_layer: QgsVectorLayer
-    :param context: processing context
-    :type context: QgsProcessingContext
     :return: modified features
     :rtype: list of QgsFeatures
     """
@@ -93,13 +91,46 @@ def make_features_compatible(new_features, input_layer, context):
     result_features = []
     for new_f in new_features:
         # Fix attributes
-        if len(new_f.attributes()) > len(input_layer.fields()):
+        if new_f.fields().count() > 0:
+            attributes = []
+            for field in input_layer.fields():
+                if new_f.fields().indexFromName(field.name()) >= 0:
+                    attributes.append(new_f[field.name()])
+                else:
+                    attributes.append(None)
             f = QgsFeature(input_layer.fields())
+            f.setAttributes(attributes)
             f.setGeometry(new_f.geometry())
-            f.setAttributes(new_f.attributes()[:len(input_layer.fields())])
             new_f = f
-        # Fix geometry
-        if new_f.geometry().wkbType() != input_wkb_type:
+        else:
+            lendiff = len(new_f.attributes()) - len(input_layer.fields())
+            if lendiff > 0:
+                f = QgsFeature(input_layer.fields())
+                f.setGeometry(new_f.geometry())
+                f.setAttributes(new_f.attributes()[:len(input_layer.fields())])
+                new_f = f
+            elif lendiff < 0:
+                f = QgsFeature(input_layer.fields())
+                f.setGeometry(new_f.geometry())
+                attributes = new_f.attributes() + [None for i in range(-lendiff)]
+                f.setAttributes(attributes)
+                new_f = f
+
+        # Check if we need geometry manipulation
+        new_f_geom_type = QgsWkbTypes.geometryType(new_f.geometry().wkbType())
+        new_f_has_geom = new_f_geom_type not in (QgsWkbTypes.UnknownGeometry, QgsWkbTypes.NullGeometry)
+        input_layer_has_geom = input_wkb_type not in (QgsWkbTypes.NoGeometry, QgsWkbTypes.Unknown)
+        if input_layer_has_geom and not new_f_has_geom:
+            continue  # Skip this feature completely
+        elif not input_layer_has_geom and new_f_has_geom:
+            # Drop geometry
+            f = QgsFeature(input_layer.fields())
+            f.setAttributes(new_f.attributes())
+            new_f = f
+            result_features.append(new_f)
+            continue  # skip the rest
+
+        if input_layer_has_geom and new_f.geometry().wkbType() != input_wkb_type:  # Fix geometry
             # Single -> Multi
             if (QgsWkbTypes.isMultiType(input_wkb_type) and not
                     new_f.geometry().isMultipart()):
@@ -177,7 +208,7 @@ def execute_in_place_run(alg, active_layer, parameters, context=None, feedback=N
     try:
         new_feature_ids = []
 
-        active_layer.beginEditCommand(tr('In-place editing by %s') % alg.name())
+        active_layer.beginEditCommand(alg.name())
 
         req = QgsFeatureRequest(QgsExpression(r"$id < 0"))
         req.setFlags(QgsFeatureRequest.NoGeometry)
@@ -196,7 +227,7 @@ def execute_in_place_run(alg, active_layer, parameters, context=None, feedback=N
             feature_iterator = active_layer.getFeatures(QgsFeatureRequest(active_layer.selectedFeatureIds())) if parameters['INPUT'].selectedFeaturesOnly else active_layer.getFeatures()
             for f in feature_iterator:
                 new_features = alg.processFeature(f, context, feedback)
-                new_features = make_features_compatible(new_features, active_layer, context)
+                new_features = make_features_compatible(new_features, active_layer)
                 if len(new_features) == 0:
                     active_layer.deleteFeature(f.id())
                 elif len(new_features) == 1:
@@ -226,11 +257,12 @@ def execute_in_place_run(alg, active_layer, parameters, context=None, feedback=N
                 active_layer.deleteFeatures(active_layer.selectedFeatureIds())
                 new_features = []
                 for f in result_layer.getFeatures():
-                    new_features.extend(make_features_compatible([f], active_layer, context))
+                    new_features.extend(make_features_compatible([f], active_layer))
 
                 # Get the new ids
                 old_ids = set([f.id() for f in active_layer.getFeatures(req)])
-                active_layer.addFeatures(new_features)
+                if not active_layer.addFeatures(new_features):
+                    raise QgsProcessingException(tr("Error adding processed features back into the layer."))
                 new_ids = set([f.id() for f in active_layer.getFeatures(req)])
                 new_feature_ids += list(new_ids - old_ids)
 
diff --git a/tests/src/python/test_qgsprocessinginplace.py b/tests/src/python/test_qgsprocessinginplace.py
@@ -117,7 +117,7 @@ def _support_inplace_edit_tester(self, alg_name, expected):
         alg = self.registry.createAlgorithmById(alg_name)
         for layer_wkb_name, supported in expected.items():
             layer = self._make_layer(layer_wkb_name)
-            print("Checking %s ( %s ) : %s" % (alg_name, layer_wkb_name, supported))
+            #print("Checking %s ( %s ) : %s" % (alg_name, layer_wkb_name, supported))
             self.assertEqual(alg.supportInPlaceEdit(layer), supported, "Expected: %s - %s = supported: %s" % (alg_name, layer_wkb_name, supported))
 
     def test_support_in_place_edit(self):
@@ -175,7 +175,7 @@ def _make_compatible_tester(self, feature_wkt, layer_wkb_name, attrs=[1]):
         context.setProject(QgsProject.instance())
 
         # Fix it!
-        new_features = make_features_compatible([f], layer, context)
+        new_features = make_features_compatible([f], layer)
 
         for new_f in new_features:
             self.assertEqual(new_f.geometry().wkbType(), layer.wkbType())
@@ -259,6 +259,71 @@ def test_make_features_compatible(self):
         self.assertEqual(f[0].geometry().asWkt(), 'LineString (1 1, 2 2, 3 3, 1 1)')
         self.assertEqual(f[1].geometry().asWkt(), 'LineString (10 1, 20 2, 30 3, 10 1)')
 
+    def test_make_features_compatible_attributes(self):
+        """Test corner cases for attributes"""
+
+        # Test feature without attributes
+        fields = QgsFields()
+        fields.append(QgsField('int_f', QVariant.Int))
+        fields.append(QgsField('str_f', QVariant.String))
+        layer = QgsMemoryProviderUtils.createMemoryLayer(
+            'mkfca_layer', fields, QgsWkbTypes.Point, QgsCoordinateReferenceSystem(4326))
+        self.assertTrue(layer.isValid())
+        f1 = QgsFeature(layer.fields())
+        f1['int_f'] = 1
+        f1['str_f'] = 'str'
+        f1.setGeometry(QgsGeometry.fromWkt('Point(9 45)'))
+        new_features = make_features_compatible([f1], layer)
+        self.assertEqual(new_features[0].attributes(), f1.attributes())
+        self.assertTrue(new_features[0].geometry().asWkt(), f1.geometry().asWkt())
+
+        # Test pad with 0 with fields
+        f1.setAttributes([])
+        new_features = make_features_compatible([f1], layer)
+        self.assertEqual(len(new_features[0].attributes()), 2)
+        self.assertEqual(new_features[0].attributes()[0], QVariant())
+        self.assertEqual(new_features[0].attributes()[1], QVariant())
+
+        # Test pad with 0 without fields
+        f1 = QgsFeature()
+        f1.setGeometry(QgsGeometry.fromWkt('Point(9 45)'))
+        new_features = make_features_compatible([f1], layer)
+        self.assertEqual(len(new_features[0].attributes()), 2)
+        self.assertEqual(new_features[0].attributes()[0], QVariant())
+        self.assertEqual(new_features[0].attributes()[1], QVariant())
+
+        # Test drop extra attrs
+        f1 = QgsFeature(layer.fields())
+        f1.setAttributes([1, 'foo', 'extra'])
+        f1.setGeometry(QgsGeometry.fromWkt('Point(9 45)'))
+        new_features = make_features_compatible([f1], layer)
+        self.assertEqual(len(new_features[0].attributes()), 2)
+        self.assertEqual(new_features[0].attributes()[0], 1)
+        self.assertEqual(new_features[0].attributes()[1], 'foo')
+
+    def test_make_features_compatible_geometry(self):
+        """Test corner cases for geometries"""
+        layer = self._make_layer('Point')
+        self.assertTrue(layer.isValid())
+        self.assertTrue(layer.startEditing())
+        f1 = QgsFeature(layer.fields())
+        f1.setAttributes([1])
+        new_features = make_features_compatible([f1], layer)
+        self.assertEqual(len(new_features), 0)
+
+        nogeom_layer = QgsMemoryProviderUtils.createMemoryLayer(
+            'nogeom_layer', layer.fields(), QgsWkbTypes.NoGeometry, QgsCoordinateReferenceSystem(4326))
+        new_features = make_features_compatible([f1], nogeom_layer)
+        self.assertEqual(len(new_features), 1)
+        self.assertEqual(new_features[0].geometry().asWkt(), '')
+
+        nogeom_layer = QgsMemoryProviderUtils.createMemoryLayer(
+            'nogeom_layer', layer.fields(), QgsWkbTypes.NoGeometry, QgsCoordinateReferenceSystem(4326))
+        f1.setGeometry(QgsGeometry.fromWkt('Point(9 45)'))
+        new_features = make_features_compatible([f1], nogeom_layer)
+        self.assertEqual(len(new_features), 1)
+        self.assertEqual(new_features[0].geometry().asWkt(), '')
+
     def _alg_tester(self, alg_name, input_layer, parameters):
 
         alg = self.registry.createAlgorithmById(alg_name)
