fix(explore): refuse a catalog merge that does not land every row
The walk's predicates partition the artifact's key space, so a merge that ends with fewer rows than the artifact declares does not mean the artifact was smaller than it said — it means a predicate filtered rows out, and the catalog is quietly partial while reporting complete. Equality rather than a lower bound: RowsAffected counts an upsert that changes nothing, and a row already merged locally is counted again here. One reachable case, so this is not merely a tripwire. A row whose mbid is empty is excluded by `mbid > ?` in both encodings, so an artifact carrying one imports as a success with a row missing — which is the shape #258 had, one cause over. The test covers exactly that artifact. Refs #258
This commit is contained in:
@@ -285,6 +285,25 @@ func (si *SearchIndex) importCoreArtifact(ctx context.Context, path string) erro
|
|||||||
}
|
}
|
||||||
|
|
||||||
merged, mergeErr := si.mergeArtifactRows(ctx, info.rows)
|
merged, mergeErr := si.mergeArtifactRows(ctx, info.rows)
|
||||||
|
|
||||||
|
// Every row the artifact declares has to land. The walk partitions
|
||||||
|
// the artifact's key space, so a total short of info.rows does not
|
||||||
|
// mean the artifact was smaller than it said -- it means a predicate
|
||||||
|
// filtered rows out, and the catalog is quietly partial. Equality
|
||||||
|
// rather than a lower bound because RowsAffected counts an upsert
|
||||||
|
// that changes nothing, and a row already merged locally is counted
|
||||||
|
// again here.
|
||||||
|
//
|
||||||
|
// One reachable case, so this is not merely a tripwire: a row whose
|
||||||
|
// mbid is empty is excluded by `mbid > ?` in both encodings, and an
|
||||||
|
// artifact carrying one would otherwise import as complete.
|
||||||
|
if mergeErr == nil && merged != info.rows {
|
||||||
|
mergeErr = fmt.Errorf(
|
||||||
|
"%w: merged %d of %d rows — a row the artifact holds was not selected",
|
||||||
|
ErrArtifactUnusable, merged, info.rows,
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
if mergeErr == nil {
|
if mergeErr == nil {
|
||||||
si.mergeArtifactCredits(ctx)
|
si.mergeArtifactCredits(ctx)
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -852,6 +852,41 @@ func TestImportCoreArtifactWithoutCredits(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestImportCoreArtifactRefusesAMergeThatLosesRows is the count guard's
|
||||||
|
// positive case.
|
||||||
|
//
|
||||||
|
// The walk's predicates partition the artifact's key space, so a merge
|
||||||
|
// that lands fewer rows than the artifact declares means a predicate
|
||||||
|
// dropped some — and the failure is a catalog that looks populated and
|
||||||
|
// is missing things nobody can name. An empty mbid is the reachable
|
||||||
|
// way to get there: `mbid > ?` is false of it in both encodings, so it
|
||||||
|
// is never selected, and nothing else in the import would notice.
|
||||||
|
func TestImportCoreArtifactRefusesAMergeThatLosesRows(t *testing.T) {
|
||||||
|
db := database.NewTestDB(t)
|
||||||
|
si := NewSearchIndex(db, nil, nil, testLogger())
|
||||||
|
|
||||||
|
path := writeCompactTestArtifact(t, validMeta(), []artifactRow{
|
||||||
|
{EntityArtist, artA, "Artist A", "Artist A", artA, 5000},
|
||||||
|
{EntityArtist, "", "Nameless", "Artist A", artA, 4000},
|
||||||
|
})
|
||||||
|
|
||||||
|
err := si.importCoreArtifact(context.Background(), path)
|
||||||
|
if err == nil {
|
||||||
|
t.Fatal("a merge that lost a row was reported as a complete import")
|
||||||
|
}
|
||||||
|
|
||||||
|
if !strings.Contains(err.Error(), "merged 1 of 2 rows") {
|
||||||
|
t.Errorf("error = %v, want it to name the shortfall", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// And the same rule as every other rejection: a failed merge must not
|
||||||
|
// leave the index claiming it has a catalog, or the real build would
|
||||||
|
// never run again.
|
||||||
|
if si.hasMeta(dumpImportDoneKey) {
|
||||||
|
t.Error("a failed import still stamped dump_import_done")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
// TestArtifactKeyBindsInTheArtifactsOwnEncoding pins the one place the
|
// TestArtifactKeyBindsInTheArtifactsOwnEncoding pins the one place the
|
||||||
// batch walk's comparison type is decided.
|
// batch walk's comparison type is decided.
|
||||||
//
|
//
|
||||||
|
|||||||
Reference in New Issue
Block a user