Fix unique constraint violation when updating remap table

Fixes #38

This could happen because we were keeping outdated entries in remap
tables of work packages - the tuples (master_fid, wp_fid) would get
added as needed, but they were never getting deleting when the corresponding
features got deleted. As a result, users could get an error from SQLite
about unique constraint violation when inserting to the remap table
because the master_fid already existed there from some previous runs.

Author: wonder-sk

workpackages/remapping.py

@@ -51,6 +51,9 @@ def remap_table_master_to_wp(cursor, table_name, wp_name):
     For each row:
     - remap row exists for master_fid -> use wp_fid
     - remap does not exist for master_fid -> insert (master_fid, 1000000+master_fid)
+
+    We also clean up remap table not to include any entries where master_fid
+    does not exist anymore.
     """
     remap_table_escaped = remap_table_name(table_name, wp_name)
     _create_remap_table_if_not_exists(cursor, remap_table_escaped)
@@ -67,7 +70,7 @@ def remap_table_master_to_wp(cursor, table_name, wp_name):
     for row in cursor.execute(sql):
         master_fids_missing.add(row[0])
 
-    # 2. insert missing mapped ids
+    # 2. insert missing mapped ids (after a feature got added in master)
     cursor.execute(f"""SELECT max(wp_fid) FROM {remap_table_escaped}""")
     new_wp_fid = cursor.fetchone()[0]
     if new_wp_fid is None:
@@ -79,7 +82,14 @@ def remap_table_master_to_wp(cursor, table_name, wp_name):
         cursor.execute(f"""INSERT INTO {remap_table_escaped} VALUES (?, ?)""", (master_fid, new_wp_fid))
         new_wp_fid += 1
 
-    # 3. remap master ids to WP ids
+    # 3. delete redundant master ids (after a feature got deleted in master)
+    sql = (
+        f"""DELETE FROM {remap_table_escaped} WHERE master_fid NOT IN """
+        f"""   (SELECT {pkey_column_escaped} FROM {table_name_escaped})"""
+    )
+    cursor.execute(sql)
+
+    # 4. remap master ids to WP ids
     mapping = []
     sql = (
         f"""SELECT {pkey_column_escaped}, mapped.wp_fid FROM {table_name_escaped} """
@@ -105,6 +115,9 @@ def remap_table_wp_to_master(cursor, table_name, wp_name, new_master_fid):
     For each row:
     - remap row exists for wp_fid -> use master_fid
     - remap does not exist for wp_fid -> insert ([first unused master fid], wp_fid)
+
+    We also clean up remap table not to include any entries where wp_fid
+    does not exist anymore.
     """
 
     remap_table = remap_table_name(table_name, wp_name)
@@ -122,12 +135,19 @@ def remap_table_wp_to_master(cursor, table_name, wp_name, new_master_fid):
     for row in cursor.execute(sql):
         wp_fids_missing.add(row[0])
 
-    # 2. insert missing mapped ids
+    # 2. insert missing mapped ids (after a feature got added in WP)
     for wp_fid in wp_fids_missing:
         cursor.execute(f"""INSERT INTO {remap_table} VALUES (?, ?)""", (new_master_fid, wp_fid))
         new_master_fid += 1
 
