diff --git a/directory/README.md b/directory/README.md index bf7c2b8b0..d1859fc17 100644 --- a/directory/README.md +++ b/directory/README.md @@ -252,6 +252,75 @@ 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, +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. 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 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 +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..9a6e499fb 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: @@ -3068,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 7494c57ff..e43183bbf 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) @@ -785,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), @@ -1082,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/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/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/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/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_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..8e73f07e0 --- /dev/null +++ b/directory/test/entry_search_test.go @@ -0,0 +1,220 @@ +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") + symbolID := uuid.New() + _, 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: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) + _, 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) + require.NoError(t, err) + defer func() { _ = tx.Rollback(ctx) }() + _, 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="updatedsearch"`, 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) + _, 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) { + 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) +} 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"`) +}