From 86b067a1b23ac8f73bfd27dc6faf09612ac40787 Mon Sep 17 00:00:00 2001 From: Kurt Nordstrom Date: Fri, 13 Nov 2026 16:47:09 -0500 Subject: [PATCH 1/4] Normalize prefixes from mod-rs with hyphens, make sure hrid count stays above highest imported value --- broker/import/db/repo.go | 30 +++++++++++++++++++ .../import/test/export_hrid_preflight_test.go | 17 +++++++---- broker/import/test/patron_request_test.go | 29 ++++++++++++++++++ broker/migrations/080_advance_hrid.down.sql | 1 + broker/migrations/080_advance_hrid.up.sql | 16 ++++++++++ broker/sqlc/import_query.sql | 3 ++ broker/sqlc/pr_schema.sql | 14 +++++++++ directory/test/request_id_prefix_test.go | 13 ++++---- ...xport-crosslink-directory-field-mapping.md | 2 +- migration/export-crosslink-directory.sql | 13 ++++++-- .../export-crosslink-open-patron-requests.sql | 12 ++++---- migration/scripts.md | 14 +++++++-- 12 files changed, 141 insertions(+), 23 deletions(-) create mode 100644 broker/migrations/080_advance_hrid.down.sql create mode 100644 broker/migrations/080_advance_hrid.up.sql diff --git a/broker/import/db/repo.go b/broker/import/db/repo.go index fc9df9fed..8d43c0e7e 100644 --- a/broker/import/db/repo.go +++ b/broker/import/db/repo.go @@ -3,6 +3,8 @@ package importdb import ( "errors" "fmt" + "regexp" + "strconv" "strings" "github.com/indexdata/crosslink/broker/common" @@ -150,6 +152,11 @@ func (r *PgImportRepo) ImportPatronRequest(ctx common.ExtendedContext, bundle Pa return fmt.Errorf("insert patron request %q: %w", bundle.PatronRequest.ID, err) } } + if bundle.PatronRequest.Side == prservice.SideBorrowing { + if err := r.advanceHrid(ctx, tx, bundle.PatronRequest.ID); err != nil { + return err + } + } itemIDs := make([]string, 0, len(bundle.Items)) for _, item := range bundle.Items { @@ -194,6 +201,29 @@ func (r *PgImportRepo) ImportPatronRequest(ctx common.ExtendedContext, bundle Pa return result, err } +// hridPattern matches the PREFIX-NUMBER form that get_next_hrid generates. +var hridPattern = regexp.MustCompile(`^([A-Za-z0-9]+)-([0-9]+)$`) + +// advanceHrid keeps generated request IDs from colliding with an imported one. +// When id has the PREFIX-NUMBER form, the HRID sequence for PREFIX is raised so +// get_next_hrid only returns numbers above NUMBER. Other IDs are left alone. +// The sequence is not locked against concurrent get_next_hrid calls, so imports +// of such IDs should finish before the tenant creates new requests. +func (r *PgImportRepo) advanceHrid(ctx common.ExtendedContext, tx DBTX, id string) error { + match := hridPattern.FindStringSubmatch(id) + if match == nil { + return nil + } + used, err := strconv.ParseInt(match[2], 10, 64) + if err != nil { + return fmt.Errorf("parse HRID number of patron request %q: %w", id, err) + } + if err := r.queries.AdvanceImportedHrid(ctx, tx, AdvanceImportedHridParams{Prefix: match[1], Used: used}); err != nil { + return fmt.Errorf("advance HRID sequence for patron request %q: %w", id, err) + } + return nil +} + func createImportedPatronRequestParams(params pr_db.CreatePatronRequestParams) CreateImportedPatronRequestParams { return CreateImportedPatronRequestParams{ ID: params.ID, CreatedAt: params.CreatedAt, IllRequest: params.IllRequest, diff --git a/broker/import/test/export_hrid_preflight_test.go b/broker/import/test/export_hrid_preflight_test.go index 268aeb79d..0f6251b51 100644 --- a/broker/import/test/export_hrid_preflight_test.go +++ b/broker/import/test/export_hrid_preflight_test.go @@ -11,7 +11,9 @@ import ( "github.com/stretchr/testify/require" ) -func TestExportPreflightRejectsNonAlphanumericHrid(t *testing.T) { +const hridProblem = " must be ASCII letters and digits, optionally followed by a hyphen and digits" + +func TestExportPreflightRejectsInvalidHrid(t *testing.T) { // Execute the actual preflight check without requiring the full legacy schema. source, err := os.ReadFile("../../../migration/export-crosslink-open-patron-requests.sql") require.NoError(t, err) @@ -32,13 +34,16 @@ func TestExportPreflightRejectsNonAlphanumericHrid(t *testing.T) { {name: "lowercase", side: "borrowing", hrid: pgtype.Text{String: "abc123", Valid: true}}, {name: "digits only", side: "borrowing", hrid: pgtype.Text{String: "123", Valid: true}}, {name: "missing HRID keeps UUID", side: "borrowing"}, - {name: "lending is not checked", side: "lending", hrid: pgtype.Text{String: "ABC-1", Valid: true}}, - {name: "hyphen", side: "borrowing", hrid: pgtype.Text{String: "ABC-1", Valid: true}, - want: "request pr1: HRID ABC-1 must contain only ASCII letters and digits"}, + {name: "lending is not checked", side: "lending", hrid: pgtype.Text{String: "AB 1", Valid: true}}, + {name: "hyphen before number", side: "borrowing", hrid: pgtype.Text{String: "ABC-10008", Valid: true}}, + {name: "hyphen before letters", side: "borrowing", hrid: pgtype.Text{String: "ABC-X1", Valid: true}, + want: "request pr1: HRID ABC-X1" + hridProblem}, + {name: "two hyphens", side: "borrowing", hrid: pgtype.Text{String: "A-B-1", Valid: true}, + want: "request pr1: HRID A-B-1" + hridProblem}, {name: "space", side: "borrowing", hrid: pgtype.Text{String: "AB 1", Valid: true}, - want: "request pr1: HRID AB 1 must contain only ASCII letters and digits"}, + want: "request pr1: HRID AB 1" + hridProblem}, {name: "non-ASCII", side: "borrowing", hrid: pgtype.Text{String: "ÅB1", Valid: true}, - want: "request pr1: HRID ÅB1 must contain only ASCII letters and digits"}, + want: "request pr1: HRID ÅB1" + hridProblem}, } { t.Run(tc.name, func(t *testing.T) { ctx := context.Background() diff --git a/broker/import/test/patron_request_test.go b/broker/import/test/patron_request_test.go index b65e0048a..6f5faa290 100644 --- a/broker/import/test/patron_request_test.go +++ b/broker/import/test/patron_request_test.go @@ -3,6 +3,7 @@ package test import ( "context" "os" + "strings" "testing" "time" @@ -573,3 +574,31 @@ func TestImportPatronRequestPersistsBibRecordID(t *testing.T) { assert.Equal(t, expected, actual) } } + +func TestImportBorrowingPatronRequestAdvancesHridSequence(t *testing.T) { + hridPrefix := "T" + strings.ReplaceAll(uuid.NewString(), "-", "")[:12] + importID := func(id string, lending bool) { + t.Helper() + prefix := uuid.NewString() + bundle := testPatronBundle(prefix, prefix+"-request") + if lending { + bundle = testLendingPatronBundle(prefix, prefix+"-request", prefix+"-SUP") + } + bundle.PatronRequest.ID = id + _, err := importTestRepo.ImportPatronRequest(importTestCtx, bundle, importdb.ConflictPolicyFail) + require.NoError(t, err) + } + nextHrid := func() string { + t.Helper() + var hrid string + require.NoError(t, importTestPool.QueryRow(context.Background(), "SELECT get_next_hrid($1)", hridPrefix).Scan(&hrid)) + return hrid + } + + importID(hridPrefix+"-41", false) + assert.Equal(t, strings.ToUpper(hridPrefix)+"-42", nextHrid()) + importID(hridPrefix+"-10", false) + assert.Equal(t, strings.ToUpper(hridPrefix)+"-43", nextHrid(), "a lower imported number does not move the sequence back") + importID(hridPrefix+"-500", true) + assert.Equal(t, strings.ToUpper(hridPrefix)+"-44", nextHrid(), "lending request IDs are not generated locally") +} diff --git a/broker/migrations/080_advance_hrid.down.sql b/broker/migrations/080_advance_hrid.down.sql new file mode 100644 index 000000000..94c9172a0 --- /dev/null +++ b/broker/migrations/080_advance_hrid.down.sql @@ -0,0 +1 @@ +DROP FUNCTION advance_hrid; diff --git a/broker/migrations/080_advance_hrid.up.sql b/broker/migrations/080_advance_hrid.up.sql new file mode 100644 index 000000000..bac6b428f --- /dev/null +++ b/broker/migrations/080_advance_hrid.up.sql @@ -0,0 +1,16 @@ +-- Raises the HRID sequence for prefix so get_next_hrid never returns an ID at +-- or below used, which an imported request already holds. The sequence never +-- moves backwards. +CREATE OR REPLACE FUNCTION advance_hrid(prefix text, used bigint) RETURNS void AS $$ +DECLARE + seq text := LOWER(prefix) || '_hrid_seq'; + current_value bigint; + called boolean; +BEGIN + EXECUTE format('CREATE SEQUENCE IF NOT EXISTS %I START 1', seq); + EXECUTE format('SELECT last_value, is_called FROM %I', seq) INTO current_value, called; + IF NOT called OR current_value < used THEN + PERFORM setval(quote_ident(seq)::regclass, GREATEST(current_value, used)); + END IF; +END; +$$ LANGUAGE plpgsql; diff --git a/broker/sqlc/import_query.sql b/broker/sqlc/import_query.sql index e9b5f99a8..1357e5cd7 100644 --- a/broker/sqlc/import_query.sql +++ b/broker/sqlc/import_query.sql @@ -16,6 +16,9 @@ WHERE id = sqlc.arg(id) ORDER BY id FOR UPDATE; +-- name: AdvanceImportedHrid :exec +SELECT advance_hrid(sqlc.arg(prefix)::text, sqlc.arg(used)::bigint); + -- name: CreateImportedPatronRequest :exec INSERT INTO patron_request ( id, created_at, ill_request, state, side, patron, requester_symbol, diff --git a/broker/sqlc/pr_schema.sql b/broker/sqlc/pr_schema.sql index a4b85b52e..6b3e2e51c 100644 --- a/broker/sqlc/pr_schema.sql +++ b/broker/sqlc/pr_schema.sql @@ -58,6 +58,20 @@ BEGIN END; $$ LANGUAGE plpgsql; +CREATE OR REPLACE FUNCTION advance_hrid(prefix text, used bigint) RETURNS void AS $$ +DECLARE + seq text := LOWER(prefix) || '_hrid_seq'; + current_value bigint; + called boolean; +BEGIN + EXECUTE format('CREATE SEQUENCE IF NOT EXISTS %I START 1', seq); + EXECUTE format('SELECT last_value, is_called FROM %I', seq) INTO current_value, called; + IF NOT called OR current_value < used THEN + PERFORM setval(quote_ident(seq)::regclass, GREATEST(current_value, used)); + END IF; +END; +$$ LANGUAGE plpgsql; + CREATE TABLE item ( id VARCHAR PRIMARY KEY, diff --git a/directory/test/request_id_prefix_test.go b/directory/test/request_id_prefix_test.go index 9e72a3958..52b7d8e2a 100644 --- a/directory/test/request_id_prefix_test.go +++ b/directory/test/request_id_prefix_test.go @@ -14,7 +14,9 @@ import ( var longestRequestIdPrefix = strings.Repeat("A", 32) -var invalidRequestIdPrefixes = []string{"ABC-", "A B", "A.B", "A_B", "ÅBC", " ABC", longestRequestIdPrefix + "A"} +const exportPrefixError = "request_id_prefix must be at most 32 ASCII letters and digits after removing punctuation and spaces: " + +var invalidRequestIdPrefixes =[]string{"ABC-", "A B", "A.B", "A_B", "ÅBC", " ABC", longestRequestIdPrefix + "A"} func TestRequestIdPrefixPersistence(t *testing.T) { resetDb() @@ -143,10 +145,11 @@ func TestDirectoryExportRequestIdPrefix(t *testing.T) { {name: "local", setting: "ABC", local: true, want: "ABC"}, {name: "trimmed", setting: " ABC ", local: true, want: "ABC"}, {name: "longest", setting: longestRequestIdPrefix, local: true, want: longestRequestIdPrefix}, - {name: "too long", setting: longestRequestIdPrefix + "A", local: true, wantError: "request_id_prefix must be at most 32 ASCII letters and digits: " + longestRequestIdPrefix + "A"}, - {name: "trailing delimiter", setting: "ABC-", local: true, wantError: "request_id_prefix must be at most 32 ASCII letters and digits: ABC-"}, - {name: "inner space", setting: "A B", local: true, wantError: "request_id_prefix must be at most 32 ASCII letters and digits: A B"}, - {name: "invalid default value", setting: " ", fallback: "A.B", local: true, wantError: "request_id_prefix must be at most 32 ASCII letters and digits: A.B"}, + {name: "too long", setting: longestRequestIdPrefix + "-A", local: true, wantError: exportPrefixError + longestRequestIdPrefix + "-A"}, + {name: "trailing delimiter removed", setting: "ABC-", local: true, want: "ABC"}, + {name: "inner punctuation and space removed", setting: "A.B C", local: true, want: "ABC"}, + {name: "only punctuation", setting: "-", local: true, wantError: exportPrefixError + "-"}, + {name: "invalid default value", setting: " ", fallback: "ÅB-", local: true, wantError: exportPrefixError + "ÅB-"}, {name: "default value", setting: " ", fallback: "DEF", local: true, want: "DEF"}, {name: "direct branch inherits", setting: "ABC", branch: true}, {name: "unset", local: true}, diff --git a/migration/export-crosslink-directory-field-mapping.md b/migration/export-crosslink-directory-field-mapping.md index c4cb98678..8558652d8 100644 --- a/migration/export-crosslink-directory-field-mapping.md +++ b/migration/export-crosslink-directory-field-mapping.md @@ -181,7 +181,7 @@ exported defaults. | Presence of the ISO18626 endpoint | `data.illConfig.iso18626Vendor` | Set to `ReShare`. | | `app_setting.max_requests` | `data.illConfig.maxRequestsPerPatron` | Cast to an integer for the selected local entry. Values must be from 0 through 2147483647. The value is `null` for branches, which inherit it, and for other non-local pickup entries. | | `app_setting.check_duplicate_time` | `data.illConfig.duplicateCheckWindowHours` | Cast to an integer number of hours for the selected local entry. Values must be from 0 through 2147483647. The value is `null` for branches, which inherit it, and for other non-local pickup entries. | -| `app_setting.request_id_prefix` | `data.illConfig.requestIdPrefix` | Copied with surrounding spaces trimmed for the selected local entry, using the nonblank `st_value` and falling back to the nonblank `st_default_value`. The value is `null` when unset, for branches, which inherit it, and for other non-local pickup entries. A value longer than 32 characters, or containing anything other than ASCII letters and digits, fails the export before any records are emitted. | +| `app_setting.request_id_prefix` | `data.illConfig.requestIdPrefix` | Copied with surrounding spaces trimmed for the selected local entry, using the nonblank `st_value` and falling back to the nonblank `st_default_value`. The value is `null` when unset, for branches, which inherit it, and for other non-local pickup entries. ASCII punctuation and spaces are removed, because CrossLink adds its own `-` separator: `ALV-` becomes `ALV`. A value that is then empty, longer than 32 characters, or contains anything other than ASCII letters and digits fails the export before any records are emitted. | | `app_setting.default_copyright_type` resolved through `copyrightType` refdata | `data.illConfig.defaultCopyrightType` | Exported only on the selected local owner as a single object: `rdv_value` becomes `code` as its ISO18626 CopyrightCompliance code, and `rdv_label` becomes `defaultDesc`, falling back to the code for a null or whitespace-only label. | | Unsupported mod-rs agency-information behavior | `data.illConfig.includeRequestingAgencyInfo` | Set to `false`; mod-rs does not populate requesting-agency information. | | Unsupported mod-rs agency-information behavior | `data.illConfig.includeSupplierInfo` | Set to `false`; mod-rs does not populate supplier information. | diff --git a/migration/export-crosslink-directory.sql b/migration/export-crosslink-directory.sql index c066886f6..e651ed6db 100644 --- a/migration/export-crosslink-directory.sql +++ b/migration/export-crosslink-directory.sql @@ -324,7 +324,14 @@ SELECT max(coalesce(nullif(btrim(st_value), ''), nullif(btrim(st_default_value), ''))) FILTER (WHERE st_key = 'default_copyright_type') AS default_copyright_type, max(coalesce(nullif(btrim(st_value), ''), nullif(btrim(st_default_value), ''))) - FILTER (WHERE st_key = 'request_id_prefix') AS request_id_prefix, + FILTER (WHERE st_key = 'request_id_prefix') AS request_id_prefix_source, + -- mod-rs appends the HRID number directly to the prefix, so prefixes often + -- end in a separator such as "ALV-". CrossLink adds its own "-" separator, + -- so ASCII punctuation and whitespace are dropped: "ALV-" becomes "ALV". + regexp_replace( + max(coalesce(nullif(btrim(st_value), ''), nullif(btrim(st_default_value), ''))) + FILTER (WHERE st_key = 'request_id_prefix'), + '[[:punct:][:space:]]', '', 'g') AS request_id_prefix, max(coalesce(nullif(btrim(st_value), ''), nullif(btrim(st_default_value), ''))) FILTER (WHERE st_key = 'last_resort_lenders') AS last_resort_lenders FROM app_setting; @@ -380,10 +387,10 @@ DO $$ DECLARE problem text; BEGIN - SELECT request_id_prefix INTO problem FROM crosslink_tenant_settings + SELECT request_id_prefix_source INTO problem FROM crosslink_tenant_settings WHERE request_id_prefix !~ '^[A-Za-z0-9]{1,32}$'; IF problem IS NOT NULL THEN - RAISE EXCEPTION 'request_id_prefix must be at most 32 ASCII letters and digits: %', problem; + RAISE EXCEPTION 'request_id_prefix must be at most 32 ASCII letters and digits after removing punctuation and spaces: %', problem; END IF; END $$; diff --git a/migration/export-crosslink-open-patron-requests.sql b/migration/export-crosslink-open-patron-requests.sql index 01f564273..c5696912d 100644 --- a/migration/export-crosslink-open-patron-requests.sql +++ b/migration/export-crosslink-open-patron-requests.sql @@ -74,9 +74,11 @@ -- -- The preflight block runs before any NDJSON is emitted and stops the export if -- an open request or rota entry has an unmapped state or is missing required --- import data, or if a borrowing request's HRID is not alphanumeric. Broker --- generated IDs have the form PREFIX-N, so an alphanumeric HRID cannot collide --- with one. Borrowing requests without an HRID keep their UUID. +-- import data, or if a borrowing request's HRID is neither alphanumeric nor +-- PREFIX-N (as when the mod-rs prefix ends in "-"). Broker generated IDs have +-- the form PREFIX-N, so an alphanumeric HRID cannot collide with one. For a +-- PREFIX-N HRID, the broker import raises PREFIX's HRID sequence past N so new +-- IDs cannot collide either. Borrowing requests without an HRID keep their UUID. \set ON_ERROR_STOP on @@ -742,10 +744,10 @@ BEGIN UNION ALL SELECT 'request ' || pr_id || ': HRID ' || pr_hrid - || ' must contain only ASCII letters and digits' + || ' must be ASCII letters and digits, optionally followed by a hyphen and digits' FROM crosslink_open_requests WHERE import_side = 'borrowing' - AND pr_hrid !~ '^[A-Za-z0-9]+$' + AND pr_hrid !~ '^[A-Za-z0-9]+(-[0-9]+)?$' ) AS validation; IF problems IS NOT NULL THEN diff --git a/migration/scripts.md b/migration/scripts.md index c4d0aa2f0..e0182d802 100644 --- a/migration/scripts.md +++ b/migration/scripts.md @@ -59,9 +59,17 @@ It also exports the selected local owner's `defaultCopyrightType`, resolving the `default_copyright_type` setting through the `copyrightType` refdata vocabulary. An unset setting is omitted; a configured value without a unique refdata match fails the export. The `request_id_prefix` setting is exported as `requestIdPrefix` -on the selected local owner; its branches inherit it. A value longer than 32 -characters, or containing anything other than ASCII letters and digits, fails the -export. +on the selected local owner; its branches inherit it. ASCII punctuation and +spaces are removed, because CrossLink adds its own `-` separator: `ALV-` is +exported as `ALV`. A value that is then empty, longer than 32 characters, or +contains anything other than ASCII letters and digits fails the export. + +The patron-request export keeps mod-rs HRIDs as borrowing request IDs. An HRID +must be ASCII letters and digits, or such a prefix followed by `-` and digits +(for example `ALV-10008`). When the broker imports a borrowing request with a +`PREFIX-N` ID, it raises the HRID sequence for `PREFIX` past `N`, so new +CrossLink IDs cannot repeat a migrated one. Finish the patron-request import +before the tenant starts creating requests in CrossLink. See the [ILL configuration field mappings](export-crosslink-directory-field-mapping.md#ill-configuration) for source fields, setting and label fallbacks, ordering, and missing-list behavior. From c4dd1f680800860a173dc1f00046f12a5c19bac8 Mon Sep 17 00:00:00 2001 From: Kurt Nordstrom Date: Sat, 14 Nov 2026 08:52:41 -0500 Subject: [PATCH 2/4] Fix potential race and issue with parsing prefixes with dashes --- broker/import/README.md | 2 + broker/import/db/repo.go | 26 +++++++--- broker/import/test/patron_request_test.go | 61 +++++++++++++++++++++++ broker/migrations/080_advance_hrid.up.sql | 5 +- broker/sqlc/pr_schema.sql | 1 + 5 files changed, 87 insertions(+), 8 deletions(-) diff --git a/broker/import/README.md b/broker/import/README.md index 26039d529..2268e9aad 100644 --- a/broker/import/README.md +++ b/broker/import/README.md @@ -114,6 +114,8 @@ Important validation rules include: - An `illTransaction`, when supplied, must use the same requester symbol and requester request ID. `locatedSuppliers` require an `illTransaction`. - Item, notification, and located-supplier IDs must be unique within their respective arrays. At most one located supplier may have status `selected`. +When a borrowing request's `id` has the `PREFIX-N` form that the broker generates, the import raises the request-ID sequence for `PREFIX` past `N`, so new requests cannot reuse the ID. `PREFIX` must be ASCII letters and digits, or equal the value of `requesterSymbol` (`US-RS1` in `US-RS1-1001`), which the broker uses when Directory sets no `requestIdPrefix`. Other IDs, such as UUIDs, leave the sequences unchanged. Finish such imports before the requester creates new requests. + Minimal borrowing example: ```ndjson diff --git a/broker/import/db/repo.go b/broker/import/db/repo.go index 8d43c0e7e..c72093880 100644 --- a/broker/import/db/repo.go +++ b/broker/import/db/repo.go @@ -153,7 +153,7 @@ func (r *PgImportRepo) ImportPatronRequest(ctx common.ExtendedContext, bundle Pa } } if bundle.PatronRequest.Side == prservice.SideBorrowing { - if err := r.advanceHrid(ctx, tx, bundle.PatronRequest.ID); err != nil { + if err := r.advanceHrid(ctx, tx, bundle.PatronRequest.ID, bundle.PatronRequest.RequesterSymbol.String); err != nil { return err } } @@ -201,19 +201,31 @@ func (r *PgImportRepo) ImportPatronRequest(ctx common.ExtendedContext, bundle Pa return result, err } -// hridPattern matches the PREFIX-NUMBER form that get_next_hrid generates. -var hridPattern = regexp.MustCompile(`^([A-Za-z0-9]+)-([0-9]+)$`) +var ( + // hridPattern splits the PREFIX-NUMBER form that get_next_hrid generates. + hridPattern = regexp.MustCompile(`^(.+)-([0-9]+)$`) + // alphanumericPrefix matches prefixes configured in Directory. + alphanumericPrefix = regexp.MustCompile(`^[A-Za-z0-9]+$`) +) // advanceHrid keeps generated request IDs from colliding with an imported one. // When id has the PREFIX-NUMBER form, the HRID sequence for PREFIX is raised so -// get_next_hrid only returns numbers above NUMBER. Other IDs are left alone. -// The sequence is not locked against concurrent get_next_hrid calls, so imports -// of such IDs should finish before the tenant creates new requests. -func (r *PgImportRepo) advanceHrid(ctx common.ExtendedContext, tx DBTX, id string) error { +// get_next_hrid only returns numbers above NUMBER. PREFIX must be alphanumeric, +// like a Directory requestIdPrefix, or equal the value of requesterSymbol, which +// RequestIdPrefix falls back to and which may contain hyphens ("US-RS1-1001"). +// Other IDs, including UUIDs whose last group is all digits, are left alone. +// advance_hrid serializes concurrent advances of one sequence until commit, but +// get_next_hrid does not take that lock, so imports of such IDs should finish +// before the tenant creates new requests. +func (r *PgImportRepo) advanceHrid(ctx common.ExtendedContext, tx DBTX, id string, requesterSymbol string) error { match := hridPattern.FindStringSubmatch(id) if match == nil { return nil } + _, symbolValue, _ := common.SplitSymbol(requesterSymbol) + if !alphanumericPrefix.MatchString(match[1]) && (symbolValue == "" || !strings.EqualFold(match[1], symbolValue)) { + return nil + } used, err := strconv.ParseInt(match[2], 10, 64) if err != nil { return fmt.Errorf("parse HRID number of patron request %q: %w", id, err) diff --git a/broker/import/test/patron_request_test.go b/broker/import/test/patron_request_test.go index 6f5faa290..510fc56bb 100644 --- a/broker/import/test/patron_request_test.go +++ b/broker/import/test/patron_request_test.go @@ -602,3 +602,64 @@ func TestImportBorrowingPatronRequestAdvancesHridSequence(t *testing.T) { importID(hridPrefix+"-500", true) assert.Equal(t, strings.ToUpper(hridPrefix)+"-44", nextHrid(), "lending request IDs are not generated locally") } + +func TestImportHridAdvanceWaitsForConcurrentAdvance(t *testing.T) { + ctx := context.Background() + hridPrefix := "T" + strings.ReplaceAll(uuid.NewString(), "-", "")[:12] + blocker, err := importTestPool.Begin(ctx) + require.NoError(t, err) + t.Cleanup(func() { _ = blocker.Rollback(ctx) }) + // An uncommitted advance to 90 holds the sequence's advisory lock. + _, err = blocker.Exec(ctx, "SELECT advance_hrid($1, 90)", hridPrefix) + require.NoError(t, err) + + prefix := uuid.NewString() + bundle := testPatronBundle(prefix, prefix+"-request") + bundle.PatronRequest.ID = hridPrefix + "-41" + done := make(chan error, 1) + go func() { + _, err := importTestRepo.ImportPatronRequest(importTestCtx, bundle, importdb.ConflictPolicyFail) + done <- err + }() + require.Eventually(t, func() bool { + var waiting int + err := importTestPool.QueryRow(ctx, "SELECT count(*) FROM pg_locks WHERE locktype = 'advisory' AND NOT granted").Scan(&waiting) + return err == nil && waiting > 0 + }, 5*time.Second, 10*time.Millisecond, "the import must wait for the advisory lock") + select { + case err := <-done: + t.Fatalf("import finished while another advance held the lock: %v", err) + default: + } + + require.NoError(t, blocker.Commit(ctx)) + require.NoError(t, <-done) + var hrid string + require.NoError(t, importTestPool.QueryRow(ctx, "SELECT get_next_hrid($1)", hridPrefix).Scan(&hrid)) + assert.Equal(t, strings.ToUpper(hridPrefix)+"-91", hrid, "the later advance to 41 does not lower the sequence") +} + +func TestImportBorrowingPatronRequestAdvancesSymbolHridSequence(t *testing.T) { + // Without a Directory prefix, IDs use the requester symbol value, which may + // contain hyphens. + prefix := uuid.NewString() + bundle := testPatronBundle(prefix, prefix+"-request") + symbolValue := "T-" + strings.ReplaceAll(uuid.NewString(), "-", "")[:12] + bundle.PatronRequest.RequesterSymbol = pgtype.Text{String: "ISIL:" + symbolValue, Valid: true} + bundle.PatronRequest.ID = symbolValue + "-41" + _, err := importTestRepo.ImportPatronRequest(importTestCtx, bundle, importdb.ConflictPolicyFail) + require.NoError(t, err) + var hrid string + require.NoError(t, importTestPool.QueryRow(context.Background(), "SELECT get_next_hrid($1)", symbolValue).Scan(&hrid)) + assert.Equal(t, strings.ToUpper(symbolValue)+"-42", hrid) + + // A UUID whose last group is all digits is not an HRID. + uuidPrefix := uuid.NewString()[:23] + bundle = testPatronBundle(uuid.NewString(), uuid.NewString()+"-request") + bundle.PatronRequest.ID = uuidPrefix + "-" + strings.Repeat("7", 12) + _, err = importTestRepo.ImportPatronRequest(importTestCtx, bundle, importdb.ConflictPolicyFail) + require.NoError(t, err) + var sequences int + require.NoError(t, importTestPool.QueryRow(context.Background(), "SELECT count(*) FROM pg_class WHERE relkind = 'S' AND relname = $1", uuidPrefix+"_hrid_seq").Scan(&sequences)) + assert.Zero(t, sequences) +} diff --git a/broker/migrations/080_advance_hrid.up.sql b/broker/migrations/080_advance_hrid.up.sql index bac6b428f..37069c2f4 100644 --- a/broker/migrations/080_advance_hrid.up.sql +++ b/broker/migrations/080_advance_hrid.up.sql @@ -1,12 +1,15 @@ -- Raises the HRID sequence for prefix so get_next_hrid never returns an ID at -- or below used, which an imported request already holds. The sequence never --- moves backwards. +-- moves backwards. setval is not transactional, so concurrent advances of one +-- sequence are serialized until commit; otherwise a lower value read earlier +-- could overwrite a higher one. CREATE OR REPLACE FUNCTION advance_hrid(prefix text, used bigint) RETURNS void AS $$ DECLARE seq text := LOWER(prefix) || '_hrid_seq'; current_value bigint; called boolean; BEGIN + PERFORM pg_advisory_xact_lock(hashtextextended('broker:hrid-seq:' || seq, 0)); EXECUTE format('CREATE SEQUENCE IF NOT EXISTS %I START 1', seq); EXECUTE format('SELECT last_value, is_called FROM %I', seq) INTO current_value, called; IF NOT called OR current_value < used THEN diff --git a/broker/sqlc/pr_schema.sql b/broker/sqlc/pr_schema.sql index 6b3e2e51c..03f121c71 100644 --- a/broker/sqlc/pr_schema.sql +++ b/broker/sqlc/pr_schema.sql @@ -64,6 +64,7 @@ DECLARE current_value bigint; called boolean; BEGIN + PERFORM pg_advisory_xact_lock(hashtextextended('broker:hrid-seq:' || seq, 0)); EXECUTE format('CREATE SEQUENCE IF NOT EXISTS %I START 1', seq); EXECUTE format('SELECT last_value, is_called FROM %I', seq) INTO current_value, called; IF NOT called OR current_value < used THEN From bc4f5adfbb17af15ad3f4d6f9fd1296c7e446925 Mon Sep 17 00:00:00 2001 From: Kurt Nordstrom Date: Sat, 14 Nov 2026 10:20:08 -0500 Subject: [PATCH 3/4] Add locale for postgres conversion --- directory/test/request_id_prefix_test.go | 1 + migration/export-crosslink-directory.sql | 7 +++++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/directory/test/request_id_prefix_test.go b/directory/test/request_id_prefix_test.go index 52b7d8e2a..613b924ca 100644 --- a/directory/test/request_id_prefix_test.go +++ b/directory/test/request_id_prefix_test.go @@ -149,6 +149,7 @@ func TestDirectoryExportRequestIdPrefix(t *testing.T) { {name: "trailing delimiter removed", setting: "ABC-", local: true, want: "ABC"}, {name: "inner punctuation and space removed", setting: "A.B C", local: true, want: "ABC"}, {name: "only punctuation", setting: "-", local: true, wantError: exportPrefixError + "-"}, + {name: "non-ASCII punctuation kept", setting: "ABC—", local: true, wantError: exportPrefixError + "ABC—"}, {name: "invalid default value", setting: " ", fallback: "ÅB-", local: true, wantError: exportPrefixError + "ÅB-"}, {name: "default value", setting: " ", fallback: "DEF", local: true, want: "DEF"}, {name: "direct branch inherits", setting: "ABC", branch: true}, diff --git a/migration/export-crosslink-directory.sql b/migration/export-crosslink-directory.sql index e651ed6db..2c42d5cbc 100644 --- a/migration/export-crosslink-directory.sql +++ b/migration/export-crosslink-directory.sql @@ -328,9 +328,12 @@ SELECT -- mod-rs appends the HRID number directly to the prefix, so prefixes often -- end in a separator such as "ALV-". CrossLink adds its own "-" separator, -- so ASCII punctuation and whitespace are dropped: "ALV-" becomes "ALV". + -- The C collation keeps the character classes ASCII-only whatever the + -- database locale, so non-ASCII characters such as "—" stay and fail + -- validation. regexp_replace( - max(coalesce(nullif(btrim(st_value), ''), nullif(btrim(st_default_value), ''))) - FILTER (WHERE st_key = 'request_id_prefix'), + (max(coalesce(nullif(btrim(st_value), ''), nullif(btrim(st_default_value), ''))) + FILTER (WHERE st_key = 'request_id_prefix')) COLLATE "C", '[[:punct:][:space:]]', '', 'g') AS request_id_prefix, max(coalesce(nullif(btrim(st_value), ''), nullif(btrim(st_default_value), ''))) FILTER (WHERE st_key = 'last_resort_lenders') AS last_resort_lenders From 7a87eb27b20e3869238864dc3edb0e07be301e5b Mon Sep 17 00:00:00 2001 From: Kurt Nordstrom Date: Sat, 14 Nov 2026 14:24:21 -0500 Subject: [PATCH 4/4] Fix gofmt bug. Clean up --- broker/import/README.md | 2 +- broker/import/db/repo.go | 5 +- broker/import/test/patron_request_test.go | 81 +++++++++++++-------- broker/migrations/080_advance_hrid.down.sql | 7 ++ broker/migrations/080_advance_hrid.up.sql | 17 ++++- broker/sqlc/pr_schema.sql | 1 + directory/test/request_id_prefix_test.go | 2 +- migration/export-crosslink-directory.sql | 24 +++--- migration/scripts.md | 4 +- 9 files changed, 90 insertions(+), 53 deletions(-) diff --git a/broker/import/README.md b/broker/import/README.md index 2268e9aad..cbf603554 100644 --- a/broker/import/README.md +++ b/broker/import/README.md @@ -114,7 +114,7 @@ Important validation rules include: - An `illTransaction`, when supplied, must use the same requester symbol and requester request ID. `locatedSuppliers` require an `illTransaction`. - Item, notification, and located-supplier IDs must be unique within their respective arrays. At most one located supplier may have status `selected`. -When a borrowing request's `id` has the `PREFIX-N` form that the broker generates, the import raises the request-ID sequence for `PREFIX` past `N`, so new requests cannot reuse the ID. `PREFIX` must be ASCII letters and digits, or equal the value of `requesterSymbol` (`US-RS1` in `US-RS1-1001`), which the broker uses when Directory sets no `requestIdPrefix`. Other IDs, such as UUIDs, leave the sequences unchanged. Finish such imports before the requester creates new requests. +When a borrowing request's `id` has the `PREFIX-N` form that the broker generates, the import raises the request-ID sequence for `PREFIX` past `N`, so new requests cannot reuse the ID. `PREFIX` must be ASCII letters and digits, or equal the value of `requesterSymbol` (`US-RS1` in `US-RS1-1001`), which the broker uses when Directory sets no `requestIdPrefix`. Other IDs, such as UUIDs, leave the sequences unchanged. Request creation for that prefix waits while an import is advancing its sequence, so imports and new requests can run at the same time. Minimal borrowing example: diff --git a/broker/import/db/repo.go b/broker/import/db/repo.go index c72093880..17a7af5f7 100644 --- a/broker/import/db/repo.go +++ b/broker/import/db/repo.go @@ -214,9 +214,8 @@ var ( // like a Directory requestIdPrefix, or equal the value of requesterSymbol, which // RequestIdPrefix falls back to and which may contain hyphens ("US-RS1-1001"). // Other IDs, including UUIDs whose last group is all digits, are left alone. -// advance_hrid serializes concurrent advances of one sequence until commit, but -// get_next_hrid does not take that lock, so imports of such IDs should finish -// before the tenant creates new requests. +// advance_hrid locks the sequence against other advances and get_next_hrid +// until commit, so imports may run while the tenant creates requests. func (r *PgImportRepo) advanceHrid(ctx common.ExtendedContext, tx DBTX, id string, requesterSymbol string) error { match := hridPattern.FindStringSubmatch(id) if match == nil { diff --git a/broker/import/test/patron_request_test.go b/broker/import/test/patron_request_test.go index 510fc56bb..528b40ff3 100644 --- a/broker/import/test/patron_request_test.go +++ b/broker/import/test/patron_request_test.go @@ -603,40 +603,57 @@ func TestImportBorrowingPatronRequestAdvancesHridSequence(t *testing.T) { assert.Equal(t, strings.ToUpper(hridPrefix)+"-44", nextHrid(), "lending request IDs are not generated locally") } -func TestImportHridAdvanceWaitsForConcurrentAdvance(t *testing.T) { - ctx := context.Background() - hridPrefix := "T" + strings.ReplaceAll(uuid.NewString(), "-", "")[:12] - blocker, err := importTestPool.Begin(ctx) - require.NoError(t, err) - t.Cleanup(func() { _ = blocker.Rollback(ctx) }) - // An uncommitted advance to 90 holds the sequence's advisory lock. - _, err = blocker.Exec(ctx, "SELECT advance_hrid($1, 90)", hridPrefix) - require.NoError(t, err) +func TestImportHridAdvanceWaitsForConcurrentSequenceUse(t *testing.T) { + // Counts waiters on this prefix's sequence lock; a bigint advisory key is + // stored as classid (high 32 bits) and objid (low 32 bits). + const waitingQuery = `WITH lock_key AS ( + SELECT hashtextextended('broker:hrid-seq:' || lower($1) || '_hrid_seq', 0) AS k + ) + SELECT count(*) FROM pg_locks, lock_key + WHERE locktype = 'advisory' AND NOT granted AND objsubid = 1 + AND classid::bigint = (k >> 32) & 4294967295 + AND objid::bigint = k & 4294967295` + for _, tc := range []struct { + name, blockerSQL, wantNext string + }{ + {name: "uncommitted advance", blockerSQL: "SELECT advance_hrid($1, 90)", wantNext: "-91"}, + {name: "uncommitted request creation", blockerSQL: "SELECT get_next_hrid($1)", wantNext: "-42"}, + } { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + hridPrefix := "T" + strings.ReplaceAll(uuid.NewString(), "-", "")[:12] + blocker, err := importTestPool.Begin(ctx) + require.NoError(t, err) + t.Cleanup(func() { _ = blocker.Rollback(ctx) }) + _, err = blocker.Exec(ctx, tc.blockerSQL, hridPrefix) + require.NoError(t, err) - prefix := uuid.NewString() - bundle := testPatronBundle(prefix, prefix+"-request") - bundle.PatronRequest.ID = hridPrefix + "-41" - done := make(chan error, 1) - go func() { - _, err := importTestRepo.ImportPatronRequest(importTestCtx, bundle, importdb.ConflictPolicyFail) - done <- err - }() - require.Eventually(t, func() bool { - var waiting int - err := importTestPool.QueryRow(ctx, "SELECT count(*) FROM pg_locks WHERE locktype = 'advisory' AND NOT granted").Scan(&waiting) - return err == nil && waiting > 0 - }, 5*time.Second, 10*time.Millisecond, "the import must wait for the advisory lock") - select { - case err := <-done: - t.Fatalf("import finished while another advance held the lock: %v", err) - default: - } + prefix := uuid.NewString() + bundle := testPatronBundle(prefix, prefix+"-request") + bundle.PatronRequest.ID = hridPrefix + "-41" + done := make(chan error, 1) + go func() { + _, err := importTestRepo.ImportPatronRequest(importTestCtx, bundle, importdb.ConflictPolicyFail) + done <- err + }() + require.Eventually(t, func() bool { + var waiting int + err := importTestPool.QueryRow(ctx, waitingQuery, hridPrefix).Scan(&waiting) + return err == nil && waiting > 0 + }, 5*time.Second, 10*time.Millisecond, "the import must wait for the sequence lock") + select { + case err := <-done: + t.Fatalf("import finished while the sequence lock was held: %v", err) + default: + } - require.NoError(t, blocker.Commit(ctx)) - require.NoError(t, <-done) - var hrid string - require.NoError(t, importTestPool.QueryRow(ctx, "SELECT get_next_hrid($1)", hridPrefix).Scan(&hrid)) - assert.Equal(t, strings.ToUpper(hridPrefix)+"-91", hrid, "the later advance to 41 does not lower the sequence") + require.NoError(t, blocker.Commit(ctx)) + require.NoError(t, <-done) + var hrid string + require.NoError(t, importTestPool.QueryRow(ctx, "SELECT get_next_hrid($1)", hridPrefix).Scan(&hrid)) + assert.Equal(t, strings.ToUpper(hridPrefix)+tc.wantNext, hrid) + }) + } } func TestImportBorrowingPatronRequestAdvancesSymbolHridSequence(t *testing.T) { diff --git a/broker/migrations/080_advance_hrid.down.sql b/broker/migrations/080_advance_hrid.down.sql index 94c9172a0..8b28f697c 100644 --- a/broker/migrations/080_advance_hrid.down.sql +++ b/broker/migrations/080_advance_hrid.down.sql @@ -1 +1,8 @@ DROP FUNCTION advance_hrid; + +CREATE OR REPLACE FUNCTION get_next_hrid(prefix text) RETURNS varchar AS $$ +BEGIN + EXECUTE format('CREATE SEQUENCE IF NOT EXISTS %I START 1', LOWER(prefix) || '_hrid_seq'); + RETURN UPPER(prefix) || '-' || nextval(LOWER(prefix) || '_hrid_seq')::TEXT; +END; +$$ LANGUAGE plpgsql; diff --git a/broker/migrations/080_advance_hrid.up.sql b/broker/migrations/080_advance_hrid.up.sql index 37069c2f4..d5e26c720 100644 --- a/broker/migrations/080_advance_hrid.up.sql +++ b/broker/migrations/080_advance_hrid.up.sql @@ -1,8 +1,11 @@ -- Raises the HRID sequence for prefix so get_next_hrid never returns an ID at -- or below used, which an imported request already holds. The sequence never --- moves backwards. setval is not transactional, so concurrent advances of one --- sequence are serialized until commit; otherwise a lower value read earlier --- could overwrite a higher one. +-- moves backwards. setval is not transactional, so a read followed by setval +-- must not interleave with another advance or with nextval: a value read +-- earlier would overwrite a higher one. advance_hrid therefore holds an +-- exclusive advisory lock on the sequence until commit, and get_next_hrid holds +-- a shared one, so request creation only waits while an import advances the +-- same prefix. CREATE OR REPLACE FUNCTION advance_hrid(prefix text, used bigint) RETURNS void AS $$ DECLARE seq text := LOWER(prefix) || '_hrid_seq'; @@ -17,3 +20,11 @@ BEGIN END IF; END; $$ LANGUAGE plpgsql; + +CREATE OR REPLACE FUNCTION get_next_hrid(prefix text) RETURNS varchar AS $$ +BEGIN + PERFORM pg_advisory_xact_lock_shared(hashtextextended('broker:hrid-seq:' || LOWER(prefix) || '_hrid_seq', 0)); + EXECUTE format('CREATE SEQUENCE IF NOT EXISTS %I START 1', LOWER(prefix) || '_hrid_seq'); + RETURN UPPER(prefix) || '-' || nextval(LOWER(prefix) || '_hrid_seq')::TEXT; +END; +$$ LANGUAGE plpgsql; diff --git a/broker/sqlc/pr_schema.sql b/broker/sqlc/pr_schema.sql index 03f121c71..1d3113342 100644 --- a/broker/sqlc/pr_schema.sql +++ b/broker/sqlc/pr_schema.sql @@ -53,6 +53,7 @@ CREATE TABLE archived_resource ( CREATE OR REPLACE FUNCTION get_next_hrid(prefix VARCHAR) RETURNS VARCHAR AS $$ BEGIN + PERFORM pg_advisory_xact_lock_shared(hashtextextended('broker:hrid-seq:' || LOWER(prefix) || '_hrid_seq', 0)); EXECUTE format('CREATE SEQUENCE IF NOT EXISTS %I START 1', LOWER(prefix) || '_hrid_seq'); RETURN UPPER(prefix) || '-' || nextval(LOWER(prefix) || '_hrid_seq')::TEXT; END; diff --git a/directory/test/request_id_prefix_test.go b/directory/test/request_id_prefix_test.go index 613b924ca..adf3144aa 100644 --- a/directory/test/request_id_prefix_test.go +++ b/directory/test/request_id_prefix_test.go @@ -16,7 +16,7 @@ var longestRequestIdPrefix = strings.Repeat("A", 32) const exportPrefixError = "request_id_prefix must be at most 32 ASCII letters and digits after removing punctuation and spaces: " -var invalidRequestIdPrefixes =[]string{"ABC-", "A B", "A.B", "A_B", "ÅBC", " ABC", longestRequestIdPrefix + "A"} +var invalidRequestIdPrefixes = []string{"ABC-", "A B", "A.B", "A_B", "ÅBC", " ABC", longestRequestIdPrefix + "A"} func TestRequestIdPrefixPersistence(t *testing.T) { resetDb() diff --git a/migration/export-crosslink-directory.sql b/migration/export-crosslink-directory.sql index 2c42d5cbc..d9de7f428 100644 --- a/migration/export-crosslink-directory.sql +++ b/migration/export-crosslink-directory.sql @@ -286,6 +286,17 @@ SELECT FROM settings; CREATE TEMP TABLE crosslink_tenant_settings ON COMMIT DROP AS +SELECT + settings.*, + -- mod-rs appends the HRID number directly to the prefix, so prefixes often + -- end in a separator such as "ALV-". CrossLink adds its own "-" separator, + -- so ASCII punctuation and whitespace are dropped: "ALV-" becomes "ALV". + -- The C collation keeps the character classes ASCII-only whatever the + -- database locale, so non-ASCII characters such as "—" stay and fail + -- validation. + regexp_replace(settings.request_id_prefix_source COLLATE "C", + '[[:punct:][:space:]]', '', 'g') AS request_id_prefix +FROM ( SELECT max(coalesce(nullif(btrim(st_value), ''), nullif(btrim(st_default_value), ''))) FILTER (WHERE st_key = 'default_request_symbol') AS default_request_symbol, @@ -325,19 +336,10 @@ SELECT FILTER (WHERE st_key = 'default_copyright_type') AS default_copyright_type, max(coalesce(nullif(btrim(st_value), ''), nullif(btrim(st_default_value), ''))) FILTER (WHERE st_key = 'request_id_prefix') AS request_id_prefix_source, - -- mod-rs appends the HRID number directly to the prefix, so prefixes often - -- end in a separator such as "ALV-". CrossLink adds its own "-" separator, - -- so ASCII punctuation and whitespace are dropped: "ALV-" becomes "ALV". - -- The C collation keeps the character classes ASCII-only whatever the - -- database locale, so non-ASCII characters such as "—" stay and fail - -- validation. - regexp_replace( - (max(coalesce(nullif(btrim(st_value), ''), nullif(btrim(st_default_value), ''))) - FILTER (WHERE st_key = 'request_id_prefix')) COLLATE "C", - '[[:punct:][:space:]]', '', 'g') AS request_id_prefix, max(coalesce(nullif(btrim(st_value), ''), nullif(btrim(st_default_value), ''))) FILTER (WHERE st_key = 'last_resort_lenders') AS last_resort_lenders -FROM app_setting; +FROM app_setting +) AS settings; -- ISO18626 CopyrightCompliance codes. mod-rs copyrightType values are these -- codes lowercased, so they are matched case-insensitively. diff --git a/migration/scripts.md b/migration/scripts.md index e0182d802..4c625fc5f 100644 --- a/migration/scripts.md +++ b/migration/scripts.md @@ -68,8 +68,8 @@ The patron-request export keeps mod-rs HRIDs as borrowing request IDs. An HRID must be ASCII letters and digits, or such a prefix followed by `-` and digits (for example `ALV-10008`). When the broker imports a borrowing request with a `PREFIX-N` ID, it raises the HRID sequence for `PREFIX` past `N`, so new -CrossLink IDs cannot repeat a migrated one. Finish the patron-request import -before the tenant starts creating requests in CrossLink. +CrossLink IDs cannot repeat a migrated one, even when the tenant creates requests +during the import. See the [ILL configuration field mappings](export-crosslink-directory-field-mapping.md#ill-configuration) for source fields, setting and label fallbacks, ordering, and missing-list behavior.