-    # 3. remap WP ids to master ids
+    # 3. delete redundant master ids (after a feature got deleted in WP)
+    sql = (
+        f"""DELETE FROM {remap_table} WHERE wp_fid NOT IN """
+        f"""   (SELECT {pkey_column_escaped} FROM {table_name_escaped})"""
+    )
+    cursor.execute(sql)
+
+    # 4. remap WP ids to master ids
     mapping = []  # list of tuples (wp_fid, master_fid)
     sql = (
         f"""SELECT {pkey_column_escaped}, mapped.master_fid FROM {table_name_escaped} """

workpackages/test/config-farm-duplicate-wp.yml

@@ -0,0 +1,25 @@
+file: farms.gpkg
+
+work-packages:
+  - name: Kyle
+    value: 4
+    mergin-project: martin/farms-Kyle
+
+  - name: Kyle_duplicate
+    value: 4
+    mergin-project: martin/farms-Kyle-duplicate
+
+  - name: Emma
+    value:
+      - 1
+      - 2
+    mergin-project: martin/farms-Emma
+
+tables:
+  - name: farms
+    method: filter-column
+    filter-column-name: fid
+  - name: trees
+    method: filter-column
+    filter-column-name: farm_id
+    

workpackages/test/test_basic.py

@@ -60,6 +60,16 @@ def _assert_row_exists(gpkg_filename, table_name, fid):
     assert row[0] == 1, f"Row for fid {fid} is not present but it should be"
 
 
+def _assert_remap_entries(db_filename, table_name, expected_entries):
+    """Asserts that the remap table has exactly the given entries"""
+    db = sqlite3.connect(db_filename)
+    c = db.cursor()
+    actual_entries = set()
+    for row in c.execute(f"""SELECT master_fid, wp_fid FROM {table_name}"""):
+        actual_entries.add((row[0], row[1]))
+    assert actual_entries == expected_entries
+
+
 def _make_initial_farm_work_packages(config_file):
     """
     1. create the initial "farms" dataset
@@ -393,6 +403,136 @@ def test_delete_row_master_wp():
     _assert_row_missing(os.path.join(output_dir, "Kyle.gpkg"), "trees", 1000001)
 
 
+def test_remapping_table_wp_to_master():
+    """
+    Test that the remapping tables get correctly updated when features
+    are inserted/deleted in WP.
+
+    Test following workflow scenario (with running make_work_packages function after each step):
+    - we have 2 WPs with a same filter condition (Kyle, Kyle_duplicate);
+    - we are adding a new feature (FID: 1111111) to Kyle;
+    - we are deleting feature (FID: 1111111) from Kyle;
+    - we are adding a new feature (FID: 2222222) to Kyle_duplicate;
+    After each step we test that the remap database has expected content.
+    """
+    config_file = os.path.join(this_dir, "config-farm-duplicate-wp.yml")
+    tmp_dir_1 = _make_initial_farm_work_packages(config_file)
+    wp_config = load_config_from_yaml(config_file)
+
+    # modify 'Kyle' WP by adding a new 'trees' feature with a FID '1111111'
+    # and run work packaging
+    tmp_dir_2 = _prepare_next_run_work_packages(tmp_dir_1)
+    open_layer_and_create_feature(
+        os.path.join(tmp_dir_2.name, "input", "Kyle.gpkg"),
+        "trees",
+        "POINT(6 16)",
+        {"tree_species_id": 1, "farm_id": 4},
+        fid=1111111,
+    )
+    make_work_packages(tmp_dir_2.name, wp_config)
+
+    _assert_remap_entries(
+        os.path.join(tmp_dir_2.name, "output", "remap.db"),
+        "trees_Kyle",
+        set([(8, 1000000), (9, 1000001), (10, 1111111)]),
+    )
+    _assert_remap_entries(
+        os.path.join(tmp_dir_2.name, "output", "remap.db"),
+        "trees_Kyle_duplicate",
+        set([(8, 1000000), (9, 1000001), (10, 1000002)]),
+    )
+
+    # modify 'Kyle' WP by removing 'trees' feature with a FID '1111111'
+    # run work packaging 2nd time
+    tmp_dir_3 = _prepare_next_run_work_packages(tmp_dir_2)
+    open_layer_and_delete_feature(os.path.join(tmp_dir_3.name, "input", "Kyle.gpkg"), "trees", 1111111)
+    make_work_packages(tmp_dir_3.name, wp_config)
+
+    _assert_remap_entries(
+        os.path.join(tmp_dir_3.name, "output", "remap.db"), "trees_Kyle", set([(8, 1000000), (9, 1000001)])
+    )
+    _assert_remap_entries(
+        os.path.join(tmp_dir_3.name, "output", "remap.db"), "trees_Kyle_duplicate", set([(8, 1000000), (9, 1000001)])
+    )
+
+    # modify 'Kyle_duplicate' WP by adding a new 'trees' feature with FID '2222222'
+    # run work packaging 3rd time
+    tmp_dir_4 = _prepare_next_run_work_packages(tmp_dir_3)
+    open_layer_and_create_feature(
+        os.path.join(tmp_dir_4.name, "input", "Kyle_duplicate.gpkg"),
+        "trees",
+        "POINT(6 16)",
+        {"tree_species_id": 1, "farm_id": 4},
+        fid=2222222,
+    )
+    make_work_packages(tmp_dir_4.name, wp_config)
+
+    _assert_remap_entries(
+        os.path.join(tmp_dir_4.name, "output", "remap.db"),
+        "trees_Kyle_duplicate",
+        set([(8, 1000000), (9, 1000001), (10, 2222222)]),
+    )
+    _assert_remap_entries(
+        os.path.join(tmp_dir_4.name, "output", "remap.db"),
+        "trees_Kyle",
+        set([(8, 1000000), (9, 1000001), (10, 1000002)]),
+    )
+
+
+def test_remapping_table_master_to_wp():
+    """
+    Test that the remapping tables get correctly updated when features
+    are inserted/deleted in master.
+
+    Test following workflow scenario (with running make_work_packages function after each step):
+    - we are adding a new feature (FID: 10) to master;
+    - we are deleting feature (FID: 10) from master;
+    After each step we test that the remap database has expected content.
+    """
+    config_file = os.path.join(this_dir, "config-farm-duplicate-wp.yml")
+    tmp_dir_1 = _make_initial_farm_work_packages(config_file)
+    output_dir = os.path.join(tmp_dir_1.name, "output")
+    output_files = os.listdir(output_dir)
+    assert "Emma.gpkg" in output_files
+    assert "Kyle.gpkg" in output_files
+    assert "Kyle_duplicate.gpkg" in output_files
+    assert "master.gpkg" in output_files
+
+    _assert_remap_entries(
+        os.path.join(tmp_dir_1.name, "output", "remap.db"), "trees_Kyle", set([(8, 1000000), (9, 1000001)])
+    )
+
+    tmp_dir_2 = _prepare_next_run_work_packages(tmp_dir_1)
+
+    # modify master by adding a new 'trees' feature with a FID 10
+    open_layer_and_create_feature(
+        os.path.join(tmp_dir_2.name, "input", "master.gpkg"),
+        "trees",
+        "POINT(6 16)",
+        {"tree_species_id": 1, "farm_id": 4},
+        fid=10,
+    )
+    # run work packaging
+    wp_config = load_config_from_yaml(config_file)
+    make_work_packages(tmp_dir_2.name, wp_config)
+
+    _assert_remap_entries(
+        os.path.join(tmp_dir_2.name, "output", "remap.db"),
+        "trees_Kyle",
+        set([(8, 1000000), (9, 1000001), (10, 1000002)]),
+    )
+
+    tmp_dir_3 = _prepare_next_run_work_packages(tmp_dir_2)
+    # modify master by removing 'trees' feature with a FID 10
+    open_layer_and_delete_feature(os.path.join(tmp_dir_3.name, "input", "master.gpkg"), "trees", 10)
+    # run work packaging 2nd time
+    make_work_packages(tmp_dir_3.name, wp_config)
+
+    _assert_remap_entries(
+        os.path.join(tmp_dir_3.name, "output", "remap.db"), "trees_Kyle", set([(8, 1000000), (9, 1000001)])
+    )
+
+
 # TODO: more test cases
 # - delete_master_update_wp  # one row deleted in master while it is updated in WP
 # - update_master_delete_wp  # one row updated in master while it is deleted in WP