From 0c952f33f48e3ac9aef5a2663c06f57a99fdf24e Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Wed, 27 May 2026 17:08:56 +0200 Subject: [PATCH 1/3] Pass pgcql.Query for patron requests --- broker/patron_request/api/api-handler.go | 10 ++- broker/patron_request/api/api-handler_test.go | 39 ++++++------ broker/patron_request/db/prcql.go | 41 +++++-------- broker/patron_request/db/prcql_test.go | 17 ++---- broker/patron_request/db/prrepo.go | 25 ++++---- broker/pullslip/api/api_handler.go | 9 ++- broker/pullslip/api/api_handler_test.go | 25 ++++---- broker/sqlc/pr_query.sql | 2 +- broker/test/patron_request/db/prrepo_test.go | 61 ++++++++++++------- 9 files changed, 125 insertions(+), 104 deletions(-) diff --git a/broker/patron_request/api/api-handler.go b/broker/patron_request/api/api-handler.go index e75a430f..53d505ad 100644 --- a/broker/patron_request/api/api-handler.go +++ b/broker/patron_request/api/api-handler.go @@ -174,7 +174,13 @@ func (a *PatronRequestApiHandler) GetPatronRequests(w http.ResponseWriter, r *ht return } cqlStr := cql.String() - prs, count, err := a.prRepo.ListPatronRequestsSearchView(ctx, pr_db.ListPatronRequestsParams{Limit: limit, Offset: offset}, &cqlStr) + pgcql, err := pr_db.ParsePatronRequestsCQL(cqlStr, 2) + if err != nil { + api.AddBadRequestError(ctx, w, err) + return + } + + prs, count, err := a.prRepo.ListPatronRequestsSearchView(ctx, pr_db.ListPatronRequestsParams{Limit: limit, Offset: offset}, pgcql) if err != nil && !errors.Is(err, pgx.ErrNoRows) { //DB error api.AddInternalError(ctx, w, err) return @@ -188,7 +194,7 @@ func (a *PatronRequestApiHandler) GetPatronRequests(w http.ResponseWriter, r *ht resp.About = toProAboutWithFacets(api.CollectAboutData(count, offset, limit, r)) var facets []pr_db.Facet if params.Facets != nil { - facets, err = a.prRepo.GetPatronRequestsFacets(ctx, *params.Facets, cqlStr) + facets, err = a.prRepo.GetPatronRequestsFacets(ctx, *params.Facets, pgcql) } if err != nil { if errors.Is(err, pr_db.ErrUnsupportedFacet) { diff --git a/broker/patron_request/api/api-handler_test.go b/broker/patron_request/api/api-handler_test.go index 6cd3f9da..556a54c5 100644 --- a/broker/patron_request/api/api-handler_test.go +++ b/broker/patron_request/api/api-handler_test.go @@ -14,6 +14,7 @@ import ( "time" "github.com/google/uuid" + "github.com/indexdata/cql-go/pgcql" "github.com/indexdata/crosslink/broker/common" "github.com/indexdata/crosslink/broker/events" "github.com/indexdata/crosslink/broker/handler" @@ -205,10 +206,10 @@ func TestGetPatronRequestsNoSymbol(t *testing.T) { } handler.GetPatronRequests(rr, req, params) assert.Equal(t, http.StatusOK, rr.Code) - if assert.NotNil(t, repo.cql) { - assert.Contains(t, *repo.cql, "side = lending") - assert.NotContains(t, *repo.cql, "supplier_symbol =") - assert.NotContains(t, *repo.cql, "requester_symbol =") + if assert.NotNil(t, repo.pgcql) { + assert.Contains(t, repo.pgcql.GetWhereClause(), "side =") + assert.NotContains(t, repo.pgcql.GetWhereClause(), "supplier_symbol =") + assert.NotContains(t, repo.pgcql.GetWhereClause(), "requester_symbol =") } } @@ -244,10 +245,10 @@ func TestGetPatronRequestsWithRequesterReqId(t *testing.T) { } handler.GetPatronRequests(rr, req, params) assert.Equal(t, http.StatusOK, rr.Code) - if assert.NotNil(t, repo.cql) { - assert.Contains(t, *repo.cql, "requester_req_id_exact = req-123") - assert.Contains(t, *repo.cql, "side = lending") - assert.Contains(t, *repo.cql, "supplier_symbol_exact = ISIL:REQ") + if assert.NotNil(t, repo.pgcql) { + assert.Contains(t, repo.pgcql.GetWhereClause(), "requester_req_id =") + assert.Contains(t, repo.pgcql.GetWhereClause(), "side =") + assert.Contains(t, repo.pgcql.GetWhereClause(), "supplier_symbol =") } } @@ -265,8 +266,8 @@ func TestGetPatronRequestsWithSymbolNoSideGroupsOwnerRestriction(t *testing.T) { handler.GetPatronRequests(rr, req, params) assert.Equal(t, http.StatusOK, rr.Code) - if assert.NotNil(t, repo.cql) { - assert.Equal(t, "id = pr-1 and (side = lending and supplier_symbol_exact = ISIL:REQ or (side = borrowing and requester_symbol_exact = ISIL:REQ))", *repo.cql) + if assert.NotNil(t, repo.pgcql) { + assert.Equal(t, "id = $3 AND ((side = $4 AND supplier_symbol = $5) OR (side = $6 AND requester_symbol = $7))", repo.pgcql.GetWhereClause()) } } @@ -940,7 +941,7 @@ func (r *PrRepoOkapiOwner) GetPatronRequestSearchView(ctx common.ExtendedContext type PrRepoCapture struct { PrRepoError - cql *string + pgcql pgcql.Query } type PrRepoNotificationsCapture struct { @@ -950,13 +951,13 @@ type PrRepoNotificationsCapture struct { fullCount int64 } -func (r *PrRepoCapture) ListPatronRequests(ctx common.ExtendedContext, args pr_db.ListPatronRequestsParams, cql *string) ([]pr_db.PatronRequest, int64, error) { - r.cql = cql +func (r *PrRepoCapture) ListPatronRequests(ctx common.ExtendedContext, args pr_db.ListPatronRequestsParams, pgcql pgcql.Query) ([]pr_db.PatronRequest, int64, error) { + r.pgcql = pgcql return []pr_db.PatronRequest{}, 0, nil } -func (r *PrRepoCapture) ListPatronRequestsSearchView(ctx common.ExtendedContext, args pr_db.ListPatronRequestsParams, cql *string) ([]pr_db.PatronRequestSearchView, int64, error) { - r.cql = cql +func (r *PrRepoCapture) ListPatronRequestsSearchView(ctx common.ExtendedContext, args pr_db.ListPatronRequestsParams, pgcql pgcql.Query) ([]pr_db.PatronRequestSearchView, int64, error) { + r.pgcql = pgcql return []pr_db.PatronRequestSearchView{}, 0, nil } @@ -986,11 +987,11 @@ func (r *PrRepoError) GetPatronRequestSearchView(ctx common.ExtendedContext, id return patronRequestSearchViewFromPatronRequest(pr, false), err } -func (r *PrRepoError) ListPatronRequests(ctx common.ExtendedContext, args pr_db.ListPatronRequestsParams, cql *string) ([]pr_db.PatronRequest, int64, error) { +func (r *PrRepoError) ListPatronRequests(ctx common.ExtendedContext, args pr_db.ListPatronRequestsParams, pgcql pgcql.Query) ([]pr_db.PatronRequest, int64, error) { return []pr_db.PatronRequest{}, 0, errors.New("DB error") } -func (r *PrRepoError) ListPatronRequestsSearchView(ctx common.ExtendedContext, args pr_db.ListPatronRequestsParams, cql *string) ([]pr_db.PatronRequestSearchView, int64, error) { +func (r *PrRepoError) ListPatronRequestsSearchView(ctx common.ExtendedContext, args pr_db.ListPatronRequestsParams, pgcql pgcql.Query) ([]pr_db.PatronRequestSearchView, int64, error) { return []pr_db.PatronRequestSearchView{}, 0, errors.New("DB error") } @@ -1044,7 +1045,7 @@ func (r *PrRepoError) GetNotificationById(ctx common.ExtendedContext, id string) } } -func (r *PrRepoError) GetPatronRequestsFacets(_ common.ExtendedContext, _ []string, _ string) ([]pr_db.Facet, error) { +func (r *PrRepoError) GetPatronRequestsFacets(_ common.ExtendedContext, _ []string, _ pgcql.Query) ([]pr_db.Facet, error) { return nil, errors.New("DB error") } @@ -1052,7 +1053,7 @@ type PrRepoFacetsUnsupported struct { PrRepoCapture } -func (r *PrRepoFacetsUnsupported) GetPatronRequestsFacets(_ common.ExtendedContext, _ []string, _ string) ([]pr_db.Facet, error) { +func (r *PrRepoFacetsUnsupported) GetPatronRequestsFacets(_ common.ExtendedContext, _ []string, _ pgcql.Query) ([]pr_db.Facet, error) { return nil, fmt.Errorf("%w: nosuch", pr_db.ErrUnsupportedFacet) } diff --git a/broker/patron_request/db/prcql.go b/broker/patron_request/db/prcql.go index da2d9d3d..c2ed86dd 100644 --- a/broker/patron_request/db/prcql.go +++ b/broker/patron_request/db/prcql.go @@ -74,7 +74,7 @@ func (f *FieldTextArrayContains) Generate(sc cql.SearchClause, queryArgumentInde } } -func handlePatronRequestsQuery(cqlString string, noBaseArgs int) (pgcql.Query, error) { +func ParsePatronRequestsCQL(cqlString string, noBaseArgs int) (pgcql.Query, error) { def := pgcql.NewPgDefinition() fa := &FieldAllRecords{} @@ -174,7 +174,7 @@ func handlePatronRequestsQuery(cqlString string, noBaseArgs int) (pgcql.Query, e // FacetsPatronRequestsCql substitutes it with the validated facet field at runtime. const facetFieldPlaceholder = "requester_symbol" -func (q *Queries) FacetsPatronRequestsCql(ctx context.Context, db DBTX, facetField string, cqlString string) ([]FacetsPatronRequestsRow, error) { +func (q *Queries) FacetsPatronRequestsCql(ctx context.Context, db DBTX, facetField string, pgcql pgcql.Query) ([]FacetsPatronRequestsRow, error) { // facetField is validated against an allowlist by the caller (GetPatronRequestsFacets), // so it is safe to substitute directly as a column name. sql := strings.Replace(facetsPatronRequests, facetFieldPlaceholder, facetField, 1) @@ -183,14 +183,13 @@ func (q *Queries) FacetsPatronRequestsCql(ctx context.Context, db DBTX, facetFie if idx == -1 { return nil, fmt.Errorf("base SQL query missing GROUP BY clause") } - res, err := handlePatronRequestsQuery(cqlString, 0) - if err != nil { - return nil, fmt.Errorf("failed to handle CQL query: %w", err) - } - if res.GetWhereClause() != "" { - sql = sql[:idx] + "AND (" + res.GetWhereClause() + ") " + sql[idx:] + if pgcql.GetWhereClause() != "" { + sql = sql[:idx] + "AND (" + pgcql.GetWhereClause() + ") " + sql[idx:] } - rows, err := db.Query(ctx, sql, res.GetQueryArguments()...) + sqlArguments := make([]interface{}, 0, 2+len(pgcql.GetQueryArguments())) + sqlArguments = append(sqlArguments, int64(100), int64(0)) // 100 facet values should be more than enough; offset is always 0 for facets + sqlArguments = append(sqlArguments, pgcql.GetQueryArguments()...) + rows, err := db.Query(ctx, sql, sqlArguments...) if err != nil { return nil, fmt.Errorf("failed to execute facets query: %w", err) } @@ -210,16 +209,8 @@ func (q *Queries) FacetsPatronRequestsCql(ctx context.Context, db DBTX, facetFie } func (q *Queries) ListPatronRequestsCql(ctx context.Context, db DBTX, arg ListPatronRequestsParams, - cqlString *string, explainAnalyze bool) ([]ListPatronRequestsRow, []string, error) { - if cqlString == nil { - rows, err := q.ListPatronRequests(ctx, db, arg) - return rows, nil, err - } + pgcql pgcql.Query, explainAnalyze bool) ([]ListPatronRequestsRow, []string, error) { noBaseArgs := 2 // we have two base arguments: limit and offset - res, err := handlePatronRequestsQuery(*cqlString, noBaseArgs) - if err != nil { - return nil, nil, err - } orgSql := listPatronRequests pos := strings.Index(orgSql, "ORDER BY") if pos == -1 { @@ -230,21 +221,21 @@ func (q *Queries) ListPatronRequestsCql(ctx context.Context, db DBTX, arg ListPa return nil, nil, fmt.Errorf("base query missing LIMIT") } orderBy := orgSql[pos:limitPos] - if res.GetOrderByClause() != "" { - orderBy = res.GetOrderByClause() + " " + if pgcql.GetOrderByClause() != "" { + orderBy = pgcql.GetOrderByClause() + " " } sqlPrefix := orgSql[:pos] - if res.GetWhereClause() != "" { + if pgcql.GetWhereClause() != "" { if strings.Contains(strings.ToUpper(sqlPrefix), "WHERE ") { - sqlPrefix += "AND " + res.GetWhereClause() + " " + sqlPrefix += "AND " + pgcql.GetWhereClause() + " " } else { - sqlPrefix += "WHERE " + res.GetWhereClause() + " " + sqlPrefix += "WHERE " + pgcql.GetWhereClause() + " " } } sql := sqlPrefix + orderBy + orgSql[limitPos:] - sqlArguments := make([]interface{}, 0, noBaseArgs+len(res.GetQueryArguments())) + sqlArguments := make([]interface{}, 0, noBaseArgs+len(pgcql.GetQueryArguments())) sqlArguments = append(sqlArguments, arg.Limit, arg.Offset) - sqlArguments = append(sqlArguments, res.GetQueryArguments()...) + sqlArguments = append(sqlArguments, pgcql.GetQueryArguments()...) explainResult := []string{} if explainAnalyze { explainRows, err := db.Query(ctx, "EXPLAIN ANALYZE "+sql, sqlArguments...) diff --git a/broker/patron_request/db/prcql_test.go b/broker/patron_request/db/prcql_test.go index 895a3c0b..39ad642b 100644 --- a/broker/patron_request/db/prcql_test.go +++ b/broker/patron_request/db/prcql_test.go @@ -4,15 +4,14 @@ import ( "testing" "github.com/indexdata/cql-go/cql" + "github.com/stretchr/testify/assert" ) func TestHandlePatronRequestsQueryKeepsOwnerRestrictionGrouped(t *testing.T) { cql := "cql.allRecords = 1 and (side = lending and supplier_symbol_exact = ISIL:REQ or (side = borrowing and requester_symbol_exact = ISIL:REQ))" - query, err := handlePatronRequestsQuery(cql, 2) - if err != nil { - t.Fatalf("handlePatronRequestsQuery() error = %v", err) - } + query, err := ParsePatronRequestsCQL(cql, 2) + assert.NoError(t, err, "ParsePatronRequestsCQL() error = %v", err) want := "TRUE AND ((side = $3 AND supplier_symbol = $4) OR (side = $5 AND requester_symbol = $6))" if got := query.GetWhereClause(); got != want { @@ -87,15 +86,11 @@ func TestFieldTextArrayContainsGenerate(t *testing.T) { func TestHandlePatronRequestsQueryIsbnUsesNormIsxn(t *testing.T) { cql := `isbn = "978-3-16-148410-0"` - query, err := handlePatronRequestsQuery(cql, 2) - if err != nil { - t.Fatalf("handlePatronRequestsQuery() error = %v", err) - } + query, err := ParsePatronRequestsCQL(cql, 2) + assert.NoError(t, err, "ParsePatronRequestsCQL() error = %v", err) wantWhere := "bibliographic_item_identifiers(ill_request, 'ISBN') @> ARRAY[norm_isxn($3)]::text[]" - if got := query.GetWhereClause(); got != wantWhere { - t.Fatalf("where clause = %q, want %q", got, wantWhere) - } + assert.Equal(t, wantWhere, query.GetWhereClause(), "where clause = %q, want %q", query.GetWhereClause(), wantWhere) } func searchClauseForTest(term, relation string) cql.SearchClause { diff --git a/broker/patron_request/db/prrepo.go b/broker/patron_request/db/prrepo.go index 39c71378..ceed9fdc 100644 --- a/broker/patron_request/db/prrepo.go +++ b/broker/patron_request/db/prrepo.go @@ -5,6 +5,7 @@ import ( "fmt" "strings" + "github.com/indexdata/cql-go/pgcql" "github.com/indexdata/crosslink/broker/common" "github.com/indexdata/crosslink/broker/repo" "github.com/jackc/pgx/v5" @@ -18,9 +19,9 @@ type PrRepo interface { GetPatronRequestSearchView(ctx common.ExtendedContext, id string) (PatronRequestSearchView, error) GetPatronRequestByIdForUpdate(ctx common.ExtendedContext, id string) (PatronRequest, error) GetPatronRequestByIdAndSide(ctx common.ExtendedContext, id string, side PatronRequestSide) (PatronRequest, error) - ListPatronRequests(ctx common.ExtendedContext, args ListPatronRequestsParams, cql *string) ([]PatronRequest, int64, error) - ListPatronRequestsSearchView(ctx common.ExtendedContext, args ListPatronRequestsParams, cql *string) ([]PatronRequestSearchView, int64, error) - GetPatronRequestsFacets(ctx common.ExtendedContext, facetFields []string, cql string) ([]Facet, error) + ListPatronRequests(ctx common.ExtendedContext, args ListPatronRequestsParams, pgcql pgcql.Query) ([]PatronRequest, int64, error) + ListPatronRequestsSearchView(ctx common.ExtendedContext, args ListPatronRequestsParams, pgcql pgcql.Query) ([]PatronRequestSearchView, int64, error) + GetPatronRequestsFacets(ctx common.ExtendedContext, facetFields []string, pgcql pgcql.Query) ([]Facet, error) UpdatePatronRequest(ctx common.ExtendedContext, params UpdatePatronRequestParams) (PatronRequest, error) UpdatePatronRequestInternalNote(ctx common.ExtendedContext, id string, internalNote pgtype.Text) error CreatePatronRequest(ctx common.ExtendedContext, params CreatePatronRequestParams) (PatronRequest, error) @@ -105,8 +106,8 @@ func (r *PgPrRepo) GetPatronRequestByIdAndSide(ctx common.ExtendedContext, id st return pr, nil } -func (r *PgPrRepo) ListPatronRequests(ctx common.ExtendedContext, params ListPatronRequestsParams, cql *string) ([]PatronRequest, int64, error) { - rows, fullCount, err := r.listPatronRequestRows(ctx, params, cql) +func (r *PgPrRepo) ListPatronRequests(ctx common.ExtendedContext, params ListPatronRequestsParams, pgcql pgcql.Query) ([]PatronRequest, int64, error) { + rows, fullCount, err := r.listPatronRequestRows(ctx, params, pgcql) if err != nil { return nil, fullCount, err } @@ -117,12 +118,12 @@ func (r *PgPrRepo) ListPatronRequests(ctx common.ExtendedContext, params ListPat return list, fullCount, nil } -func (r *PgPrRepo) GetPatronRequestsFacets(ctx common.ExtendedContext, facetFields []string, cql string) ([]Facet, error) { +func (r *PgPrRepo) GetPatronRequestsFacets(ctx common.ExtendedContext, facetFields []string, pgcql pgcql.Query) ([]Facet, error) { var facets []Facet for _, field := range facetFields { switch field { case "requester_symbol", "supplier_symbol": - rows, err := r.queries.FacetsPatronRequestsCql(ctx, r.GetConnOrTx(), field, cql) + rows, err := r.queries.FacetsPatronRequestsCql(ctx, r.GetConnOrTx(), field, pgcql) if err != nil { return nil, err } @@ -146,8 +147,8 @@ func (r *PgPrRepo) GetPatronRequestsFacets(ctx common.ExtendedContext, facetFiel return facets, nil } -func (r *PgPrRepo) ListPatronRequestsSearchView(ctx common.ExtendedContext, params ListPatronRequestsParams, cql *string) ([]PatronRequestSearchView, int64, error) { - rows, fullCount, err := r.listPatronRequestRows(ctx, params, cql) +func (r *PgPrRepo) ListPatronRequestsSearchView(ctx common.ExtendedContext, params ListPatronRequestsParams, pgcql pgcql.Query) ([]PatronRequestSearchView, int64, error) { + rows, fullCount, err := r.listPatronRequestRows(ctx, params, pgcql) if err != nil { return nil, fullCount, err } @@ -158,8 +159,8 @@ func (r *PgPrRepo) ListPatronRequestsSearchView(ctx common.ExtendedContext, para return list, fullCount, nil } -func (r *PgPrRepo) listPatronRequestRows(ctx common.ExtendedContext, params ListPatronRequestsParams, cql *string) ([]ListPatronRequestsRow, int64, error) { - rows, explainResult, err := r.queries.ListPatronRequestsCql(ctx, r.GetConnOrTx(), params, cql, r.explainAnalyze) +func (r *PgPrRepo) listPatronRequestRows(ctx common.ExtendedContext, params ListPatronRequestsParams, pgcql pgcql.Query) ([]ListPatronRequestsRow, int64, error) { + rows, explainResult, err := r.queries.ListPatronRequestsCql(ctx, r.GetConnOrTx(), params, pgcql, r.explainAnalyze) var fullCount int64 if err == nil { for _, line := range explainResult { @@ -170,7 +171,7 @@ func (r *PgPrRepo) listPatronRequestRows(ctx common.ExtendedContext, params List } else { params.Limit = 1 params.Offset = 0 - countRows, _, countErr := r.queries.ListPatronRequestsCql(ctx, r.GetConnOrTx(), params, cql, false) + countRows, _, countErr := r.queries.ListPatronRequestsCql(ctx, r.GetConnOrTx(), params, pgcql, false) err = countErr if err == nil && len(countRows) > 0 { fullCount = countRows[0].FullCount diff --git a/broker/pullslip/api/api_handler.go b/broker/pullslip/api/api_handler.go index 385a0272..6e3f0e65 100644 --- a/broker/pullslip/api/api_handler.go +++ b/broker/pullslip/api/api_handler.go @@ -3,6 +3,7 @@ package psapi import ( "encoding/json" "errors" + "fmt" "net/http" "strings" "time" @@ -215,7 +216,13 @@ func (p PullSlipApiHandler) getPullSlip(ctx common.ExtendedContext, w http.Respo } func (p PullSlipApiHandler) getPdfByte(ctx common.ExtendedContext, w http.ResponseWriter, cql string) ([]byte, error) { - prs, _, err := p.prRepo.ListPatronRequests(ctx, pr_db.ListPatronRequestsParams{Limit: MAX_RECORDS_PER_PDF, Offset: 0}, &cql) + pgcql, err := pr_db.ParsePatronRequestsCQL(cql, 2) + if err != nil { + api.AddBadRequestError(ctx, w, fmt.Errorf("invalid CQL query: %w", err)) + return []byte{}, err + } + + prs, _, err := p.prRepo.ListPatronRequests(ctx, pr_db.ListPatronRequestsParams{Limit: MAX_RECORDS_PER_PDF, Offset: 0}, pgcql) if err != nil { if errors.Is(err, pgx.ErrNoRows) { api.AddNotFoundError(w) diff --git a/broker/pullslip/api/api_handler_test.go b/broker/pullslip/api/api_handler_test.go index 23f030d2..f30ff5a8 100644 --- a/broker/pullslip/api/api_handler_test.go +++ b/broker/pullslip/api/api_handler_test.go @@ -8,6 +8,7 @@ import ( "strings" "testing" + "github.com/indexdata/cql-go/pgcql" "github.com/indexdata/crosslink/broker/common" pr_db "github.com/indexdata/crosslink/broker/patron_request/db" ps_db "github.com/indexdata/crosslink/broker/pullslip/db" @@ -41,8 +42,8 @@ type MockPrRepo struct { pr_db.PrRepo } -func (m *MockPrRepo) ListPatronRequests(ctx common.ExtendedContext, params pr_db.ListPatronRequestsParams, cql *string) ([]pr_db.PatronRequest, int64, error) { - args := m.Called(*cql) +func (m *MockPrRepo) ListPatronRequests(ctx common.ExtendedContext, params pr_db.ListPatronRequestsParams, pgcql pgcql.Query) ([]pr_db.PatronRequest, int64, error) { + args := m.Called(pgcql) return args.Get(0).([]pr_db.PatronRequest), args.Get(1).(int64), args.Error(2) } @@ -207,7 +208,9 @@ func patronRequest(id, requesterSym string) pr_db.PatronRequest { func TestPostPullslips_OK(t *testing.T) { pr := patronRequest("pr-1", sym) prRepo := new(MockPrRepo) - prRepo.On("ListPatronRequests", "id any pr-1 and (side = lending and supplier_symbol_exact = ISIL:TEST or (side = borrowing and requester_symbol_exact = ISIL:TEST))").Return([]pr_db.PatronRequest{pr}, int64(1), nil) + + prRepo.On("ListPatronRequests", mock.Anything).Return([]pr_db.PatronRequest{pr}, int64(1), nil) + prRepo.On("GetNotificationsByPrId", "pr-1", string(pr_db.NotificationKindNote)).Return([]pr_db.Notification{{Note: pgtype.Text{String: "Be careful with item"}}}, int64(1), nil) prRepo.On("GetNotificationsByPrId", "pr-1", string(pr_db.NotificationKindCondition)).Return([]pr_db.Notification{{Condition: pgtype.Text{String: "Library use only"}}}, int64(1), nil) @@ -249,7 +252,7 @@ func TestPostPullslips_InvalidJSON(t *testing.T) { func TestPostPullslips_PrNotFound(t *testing.T) { prRepo := new(MockPrRepo) - prRepo.On("ListPatronRequests", "id any pr-missing and (side = lending and supplier_symbol_exact = ISIL:TEST or (side = borrowing and requester_symbol_exact = ISIL:TEST))").Return([]pr_db.PatronRequest{}, int64(1), pgx.ErrNoRows) + prRepo.On("ListPatronRequests", mock.Anything).Return([]pr_db.PatronRequest{}, int64(1), pgx.ErrNoRows) h := newHandler(nil, prRepo) req := newRequest(http.MethodPost, postBody("pr-missing")) @@ -262,7 +265,7 @@ func TestPostPullslips_PrNotFound(t *testing.T) { func TestPostPullslips_PrRepoError(t *testing.T) { prRepo := new(MockPrRepo) - prRepo.On("ListPatronRequests", "id any pr-err and (side = lending and supplier_symbol_exact = ISIL:TEST or (side = borrowing and requester_symbol_exact = ISIL:TEST))").Return([]pr_db.PatronRequest{}, int64(1), errors.New("db err")) + prRepo.On("ListPatronRequests", mock.Anything).Return([]pr_db.PatronRequest{}, int64(1), errors.New("db err")) h := newHandler(nil, prRepo) req := newRequest(http.MethodPost, postBody("pr-err")) @@ -277,7 +280,7 @@ func TestPostPullslips_SaveError(t *testing.T) { pr := patronRequest("pr-1", sym) prRepo := new(MockPrRepo) - prRepo.On("ListPatronRequests", "id any pr-1 and (side = lending and supplier_symbol_exact = ISIL:TEST or (side = borrowing and requester_symbol_exact = ISIL:TEST))").Return([]pr_db.PatronRequest{pr}, int64(1), nil) + prRepo.On("ListPatronRequests", mock.Anything).Return([]pr_db.PatronRequest{pr}, int64(1), nil) prRepo.On("GetNotificationsByPrId", "pr-1", string(pr_db.NotificationKindNote)).Return([]pr_db.Notification{{Note: pgtype.Text{String: "Be careful with item"}}}, int64(1), nil) prRepo.On("GetNotificationsByPrId", "pr-1", string(pr_db.NotificationKindCondition)).Return([]pr_db.Notification{{Condition: pgtype.Text{String: "Library use only"}}}, int64(1), nil) @@ -323,7 +326,7 @@ func cqlBody(cql string) string { func TestPostPullslips_CqlBased_OK(t *testing.T) { pr := patronRequest("pr-cql", sym) prRepo := new(MockPrRepo) - prRepo.On("ListPatronRequests", mock.AnythingOfType("string")).Return([]pr_db.PatronRequest{pr}, int64(1), nil) + prRepo.On("ListPatronRequests", mock.Anything).Return([]pr_db.PatronRequest{pr}, int64(1), nil) prRepo.On("GetNotificationsByPrId", "pr-cql", string(pr_db.NotificationKindNote)).Return([]pr_db.Notification{}, int64(0), nil) prRepo.On("GetNotificationsByPrId", "pr-cql", string(pr_db.NotificationKindCondition)).Return([]pr_db.Notification{}, int64(0), nil) @@ -343,7 +346,7 @@ func TestPostPullslips_CqlBased_OK(t *testing.T) { func TestPostPullslips_EmptyPrList(t *testing.T) { prRepo := new(MockPrRepo) - prRepo.On("ListPatronRequests", mock.AnythingOfType("string")).Return([]pr_db.PatronRequest{}, int64(0), nil) + prRepo.On("ListPatronRequests", mock.Anything).Return([]pr_db.PatronRequest{}, int64(0), nil) h := newHandler(nil, prRepo) req := newRequest(http.MethodPost, postBody("pr-none")) @@ -362,7 +365,7 @@ func TestPostPullslipsIdRegenerate_OK(t *testing.T) { pr := patronRequest("pr-1", sym) prRepo := new(MockPrRepo) - prRepo.On("ListPatronRequests", slip.SearchCriteria).Return([]pr_db.PatronRequest{pr}, int64(1), nil) + prRepo.On("ListPatronRequests", mock.Anything).Return([]pr_db.PatronRequest{pr}, int64(1), nil) prRepo.On("GetNotificationsByPrId", "pr-1", string(pr_db.NotificationKindNote)).Return([]pr_db.Notification{}, int64(0), nil) prRepo.On("GetNotificationsByPrId", "pr-1", string(pr_db.NotificationKindCondition)).Return([]pr_db.Notification{}, int64(0), nil) @@ -403,7 +406,7 @@ func TestPostPullslipsIdRegenerate_PrRepoError(t *testing.T) { psRepo.On("GetPullSlipByIdAndOwner", "ps-err", sym).Return(slip, nil) prRepo := new(MockPrRepo) - prRepo.On("ListPatronRequests", slip.SearchCriteria).Return([]pr_db.PatronRequest{}, int64(0), errors.New("db err")) + prRepo.On("ListPatronRequests", mock.Anything).Return([]pr_db.PatronRequest{}, int64(0), errors.New("db err")) h := newHandler(psRepo, prRepo) req := newRequest(http.MethodPost, "") @@ -421,7 +424,7 @@ func TestPostPullslipsIdRegenerate_SaveError(t *testing.T) { pr := patronRequest("pr-1", sym) prRepo := new(MockPrRepo) - prRepo.On("ListPatronRequests", slip.SearchCriteria).Return([]pr_db.PatronRequest{pr}, int64(1), nil) + prRepo.On("ListPatronRequests", mock.Anything).Return([]pr_db.PatronRequest{pr}, int64(1), nil) prRepo.On("GetNotificationsByPrId", "pr-1", string(pr_db.NotificationKindNote)).Return([]pr_db.Notification{}, int64(0), nil) prRepo.On("GetNotificationsByPrId", "pr-1", string(pr_db.NotificationKindCondition)).Return([]pr_db.Notification{}, int64(0), nil) diff --git a/broker/sqlc/pr_query.sql b/broker/sqlc/pr_query.sql index 5221481f..9bf20bcf 100644 --- a/broker/sqlc/pr_query.sql +++ b/broker/sqlc/pr_query.sql @@ -33,7 +33,7 @@ FROM patron_request_search_view WHERE ill_request IS NOT NULL GROUP BY 1 ORDER BY count DESC, value ASC -LIMIT 100; +LIMIT $1 OFFSET $2; -- name: UpdatePatronRequest :one -- internal_note ($20) is a pass-through to keep PatronRequest <-> UpdatePatronRequestParams diff --git a/broker/test/patron_request/db/prrepo_test.go b/broker/test/patron_request/db/prrepo_test.go index 9c0c1dca..34d4bf56 100644 --- a/broker/test/patron_request/db/prrepo_test.go +++ b/broker/test/patron_request/db/prrepo_test.go @@ -492,122 +492,137 @@ func TestListPatronRequests(t *testing.T) { assert.NoError(t, err) } cql := "title = Androids" + pgcql, err := pr_db.ParsePatronRequestsCQL(cql, 2) + assert.NoError(t, err) list, fullCount, err := prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ Limit: 1, Offset: 0, - }, &cql) + }, pgcql) assert.NoError(t, err) assert.Len(t, list, 1) assert.Equal(t, int64(2), fullCount) cql = "requester_symbol = isil:req" + pgcql, err = pr_db.ParsePatronRequestsCQL(cql, 2) + assert.NoError(t, err) list, fullCount, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ Limit: 10, Offset: 0, - }, &cql) + }, pgcql) assert.NoError(t, err) assert.Len(t, list, 2) assert.Equal(t, int64(2), fullCount) cql = "supplier_symbol = isil:sup" + pgcql, err = pr_db.ParsePatronRequestsCQL(cql, 2) + assert.NoError(t, err) list, fullCount, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ Limit: 10, Offset: 0, - }, &cql) + }, pgcql) assert.NoError(t, err) assert.Len(t, list, 2) assert.Equal(t, int64(2), fullCount) cql = "requester_req_id = req-123" + pgcql, err = pr_db.ParsePatronRequestsCQL(cql, 2) + assert.NoError(t, err) list, fullCount, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ Limit: 10, Offset: 0, - }, &cql) + }, pgcql) assert.NoError(t, err) assert.Len(t, list, 2) assert.Equal(t, int64(2), fullCount) cql = `isbn = "978-3-16-148410-0"` + pgcql, err = pr_db.ParsePatronRequestsCQL(cql, 2) + assert.NoError(t, err) list, fullCount, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ Limit: 10, Offset: 0, - }, &cql) + }, pgcql) assert.NoError(t, err) assert.Len(t, list, 2) assert.Equal(t, int64(2), fullCount) cql = `issn = "2049-3630"` + pgcql, err = pr_db.ParsePatronRequestsCQL(cql, 2) + assert.NoError(t, err) list, fullCount, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ Limit: 10, Offset: 0, - }, &cql) + }, pgcql) assert.NoError(t, err) assert.Len(t, list, 2) assert.Equal(t, int64(2), fullCount) cql = `isbn = "9783161484100"` + pgcql, err = pr_db.ParsePatronRequestsCQL(cql, 2) + assert.NoError(t, err) list, fullCount, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ Limit: 10, Offset: 0, - }, &cql) + }, pgcql) assert.NoError(t, err) assert.Len(t, list, 2) assert.Equal(t, int64(2), fullCount) cql = `issn = "20493630"` + pgcql, err = pr_db.ParsePatronRequestsCQL(cql, 2) + assert.NoError(t, err) list, fullCount, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ Limit: 10, Offset: 0, - }, &cql) + }, pgcql) assert.NoError(t, err) assert.Len(t, list, 2) assert.Equal(t, int64(2), fullCount) cql = `issn = "1234567x"` + pgcql, err = pr_db.ParsePatronRequestsCQL(cql, 2) + assert.NoError(t, err) list, fullCount, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ Limit: 10, Offset: 0, - }, &cql) + }, pgcql) assert.NoError(t, err) assert.Len(t, list, 2) assert.Equal(t, int64(2), fullCount) // not found cql = "title = banners" + pgcql, err = pr_db.ParsePatronRequestsCQL(cql, 2) + assert.NoError(t, err) list, fullCount, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ Limit: 10, Offset: 0, - }, &cql) + }, pgcql) assert.NoError(t, err) assert.Len(t, list, 0) assert.Equal(t, int64(0), fullCount) - // no CQL - list, fullCount, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ - Limit: 1, - Offset: 0, - }, nil) - assert.NoError(t, err) - assert.Len(t, list, 1) - assert.Equal(t, int64(2), fullCount) - cql = "cql.allRecords=1" + pgcql, err = pr_db.ParsePatronRequestsCQL(cql, 2) + assert.NoError(t, err) list, fullCount, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ Limit: 1, Offset: 0, - }, &cql) + }, pgcql) assert.NoError(t, err) assert.Len(t, list, 1) assert.Equal(t, int64(2), fullCount) // has_internal_note=true selects requests that have a note (and round-trips its value) cql = `requester_req_id_exact = REQ-123 and has_internal_note=true` + pgcql, err = pr_db.ParsePatronRequestsCQL(cql, 2) + assert.NoError(t, err) list, _, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ Limit: 10, Offset: 0, - }, &cql) + }, pgcql) assert.NoError(t, err) if assert.Len(t, list, 1) { assert.Equal(t, prIds[0], list[0].ID) @@ -616,10 +631,12 @@ func TestListPatronRequests(t *testing.T) { // has_internal_note=false selects requests without one cql = `requester_req_id_exact = REQ-123 and has_internal_note=false` + pgcql, err = pr_db.ParsePatronRequestsCQL(cql, 2) + assert.NoError(t, err) list, _, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ Limit: 10, Offset: 0, - }, &cql) + }, pgcql) assert.NoError(t, err) if assert.Len(t, list, 1) { assert.Equal(t, prIds[1], list[0].ID) From 031ade2ba4e7efad187bea256640161c359cefb1 Mon Sep 17 00:00:00 2001 From: Jakub Skoczen Date: Wed, 27 May 2026 19:54:52 +0200 Subject: [PATCH 2/3] Copilot + cosmetics --- broker/patron_request/api/api-handler.go | 2 +- broker/patron_request/db/prcql.go | 33 +++++++++++++------- broker/patron_request/db/prcql_test.go | 4 +-- broker/patron_request/db/prrepo.go | 2 +- broker/pullslip/api/api_handler.go | 7 +++-- broker/pullslip/api/api_handler_test.go | 30 ++++++++++++------ broker/sqlc/pr_query.sql | 4 +-- broker/test/patron_request/db/prrepo_test.go | 26 +++++++-------- 8 files changed, 66 insertions(+), 42 deletions(-) diff --git a/broker/patron_request/api/api-handler.go b/broker/patron_request/api/api-handler.go index 53d505ad..3ed0d7d2 100644 --- a/broker/patron_request/api/api-handler.go +++ b/broker/patron_request/api/api-handler.go @@ -174,7 +174,7 @@ func (a *PatronRequestApiHandler) GetPatronRequests(w http.ResponseWriter, r *ht return } cqlStr := cql.String() - pgcql, err := pr_db.ParsePatronRequestsCQL(cqlStr, 2) + pgcql, err := pr_db.ParsePatronRequestsCql(cqlStr) if err != nil { api.AddBadRequestError(ctx, w, err) return diff --git a/broker/patron_request/db/prcql.go b/broker/patron_request/db/prcql.go index c2ed86dd..0069e640 100644 --- a/broker/patron_request/db/prcql.go +++ b/broker/patron_request/db/prcql.go @@ -12,6 +12,8 @@ import ( var LANGUAGE = utils.GetEnv("LANGUAGE", "english") +const NumberBaseArgs = 2 // SQLC base query has two args: $1=limit, $2=offset + type FieldAllRecords struct{} func (f *FieldAllRecords) GetColumn() string { return "" } @@ -74,7 +76,11 @@ func (f *FieldTextArrayContains) Generate(sc cql.SearchClause, queryArgumentInde } } -func ParsePatronRequestsCQL(cqlString string, noBaseArgs int) (pgcql.Query, error) { +// ParsePatronRequestsCql parses cqlString into a pgcql.Query whose placeholder +// numbering starts at $3, matching the two base SQL arguments (limit and offset) +// used by both ListPatronRequestsCql and GetPatronRequestsFacetsCql. +func ParsePatronRequestsCql(cqlString string) (pgcql.Query, error) { + def := pgcql.NewPgDefinition() fa := &FieldAllRecords{} @@ -167,17 +173,20 @@ func ParsePatronRequestsCQL(cqlString string, noBaseArgs int) (pgcql.Query, erro if err != nil { return nil, err } - return def.Parse(query, noBaseArgs+1) + return def.Parse(query, NumberBaseArgs+1) } -// facetFieldPlaceholder is the column name used in the facetsPatronRequests SQL template. -// FacetsPatronRequestsCql substitutes it with the validated facet field at runtime. +// facetFieldPlaceholder is the column name used in the getPatronRequestsFacets SQL template. +// GetPatronRequestsFacetsCql substitutes it with the validated facet field at runtime. const facetFieldPlaceholder = "requester_symbol" -func (q *Queries) FacetsPatronRequestsCql(ctx context.Context, db DBTX, facetField string, pgcql pgcql.Query) ([]FacetsPatronRequestsRow, error) { +func (q *Queries) GetPatronRequestsFacetsCql(ctx context.Context, db DBTX, facetField string, pgcql pgcql.Query) ([]GetPatronRequestsFacetsRow, error) { + if pgcql == nil { + return nil, fmt.Errorf("pgcql.Query must not be nil; use cql.allRecords=1 for no filter") + } // facetField is validated against an allowlist by the caller (GetPatronRequestsFacets), // so it is safe to substitute directly as a column name. - sql := strings.Replace(facetsPatronRequests, facetFieldPlaceholder, facetField, 1) + sql := strings.Replace(getPatronRequestsFacets, facetFieldPlaceholder, facetField, 1) idx := strings.Index(sql, "GROUP BY") if idx == -1 { @@ -186,7 +195,7 @@ func (q *Queries) FacetsPatronRequestsCql(ctx context.Context, db DBTX, facetFie if pgcql.GetWhereClause() != "" { sql = sql[:idx] + "AND (" + pgcql.GetWhereClause() + ") " + sql[idx:] } - sqlArguments := make([]interface{}, 0, 2+len(pgcql.GetQueryArguments())) + sqlArguments := make([]interface{}, 0, NumberBaseArgs+len(pgcql.GetQueryArguments())) sqlArguments = append(sqlArguments, int64(100), int64(0)) // 100 facet values should be more than enough; offset is always 0 for facets sqlArguments = append(sqlArguments, pgcql.GetQueryArguments()...) rows, err := db.Query(ctx, sql, sqlArguments...) @@ -194,9 +203,9 @@ func (q *Queries) FacetsPatronRequestsCql(ctx context.Context, db DBTX, facetFie return nil, fmt.Errorf("failed to execute facets query: %w", err) } defer rows.Close() - var items []FacetsPatronRequestsRow + var items []GetPatronRequestsFacetsRow for rows.Next() { - var i FacetsPatronRequestsRow + var i GetPatronRequestsFacetsRow if err := rows.Scan(&i.Value, &i.Count); err != nil { return nil, err } @@ -210,7 +219,9 @@ func (q *Queries) FacetsPatronRequestsCql(ctx context.Context, db DBTX, facetFie func (q *Queries) ListPatronRequestsCql(ctx context.Context, db DBTX, arg ListPatronRequestsParams, pgcql pgcql.Query, explainAnalyze bool) ([]ListPatronRequestsRow, []string, error) { - noBaseArgs := 2 // we have two base arguments: limit and offset + if pgcql == nil { + return nil, nil, fmt.Errorf("pgcql.Query must not be nil; use cql.allRecords=1 for no filter") + } orgSql := listPatronRequests pos := strings.Index(orgSql, "ORDER BY") if pos == -1 { @@ -233,7 +244,7 @@ func (q *Queries) ListPatronRequestsCql(ctx context.Context, db DBTX, arg ListPa } } sql := sqlPrefix + orderBy + orgSql[limitPos:] - sqlArguments := make([]interface{}, 0, noBaseArgs+len(pgcql.GetQueryArguments())) + sqlArguments := make([]interface{}, 0, NumberBaseArgs+len(pgcql.GetQueryArguments())) sqlArguments = append(sqlArguments, arg.Limit, arg.Offset) sqlArguments = append(sqlArguments, pgcql.GetQueryArguments()...) explainResult := []string{} diff --git a/broker/patron_request/db/prcql_test.go b/broker/patron_request/db/prcql_test.go index 39ad642b..9934d1a5 100644 --- a/broker/patron_request/db/prcql_test.go +++ b/broker/patron_request/db/prcql_test.go @@ -10,7 +10,7 @@ import ( func TestHandlePatronRequestsQueryKeepsOwnerRestrictionGrouped(t *testing.T) { cql := "cql.allRecords = 1 and (side = lending and supplier_symbol_exact = ISIL:REQ or (side = borrowing and requester_symbol_exact = ISIL:REQ))" - query, err := ParsePatronRequestsCQL(cql, 2) + query, err := ParsePatronRequestsCql(cql) assert.NoError(t, err, "ParsePatronRequestsCQL() error = %v", err) want := "TRUE AND ((side = $3 AND supplier_symbol = $4) OR (side = $5 AND requester_symbol = $6))" @@ -86,7 +86,7 @@ func TestFieldTextArrayContainsGenerate(t *testing.T) { func TestHandlePatronRequestsQueryIsbnUsesNormIsxn(t *testing.T) { cql := `isbn = "978-3-16-148410-0"` - query, err := ParsePatronRequestsCQL(cql, 2) + query, err := ParsePatronRequestsCql(cql) assert.NoError(t, err, "ParsePatronRequestsCQL() error = %v", err) wantWhere := "bibliographic_item_identifiers(ill_request, 'ISBN') @> ARRAY[norm_isxn($3)]::text[]" diff --git a/broker/patron_request/db/prrepo.go b/broker/patron_request/db/prrepo.go index ceed9fdc..4ec810ce 100644 --- a/broker/patron_request/db/prrepo.go +++ b/broker/patron_request/db/prrepo.go @@ -123,7 +123,7 @@ func (r *PgPrRepo) GetPatronRequestsFacets(ctx common.ExtendedContext, facetFiel for _, field := range facetFields { switch field { case "requester_symbol", "supplier_symbol": - rows, err := r.queries.FacetsPatronRequestsCql(ctx, r.GetConnOrTx(), field, pgcql) + rows, err := r.queries.GetPatronRequestsFacetsCql(ctx, r.GetConnOrTx(), field, pgcql) if err != nil { return nil, err } diff --git a/broker/pullslip/api/api_handler.go b/broker/pullslip/api/api_handler.go index 6e3f0e65..f8cb2108 100644 --- a/broker/pullslip/api/api_handler.go +++ b/broker/pullslip/api/api_handler.go @@ -216,10 +216,11 @@ func (p PullSlipApiHandler) getPullSlip(ctx common.ExtendedContext, w http.Respo } func (p PullSlipApiHandler) getPdfByte(ctx common.ExtendedContext, w http.ResponseWriter, cql string) ([]byte, error) { - pgcql, err := pr_db.ParsePatronRequestsCQL(cql, 2) + pgcql, err := pr_db.ParsePatronRequestsCql(cql) if err != nil { - api.AddBadRequestError(ctx, w, fmt.Errorf("invalid CQL query: %w", err)) - return []byte{}, err + wrappedErr := fmt.Errorf("invalid CQL query: %w", err) + api.AddBadRequestError(ctx, w, wrappedErr) + return []byte{}, wrappedErr } prs, _, err := p.prRepo.ListPatronRequests(ctx, pr_db.ListPatronRequestsParams{Limit: MAX_RECORDS_PER_PDF, Offset: 0}, pgcql) diff --git a/broker/pullslip/api/api_handler_test.go b/broker/pullslip/api/api_handler_test.go index f30ff5a8..db5f7e0b 100644 --- a/broker/pullslip/api/api_handler_test.go +++ b/broker/pullslip/api/api_handler_test.go @@ -56,6 +56,16 @@ func (m *MockPrRepo) GetNotificationsByPrId(ctx common.ExtendedContext, params p var sym = "ISIL:TEST" +// withOwnerRestriction returns a mock matcher that asserts the pgcql.Query WHERE +// clause contains the owner restriction (supplier_symbol and requester_symbol). +func withOwnerRestriction() interface{} { + return mock.MatchedBy(func(q pgcql.Query) bool { + wc := q.GetWhereClause() + return strings.Contains(wc, "supplier_symbol") && + strings.Contains(wc, "requester_symbol") + }) +} + func newHandler(psRepo ps_db.PsRepo, prRepo pr_db.PrRepo) PullSlipApiHandler { return NewPsApiHandler(psRepo, prRepo, tenant.NewResolver()) } @@ -209,7 +219,7 @@ func TestPostPullslips_OK(t *testing.T) { pr := patronRequest("pr-1", sym) prRepo := new(MockPrRepo) - prRepo.On("ListPatronRequests", mock.Anything).Return([]pr_db.PatronRequest{pr}, int64(1), nil) + prRepo.On("ListPatronRequests", withOwnerRestriction()).Return([]pr_db.PatronRequest{pr}, int64(1), nil) prRepo.On("GetNotificationsByPrId", "pr-1", string(pr_db.NotificationKindNote)).Return([]pr_db.Notification{{Note: pgtype.Text{String: "Be careful with item"}}}, int64(1), nil) prRepo.On("GetNotificationsByPrId", "pr-1", string(pr_db.NotificationKindCondition)).Return([]pr_db.Notification{{Condition: pgtype.Text{String: "Library use only"}}}, int64(1), nil) @@ -252,7 +262,7 @@ func TestPostPullslips_InvalidJSON(t *testing.T) { func TestPostPullslips_PrNotFound(t *testing.T) { prRepo := new(MockPrRepo) - prRepo.On("ListPatronRequests", mock.Anything).Return([]pr_db.PatronRequest{}, int64(1), pgx.ErrNoRows) + prRepo.On("ListPatronRequests", withOwnerRestriction()).Return([]pr_db.PatronRequest{}, int64(1), pgx.ErrNoRows) h := newHandler(nil, prRepo) req := newRequest(http.MethodPost, postBody("pr-missing")) @@ -265,7 +275,7 @@ func TestPostPullslips_PrNotFound(t *testing.T) { func TestPostPullslips_PrRepoError(t *testing.T) { prRepo := new(MockPrRepo) - prRepo.On("ListPatronRequests", mock.Anything).Return([]pr_db.PatronRequest{}, int64(1), errors.New("db err")) + prRepo.On("ListPatronRequests", withOwnerRestriction()).Return([]pr_db.PatronRequest{}, int64(1), errors.New("db err")) h := newHandler(nil, prRepo) req := newRequest(http.MethodPost, postBody("pr-err")) @@ -280,7 +290,7 @@ func TestPostPullslips_SaveError(t *testing.T) { pr := patronRequest("pr-1", sym) prRepo := new(MockPrRepo) - prRepo.On("ListPatronRequests", mock.Anything).Return([]pr_db.PatronRequest{pr}, int64(1), nil) + prRepo.On("ListPatronRequests", withOwnerRestriction()).Return([]pr_db.PatronRequest{pr}, int64(1), nil) prRepo.On("GetNotificationsByPrId", "pr-1", string(pr_db.NotificationKindNote)).Return([]pr_db.Notification{{Note: pgtype.Text{String: "Be careful with item"}}}, int64(1), nil) prRepo.On("GetNotificationsByPrId", "pr-1", string(pr_db.NotificationKindCondition)).Return([]pr_db.Notification{{Condition: pgtype.Text{String: "Library use only"}}}, int64(1), nil) @@ -326,7 +336,7 @@ func cqlBody(cql string) string { func TestPostPullslips_CqlBased_OK(t *testing.T) { pr := patronRequest("pr-cql", sym) prRepo := new(MockPrRepo) - prRepo.On("ListPatronRequests", mock.Anything).Return([]pr_db.PatronRequest{pr}, int64(1), nil) + prRepo.On("ListPatronRequests", withOwnerRestriction()).Return([]pr_db.PatronRequest{pr}, int64(1), nil) prRepo.On("GetNotificationsByPrId", "pr-cql", string(pr_db.NotificationKindNote)).Return([]pr_db.Notification{}, int64(0), nil) prRepo.On("GetNotificationsByPrId", "pr-cql", string(pr_db.NotificationKindCondition)).Return([]pr_db.Notification{}, int64(0), nil) @@ -346,7 +356,7 @@ func TestPostPullslips_CqlBased_OK(t *testing.T) { func TestPostPullslips_EmptyPrList(t *testing.T) { prRepo := new(MockPrRepo) - prRepo.On("ListPatronRequests", mock.Anything).Return([]pr_db.PatronRequest{}, int64(0), nil) + prRepo.On("ListPatronRequests", withOwnerRestriction()).Return([]pr_db.PatronRequest{}, int64(0), nil) h := newHandler(nil, prRepo) req := newRequest(http.MethodPost, postBody("pr-none")) @@ -365,7 +375,7 @@ func TestPostPullslipsIdRegenerate_OK(t *testing.T) { pr := patronRequest("pr-1", sym) prRepo := new(MockPrRepo) - prRepo.On("ListPatronRequests", mock.Anything).Return([]pr_db.PatronRequest{pr}, int64(1), nil) + prRepo.On("ListPatronRequests", withOwnerRestriction()).Return([]pr_db.PatronRequest{pr}, int64(1), nil) prRepo.On("GetNotificationsByPrId", "pr-1", string(pr_db.NotificationKindNote)).Return([]pr_db.Notification{}, int64(0), nil) prRepo.On("GetNotificationsByPrId", "pr-1", string(pr_db.NotificationKindCondition)).Return([]pr_db.Notification{}, int64(0), nil) @@ -406,7 +416,9 @@ func TestPostPullslipsIdRegenerate_PrRepoError(t *testing.T) { psRepo.On("GetPullSlipByIdAndOwner", "ps-err", sym).Return(slip, nil) prRepo := new(MockPrRepo) - prRepo.On("ListPatronRequests", mock.Anything).Return([]pr_db.PatronRequest{}, int64(0), errors.New("db err")) + prRepo.On("ListPatronRequests", mock.MatchedBy(func(q pgcql.Query) bool { + return strings.Contains(q.GetWhereClause(), "state") + })).Return([]pr_db.PatronRequest{}, int64(0), errors.New("db err")) h := newHandler(psRepo, prRepo) req := newRequest(http.MethodPost, "") @@ -424,7 +436,7 @@ func TestPostPullslipsIdRegenerate_SaveError(t *testing.T) { pr := patronRequest("pr-1", sym) prRepo := new(MockPrRepo) - prRepo.On("ListPatronRequests", mock.Anything).Return([]pr_db.PatronRequest{pr}, int64(1), nil) + prRepo.On("ListPatronRequests", withOwnerRestriction()).Return([]pr_db.PatronRequest{pr}, int64(1), nil) prRepo.On("GetNotificationsByPrId", "pr-1", string(pr_db.NotificationKindNote)).Return([]pr_db.Notification{}, int64(0), nil) prRepo.On("GetNotificationsByPrId", "pr-1", string(pr_db.NotificationKindCondition)).Return([]pr_db.Notification{}, int64(0), nil) diff --git a/broker/sqlc/pr_query.sql b/broker/sqlc/pr_query.sql index 9bf20bcf..c0483223 100644 --- a/broker/sqlc/pr_query.sql +++ b/broker/sqlc/pr_query.sql @@ -24,9 +24,9 @@ WHERE ill_request IS NOT NULL ORDER BY created_at LIMIT $1 OFFSET $2; --- name: FacetsPatronRequests :many +-- name: GetPatronRequestsFacets :many -- TEMPLATE: 'requester_symbol' is a placeholder column name. This query is not --- meant to be called directly; use FacetsPatronRequestsCql in prcql.go, which +-- meant to be called directly; use GetPatronRequestsFacetsCql in prcql.go, which -- substitutes the column with the validated facet field at runtime. SELECT requester_symbol AS value, COUNT(*) AS count FROM patron_request_search_view diff --git a/broker/test/patron_request/db/prrepo_test.go b/broker/test/patron_request/db/prrepo_test.go index 34d4bf56..4b24bc83 100644 --- a/broker/test/patron_request/db/prrepo_test.go +++ b/broker/test/patron_request/db/prrepo_test.go @@ -492,7 +492,7 @@ func TestListPatronRequests(t *testing.T) { assert.NoError(t, err) } cql := "title = Androids" - pgcql, err := pr_db.ParsePatronRequestsCQL(cql, 2) + pgcql, err := pr_db.ParsePatronRequestsCql(cql) assert.NoError(t, err) list, fullCount, err := prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ Limit: 1, @@ -504,7 +504,7 @@ func TestListPatronRequests(t *testing.T) { assert.Equal(t, int64(2), fullCount) cql = "requester_symbol = isil:req" - pgcql, err = pr_db.ParsePatronRequestsCQL(cql, 2) + pgcql, err = pr_db.ParsePatronRequestsCql(cql) assert.NoError(t, err) list, fullCount, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ Limit: 10, @@ -515,7 +515,7 @@ func TestListPatronRequests(t *testing.T) { assert.Equal(t, int64(2), fullCount) cql = "supplier_symbol = isil:sup" - pgcql, err = pr_db.ParsePatronRequestsCQL(cql, 2) + pgcql, err = pr_db.ParsePatronRequestsCql(cql) assert.NoError(t, err) list, fullCount, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ Limit: 10, @@ -526,7 +526,7 @@ func TestListPatronRequests(t *testing.T) { assert.Equal(t, int64(2), fullCount) cql = "requester_req_id = req-123" - pgcql, err = pr_db.ParsePatronRequestsCQL(cql, 2) + pgcql, err = pr_db.ParsePatronRequestsCql(cql) assert.NoError(t, err) list, fullCount, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ Limit: 10, @@ -537,7 +537,7 @@ func TestListPatronRequests(t *testing.T) { assert.Equal(t, int64(2), fullCount) cql = `isbn = "978-3-16-148410-0"` - pgcql, err = pr_db.ParsePatronRequestsCQL(cql, 2) + pgcql, err = pr_db.ParsePatronRequestsCql(cql) assert.NoError(t, err) list, fullCount, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ Limit: 10, @@ -548,7 +548,7 @@ func TestListPatronRequests(t *testing.T) { assert.Equal(t, int64(2), fullCount) cql = `issn = "2049-3630"` - pgcql, err = pr_db.ParsePatronRequestsCQL(cql, 2) + pgcql, err = pr_db.ParsePatronRequestsCql(cql) assert.NoError(t, err) list, fullCount, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ Limit: 10, @@ -559,7 +559,7 @@ func TestListPatronRequests(t *testing.T) { assert.Equal(t, int64(2), fullCount) cql = `isbn = "9783161484100"` - pgcql, err = pr_db.ParsePatronRequestsCQL(cql, 2) + pgcql, err = pr_db.ParsePatronRequestsCql(cql) assert.NoError(t, err) list, fullCount, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ Limit: 10, @@ -570,7 +570,7 @@ func TestListPatronRequests(t *testing.T) { assert.Equal(t, int64(2), fullCount) cql = `issn = "20493630"` - pgcql, err = pr_db.ParsePatronRequestsCQL(cql, 2) + pgcql, err = pr_db.ParsePatronRequestsCql(cql) assert.NoError(t, err) list, fullCount, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ Limit: 10, @@ -581,7 +581,7 @@ func TestListPatronRequests(t *testing.T) { assert.Equal(t, int64(2), fullCount) cql = `issn = "1234567x"` - pgcql, err = pr_db.ParsePatronRequestsCQL(cql, 2) + pgcql, err = pr_db.ParsePatronRequestsCql(cql) assert.NoError(t, err) list, fullCount, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ Limit: 10, @@ -593,7 +593,7 @@ func TestListPatronRequests(t *testing.T) { // not found cql = "title = banners" - pgcql, err = pr_db.ParsePatronRequestsCQL(cql, 2) + pgcql, err = pr_db.ParsePatronRequestsCql(cql) assert.NoError(t, err) list, fullCount, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ Limit: 10, @@ -605,7 +605,7 @@ func TestListPatronRequests(t *testing.T) { assert.Equal(t, int64(0), fullCount) cql = "cql.allRecords=1" - pgcql, err = pr_db.ParsePatronRequestsCQL(cql, 2) + pgcql, err = pr_db.ParsePatronRequestsCql(cql) assert.NoError(t, err) list, fullCount, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ Limit: 1, @@ -617,7 +617,7 @@ func TestListPatronRequests(t *testing.T) { // has_internal_note=true selects requests that have a note (and round-trips its value) cql = `requester_req_id_exact = REQ-123 and has_internal_note=true` - pgcql, err = pr_db.ParsePatronRequestsCQL(cql, 2) + pgcql, err = pr_db.ParsePatronRequestsCql(cql) assert.NoError(t, err) list, _, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ Limit: 10, @@ -631,7 +631,7 @@ func TestListPatronRequests(t *testing.T) { // has_internal_note=false selects requests without one cql = `requester_req_id_exact = REQ-123 and has_internal_note=false` - pgcql, err = pr_db.ParsePatronRequestsCQL(cql, 2) + pgcql, err = pr_db.ParsePatronRequestsCql(cql) assert.NoError(t, err) list, _, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ Limit: 10, From e81146a802aba567fafed46da5d389e4794972ca Mon Sep 17 00:00:00 2001 From: Jakub Skoczen Date: Wed, 27 May 2026 19:56:13 +0200 Subject: [PATCH 3/3] Lint --- broker/patron_request/db/prcql.go | 1 - 1 file changed, 1 deletion(-) diff --git a/broker/patron_request/db/prcql.go b/broker/patron_request/db/prcql.go index 0069e640..bb4dabb5 100644 --- a/broker/patron_request/db/prcql.go +++ b/broker/patron_request/db/prcql.go @@ -80,7 +80,6 @@ func (f *FieldTextArrayContains) Generate(sc cql.SearchClause, queryArgumentInde // numbering starts at $3, matching the two base SQL arguments (limit and offset) // used by both ListPatronRequestsCql and GetPatronRequestsFacetsCql. func ParsePatronRequestsCql(cqlString string) (pgcql.Query, error) { - def := pgcql.NewPgDefinition() fa := &FieldAllRecords{}