Ensure fields to be consistent after editing (#45)

+ Ensure xyz_id uniqueness via sqlite trigger instead of 'on replace conflict' constraint
+ Ensure fields to be consistent with provider fields to avoid data corruption, especially after editing
     - map_fields, feat.fields
+ Ensure the paired order of fields-vector layer after a layer is removed (set fields to empty for deleted layer)
+ Handle virtual fields (expression field) in data loading
     - virtual fields shall never be appended to provider fields, it only stays as vector layer fields
+ Update test for parser and render
+ Disconnect signal silently to avoid exception

XYZHubConnector/plugin.py

@@ -20,6 +20,7 @@
 
 from .gui.space_dialog import MainDialog
 from .gui.util_dialog import ConfirmDialog, exec_warning_dialog
+from .xyz_qgis.common.utils import disconnect_silent
 
 from .xyz_qgis.models import (
     LOADING_MODES,
@@ -275,9 +276,10 @@ def init_modules(self):
 
     def unload_modules(self):
         # self.con_man.disconnect_ux( self.iface)
-        QgsProject.instance().cleared.disconnect(self.new_session)
-        QgsProject.instance().layersWillBeRemoved["QStringList"].disconnect(
-            self.edit_buffer.remove_layers
+        disconnect_silent(QgsProject.instance().cleared, self.new_session)
+        disconnect_silent(
+            QgsProject.instance().layersWillBeRemoved["QStringList"],
+            self.edit_buffer.remove_layers,
         )
         # QgsProject.instance().layersWillBeRemoved["QStringList"].disconnect(
         # self.layer_man.remove_layers)
@@ -291,15 +293,17 @@ def unload_modules(self):
 
         self.iface.mapCanvas().extentsChanged.disconnect(self.reload_tile)
 
-        QgsProject.instance().layerTreeRoot().willRemoveChildren.disconnect(
-            self.cb_qnodes_deleting
+        disconnect_silent(
+            QgsProject.instance().layerTreeRoot().willRemoveChildren, self.cb_qnodes_deleting
         )
-        QgsProject.instance().layerTreeRoot().removedChildren.disconnect(self.cb_qnodes_deleted)
-        QgsProject.instance().layerTreeRoot().visibilityChanged.disconnect(
-            self.cb_qnode_visibility_changed
+        disconnect_silent(
+            QgsProject.instance().layerTreeRoot().removedChildren, self.cb_qnodes_deleted
         )
-
-        QgsProject.instance().readProject.disconnect(self.import_project)
+        disconnect_silent(
+            QgsProject.instance().layerTreeRoot().visibilityChanged,
+            self.cb_qnode_visibility_changed,
+        )
+        disconnect_silent(QgsProject.instance().readProject, self.import_project)
 
         # utils.disconnect_silent(self.iface.currentLayerChanged)
 

XYZHubConnector/xyz_qgis/common/utils.py

@@ -21,10 +21,13 @@ def get_current_millis_time():
     return int(round(time.time() * 1000))
 
 
-def disconnect_silent(signal):
+def disconnect_silent(signal, fn=None):
     ok = True
     try:
-        signal.disconnect()
+        if fn is None:
+            signal.disconnect()
+        else:
+            signal.disconnect(fn)
     except TypeError:
         ok = False
     return ok

XYZHubConnector/xyz_qgis/layer/edit_buffer.py

@@ -22,6 +22,7 @@
     get_conn_info_from_layer,
     get_layer,
 )
+from ..common.utils import disconnect_silent
 
 from ..common.signal import make_print_qgis
 
@@ -275,7 +276,7 @@ def cache_xyz_id_from_feat(self, fid, feat):
 
     def update_synced_feat(self, fid, feat):
         vlayer = get_layer(self.layer_id)
-        fields = vlayer.fields()
+        fields = vlayer.dataProvider().fields()
         ft = parser.xyz_json_to_feat(feat, fields)
         ft.setId(fid)
         update_feat_non_null(vlayer, ft)
@@ -426,7 +427,7 @@ def config_connection(self, callback_pairs):
 
     def unload_connection(self):
         for signal, callback in self.callback_pairs:
-            signal.disconnect(callback)
+            disconnect_silent(signal, callback)
 
 
 class EditBuffer(object):

XYZHubConnector/xyz_qgis/layer/layer.py

@@ -23,13 +23,13 @@
     QgsVectorLayer,
     QgsCoordinateTransform,
     QgsWkbTypes,
+    QgsFields,
 )
 
 from qgis.utils import iface
-from qgis.PyQt.QtCore import QObject, pyqtSignal
 from qgis.PyQt.QtXml import QDomDocument
 
-from . import parser, render
+from . import parser
 from .layer_props import QProps
 from .layer_utils import get_feat_cnt_from_src, get_customProperty_str, load_json_default
 from ..models import SpaceConnectionInfo, parse_copyright
@@ -105,9 +105,12 @@ def load_from_qnode(cls, qnode):
         # obj._save_meta_node(qnode)
         for i in qnode.findLayers():
             vlayer = i.layer()
+            if not vlayer.isValid():
+                continue
             geom_str = QgsWkbTypes.displayString(vlayer.wkbType())
             obj.map_vlayer.setdefault(geom_str, list()).append(vlayer)
-            obj.map_fields.setdefault(geom_str, list()).append(vlayer.fields())
+            obj.map_fields.setdefault(geom_str, list()).append(vlayer.dataProvider().fields())
+            obj.update_constraint_trigger(geom_str, len(obj.map_vlayer[geom_str]) - 1)
             # obj._save_meta_vlayer(vlayer)
         return obj
 
@@ -395,7 +398,9 @@ def add_ext_layer(self, geom_str, idx):
         qgroups: dict["main"] = group
             dict[geom] = list([vlayer1, vlayer2,...])
         map_vlayer: dict[geom_str] = list([vlayer1, vlayer2,...])
+            vlayer order in list shall always be fixed, deleted vlayer hall be set to None
         map_fields: dict[geom_str] = list([fields1, fields2,...])
+            fields order in list shall always be fixed and not be deleted
         geom_str: QgsWkbTypes.displayString (detailed geometry, e.g. Multi-)
         geom: QgsWkbTypes.geometryDisplayString (generic geometry,)
         """
@@ -435,8 +440,15 @@ def _add_layer(self, geom_str, vlayer, idx):
 
     def _remove_layer(self, geom_str, idx):
         """Remove vlayer from the internal map without messing the index"""
-        self.map_vlayer[geom_str].pop(idx)
-        self.map_fields[geom_str].pop(idx)
+        self.map_vlayer[geom_str][idx] = None
+        self.map_fields[geom_str][idx] = parser.new_fields_gpkg()
+
+    def refresh_map_fields(self):
+        for geom_str in self.map_vlayer:
+            for idx, vlayer in enumerate(self.map_vlayer.get(geom_str, list())):
+                if vlayer is None:
+                    continue
+                self.map_fields[geom_str][idx] = vlayer.dataProvider().fields()
 
     def _init_ext_layer(self, geom_str, idx, crs):
         """given non map of feat, init a qgis layer
@@ -488,64 +500,28 @@ def _init_ext_layer(self, geom_str, idx, crs):
         if err[0] != QgsVectorFileWriter.NoError:
             raise Exception("%s: %s" % err)
 
-        sql_constraint = '"%s" TEXT UNIQUE ON CONFLICT REPLACE' % (
-            parser.QGS_XYZ_ID
-        )  # replace older duplicate
-        # sql_constraint = '"%s" TEXT UNIQUE ON CONFLICT IGNORE'%(parser.QGS_XYZ_ID) # discard
-        # newer duplicate
-        self._init_constraint(fname, sql_constraint, db_layer_name)
+        self._update_constraint_trigger(fname, db_layer_name)
 
         uri = "%s|layername=%s" % (fname, db_layer_name)
         vlayer = QgsVectorLayer(uri, layer_name, "ogr")
         self._save_meta_vlayer(vlayer)
 
         return vlayer
 
-    def _init_constraint(self, fname, sql_constraint, layer_name):
-        # https://sqlite.org/lang_altertable.html
-
-        tmp_name = "tmp_" + layer_name
+    def update_constraint_trigger(self, geom_str, idx):
+        fname = make_fixed_full_path(self._layer_fname(), ext=self.ext)
+        db_layer_name = self._db_layer_name(geom_str, idx)
+        self._update_constraint_trigger(fname, db_layer_name)
 
+    def _update_constraint_trigger(self, fname, layer_name):
+        sql_trigger = """CREATE TRIGGER IF NOT EXISTS "trigger_{layer_name}_{id_column}_insert"
+        BEFORE INSERT ON "{layer_name}" BEGIN DELETE FROM "{layer_name}"
+        WHERE "{id_column}" = NEW."{id_column}"; END""".format(
+            layer_name=layer_name, id_column=parser.QGS_XYZ_ID
+        )
         conn = sqlite3.connect(fname)
         cur = conn.cursor()
-
-        sql = 'SELECT type, sql FROM sqlite_master WHERE tbl_name="%s"' % (layer_name)
-        cur.execute(sql)
-        lst = cur.fetchall()
-        if len(lst) == 0:
-            raise Exception("No layer found in GPKG")
-        lst_old_sql = [p[1] for p in lst]
-        sql_create = lst_old_sql.pop(0)
-
-        sql_create = sql_create.replace(layer_name, tmp_name)
-        parts = sql_create.partition(")")
-        sql_create = "".join([parts[0], ", %s" % (sql_constraint), parts[1], parts[2]])
-        # empty table, so insert is skipped
-        lst_sql = (
-            [
-                "PRAGMA foreign_keys = '0'",
-                "BEGIN TRANSACTION",
-                sql_create,
-                "PRAGMA defer_foreign_keys = '1'",
-                'DROP TABLE "%s"' % (layer_name),
-                'ALTER TABLE "%s" RENAME TO "%s"' % (tmp_name, layer_name),
-                "PRAGMA defer_foreign_keys = '0'",
-            ]
-            + lst_old_sql
-            + [
-                # 'PRAGMA "main".foreign_key_check', # does not return anything -> unused
-                "COMMIT",
-                "PRAGMA foreign_keys = '1'",
-            ]
-        )
-        for s in lst_sql:
-            if "COMMIT" in s:
-                conn.commit()
-                continue
-            # if "COMMIT" in s and not conn.in_transaction: continue
-
-            cur.execute(s)
-
+        cur.execute(sql_trigger)
         conn.commit()
         conn.close()
 

XYZHubConnector/xyz_qgis/layer/parser.py

@@ -143,6 +143,14 @@ def make_valid_xyz_json_geom(geom: dict):
     return geom
 
 
+def non_expression_fields(fields):
+    return [f for i, f in enumerate(fields) if fields.fieldOrigin(i) != fields.OriginExpression]
+
+
+def check_non_expression_fields(fields):
+    return all(fields.fieldOrigin(i) != fields.OriginExpression for i, f in enumerate(fields))
+
+
 def feature_to_xyz_json(features, is_new=False, ignore_null=True):
     def _xyz_props(props, ignore_keys=tuple()):
         # for all key start with @ (internal): str to dict (disabled)
@@ -193,9 +201,9 @@ def _single_feature(feat):
                 obj[XYZ_ID] = v
         fields = feat.fields()
         expression_field_names = [
-            k
-            for k in fields.names()
-            if fields.fieldOrigin(fields.indexFromName(k)) == fields.OriginExpression
+            f.name()
+            for i, f in enumerate(fields)
+            if fields.fieldOrigin(i) == fields.OriginExpression
         ]
         # print({k.name(): fields.fieldOrigin(i) for i, k in enumerate(fields)})
         props = _xyz_props(props, ignore_keys=expression_field_names)
@@ -356,7 +364,7 @@ def xyz_json_to_feat(feat_json, fields):
     Convert xyz geojson to feature, given fields
     """
 
-    names = fields.names()
+    names = set(fields.names())
 
     qattrs = list()
 
@@ -382,7 +390,7 @@ def xyz_json_to_feat(feat_json, fields):
                         val.convert(cast)
                         break
             if not val.type() in valid_qvariant:
-                print_qgis("Invalid type", k, val.typeName())
+                print_qgis("Field '%s': Invalid type: %s. Value: %s" % (k, val.typeName(), val))
                 continue
             if k not in names:
                 fields.append(make_field(k, val))
@@ -402,6 +410,47 @@ def xyz_json_to_feat(feat_json, fields):
     return feat
 
 
+def check_same_fields(fields1: QgsFields, fields2: QgsFields):
+    """
+    Check if fields order, name and origin are equal
+
+    :param fields1: QgsFields
+    :param fields2: other QgsFields
+    :return: True if 2 fields are equal (same order, name, origin)
+    """
+    len_ok = len(fields1) == len(fields2)
+    name_ok = fields1.names() == fields2.names()
+    field_origin_ok = all(
+        fields1.fieldOrigin(i) == fields2.fieldOrigin(i)
+        for i, (f1, f2) in enumerate(zip(fields1, fields2))
+    )
+    return len_ok and name_ok and field_origin_ok
+
+
+def update_feature_fields(feat: QgsFeature, fields: QgsFields):
+    """
+    Update fields of feature and its data (QgsAttributes)
+
+    :param feat: QgsFeature
+    :param fields: new QgsFields
+    :return: new QgsFeature with updated fields
+    """
+    old_fields = feat.fields()
+    try:
+        assert set(fields.names()).issuperset(
+            set(old_fields.names())
+        ), "new fields must be a super set of existing fields of feature"
+    except AssertionError as e:
+        print_error(e)
+        return
+
+    ft = QgsFeature(fields)
+    for k in old_fields.names():
+        ft.setAttribute(k, feat.attribute(k))
+    ft.setGeometry(feat.geometry())
+    return ft
+
+
 def prepare_fields(feat_json, lst_fields, threshold=0.8):
     """
     Decide to merge fields or create new fields based on fields similarity score [0..1].
@@ -417,17 +466,30 @@ def prepare_fields(feat_json, lst_fields, threshold=0.8):
         rename_special_props(props)  # rename fid in props
         props_names = [k for k, v in props.items() if v is not None]
     lst_score = [
-        fields_similarity((fields.names()), orig_props_names, props_names) for fields in lst_fields
+        fields_similarity(
+            [f.name() for f in non_expression_fields(fields)],
+            orig_props_names,
+            props_names,
+        )
+        if fields.size() > 1
+        else -1
+        for fields in lst_fields
     ]
     idx, score = max(enumerate(lst_score), key=lambda x: x[1], default=[0, 0])
+    idx_min, score_min = min(enumerate(lst_score), key=lambda x: x[1], default=[0, 0])
 
     if score < threshold or not idx < len(lst_fields):  # new fields
-        idx = len(lst_fields)
-        fields = new_fields_gpkg()
-        lst_fields.append(fields)
-        print_qgis("len prop", len(props_names), "score", lst_score, "lst_fields", len(lst_fields))
+        if score_min >= 0:  # new fields
+            idx = len(lst_fields)
+            fields = new_fields_gpkg()
+            lst_fields.append(fields)
+        else:  # select empty fields
+            idx = idx_min
+            fields = lst_fields[idx]
     else:
         fields = lst_fields[idx]
+    # print("len prop", len(props_names), idx, "score", lst_score, "lst_fields", len(lst_fields))
+    # print("len fields", [f.size() for f in lst_fields])
 
     return fields, idx
 

XYZHubConnector/xyz_qgis/layer/render.py

@@ -103,7 +103,8 @@ def add_feature_render(vlayer, feat, new_fields):
     if transformer.isValid() and not transformer.isShortCircuited():
         feat = filter(None, (parser.transform_geom(ft, transformer) for ft in feat if ft))
 
-    names = set(vlayer.fields().names())
+    names = set(pr.fields().names())
+    assert parser.check_non_expression_fields(new_fields)
     diff_fields = [f for f in new_fields if not f.name() in names]
 
     # print_qgis(len(names), names)
@@ -119,6 +120,10 @@ def add_feature_render(vlayer, feat, new_fields):
     pr.addAttributes(diff_fields)
     vlayer.updateFields()
 
+    # update feature fields according to provider fields
+    if not parser.check_same_fields(new_fields, pr.fields()):
+        feat = filter(None, (parser.update_feature_fields(ft, pr.fields()) for ft in feat if ft))
+
     ok, out_feat = pr.addFeatures(feat)
     vlayer.updateExtents()  # will hide default progress bar
     # post_render(vlayer) # disable in order to keep default progress bar running

XYZHubConnector/xyz_qgis/loader/layer_loader.py

@@ -171,6 +171,7 @@ def _check_status(self):
 
     def _run(self):
         conn_info = self.get_conn_info()
+        self.layer.refresh_map_fields()  # ensure map fields is updated from provider
 
         # TODO refactor methods
         if not self.params_queue.has_next():