From 79dccf910b196b9846352a491c2b7ccd9bf067d3 Mon Sep 17 00:00:00 2001 From: Kurt Nordstrom Date: Fri, 9 Oct 2026 16:05:40 -0400 Subject: [PATCH 1/4] Implement full-text index for entries, make name and description indexes case insensitive --- directory/README.md | 38 ++++ directory/api.yaml | 15 +- directory/api/entries.go | 7 +- directory/api/entries_test.go | 106 +++++++++ directory/import/db/repo_test.go | 5 +- .../migrations/018_entry_search.down.sql | 10 + directory/migrations/018_entry_search.up.sql | 88 ++++++++ directory/test/entry_case_insensitive_test.go | 75 +++++++ directory/test/entry_search_migration_test.go | 72 ++++++ directory/test/entry_search_test.go | 209 ++++++++++++++++++ 10 files changed, 620 insertions(+), 5 deletions(-) create mode 100644 directory/migrations/018_entry_search.down.sql create mode 100644 directory/migrations/018_entry_search.up.sql create mode 100644 directory/test/entry_case_insensitive_test.go create mode 100644 directory/test/entry_search_migration_test.go create mode 100644 directory/test/entry_search_test.go diff --git a/directory/README.md b/directory/README.md index bf7c2b8b0..d60db1dd5 100644 --- a/directory/README.md +++ b/directory/README.md @@ -252,6 +252,44 @@ Run `make generate` before invoking `go build` or `go test` directly. See [Host LMS and catalog profiles](host-profiles.md) for `lmsConfig.vendor`, `catalogConfig.profile`, precedence, parser overrides, diagnostics and migration. +## Directory entry searches + +`cql.serverChoice` searches a combined document containing the entry's name, +description, organization ID, email, phone number, and all its own symbols +(both `authority:symbol` and bare symbol values). It uses PostgreSQL full-text +search with the fixed `simple` configuration: case is normalized without English +stemming or stop-word removal. NULL fields contribute no text. Parent and child +entry data are not included. + +| CQL example | Behavior | +| --- | --- | +| `cql.serverChoice="central library"` | Both words, potentially in different fields | +| `cql.serverChoice all "central library"` | Both words | +| `cql.serverChoice any "central library"` | Either word | +| `cql.serverChoice adj "central library"` | Adjacent words in order | +| `cql.serverChoice="centr*"` | Word-prefix matching | + +This index is available on both `/entries` and `/entries/owned`; existing access +restrictions, counts, pagination, and result ordering apply. Results are not +ranked by relevance. Email addresses, phone numbers, and symbols follow +PostgreSQL tokenization rather than arbitrary substring matching. Use dedicated +field indexes such as `symbol` for exact identifier comparisons. + +A stored search vector and GIN index are maintained transactionally by database +triggers when entry fields or symbols change, including imports. Migration 018 +backfills existing entries. Deploy with a coordinated restart: the new column +changes the result shape of existing `SELECT *` queries, so older binaries must +not serve against the migrated schema. + +The `name` and `description` CQL indexes match case insensitively by default, +using PostgreSQL `lower()` on both the stored value and the search term. For +example, `name="central library"` matches `Central Library`, and +`description="*LIBRARY*"` matches descriptions containing `library` in any case. +This also applies to `==`, `exact`, and `<>` comparisons. Existing wildcard and +escaping rules apply; no CQL relation modifier is required. Other indexes and +result ordering retain their existing behavior. This does not add full-text +search or accent normalization. + ### Pickup locations Set `illConfig.isPickupLocation` to `true` to offer an institution or branch in the request form's pickup-location selector. This flag is independent of `lmsConfig.requesterPickupLocation`: manual workflows can offer locations without an LMS code. Migration 012 enables the flag for existing entries whose requester pickup code is non-null (including empty strings), preserving other ILL settings. Other entries remain unselected unless explicitly enabled. Imports can set the flag in `illConfig`. diff --git a/directory/api.yaml b/directory/api.yaml index f5b06b723..c06bb0d23 100644 --- a/directory/api.yaml +++ b/directory/api.yaml @@ -164,7 +164,12 @@ paths: parameters: - name: cql in: query - description: CQL query filter. The parentSymbol index matches entries by their immediate parent's symbols. + description: >- + CQL query filter. cql.serverChoice searches entry name, description, + organization ID, email, phone number, and owned symbols using the simple + full-text configuration. The default relation requires all words; + all, any, adj, and trailing word-prefix wildcards are supported. + The parentSymbol index matches entries by their immediate parent's symbols. required: false style: form schema: @@ -251,7 +256,13 @@ paths: parameters: - name: cql in: query - description: CQL query filter applied within the current tenant's owned entries. The parentSymbol index matches entries by their immediate parent's symbols. + description: >- + CQL query filter applied within the current tenant's owned entries. + cql.serverChoice searches entry name, description, organization ID, + email, phone number, and owned symbols using the simple full-text + configuration. The default relation requires all words; all, any, adj, + and trailing word-prefix wildcards are supported. + The parentSymbol index matches entries by their immediate parent's symbols. required: false style: form schema: diff --git a/directory/api/entries.go b/directory/api/entries.go index 7494c57ff..399a27e8a 100644 --- a/directory/api/entries.go +++ b/directory/api/entries.go @@ -202,11 +202,14 @@ const defaultEntryLimit = 10 func handleEntryCQL(cqlString string, noBaseArgs int) (pgcql.Query, error) { def := pgcql.NewPgDefinition() - f := pgcql.NewFieldString().WithLikeOps() + def.AddField("cql.serverChoice", pgcql.NewFieldTsVector(). + WithLanguage("simple").WithServerChoiceRel(cql.ALL).WithColumn("e.search")) + + f := pgcql.NewFieldString().WithLikeOps().WithLower() f.SetColumn("e.name") def.AddField("name", f) - f = pgcql.NewFieldString().WithLikeOps() + f = pgcql.NewFieldString().WithLikeOps().WithLower() f.SetColumn("e.description") def.AddField("description", f) diff --git a/directory/api/entries_test.go b/directory/api/entries_test.go index 65297cc0e..e79bf90d2 100644 --- a/directory/api/entries_test.go +++ b/directory/api/entries_test.go @@ -104,6 +104,112 @@ func TestHandleEntryCQL(t *testing.T) { } } +func TestHandleEntryCQLCaseInsensitive(t *testing.T) { + for _, field := range []string{"name", "description"} { + t.Run(field, func(t *testing.T) { + for _, tc := range []struct { + clause string + op string + term string + }{ + {`="Central Library"`, "=", "Central Library"}, + {`=="Central Library"`, "=", "Central Library"}, + {` exact "Central Library"`, "=", "Central Library"}, + {`<>"Central Library"`, "<>", "Central Library"}, + {`="*Central?Library*"`, "LIKE", "%Central_Library%"}, + {`<>"*Central*"`, "NOT LIKE", "%Central%"}, + {`="Central\*Library"`, "=", "Central*Library"}, + {`="*A\"B\\C*"`, "LIKE", "%A\"B\\\\C%"}, + {`="*Central_100%*"`, "LIKE", "%Central\\_100\\%%"}, + } { + t.Run(tc.clause, func(t *testing.T) { + res, err := handleEntryCQL(field+tc.clause, 0) + if err != nil { + t.Fatalf("translate CQL: %v", err) + } + want := "lower(e." + field + ") " + tc.op + " lower($1)" + if got := res.GetWhereClause(); got != want { + t.Errorf("WHERE clause = %q, want %q", got, want) + } + if got := res.GetQueryArguments(); !reflect.DeepEqual(got, []any{tc.term}) { + t.Errorf("arguments = %#v, want %#v", got, []any{tc.term}) + } + }) + } + }) + } + + t.Run("combined fields with existing arguments", func(t *testing.T) { + res, err := handleEntryCQL(`name="*Central*" OR description="*Library*"`, 1) + if err != nil { + t.Fatalf("translate combined CQL: %v", err) + } + want := "lower(e.name) LIKE lower($2) OR lower(e.description) LIKE lower($3)" + if got := res.GetWhereClause(); got != want { + t.Errorf("WHERE clause = %q, want %q", got, want) + } + if got := res.GetQueryArguments(); !reflect.DeepEqual(got, []any{"%Central%", "%Library%"}) { + t.Errorf("unexpected arguments: %#v", got) + } + }) + + t.Run("type remains case sensitive", func(t *testing.T) { + res, err := handleEntryCQL(`type="*Institution*"`, 0) + if err != nil { + t.Fatalf("translate type CQL: %v", err) + } + if got := res.GetWhereClause(); got != "e.type LIKE $1" { + t.Errorf("unexpected type predicate: %s", got) + } + }) +} + +func TestHandleEntryCQLServerChoice(t *testing.T) { + for _, tc := range []struct { + query string + term string + }{ + {`cql.serverChoice="Central Library"`, "'Central'&'Library'"}, + {`"Central Library"`, "'Central'&'Library'"}, + {`cql.serverChoice all "Central Library"`, "'Central'&'Library'"}, + {`cql.serverChoice any "Central Library"`, "'Central'|'Library'"}, + {`cql.serverChoice adj "Central Library"`, "'Central'<->'Library'"}, + {`cql.serverChoice="Centr*"`, "'Centr':*"}, + {`cql.serverChoice="O'Brien"`, "'O''Brien'"}, + {`cql.serverChoice="A\"B\\C"`, "'A\"B\\C'"}, + } { + t.Run(tc.query, func(t *testing.T) { + res, err := handleEntryCQL(tc.query, 0) + if err != nil { + t.Fatalf("translate serverChoice CQL: %v", err) + } + if got := res.GetWhereClause(); got != "e.search @@ to_tsquery('simple', $1)" { + t.Errorf("unexpected search predicate: %s", got) + } + if got := res.GetQueryArguments(); !reflect.DeepEqual(got, []any{tc.term}) { + t.Errorf("arguments = %#v, want %#v", got, []any{tc.term}) + } + }) + } + t.Run("composes with existing filters and offsets", func(t *testing.T) { + res, err := handleEntryCQL(`cql.serverChoice="Central" AND isPickupLocation=true`, 1) + if err != nil { + t.Fatalf("translate combined CQL: %v", err) + } + if got := res.GetWhereClause(); !strings.HasPrefix(got, "e.search @@ to_tsquery('simple', $2) AND ") || !strings.HasSuffix(got, " = $3") { + t.Errorf("unexpected combined predicate: %s", got) + } + if got := res.GetQueryArguments(); !reflect.DeepEqual(got, []any{"'Central'", true}) { + t.Errorf("unexpected combined arguments: %#v", got) + } + }) + for _, query := range []string{`cql.serverChoice="*central*"`, `cql.serverChoice="centr?"`} { + if _, err := handleEntryCQL(query, 0); err == nil { + t.Errorf("expected unsupported wildcard error for %s", query) + } + } +} + func TestBuildOwnedEntryListQueryScopesCQL(t *testing.T) { cqlQuery := `name="Owned" OR tenant="OTHER"` limit := Limit(5) diff --git a/directory/import/db/repo_test.go b/directory/import/db/repo_test.go index afcda9b99..fefc82b25 100644 --- a/directory/import/db/repo_test.go +++ b/directory/import/db/repo_test.go @@ -526,7 +526,10 @@ func TestImportEntryUsesParentUUIDWhenParentSymbolChangesBeforeRowLock(t *testin }() waitForLockUnavailable(t, ctx, lockUnavailable) - _, err = testPool.Exec(ctx, `UPDATE symbols SET owner=$1 WHERE authority='ISIL' AND symbol='PARENT'`, replacementParentID) + // Symbol changes now lock their owners to refresh the search document. Move + // the symbol in the transaction holding the original parent's lock, before + // allowing the importer to acquire it; the parent UUID must remain stable. + _, err = blocker.Exec(ctx, `UPDATE symbols SET owner=$1 WHERE authority='ISIL' AND symbol='PARENT'`, replacementParentID) require.NoError(t, err) require.NoError(t, blocker.Commit(ctx)) require.NoError(t, <-importDone) diff --git a/directory/migrations/018_entry_search.down.sql b/directory/migrations/018_entry_search.down.sql new file mode 100644 index 000000000..a3981ca5f --- /dev/null +++ b/directory/migrations/018_entry_search.down.sql @@ -0,0 +1,10 @@ +DROP TRIGGER directory_entry_search_symbol_refresh ON symbols; +DROP TRIGGER directory_entry_search_symbol_lock ON symbols; +DROP TRIGGER directory_entry_search_entry ON entries; +DROP FUNCTION directory_entry_search_on_symbol(); +DROP FUNCTION directory_entry_search_lock_symbol_owners(); +DROP FUNCTION directory_entry_search_on_entry(); +DROP FUNCTION directory_entry_search_document(uuid, text, text, text, text, text); +DROP INDEX entries_search_idx; +DROP INDEX symbols_owner_authority_symbol_idx; +ALTER TABLE entries DROP COLUMN search; diff --git a/directory/migrations/018_entry_search.up.sql b/directory/migrations/018_entry_search.up.sql new file mode 100644 index 000000000..153ae7a68 --- /dev/null +++ b/directory/migrations/018_entry_search.up.sql @@ -0,0 +1,88 @@ +ALTER TABLE entries ADD COLUMN search tsvector NOT NULL DEFAULT ''::tsvector; + +-- Index owner lookups before backfill; trailing columns cover symbol aggregation. +CREATE INDEX symbols_owner_authority_symbol_idx ON symbols (owner, authority, symbol); + +-- Keep construction in one place for entry writes, symbol writes, and backfill. +-- VOLATILE ensures symbol reads see changes committed while waiting for an owner lock. +CREATE FUNCTION directory_entry_search_document( + entry_id uuid, entry_name text, entry_description text, + entry_organization_id text, entry_email text, entry_phone_number text +) RETURNS tsvector LANGUAGE sql VOLATILE AS $$ + SELECT to_tsvector('simple'::regconfig, + coalesce(entry_name, '') || ' ' || + coalesce(entry_description, '') || ' ' || + coalesce(entry_organization_id, '') || ' ' || + coalesce(entry_email, '') || ' ' || + coalesce(entry_phone_number, '') || ' ' || + coalesce((SELECT string_agg(authority || ':' || symbol || ' ' || symbol, + ' ' ORDER BY authority, symbol) + FROM symbols WHERE owner = entry_id), '') + ); +$$; + +CREATE FUNCTION directory_entry_search_on_entry() RETURNS trigger LANGUAGE plpgsql AS $$ +BEGIN + NEW.search := directory_entry_search_document( + NEW.id, NEW.name, NEW.description, NEW.organization_id, NEW.email, NEW.phone_number); + RETURN NEW; +END; +$$; + +CREATE TRIGGER directory_entry_search_entry + BEFORE INSERT OR UPDATE OF name, description, organization_id, email, phone_number + ON entries FOR EACH ROW EXECUTE FUNCTION directory_entry_search_on_entry(); + +-- Serialize symbol changes with entry writes before constructing the document. +-- Lock both owners in UUID order when a symbol moves. Deleted owners (including +-- ON DELETE CASCADE) need no refresh. +CREATE FUNCTION directory_entry_search_lock_symbol_owners() RETURNS trigger LANGUAGE plpgsql AS $$ +DECLARE + owners uuid[]; +BEGIN + IF TG_OP = 'INSERT' THEN + owners := ARRAY[NEW.owner]; + ELSIF TG_OP = 'DELETE' THEN + owners := ARRAY[OLD.owner]; + ELSE + owners := ARRAY[OLD.owner, NEW.owner]; + END IF; + PERFORM id FROM entries WHERE id = ANY(owners) ORDER BY id FOR UPDATE; + IF TG_OP = 'DELETE' THEN + RETURN OLD; + END IF; + RETURN NEW; +END; +$$; + +CREATE FUNCTION directory_entry_search_on_symbol() RETURNS trigger LANGUAGE plpgsql AS $$ +DECLARE + owners uuid[]; +BEGIN + IF TG_OP = 'INSERT' THEN + owners := ARRAY[NEW.owner]; + ELSIF TG_OP = 'DELETE' THEN + owners := ARRAY[OLD.owner]; + ELSE + owners := ARRAY[OLD.owner, NEW.owner]; + END IF; + -- Updating search alone does not fire the entry trigger, avoiding recursion. + UPDATE entries e SET search = directory_entry_search_document( + e.id, e.name, e.description, e.organization_id, e.email, e.phone_number) + WHERE e.id = ANY(owners); + RETURN NULL; +END; +$$; + +CREATE TRIGGER directory_entry_search_symbol_lock + BEFORE INSERT OR UPDATE OF owner, authority, symbol OR DELETE ON symbols + FOR EACH ROW EXECUTE FUNCTION directory_entry_search_lock_symbol_owners(); + +CREATE TRIGGER directory_entry_search_symbol_refresh + AFTER INSERT OR UPDATE OF owner, authority, symbol OR DELETE ON symbols + FOR EACH ROW EXECUTE FUNCTION directory_entry_search_on_symbol(); + +UPDATE entries e SET search = directory_entry_search_document( + e.id, e.name, e.description, e.organization_id, e.email, e.phone_number); + +CREATE INDEX entries_search_idx ON entries USING gin(search); diff --git a/directory/test/entry_case_insensitive_test.go b/directory/test/entry_case_insensitive_test.go new file mode 100644 index 000000000..165a11da1 --- /dev/null +++ b/directory/test/entry_case_insensitive_test.go @@ -0,0 +1,75 @@ +package test + +import ( + "context" + "encoding/json" + "net/http" + "net/url" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/require" +) + +func TestEntryCQLCaseInsensitive(t *testing.T) { + resetDb() + ownedID, otherID, decoyID := uuid.New(), uuid.New(), uuid.New() + _, err := dbpool.Exec(context.Background(), ` + INSERT INTO entries (id, name, description, type, tenant) VALUES + ($1, 'Central Library', 'Regional Research Centre', 'Institution', 'case-search'), + ($2, 'Central Library', 'Regional Research Centre', 'Institution', 'case-other'), + ($3, 'Branch Office', NULL, 'Institution', 'case-search') + `, ownedID, otherID, decoyID) + require.NoError(t, err) + + for _, tc := range []struct { + name string + query string + owned bool + ids []uuid.UUID + }{ + {"name equality", `name="cENTRAL lIBRARY"`, false, []uuid.UUID{ownedID, otherID}}, + {"name double equals", `name=="CENTRAL LIBRARY"`, false, []uuid.UUID{ownedID, otherID}}, + {"name exact", `name exact "central library"`, false, []uuid.UUID{ownedID, otherID}}, + {"name substring", `name="*lIbRaRy*"`, false, []uuid.UUID{ownedID, otherID}}, + {"name inequality", `name<>"CENTRAL LIBRARY"`, false, []uuid.UUID{decoyID}}, + {"name wildcard inequality", `name<>"*LIBRARY*"`, false, []uuid.UUID{decoyID}}, + {"description equality", `description="rEGIONAL rESEARCH cENTRE"`, false, []uuid.UUID{ownedID, otherID}}, + {"description double equals", `description=="REGIONAL RESEARCH CENTRE"`, false, []uuid.UUID{ownedID, otherID}}, + {"description exact", `description exact "regional research centre"`, false, []uuid.UUID{ownedID, otherID}}, + {"description substring", `description="*rEsEaRcH*"`, false, []uuid.UUID{ownedID, otherID}}, + {"description inequality excludes NULL", `description<>"UNRELATED"`, false, []uuid.UUID{ownedID, otherID}}, + {"combined fields", `name="*LIBRARY*" AND description="*research*"`, false, []uuid.UUID{ownedID, otherID}}, + {"owned name", `name="*LIBRARY*"`, true, []uuid.UUID{ownedID}}, + {"owned description", `description="*RESEARCH*"`, true, []uuid.UUID{ownedID}}, + {"owned OR stays scoped", `name="*LIBRARY*" OR description="*RESEARCH*"`, true, []uuid.UUID{ownedID}}, + {"type remains case sensitive", `type="institution"`, false, nil}, + } { + t.Run(tc.name, func(t *testing.T) { + endpoint := "/entries" + query := `(tenant="case-search" OR tenant="case-other") AND (` + tc.query + `)` + headers := map[string]string{"X-Okapi-Permissions": `["directory.system.all"]`} + if tc.owned { + endpoint = "/entries/owned" + query = tc.query + headers = map[string]string{ + "X-Okapi-Tenant": "case-search", + "X-Okapi-Permissions": `["directory.institution.all"]`, + } + } + response, body := jsonReq(t, http.MethodGet, endpoint+"?cql="+url.QueryEscape(query), "", headers) + require.Equal(t, http.StatusOK, response.StatusCode, body) + var result struct { + Items []struct{ ID uuid.UUID } `json:"items"` + About struct{ Count int } `json:"about"` + } + require.NoError(t, json.Unmarshal([]byte(body), &result)) + require.Equal(t, len(tc.ids), result.About.Count) + var actual []uuid.UUID + for _, item := range result.Items { + actual = append(actual, item.ID) + } + require.ElementsMatch(t, tc.ids, actual) + }) + } +} diff --git a/directory/test/entry_search_migration_test.go b/directory/test/entry_search_migration_test.go new file mode 100644 index 000000000..055f344e3 --- /dev/null +++ b/directory/test/entry_search_migration_test.go @@ -0,0 +1,72 @@ +package test + +import ( + "context" + "fmt" + "os" + "path/filepath" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/require" +) + +func TestEntrySearchMigration(t *testing.T) { + ctx := context.Background() + tx, err := dbpool.Begin(ctx) + require.NoError(t, err) + defer func() { _ = tx.Rollback(ctx) }() + exec := func(query string, args ...any) { + t.Helper() + _, err := tx.Exec(ctx, query, args...) + require.NoError(t, err) + } + readMigration := func(version int, direction string) string { + t.Helper() + paths, err := filepath.Glob(fmt.Sprintf("../migrations/%03d_*.%s.sql", version, direction)) + require.NoError(t, err) + require.Len(t, paths, 1) + data, err := os.ReadFile(paths[0]) + require.NoError(t, err) + return string(data) + } + exec("CREATE SCHEMA entry_search_migration_test; SET LOCAL search_path TO entry_search_migration_test") + for version := 1; version <= 17; version++ { + exec(readMigration(version, "up")) + } + id, nullID := uuid.New(), uuid.New() + exec(`INSERT INTO entries (id, name, type, description, organization_id, email, phone_number) + VALUES ($1, 'MigrationName', 'Institution', 'MigrationDescription', 'MigrationOrg', 'migration@example.test', '5551112222'), + ($2, 'NullableName', 'Institution', NULL, NULL, NULL, NULL)`, id, nullID) + exec(`INSERT INTO symbols (owner, authority, symbol) VALUES ($1, 'MIGRATIONAUTH', 'MIGRATIONSYMBOL')`, id) + for range 2 { + exec(readMigration(18, "up")) + var matches bool + require.NoError(t, tx.QueryRow(ctx, `SELECT search @@ plainto_tsquery('simple', + 'MigrationName MigrationDescription MigrationOrg migration@example.test 5551112222 MIGRATIONAUTH:MIGRATIONSYMBOL MIGRATIONSYMBOL') + FROM entries WHERE id=$1`, id).Scan(&matches)) + require.True(t, matches, "migration must backfill every search field and symbol") + require.NoError(t, tx.QueryRow(ctx, `SELECT search @@ to_tsquery('simple', 'nullablename') FROM entries WHERE id=$1`, nullID).Scan(&matches)) + require.True(t, matches) + var indexDef string + require.NoError(t, tx.QueryRow(ctx, `SELECT indexdef FROM pg_indexes WHERE schemaname='entry_search_migration_test' AND indexname='entries_search_idx'`).Scan(&indexDef)) + require.Contains(t, indexDef, "USING gin (search)") + require.NoError(t, tx.QueryRow(ctx, `SELECT indexdef FROM pg_indexes WHERE schemaname='entry_search_migration_test' AND indexname='symbols_owner_authority_symbol_idx'`).Scan(&indexDef)) + require.Contains(t, indexDef, "USING btree (owner, authority, symbol)") + + // Verify the triggers belong to this schema and remain usable after reapply. + exec(`UPDATE entries SET description='UpdatedMigration' WHERE id=$1`, nullID) + exec(`INSERT INTO symbols (owner, authority, symbol) VALUES ($1, 'REAPPLY', 'MIGRATION') ON CONFLICT DO NOTHING`, nullID) + require.NoError(t, tx.QueryRow(ctx, `SELECT search @@ plainto_tsquery('simple', 'updatedmigration reapply:migration') FROM entries WHERE id=$1`, nullID).Scan(&matches)) + require.True(t, matches) + exec(readMigration(18, "down")) + var columns int + require.NoError(t, tx.QueryRow(ctx, `SELECT count(*) FROM information_schema.columns WHERE table_schema='entry_search_migration_test' AND table_name='entries' AND column_name='search'`).Scan(&columns)) + require.Zero(t, columns) + var indexes int + require.NoError(t, tx.QueryRow(ctx, `SELECT count(*) FROM pg_indexes WHERE schemaname='entry_search_migration_test' AND indexname='symbols_owner_authority_symbol_idx'`).Scan(&indexes)) + require.Zero(t, indexes) + exec(`UPDATE entries SET description=NULL WHERE id=$1`, nullID) + exec(`DELETE FROM symbols WHERE owner=$1`, nullID) + } +} diff --git a/directory/test/entry_search_test.go b/directory/test/entry_search_test.go new file mode 100644 index 000000000..6258ce2a5 --- /dev/null +++ b/directory/test/entry_search_test.go @@ -0,0 +1,209 @@ +package test + +import ( + "context" + "encoding/json" + "net/http" + "net/url" + "testing" + "time" + + "github.com/google/uuid" + "github.com/stretchr/testify/require" +) + +func searchDirectoryEntries(t *testing.T, endpoint, query string, headers map[string]string) ([]uuid.UUID, int) { + t.Helper() + response, body := jsonReq(t, http.MethodGet, endpoint+"&cql="+url.QueryEscape(query), "", headers) + require.Equal(t, http.StatusOK, response.StatusCode, body) + var result struct { + Items []struct{ ID uuid.UUID } `json:"items"` + About struct{ Count int } `json:"about"` + } + require.NoError(t, json.Unmarshal([]byte(body), &result)) + var ids []uuid.UUID + for _, entry := range result.Items { + ids = append(ids, entry.ID) + } + return ids, result.About.Count +} + +func requireDirectorySearch(t *testing.T, query string, want ...uuid.UUID) { + t.Helper() + ids, count := searchDirectoryEntries(t, "/entries?limit=100", query, + map[string]string{"X-Okapi-Permissions": `["directory.system.all"]`}) + require.Equal(t, len(want), count) + require.ElementsMatch(t, want, ids) +} + +func TestEntryServerChoice(t *testing.T) { + resetDb() + ctx := context.Background() + id, otherID := uuid.New(), uuid.New() + _, err := dbpool.Exec(ctx, `INSERT INTO entries + (id, name, description, organization_id, email, phone_number, type, tenant, contact_name) + VALUES ($1, 'The Atlas Library', 'Regional research', 'ORG123', 'desk@example.test', + '5551234567', 'Institution', 'ANINST', 'NeverIndexed'), + ($2, 'Z Atlas', NULL, NULL, NULL, NULL, 'Institution', 'OTHER', NULL)`, id, otherID) + require.NoError(t, err) + _, err = dbpool.Exec(ctx, `INSERT INTO symbols (owner, authority, symbol) VALUES ($1, 'ISIL', 'US-ATLAS')`, id) + require.NoError(t, err) + + for _, tc := range []struct { + query string + ids []uuid.UUID + }{ + {`cql.serverChoice="ATLAS"`, []uuid.UUID{id, otherID}}, + {`cql.serverChoice="research"`, []uuid.UUID{id}}, + {`cql.serverChoice="org123"`, []uuid.UUID{id}}, + {`cql.serverChoice="DESK@EXAMPLE.TEST"`, []uuid.UUID{id}}, + {`cql.serverChoice="5551234567"`, []uuid.UUID{id}}, + {`cql.serverChoice="isil:us-atlas"`, []uuid.UUID{id}}, + {`cql.serverChoice="us-atlas"`, []uuid.UUID{id}}, + {`cql.serverChoice="atlas research"`, []uuid.UUID{id}}, + {`cql.serverChoice all "atlas research"`, []uuid.UUID{id}}, + {`cql.serverChoice any "research absentword"`, []uuid.UUID{id}}, + {`cql.serverChoice adj "atlas library"`, []uuid.UUID{id}}, + {`cql.serverChoice adj "library atlas"`, nil}, + {`cql.serverChoice="resear*"`, []uuid.UUID{id}}, + {`cql.serverChoice="the"`, []uuid.UUID{id}}, + {`cql.serverChoice="libraries"`, nil}, + {`cql.serverChoice="NeverIndexed"`, nil}, + {`cql.serverChoice="atlas" AND tenant="OTHER"`, []uuid.UUID{otherID}}, + } { + t.Run(tc.query, func(t *testing.T) { + requireDirectorySearch(t, tc.query, tc.ids...) + }) + } + + headers := map[string]string{"X-Okapi-Permissions": `["directory.system.all"]`} + ids, count := searchDirectoryEntries(t, "/entries?limit=1&offset=1", `cql.serverChoice="atlas"`, headers) + require.Equal(t, 2, count) + require.Equal(t, []uuid.UUID{otherID}, ids) + ids, count = searchDirectoryEntries(t, "/entries/owned?limit=100", + `cql.serverChoice="atlas" OR tenant="OTHER"`, map[string]string{ + "X-Okapi-Tenant": "ANINST", "X-Okapi-Permissions": `["directory.institution.all"]`, + }) + require.Equal(t, 1, count) + require.Equal(t, []uuid.UUID{id}, ids) +} + +func TestEntryServerChoiceAPIUpdates(t *testing.T) { + resetDb() + id := uuid.MustParse("00000000-0000-0000-0000-000000000002") + path := "/entries/by-id/" + id.String() + headers := map[string]string{"X-Okapi-Tenant": "ANINST", "X-Okapi-Permissions": `["directory.consortium.all"]`} + for _, tc := range []struct { + body string + term string + }{ + {`{"name":"SearchName"}`, "searchname"}, + {`{"description":"SearchDescription"}`, "searchdescription"}, + {`{"organizationId":"SearchOrganization"}`, "searchorganization"}, + {`{"email":"SearchEmail@example.test"}`, "searchemail@example.test"}, + {`{"phoneNumber":"5559876543"}`, "5559876543"}, + {`{"symbols":[{"authority":"SEARCHAUTH","symbol":"SEARCHSYMBOL"}]}`, "searchauth:searchsymbol"}, + } { + response, body := jsonReq(t, http.MethodPatch, path, tc.body, headers) + require.Equal(t, http.StatusNoContent, response.StatusCode, body) + requireDirectorySearch(t, `cql.serverChoice="`+tc.term+`"`, id) + } + response, body := jsonReq(t, http.MethodPatch, path, `{"description":null,"symbols":[]}`, headers) + require.Equal(t, http.StatusNoContent, response.StatusCode, body) + requireDirectorySearch(t, `cql.serverChoice any "searchdescription searchsymbol"`) + response, body = jsonReq(t, http.MethodPatch, path, `{"name":"ReplacementName","symbols":[{"authority":"SEARCH","symbol":"CASCADESEARCH"}]}`, headers) + require.Equal(t, http.StatusNoContent, response.StatusCode, body) + requireDirectorySearch(t, `cql.serverChoice="searchname"`) + requireDirectorySearch(t, `cql.serverChoice="replacementname cascadesearch"`, id) + response, body = jsonReq(t, http.MethodDelete, path, "", headers) + require.Equal(t, http.StatusNoContent, response.StatusCode, body) + requireDirectorySearch(t, `cql.serverChoice any "replacementname cascadesearch"`) +} + +func TestEntryServerChoiceImportUpdates(t *testing.T) { + resetImportState(t) + key := symbolObject("ISIL", "SEARCHIMPORT") + record := entryImportRecord(key, "ImportedSearch", nil, "Consortium") + response, result := importRequest(t, []any{record}, "fail", standardHeaders) + require.Equal(t, http.StatusOK, response.StatusCode) + require.Empty(t, result.Errors) + id := importEntryUUID(key) + requireDirectorySearch(t, `cql.serverChoice="importedsearch searchimport"`, id) + data := record["data"].(map[string]any) + data["name"] = "ReplacedSearch" + data["symbols"] = []any{symbolObject("ISIL", "NEWSEARCHIMPORT")} + response, result = importRequest(t, []any{record}, "update", standardHeaders) + require.Equal(t, http.StatusOK, response.StatusCode) + require.Empty(t, result.Errors) + requireDirectorySearch(t, `cql.serverChoice="replacedsearch newsearchimport"`, id) + requireDirectorySearch(t, `cql.serverChoice any "importedsearch searchimport"`) +} + +func TestEntryServerChoiceSymbolChanges(t *testing.T) { + resetDb() + ctx := context.Background() + owner := uuid.MustParse("00000000-0000-0000-0000-000000000001") + other := uuid.MustParse("00000000-0000-0000-0000-000000000003") + symbolID := uuid.New() + _, err := dbpool.Exec(ctx, `INSERT INTO symbols (id, owner, authority, symbol) VALUES ($1, $2, 'OLDSEARCHAUTH', 'MOVINGSEARCH')`, symbolID, owner) + require.NoError(t, err) + requireDirectorySearch(t, `cql.serverChoice="oldsearchauth:movingsearch"`, owner) + _, err = dbpool.Exec(ctx, `UPDATE symbols SET owner=$1, authority='NEWSEARCHAUTH', symbol='MOVEDSEARCH' WHERE id=$2`, other, symbolID) + require.NoError(t, err) + requireDirectorySearch(t, `cql.serverChoice any "oldsearchauth movingsearch"`) + requireDirectorySearch(t, `cql.serverChoice="newsearchauth:movedsearch"`, other) + + tx, err := dbpool.Begin(ctx) + require.NoError(t, err) + defer func() { _ = tx.Rollback(ctx) }() + _, err = tx.Exec(ctx, `UPDATE entries SET name='RolledBackSearch' WHERE id=$1`, other) + require.NoError(t, err) + _, err = tx.Exec(ctx, `DELETE FROM symbols WHERE id=$1`, symbolID) + require.NoError(t, err) + require.NoError(t, tx.Rollback(ctx)) + requireDirectorySearch(t, `cql.serverChoice="rolledbacksearch"`) + requireDirectorySearch(t, `cql.serverChoice="movedsearch"`, other) + _, err = dbpool.Exec(ctx, `DELETE FROM symbols WHERE id=$1`, symbolID) + require.NoError(t, err) + requireDirectorySearch(t, `cql.serverChoice="movedsearch"`) +} + +func TestEntryServerChoiceConcurrentUpdates(t *testing.T) { + resetDb() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + owner := uuid.MustParse("00000000-0000-0000-0000-000000000002") + tx, err := dbpool.Begin(ctx) + require.NoError(t, err) + defer func() { _ = tx.Rollback(ctx) }() + _, err = tx.Exec(ctx, `INSERT INTO symbols (owner, authority, symbol) VALUES ($1, 'SEARCH', 'FIRSTCONCURRENT')`, owner) + require.NoError(t, err) + // Both writes start before the first transaction commits. They must rebuild + // from the latest committed symbols after acquiring the entry lock. + symbolConn, err := dbpool.Acquire(ctx) + require.NoError(t, err) + defer symbolConn.Release() + entryConn, err := dbpool.Acquire(ctx) + require.NoError(t, err) + defer entryConn.Release() + finished := make(chan error, 2) + go func() { + _, err := symbolConn.Exec(ctx, `INSERT INTO symbols (owner, authority, symbol) VALUES ($1, 'SEARCH', 'SECONDCONCURRENT')`, owner) + finished <- err + }() + go func() { + _, err := entryConn.Exec(ctx, `UPDATE entries SET description='ConcurrentDescription' WHERE id=$1`, owner) + finished <- err + }() + pids := []int32{int32(symbolConn.Conn().PgConn().PID()), int32(entryConn.Conn().PgConn().PID())} + require.Eventually(t, func() bool { + var waiting int + err := dbpool.QueryRow(ctx, `SELECT count(*) FROM pg_stat_activity WHERE pid=ANY($1) AND wait_event_type='Lock'`, pids).Scan(&waiting) + return err == nil && waiting == 2 + }, 5*time.Second, 10*time.Millisecond, "both writers must wait for the entry lock") + require.NoError(t, tx.Commit(ctx)) + for range 2 { + require.NoError(t, <-finished) + } + requireDirectorySearch(t, `cql.serverChoice="firstconcurrent secondconcurrent concurrentdescription"`, owner) +} From fb69667c94ceffcc63a100dc0a28ae403e1e1d9d Mon Sep 17 00:00:00 2001 From: Kurt Nordstrom Date: Fri, 9 Oct 2026 17:00:20 -0400 Subject: [PATCH 2/4] Address potential deadlock issue. Update README to clarify queries with pre-018 binaries --- directory/README.md | 18 +++++++++++++--- directory/api.yaml | 2 +- directory/api/entries.go | 23 +++++++++++++------- directory/import/db/entry.go | 2 +- directory/query.sql | 13 ++++++------ directory/test/entry_search_test.go | 33 +++++++++++++++++++---------- 6 files changed, 61 insertions(+), 30 deletions(-) diff --git a/directory/README.md b/directory/README.md index d60db1dd5..12c6f1ec3 100644 --- a/directory/README.md +++ b/directory/README.md @@ -252,6 +252,17 @@ Run `make generate` before invoking `go build` or `go test` directly. See [Host LMS and catalog profiles](host-profiles.md) for `lmsConfig.vendor`, `catalogConfig.profile`, precedence, parser overrides, diagnostics and migration. +## Directory entry symbols + +When patching entry symbols, omit `id` to create a new symbol. A supplied `id` +must identify an existing symbol owned by the entry being patched; unknown or +foreign IDs return HTTP 400 and roll back the entire PATCH. Symbols cannot be +moved between entries through the API. + +Application writes lock the owner entry before modifying its symbols. Direct +SQL maintenance must follow the same order; the search trigger alone cannot +prevent deadlocks caused by acquiring these locks in the opposite order. + ## Directory entry searches `cql.serverChoice` searches a combined document containing the entry's name, @@ -277,9 +288,10 @@ field indexes such as `symbol` for exact identifier comparisons. A stored search vector and GIN index are maintained transactionally by database triggers when entry fields or symbols change, including imports. Migration 018 -backfills existing entries. Deploy with a coordinated restart: the new column -changes the result shape of existing `SELECT *` queries, so older binaries must -not serve against the migrated schema. +backfills existing entries. SQLC expands wildcard selections into explicit column +lists, so adding the search column preserves the result shape expected by +pre-018 binaries. Existing binaries can continue serving against the upgraded +schema. The migration takes database locks and may temporarily block requests. The `name` and `description` CQL indexes match case insensitively by default, using PostgreSQL `lower()` on both the stored value and the search term. For diff --git a/directory/api.yaml b/directory/api.yaml index c06bb0d23..9a6e499fb 100644 --- a/directory/api.yaml +++ b/directory/api.yaml @@ -3079,7 +3079,7 @@ components: id: type: string format: uuid - description: Unique id + description: Existing symbol ID owned by the entry being patched. Omit to create a new symbol. Unknown or foreign IDs return HTTP 400; symbols cannot be moved between entries. symbol: type: string minLength: 1 diff --git a/directory/api/entries.go b/directory/api/entries.go index 399a27e8a..e43183bbf 100644 --- a/directory/api/entries.go +++ b/directory/api/entries.go @@ -788,7 +788,7 @@ func (a ApiImpl) AddEntry(ctx context.Context, request AddEntryRequestObject) (A if request.Body.Symbols != nil { for _, symbol := range *request.Body.Symbols { - _, err = qtx.UpsertSymbol(ctx, db.UpsertSymbolParams{ + _, err = qtx.CreateSymbol(ctx, db.CreateSymbolParams{ Owner: insertedEntry.ID, Symbol: strings.ToUpper(symbol.Symbol), Authority: strings.ToUpper(symbol.Authority), @@ -1085,14 +1085,21 @@ func (a ApiImpl) UpdateEntry(ctx context.Context, request UpdateEntryRequestObje } } - // Update/create symbols + // The entry is already locked. Restrict updates to its own symbols so + // the search trigger never needs to lock another entry after a symbol. for _, symbol := range reqsyms { - _, err = qtx.UpsertSymbol(ctx, db.UpsertSymbolParams{ - ID: symbol.Id, - Owner: orig.ID, - Symbol: strings.ToUpper(symbol.Symbol), - Authority: strings.ToUpper(symbol.Authority), - }) + if symbol.Id == nil { + _, err = qtx.CreateSymbol(ctx, db.CreateSymbolParams{ + Owner: orig.ID, Symbol: strings.ToUpper(symbol.Symbol), Authority: strings.ToUpper(symbol.Authority), + }) + } else { + _, err = qtx.UpdateOwnedSymbol(ctx, db.UpdateOwnedSymbolParams{ + ID: *symbol.Id, Owner: orig.ID, Symbol: strings.ToUpper(symbol.Symbol), Authority: strings.ToUpper(symbol.Authority), + }) + if errors.Is(err, pgx.ErrNoRows) { + return UpdateEntry400TextResponse("Symbol ID does not belong to this entry"), nil + } + } if err != nil { var pge *pgconn.PgError if errors.As(err, &pge) { diff --git a/directory/import/db/entry.go b/directory/import/db/entry.go index 5df68d3d0..b7cd2ed9b 100644 --- a/directory/import/db/entry.go +++ b/directory/import/db/entry.go @@ -340,7 +340,7 @@ func replaceEntryChildren(ctx context.Context, queries *db.Queries, entryID uuid return err } for _, symbol := range data.Symbols { - if _, err := queries.UpsertSymbol(ctx, db.UpsertSymbolParams{Owner: entryID, Authority: symbol.Authority, Symbol: symbol.Symbol}); err != nil { + if _, err := queries.CreateSymbol(ctx, db.CreateSymbolParams{Owner: entryID, Authority: symbol.Authority, Symbol: symbol.Symbol}); err != nil { return err } } diff --git a/directory/query.sql b/directory/query.sql index 46bf34dfa..4c5642b1d 100644 --- a/directory/query.sql +++ b/directory/query.sql @@ -84,20 +84,21 @@ WHERE entries.id = symbols.owner AND symbols.authority = @authority AND symbols.symbol = @symbol; --- name: UpsertSymbol :one +-- name: CreateSymbol :one INSERT INTO symbols ( - id, owner, symbol, authority + owner, symbol, authority ) VALUES ( - coalesce(sqlc.narg('id'), gen_random_uuid()), @owner, @symbol, @authority ) -ON CONFLICT (id) DO UPDATE SET - owner = @owner, +RETURNING *; + +-- name: UpdateOwnedSymbol :one +UPDATE symbols SET symbol = @symbol, authority = @authority -WHERE symbols.id = sqlc.narg('id') +WHERE id = @id AND owner = @owner RETURNING *; -- name: DeleteOtherOwnedSymbols :exec diff --git a/directory/test/entry_search_test.go b/directory/test/entry_search_test.go index 6258ce2a5..8e73f07e0 100644 --- a/directory/test/entry_search_test.go +++ b/directory/test/entry_search_test.go @@ -143,29 +143,40 @@ func TestEntryServerChoiceSymbolChanges(t *testing.T) { resetDb() ctx := context.Background() owner := uuid.MustParse("00000000-0000-0000-0000-000000000001") - other := uuid.MustParse("00000000-0000-0000-0000-000000000003") symbolID := uuid.New() - _, err := dbpool.Exec(ctx, `INSERT INTO symbols (id, owner, authority, symbol) VALUES ($1, $2, 'OLDSEARCHAUTH', 'MOVINGSEARCH')`, symbolID, owner) + _, err := dbpool.Exec(ctx, `INSERT INTO symbols (id, owner, authority, symbol) VALUES ($1, $2, 'OLDSEARCHAUTH', 'ORIGINALSEARCH')`, symbolID, owner) require.NoError(t, err) - requireDirectorySearch(t, `cql.serverChoice="oldsearchauth:movingsearch"`, owner) - _, err = dbpool.Exec(ctx, `UPDATE symbols SET owner=$1, authority='NEWSEARCHAUTH', symbol='MOVEDSEARCH' WHERE id=$2`, other, symbolID) + requireDirectorySearch(t, `cql.serverChoice="oldsearchauth:originalsearch"`, owner) + tx, err := dbpool.Begin(ctx) + require.NoError(t, err) + defer func() { _ = tx.Rollback(ctx) }() + _, err = tx.Exec(ctx, `SELECT id FROM entries WHERE id=$1 FOR UPDATE`, owner) require.NoError(t, err) - requireDirectorySearch(t, `cql.serverChoice any "oldsearchauth movingsearch"`) - requireDirectorySearch(t, `cql.serverChoice="newsearchauth:movedsearch"`, other) + _, err = tx.Exec(ctx, `UPDATE symbols SET authority='NEWSEARCHAUTH', symbol='UPDATEDSEARCH' WHERE id=$1`, symbolID) + require.NoError(t, err) + require.NoError(t, tx.Commit(ctx)) + requireDirectorySearch(t, `cql.serverChoice any "oldsearchauth originalsearch"`) + requireDirectorySearch(t, `cql.serverChoice="newsearchauth:updatedsearch"`, owner) - tx, err := dbpool.Begin(ctx) + tx, err = dbpool.Begin(ctx) require.NoError(t, err) defer func() { _ = tx.Rollback(ctx) }() - _, err = tx.Exec(ctx, `UPDATE entries SET name='RolledBackSearch' WHERE id=$1`, other) + _, err = tx.Exec(ctx, `UPDATE entries SET name='RolledBackSearch' WHERE id=$1`, owner) require.NoError(t, err) _, err = tx.Exec(ctx, `DELETE FROM symbols WHERE id=$1`, symbolID) require.NoError(t, err) require.NoError(t, tx.Rollback(ctx)) requireDirectorySearch(t, `cql.serverChoice="rolledbacksearch"`) - requireDirectorySearch(t, `cql.serverChoice="movedsearch"`, other) - _, err = dbpool.Exec(ctx, `DELETE FROM symbols WHERE id=$1`, symbolID) + requireDirectorySearch(t, `cql.serverChoice="updatedsearch"`, owner) + tx, err = dbpool.Begin(ctx) require.NoError(t, err) - requireDirectorySearch(t, `cql.serverChoice="movedsearch"`) + defer func() { _ = tx.Rollback(ctx) }() + _, err = tx.Exec(ctx, `SELECT id FROM entries WHERE id=$1 FOR UPDATE`, owner) + require.NoError(t, err) + _, err = tx.Exec(ctx, `DELETE FROM symbols WHERE id=$1`, symbolID) + require.NoError(t, err) + require.NoError(t, tx.Commit(ctx)) + requireDirectorySearch(t, `cql.serverChoice="updatedsearch"`) } func TestEntryServerChoiceConcurrentUpdates(t *testing.T) { From d44bde280f985662a34ce8ad08af59eeb2edcbfc Mon Sep 17 00:00:00 2001 From: Kurt Nordstrom Date: Fri, 9 Oct 2026 17:00:35 -0400 Subject: [PATCH 3/4] Add symbols test --- directory/test/entry_symbols_test.go | 121 +++++++++++++++++++++++++++ 1 file changed, 121 insertions(+) create mode 100644 directory/test/entry_symbols_test.go diff --git a/directory/test/entry_symbols_test.go b/directory/test/entry_symbols_test.go new file mode 100644 index 000000000..8008c4b20 --- /dev/null +++ b/directory/test/entry_symbols_test.go @@ -0,0 +1,121 @@ +package test + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/google/uuid" + "github.com/indexdata/crosslink/directory/db" + "github.com/stretchr/testify/require" +) + +func TestEntrySymbolOwnership(t *testing.T) { + for _, key := range []string{"by-id/00000000-0000-0000-0000-000000000002", "by-symbol/TEST:ANINST"} { + for _, invalidID := range []struct { + name string + id string + }{ + {name: "foreign", id: "60000000-0000-0000-0000-000000000003"}, + {name: "missing", id: uuid.NewString()}, + } { + t.Run(key+"/"+invalidID.name, func(t *testing.T) { + resetDb() + ctx := context.Background() + path := "/entries/" + key + ownerPath := "/entries/by-id/00000000-0000-0000-0000-000000000002" + otherPath := "/entries/by-id/00000000-0000-0000-0000-000000000004" + _, beforeOwner := jsonReq(t, http.MethodGet, ownerPath, "", standardHeaders) + _, beforeOther := jsonReq(t, http.MethodGet, otherPath, "", standardHeaders) + var beforeSearch, afterSearch string + require.NoError(t, dbpool.QueryRow(ctx, `SELECT search::text FROM entries WHERE id='00000000-0000-0000-0000-000000000002'`).Scan(&beforeSearch)) + + // Earlier entry, deletion, update, and insertion changes must all + // roll back when a later symbol ID is rejected. + body := fmt.Sprintf(`{"name":"RejectedName","symbols":[ + {"id":"60000000-0000-0000-0000-000000000001","authority":"UPDATED","symbol":"RENAMED"}, + {"authority":"NEW","symbol":"CREATED"}, + {"id":%q,"authority":"REJECTED","symbol":"STOLEN"}]}`, invalidID.id) + response, data := jsonReq(t, http.MethodPatch, path, body, standardHeaders) + require.Equal(t, http.StatusBadRequest, response.StatusCode, data) + require.Contains(t, data, "Symbol ID does not belong to this entry") + _, afterOwner := jsonReq(t, http.MethodGet, ownerPath, "", standardHeaders) + _, afterOther := jsonReq(t, http.MethodGet, otherPath, "", standardHeaders) + require.JSONEq(t, beforeOwner, afterOwner) + require.JSONEq(t, beforeOther, afterOther) + require.NoError(t, dbpool.QueryRow(ctx, `SELECT search::text FROM entries WHERE id='00000000-0000-0000-0000-000000000002'`).Scan(&afterSearch)) + require.Equal(t, beforeSearch, afterSearch) + var count int + require.NoError(t, dbpool.QueryRow(ctx, `SELECT count(*) FROM symbols WHERE id=$1`, invalidID.id).Scan(&count)) + if invalidID.name == "missing" { + require.Zero(t, count, "a supplied unknown ID must not create a symbol") + } else { + require.Equal(t, 1, count) + } + }) + } + } + + t.Run("update owned symbol and create without ID", func(t *testing.T) { + resetDb() + owner := uuid.MustParse("00000000-0000-0000-0000-000000000002") + id := uuid.MustParse("60000000-0000-0000-0000-000000000001") + response, body := jsonReq(t, http.MethodPatch, "/entries/by-id/"+owner.String(), `{"symbols":[ + {"id":"60000000-0000-0000-0000-000000000001","authority":"updated","symbol":"renamed"}, + {"authority":"new","symbol":"created"}]}`, standardHeaders) + require.Equal(t, http.StatusNoContent, response.StatusCode, body) + var count int + require.NoError(t, dbpool.QueryRow(context.Background(), `SELECT count(*) FROM symbols WHERE owner=$1`, owner).Scan(&count)) + require.Equal(t, 2, count) + require.NoError(t, dbpool.QueryRow(context.Background(), `SELECT count(*) FROM symbols WHERE id=$1 AND owner=$2 AND authority='UPDATED' AND symbol='RENAMED'`, id, owner).Scan(&count)) + require.Equal(t, 1, count, "updating a symbol must preserve its ID and owner") + requireDirectorySearch(t, `cql.serverChoice="updated:renamed new:created"`, owner) + requireDirectorySearch(t, `cql.serverChoice="test:aninst"`) + }) +} + +func TestForeignSymbolPatchDoesNotWaitForOwnerLock(t *testing.T) { + resetDb() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + owner := uuid.MustParse("00000000-0000-0000-0000-000000000002") + targetPath := "/entries/by-id/00000000-0000-0000-0000-000000000003" + _, beforeTarget := jsonReq(t, http.MethodGet, targetPath, "", standardHeaders) + tx, err := dbpool.Begin(ctx) + require.NoError(t, err) + defer func() { _ = tx.Rollback(ctx) }() + _, err = db.New(tx).EntryByIdForUpdate(ctx, owner) + require.NoError(t, err) + + // A PATCH attempting to take this owner's symbol used to lock the symbol + // and then wait for this entry, deadlocking if this transaction deleted it. + request := httptest.NewRequest(http.MethodPatch, "/directory"+targetPath, + strings.NewReader(`{"symbols":[{"id":"60000000-0000-0000-0000-000000000001","authority":"TEST","symbol":"STOLEN"}]}`)).WithContext(ctx) + request.Header.Set("Content-Type", "application/json") + for key, value := range standardHeaders { + request.Header.Set(key, value) + } + done := make(chan *httptest.ResponseRecorder, 1) + go func() { + recorder := httptest.NewRecorder() + handler.ServeHTTP(recorder, request) + done <- recorder + }() + select { + case response := <-done: + require.Equal(t, http.StatusBadRequest, response.Code, response.Body.String()) + case <-time.After(2 * time.Second): + t.Fatal("foreign symbol PATCH waited for the owner lock") + } + + // The legitimate owner can still modify its symbols and commit. + require.NoError(t, db.New(tx).DeleteAllOwnedSymbols(ctx, owner)) + require.NoError(t, tx.Commit(ctx)) + _, afterTarget := jsonReq(t, http.MethodGet, targetPath, "", standardHeaders) + require.JSONEq(t, beforeTarget, afterTarget) + requireDirectorySearch(t, `cql.serverChoice any "test:aninst stolen"`) +} From 731c4d627c27a846085cf21c91cf4ba3f73d26df Mon Sep 17 00:00:00 2001 From: Kurt Nordstrom Date: Fri, 9 Oct 2026 20:32:54 -0400 Subject: [PATCH 4/4] Add more detail to readme regarding migration 018 --- directory/README.md | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/directory/README.md b/directory/README.md index 12c6f1ec3..d1859fc17 100644 --- a/directory/README.md +++ b/directory/README.md @@ -291,7 +291,26 @@ triggers when entry fields or symbols change, including imports. Migration 018 backfills existing entries. SQLC expands wildcard selections into explicit column lists, so adding the search column preserves the result shape expected by pre-018 binaries. Existing binaries can continue serving against the upgraded -schema. The migration takes database locks and may temporarily block requests. +schema after the migration completes. + +Migration 018 runs as one transaction: it adds the search column, creates the +symbols ownership index before backfill, installs the search triggers, backfills +all entries, and builds the GIN index. The initial schema change takes an +exclusive lock on `entries` that is held until the transaction completes, +blocking entry reads and writes throughout the backfill and index builds. The +regular symbols index build also blocks symbol writes until commit. Migration +duration depends on data volume and active transactions; requests may time out +while waiting. Concurrent transactions that acquire locks in the opposite order +can also deadlock with the migration. + +Plan a maintenance window for this upgrade. Stop all Directory instances and +import jobs, pause other database writers, and allow active transactions to +finish before starting one upgraded instance to apply migrations. Once migration +018 succeeds and that instance is ready, restore the remaining instances and +resume traffic and imports. A rolling update alone does not enforce this +sequence. If migration fails, inspect the error and migration version/dirty state +before retrying; a failed transaction rolls back its schema changes, but the +migration runner may leave the version marked dirty and prevent startup. The `name` and `description` CQL indexes match case insensitively by default, using PostgreSQL `lower()` on both the stored value and the search term. For