Address Claude review feedback (#382)

- gpkg-ogr-contents.ts: when repairing an existing gpkg_ogr_contents row,
  match it by the exact stored name (not `lower(table_name)`, which is
  ASCII-only in SQLite and misses non-ASCII names) and normalise table_name
  to the canonical gpkg_contents casing, so GDAL's case-sensitive lookup
  finds the repaired row instead of falling back to the threaded count path.
- tests: assert the case-insensitive repair normalises to the canonical
  casing; add a non-ASCII NULL-count repair test and a phantom-table
  (unreadable table skipped, others repaired) test.

Author: giswqs

apps/geolibre-desktop/src/lib/gpkg-ogr-contents.ts

@@ -101,15 +101,15 @@ export function ensureGpkgFeatureCountSync(
     // the row's presence (the previous behaviour) let those files through. See
     // issues #258 and #376.
     const tablesWithValidCount = new Set<string>(); // lowercase keys
-    const tablesWithRow = new Set<string>(); // lowercase keys
+    const tablesWithRow = new Map<string, string>(); // lowercase key → name as stored
     if (hasOgrContents) {
       for (const row of db.exec(
         "SELECT table_name, typeof(feature_count), feature_count FROM gpkg_ogr_contents",
       )[0]?.values ?? []) {
         const name = row[0];
         if (typeof name !== "string") continue;
         const key = name.toLowerCase();
-        tablesWithRow.add(key);
+        tablesWithRow.set(key, name);
         // A valid cached count is a non-negative integer. GDAL uses -1 as a
         // "dirty/invalid" sentinel and recomputes the count for it (the
         // multithreaded path that crashes WASM), so a negative value is not safe.
@@ -143,12 +143,18 @@ export function ensureGpkgFeatureCountSync(
           `SELECT count(*) FROM ${quoteIdentifier(tableName)}`,
         );
         const count = countResult[0]?.values[0]?.[0] ?? 0;
-        if (tablesWithRow.has(key)) {
+        const storedName = tablesWithRow.get(key);
+        if (storedName !== undefined) {
           // Repair a stale/NULL count rather than INSERT (which would collide
-          // with the existing primary-key row).
+          // with the existing primary-key row). Match on the exact stored name
+          // (SQLite's lower() is ASCII-only, so a `lower(table_name) = :key`
+          // predicate would miss non-ASCII names) and normalise table_name to
+          // the canonical (gpkg_contents) casing: GDAL looks the row up with a
+          // case-sensitive `table_name = <name from gpkg_contents>`, so a
+          // wrong-cased row would not be found and would still crash.
           db.run(
-            "UPDATE gpkg_ogr_contents SET feature_count = :count WHERE lower(table_name) = :name",
-            { ":name": key, ":count": count },
+            "UPDATE gpkg_ogr_contents SET feature_count = :count, table_name = :canonical WHERE table_name = :stored",
+            { ":canonical": tableName, ":stored": storedName, ":count": count },
           );
         } else {
           db.run(

tests/gpkg-ogr-contents.test.ts

@@ -239,10 +239,12 @@ describe("ensureGpkgFeatureCountSync", () => {
     ]);
   });
 
-  it("matches table names case-insensitively across metadata tables", () => {
+  it("matches table names case-insensitively and normalises to the canonical casing", () => {
     // gpkg_contents and gpkg_ogr_contents disagree on casing for the same
-    // (case-insensitive) SQLite table. The repair must treat them as one table
-    // and UPDATE the existing NULL row rather than INSERT a duplicate.
+    // (case-insensitive) SQLite table. The repair must treat them as one table,
+    // UPDATE the existing NULL row rather than INSERT a duplicate, and rewrite
+    // table_name to the gpkg_contents spelling so GDAL's case-sensitive lookup
+    // finds it.
     const db: Database = new SQL.Database();
     db.run(`
       CREATE TABLE gpkg_contents (
@@ -262,7 +264,34 @@ describe("ensureGpkgFeatureCountSync", () => {
     const patched = ensureGpkgFeatureCountSync(SQL, original);
     assert.notEqual(patched, original);
     assert.deepEqual(readOgrContents(patched), [
-      { table_name: "places", feature_count: 3 },
+      { table_name: "Places", feature_count: 3 },
+    ]);
+  });
+
+  it("repairs a non-ASCII table name whose count is NULL", () => {
+    // SQLite's lower() is ASCII-only, so a `lower(table_name) = :key` predicate
+    // would never match a non-ASCII name; matching on the exact stored name
+    // keeps the UPDATE working here.
+    const db: Database = new SQL.Database();
+    db.run(`
+      CREATE TABLE gpkg_contents (
+        table_name TEXT NOT NULL PRIMARY KEY, data_type TEXT NOT NULL, srs_id INTEGER
+      );
+      CREATE TABLE "Über" (fid INTEGER PRIMARY KEY, geom BLOB);
+      INSERT INTO gpkg_contents VALUES ('Über', 'features', 4326);
+      INSERT INTO "Über" (geom) VALUES (NULL), (NULL);
+      CREATE TABLE gpkg_ogr_contents (
+        table_name TEXT NOT NULL PRIMARY KEY, feature_count INTEGER
+      );
+      INSERT INTO gpkg_ogr_contents (table_name, feature_count) VALUES ('Über', NULL);
+    `);
+    const original = db.export();
+    db.close();
+
+    const patched = ensureGpkgFeatureCountSync(SQL, original);
+    assert.notEqual(patched, original);
+    assert.deepEqual(readOgrContents(patched), [
+      { table_name: "Über", feature_count: 2 },
     ]);
   });
 
@@ -321,6 +350,31 @@ describe("ensureGpkgFeatureCountSync", () => {
     ]);
   });
 
+  it("skips an unreadable phantom table and still repairs the others", () => {
+    // gpkg_contents lists a table that does not exist as a real SQLite table
+    // (a deleted/virtual/view entry). count(*) on it throws; the repair must
+    // skip it and still patch the readable feature table.
+    const db: Database = new SQL.Database();
+    db.run(`
+      CREATE TABLE gpkg_contents (
+        table_name TEXT NOT NULL PRIMARY KEY, data_type TEXT NOT NULL, srs_id INTEGER
+      );
+      CREATE TABLE real_table (fid INTEGER PRIMARY KEY, geom BLOB);
+      INSERT INTO gpkg_contents VALUES ('real_table', 'features', 4326);
+      INSERT INTO gpkg_contents VALUES ('ghost_table', 'features', 4326);
+      INSERT INTO real_table (geom) VALUES (NULL), (NULL);
+    `);
+    const original = db.export();
+    db.close();
+
+    const patched = ensureGpkgFeatureCountSync(SQL, original);
+    assert.notEqual(patched, original);
+    // ghost_table is silently skipped; real_table is repaired.
+    assert.deepEqual(readOgrContents(patched), [
+      { table_name: "real_table", feature_count: 2 },
+    ]);
+  });
+
   it("leaves a complete GeoPackage untouched", () => {
     const original = buildGpkg({ withOgrContents: true, featureCount: 3 });
     const patched = ensureGpkgFeatureCountSync(SQL, original);