Normalize prefixes from mod-rs with hyphens, make sure hrid count sta…

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. 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:
 
 ```ndjson

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, bundle.PatronRequest.RequesterSymbol.String); err != nil {
+				return err
+			}
+		}
 
 		itemIDs := make([]string, 0, len(bundle.Items))
 		for _, item := range bundle.Items {
@@ -194,6 +201,40 @@ func (r *PgImportRepo) ImportPatronRequest(ctx common.ExtendedContext, bundle Pa
 	return result, err
 }
 
+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. 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 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 {
+		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)
+	}
+	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,

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()

broker/import/test/patron_request_test.go

@@ -3,6 +3,7 @@ package test
 import (
 	"context"
 	"os"
+	"strings"
 	"testing"
 	"time"
 
@@ -573,3 +574,109 @@ 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")
+}
+
+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, 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)+tc.wantNext, hrid)
+		})
+	}
+}
+
+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)
+}

broker/migrations/080_advance_hrid.down.sql

@@ -0,0 +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;

broker/migrations/080_advance_hrid.up.sql

@@ -0,0 +1,30 @@
+-- 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 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';
+    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
+        PERFORM setval(quote_ident(seq)::regclass, GREATEST(current_value, used));
+    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;

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,

broker/sqlc/pr_schema.sql

@@ -53,11 +53,27 @@ 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;
 $$ 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
+    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
+        PERFORM setval(quote_ident(seq)::regclass, GREATEST(current_value, used));
+    END IF;
+END;
+$$ LANGUAGE plpgsql;
+
 CREATE TABLE item
 (
     id             VARCHAR PRIMARY KEY,

directory/test/request_id_prefix_test.go

@@ -14,6 +14,8 @@ import (
 
 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"}
 
 func TestRequestIdPrefixPersistence(t *testing.T) {
@@ -143,10 +145,12 @@ 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: "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},
 		{name: "unset", local: true},

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. |