From bd3eb9f7258b34dd50cc6c9aed1b187c17aa3aee Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Tue, 26 May 2026 13:58:44 +0200 Subject: [PATCH 01/33] CROSSLINK-274 openapi changes --- broker/oapi/open-api.yaml | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/broker/oapi/open-api.yaml b/broker/oapi/open-api.yaml index ac983ac9..3bf43c77 100644 --- a/broker/oapi/open-api.yaml +++ b/broker/oapi/open-api.yaml @@ -93,6 +93,12 @@ components: description: Patron request ID schema: type: string + Facets: + name: facets + in: query + description: Comma separated list of facet names to include in the response, for example "requester_symbol,supplier_symbol" + schema: + type: string schemas: Index: type: object @@ -197,6 +203,34 @@ components: description: List of peers items: $ref: '#/components/schemas/LocatedSupplier' + FacetsResult: + type: array + description: List of facets for the result + items: + type: object + required: + - name + - values + properties: + name: + type: string + description: Facet name + values: + type: array + description: List of facet values + items: + type: object + required: + - value + - count + properties: + value: + type: string + description: Facet value + count: + type: integer + format: int64 + description: Count of items for this facet value About: type: object required: @@ -206,6 +240,8 @@ components: type: integer format: int64 description: Total number of items in the result + facets: + $ref: '#/components/schemas/FacetsResult' nextLink: type: string description: Link to the next page of results @@ -1374,6 +1410,7 @@ paths: - $ref: '#/components/parameters/Offset' - $ref: '#/components/parameters/Side' - $ref: '#/components/parameters/Symbol' + - $ref: '#/components/parameters/Facets' responses: '200': description: Successful retrieval of events From 18e1a8a868bf639d20bbcdf622118f94b7482486 Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Tue, 26 May 2026 17:11:11 +0200 Subject: [PATCH 02/33] Functional --- broker/oapi/open-api.yaml | 30 ++--- broker/patron_request/api/api-handler.go | 36 +++++- broker/patron_request/db/prcql.go | 41 +++++++ broker/patron_request/db/prrepo.go | 36 ++++++ broker/sqlc/pr_query.sql | 6 + .../patron_request/api/api-handler_test.go | 109 ++++++++++++++++++ 6 files changed, 242 insertions(+), 16 deletions(-) diff --git a/broker/oapi/open-api.yaml b/broker/oapi/open-api.yaml index 3bf43c77..bded463c 100644 --- a/broker/oapi/open-api.yaml +++ b/broker/oapi/open-api.yaml @@ -219,18 +219,20 @@ components: type: array description: List of facet values items: - type: object - required: - - value - - count - properties: - value: - type: string - description: Facet value - count: - type: integer - format: int64 - description: Count of items for this facet value + $ref: '#/components/schemas/FacetResultValue' + FacetResultValue: + type: object + required: + - value + - count + properties: + value: + type: string + description: Facet value + count: + type: integer + format: int64 + description: Count of items for this facet value About: type: object required: @@ -240,8 +242,6 @@ components: type: integer format: int64 description: Total number of items in the result - facets: - $ref: '#/components/schemas/FacetsResult' nextLink: type: string description: Link to the next page of results @@ -254,6 +254,8 @@ components: lastLink: type: string description: Link to the last page of results + facets: + $ref: '#/components/schemas/FacetsResult' Event: type: object properties: diff --git a/broker/patron_request/api/api-handler.go b/broker/patron_request/api/api-handler.go index 9dc7b2d7..26ebcfb7 100644 --- a/broker/patron_request/api/api-handler.go +++ b/broker/patron_request/api/api-handler.go @@ -185,10 +185,42 @@ func (a *PatronRequestApiHandler) GetPatronRequests(w http.ResponseWriter, r *ht } resp := proapi.PatronRequests{Items: responseItems} - resp.About = proapi.About(api.CollectAboutData(count, offset, limit, r)) + resp.About = toProAbout(api.CollectAboutData(count, offset, limit, r)) + facets, err := a.prRepo.FacetsPatronRequests(ctx, params.Facets, &cqlStr) + if err != nil { + api.AddBadRequestError(ctx, w, err) + return + } + if len(facets) > 0 { + facetResults := make(proapi.FacetsResult, len(facets)) + for i, field := range facets { + facetResults[i].Name = field.Field + facetResults[i].Values = make([]proapi.FacetResultValue, len(field.Values)) + for j, value := range field.Values { + facetResults[i].Values[j] = proapi.FacetResultValue{ + Value: value.Value, + Count: value.Count, + } + } + } + resp.About.Facets = &facetResults + } api.WriteJsonResponse(w, resp) } +// toProAbout converts an oapi.About to a proapi.About by copying the pagination fields. +// The two types are generated independently from identical OpenAPI schemas but are not +// directly convertible because their FacetValue types differ. +func toProAbout(a oapi.About) proapi.About { + return proapi.About{ + Count: a.Count, + FirstLink: a.FirstLink, + LastLink: a.LastLink, + NextLink: a.NextLink, + PrevLink: a.PrevLink, + } +} + func AddOwnerRestriction(queryBuilder *cqlbuilder.QueryBuilder, symbol string, side pr_db.PatronRequestSide) (*cqlbuilder.QueryBuilder, error) { var err error switch side { @@ -648,7 +680,7 @@ func (a *PatronRequestApiHandler) GetPatronRequestsIdNotifications(w http.Respon responseList = append(responseList, apiN) } resp := proapi.PrNotifications{Items: responseList} - resp.About = proapi.About(api.CollectAboutData(fullCount, offset, limit, r)) + resp.About = toProAbout(api.CollectAboutData(fullCount, offset, limit, r)) api.WriteJsonResponse(w, resp) } diff --git a/broker/patron_request/db/prcql.go b/broker/patron_request/db/prcql.go index b61db410..b6b49386 100644 --- a/broker/patron_request/db/prcql.go +++ b/broker/patron_request/db/prcql.go @@ -160,6 +160,47 @@ func handlePatronRequestsQuery(cqlString string, noBaseArgs int) (pgcql.Query, e return def.Parse(query, noBaseArgs+1) } +func (q *Queries) FacetsPatronRequestsCql(ctx context.Context, db DBTX, facetField string, cqlString *string) ([]FacetValue, error) { + orgSql := facetsRequesterSymbol + orgSql = strings.Replace(orgSql, "requester_symbol", facetField, 1) + + idx := strings.Index(orgSql, "GROUP BY") + if idx == -1 { + return nil, fmt.Errorf("base SQL query missing GROUP BY clause") + } + var queryArguments []any + if cqlString != nil { + res, err := handlePatronRequestsQuery(*cqlString, 0) + if err != nil { + return nil, fmt.Errorf("failed to handle CQL query: %w", err) + } + if res.GetWhereClause() != "" { + orgSql = orgSql[:idx] + "WHERE " + res.GetWhereClause() + " " + orgSql[idx:] + queryArguments = res.GetQueryArguments() + } + } + rows, err := db.Query(ctx, orgSql, queryArguments...) + if err != nil { + return nil, fmt.Errorf("failed to execute facets query: %w", err) + } + defer rows.Close() + var values []FacetValue + for rows.Next() { + var i FacetsRequesterSymbolRow + if err := rows.Scan(&i.Value, &i.Count); err != nil { + return nil, err + } + values = append(values, FacetValue{ + Value: i.Value.String, + Count: i.Count, + }) + } + if err := rows.Err(); err != nil { + return nil, err + } + return values, nil +} + func (q *Queries) ListPatronRequestsCql(ctx context.Context, db DBTX, arg ListPatronRequestsParams, cqlString *string, explainAnalyze bool) ([]ListPatronRequestsRow, []string, error) { if cqlString == nil { diff --git a/broker/patron_request/db/prrepo.go b/broker/patron_request/db/prrepo.go index 866f0a16..4714210f 100644 --- a/broker/patron_request/db/prrepo.go +++ b/broker/patron_request/db/prrepo.go @@ -1,6 +1,7 @@ package pr_db import ( + "fmt" "strings" "github.com/indexdata/crosslink/broker/common" @@ -18,6 +19,7 @@ type PrRepo interface { 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) + FacetsPatronRequests(ctx common.ExtendedContext, facets *string, cql *string) ([]Facet, error) UpdatePatronRequest(ctx common.ExtendedContext, params UpdatePatronRequestParams) (PatronRequest, error) CreatePatronRequest(ctx common.ExtendedContext, params CreatePatronRequestParams) (PatronRequest, error) DeletePatronRequest(ctx common.ExtendedContext, id string) error @@ -34,6 +36,16 @@ type PrRepo interface { DeleteItemById(ctx common.ExtendedContext, id string) error } +type Facet struct { + Field string + Values []FacetValue +} + +type FacetValue struct { + Value string + Count int64 +} + type PgPrRepo struct { repo.PgBaseRepo[PrRepo] queries Queries @@ -101,6 +113,30 @@ func (r *PgPrRepo) ListPatronRequests(ctx common.ExtendedContext, params ListPat return list, fullCount, nil } +func (r *PgPrRepo) FacetsPatronRequests(ctx common.ExtendedContext, facetParameter *string, cql *string) ([]Facet, error) { + if facetParameter == nil { + return nil, nil // no facets requested, return nil + } + facetFields := strings.Split(*facetParameter, ",") + var facets []Facet + for _, field := range facetFields { + switch field { + case "requester_symbol", "supplier_symbol": + values, err := r.queries.FacetsPatronRequestsCql(ctx, r.GetConnOrTx(), field, cql) + if err != nil { + return nil, err + } + facets = append(facets, Facet{ + Field: field, + Values: values, + }) + default: + return nil, fmt.Errorf("unsupported facet field: %s", field) + } + } + 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) if err != nil { diff --git a/broker/sqlc/pr_query.sql b/broker/sqlc/pr_query.sql index 613bc05e..f59a8bf6 100644 --- a/broker/sqlc/pr_query.sql +++ b/broker/sqlc/pr_query.sql @@ -24,6 +24,12 @@ WHERE ill_request IS NOT NULL ORDER BY created_at LIMIT $1 OFFSET $2; +-- name: FacetsRequesterSymbol :many +SELECT requester_symbol AS value, COUNT(*) AS count +FROM patron_request_search_view +GROUP BY value +ORDER BY count DESC; + -- name: UpdatePatronRequest :one UPDATE patron_request SET ill_request = $3, diff --git a/broker/test/patron_request/api/api-handler_test.go b/broker/test/patron_request/api/api-handler_test.go index 89c7d177..cb339868 100644 --- a/broker/test/patron_request/api/api-handler_test.go +++ b/broker/test/patron_request/api/api-handler_test.go @@ -785,3 +785,112 @@ func httpRequest(t *testing.T, method string, uriPath string, reqbytes []byte, e func getLocalhostWithPort() string { return "http://localhost:" + strconv.Itoa(app.HTTP_PORT) } + +func TestFacetsOK(t *testing.T) { + requesterSymbols := []string{"ISIL:REQ" + uuid.NewString(), "ISIL:REQ" + uuid.NewString()} + + for _, requesterSymbol := range requesterSymbols { + reqPeer := apptest.CreatePeerWithModeAndVendor(t, illRepo, requesterSymbol, adapter.MOCK_PEER_URL, app.BROKER_MODE, directory.CrossLink, + directory.Entry{ + LmsConfig: &directory.LmsConfig{ + FromAgency: "from-agency", + Address: ncipMockUrl, + }, + }) + assert.NotNil(t, reqPeer) + } + + for i := 0; i < 10; i++ { + serviceType := "Copy" + if i%2 == 0 { + serviceType = "Loan" + } + j := 0 + if i >= 7 { + j = 1 + } + // POST + patron := "p1" + request := iso18626.Request{ + ServiceInfo: &iso18626.ServiceInfo{ + ServiceType: iso18626.TypeServiceType(serviceType), + }, + BibliographicInfo: iso18626.BibliographicInfo{ + SupplierUniqueRecordId: uuid.NewString(), + Title: "Facets title " + strconv.Itoa(i), + }, + } + newPr := proapi.CreatePatronRequest{ + RequesterSymbol: &requesterSymbols[j], + Patron: &patron, + IllRequest: request, + } + newPrBytes, err := json.Marshal(newPr) + assert.NoError(t, err, "failed to marshal patron request") + + respBytes := httpRequest(t, "POST", basePath, newPrBytes, 201) + + var foundPr proapi.PatronRequest + err = json.Unmarshal(respBytes, &foundPr) + assert.NoError(t, err, "failed to unmarshal patron request") + } + + var foundPrs proapi.PatronRequests + respBytes := httpRequest(t, "GET", basePath+"?cql=service_type%3DCopy&offset=0&limit=1", []byte{}, 200) + err := json.Unmarshal(respBytes, &foundPrs) + // a little brittle as there are patron requests created in other tests, but should be ok as long as we create more than 5 in this test + assert.GreaterOrEqual(t, foundPrs.About.Count, int64(5)) + assert.Len(t, foundPrs.Items, 1) + assert.Nil(t, foundPrs.About.Facets) + + respBytes = httpRequest(t, "GET", basePath+"?facets=requester_symbol&cql=service_type%3DCopy&offset=0&limit=0", []byte{}, 200) + err = json.Unmarshal(respBytes, &foundPrs) + assert.NoError(t, err) + // a little brittle as there are patron requests created in other tests, but should be ok as long as we create more than 5 in this test + assert.GreaterOrEqual(t, foundPrs.About.Count, int64(5)) + assert.Len(t, foundPrs.Items, 0) + assert.NotNil(t, foundPrs.About.Facets) + assert.Len(t, *foundPrs.About.Facets, 1) + assert.Equal(t, "requester_symbol", (*foundPrs.About.Facets)[0].Name) + assert.GreaterOrEqual(t, len((*foundPrs.About.Facets)[0].Values), 2) + var count0 int64 + var count1 int64 + for i := range (*foundPrs.About.Facets)[0].Values { + switch (*foundPrs.About.Facets)[0].Values[i].Value { + case requesterSymbols[0]: + count0 = (*foundPrs.About.Facets)[0].Values[i].Count + case requesterSymbols[1]: + count1 = (*foundPrs.About.Facets)[0].Values[i].Count + } + } + assert.Equal(t, count0, int64(3)) + assert.Equal(t, count1, int64(2)) + + respBytes = httpRequest(t, "GET", basePath+"?facets=requester_symbol&cql=cql.allRecords%3Dtrue%20&offset=0&limit=0", []byte{}, 200) + err = json.Unmarshal(respBytes, &foundPrs) + assert.NoError(t, err) + // a little brittle as there are patron requests created in other tests, but should be ok as long as we create more than 5 in this test + assert.GreaterOrEqual(t, foundPrs.About.Count, int64(10)) + assert.Len(t, foundPrs.Items, 0) + assert.NotNil(t, foundPrs.About.Facets) + assert.Len(t, *foundPrs.About.Facets, 1) + assert.Equal(t, "requester_symbol", (*foundPrs.About.Facets)[0].Name) + assert.GreaterOrEqual(t, len((*foundPrs.About.Facets)[0].Values), 2) + count0 = 0 + count1 = 0 + for i := range (*foundPrs.About.Facets)[0].Values { + switch (*foundPrs.About.Facets)[0].Values[i].Value { + case requesterSymbols[0]: + count0 = (*foundPrs.About.Facets)[0].Values[i].Count + case requesterSymbols[1]: + count1 = (*foundPrs.About.Facets)[0].Values[i].Count + } + } + assert.Equal(t, count0, int64(7)) + assert.Equal(t, count1, int64(3)) +} + +func TestFacetsBadRequest(t *testing.T) { + respBytes := httpRequest(t, "GET", basePath+"?facets=nosuch", []byte{}, 400) + assert.Contains(t, string(respBytes), "unsupported facet field: nosuch") +} From 7adb3abe6a24b155cc739992ddc74f8339f8adec Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Tue, 26 May 2026 17:15:35 +0200 Subject: [PATCH 03/33] Lint --- broker/test/patron_request/api/api-handler_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/broker/test/patron_request/api/api-handler_test.go b/broker/test/patron_request/api/api-handler_test.go index cb339868..53f4ba79 100644 --- a/broker/test/patron_request/api/api-handler_test.go +++ b/broker/test/patron_request/api/api-handler_test.go @@ -838,6 +838,7 @@ func TestFacetsOK(t *testing.T) { var foundPrs proapi.PatronRequests respBytes := httpRequest(t, "GET", basePath+"?cql=service_type%3DCopy&offset=0&limit=1", []byte{}, 200) err := json.Unmarshal(respBytes, &foundPrs) + assert.NoError(t, err) // a little brittle as there are patron requests created in other tests, but should be ok as long as we create more than 5 in this test assert.GreaterOrEqual(t, foundPrs.About.Count, int64(5)) assert.Len(t, foundPrs.Items, 1) From e44396638e9f224ba7f7ff0447aab9c8d6b3dc53 Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Tue, 26 May 2026 17:29:11 +0200 Subject: [PATCH 04/33] About confusion --- broker/patron_request/api/api-handler.go | 2 +- broker/scheduler/api/api_handler.go | 16 +++++++++++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/broker/patron_request/api/api-handler.go b/broker/patron_request/api/api-handler.go index 5a38a19f..21508a11 100644 --- a/broker/patron_request/api/api-handler.go +++ b/broker/patron_request/api/api-handler.go @@ -210,7 +210,7 @@ func (a *PatronRequestApiHandler) GetPatronRequests(w http.ResponseWriter, r *ht // toProAbout converts an oapi.About to a proapi.About by copying the pagination fields. // The two types are generated independently from identical OpenAPI schemas but are not -// directly convertible because their FacetValue types differ. +// directly convertible because their Facets types differ. func toProAbout(a oapi.About) proapi.About { return proapi.About{ Count: a.Count, diff --git a/broker/scheduler/api/api_handler.go b/broker/scheduler/api/api_handler.go index 4765b2fc..8b88864e 100644 --- a/broker/scheduler/api/api_handler.go +++ b/broker/scheduler/api/api_handler.go @@ -10,6 +10,7 @@ import ( brokerapi "github.com/indexdata/crosslink/broker/api" "github.com/indexdata/crosslink/broker/common" "github.com/indexdata/crosslink/broker/events" + "github.com/indexdata/crosslink/broker/oapi" sched_db "github.com/indexdata/crosslink/broker/scheduler/db" schedoapi "github.com/indexdata/crosslink/broker/scheduler/oapi" sched_service "github.com/indexdata/crosslink/broker/scheduler/service" @@ -64,12 +65,25 @@ func (h SchedulerApiHandler) GetBatchActions(w http.ResponseWriter, r *http.Requ } resp := schedoapi.BatchActions{ - About: schedoapi.About(brokerapi.CollectAboutData(count, offset, limit, r)), + About: toSchedAbout(brokerapi.CollectAboutData(count, offset, limit, r)), Items: toBatchActionList(items), } brokerapi.WriteJsonResponse(w, resp) } +// toSchedAbout converts an oapi.About to a schedoapi.About by copying the pagination fields. +// The two types are generated independently from identical OpenAPI schemas but are not +// directly convertible because their Facets types differ. +func toSchedAbout(a oapi.About) schedoapi.About { + return schedoapi.About{ + Count: a.Count, + FirstLink: a.FirstLink, + LastLink: a.LastLink, + NextLink: a.NextLink, + PrevLink: a.PrevLink, + } +} + // PostBatchActions creates a new batch action. func (h SchedulerApiHandler) PostBatchActions(w http.ResponseWriter, r *http.Request, params schedoapi.PostBatchActionsParams) { logParams := map[string]string{"method": "PostBatchActions"} From 8c8771e420d874b455290cb596371782ae8b9da9 Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Tue, 26 May 2026 17:37:31 +0200 Subject: [PATCH 05/33] Includes ill_request IS NOT NULL --- broker/patron_request/db/prcql.go | 10 +++++----- broker/sqlc/pr_query.sql | 1 + 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/broker/patron_request/db/prcql.go b/broker/patron_request/db/prcql.go index 92c43e85..943924f5 100644 --- a/broker/patron_request/db/prcql.go +++ b/broker/patron_request/db/prcql.go @@ -167,10 +167,10 @@ func handlePatronRequestsQuery(cqlString string, noBaseArgs int) (pgcql.Query, e } func (q *Queries) FacetsPatronRequestsCql(ctx context.Context, db DBTX, facetField string, cqlString *string) ([]FacetValue, error) { - orgSql := facetsRequesterSymbol - orgSql = strings.Replace(orgSql, "requester_symbol", facetField, 1) + sql := facetsRequesterSymbol + sql = strings.Replace(sql, "requester_symbol", facetField, 1) - idx := strings.Index(orgSql, "GROUP BY") + idx := strings.Index(sql, "GROUP BY") if idx == -1 { return nil, fmt.Errorf("base SQL query missing GROUP BY clause") } @@ -181,11 +181,11 @@ func (q *Queries) FacetsPatronRequestsCql(ctx context.Context, db DBTX, facetFie return nil, fmt.Errorf("failed to handle CQL query: %w", err) } if res.GetWhereClause() != "" { - orgSql = orgSql[:idx] + "WHERE " + res.GetWhereClause() + " " + orgSql[idx:] + sql = sql[:idx] + "AND " + res.GetWhereClause() + " " + sql[idx:] queryArguments = res.GetQueryArguments() } } - rows, err := db.Query(ctx, orgSql, queryArguments...) + rows, err := db.Query(ctx, sql, queryArguments...) if err != nil { return nil, fmt.Errorf("failed to execute facets query: %w", err) } diff --git a/broker/sqlc/pr_query.sql b/broker/sqlc/pr_query.sql index f59a8bf6..fe5399ae 100644 --- a/broker/sqlc/pr_query.sql +++ b/broker/sqlc/pr_query.sql @@ -27,6 +27,7 @@ LIMIT $1 OFFSET $2; -- name: FacetsRequesterSymbol :many SELECT requester_symbol AS value, COUNT(*) AS count FROM patron_request_search_view +WHERE ill_request IS NOT NULL GROUP BY value ORDER BY count DESC; From 53cbba1966999ed3f60a1d7389a3bf7d2d644579 Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Tue, 26 May 2026 17:41:20 +0200 Subject: [PATCH 06/33] Ensure base where clause is applied to whole CQL where clause --- broker/patron_request/db/prcql.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/broker/patron_request/db/prcql.go b/broker/patron_request/db/prcql.go index 943924f5..e1cb5a6a 100644 --- a/broker/patron_request/db/prcql.go +++ b/broker/patron_request/db/prcql.go @@ -181,7 +181,7 @@ func (q *Queries) FacetsPatronRequestsCql(ctx context.Context, db DBTX, facetFie return nil, fmt.Errorf("failed to handle CQL query: %w", err) } if res.GetWhereClause() != "" { - sql = sql[:idx] + "AND " + res.GetWhereClause() + " " + sql[idx:] + sql = sql[:idx] + "AND (" + res.GetWhereClause() + ") " + sql[idx:] queryArguments = res.GetQueryArguments() } } From db35bae09651b7c91f04cff23a56322b77531fb3 Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Tue, 26 May 2026 17:44:48 +0200 Subject: [PATCH 07/33] trimspace --- broker/patron_request/db/prrepo.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/broker/patron_request/db/prrepo.go b/broker/patron_request/db/prrepo.go index 4714210f..96ab4a02 100644 --- a/broker/patron_request/db/prrepo.go +++ b/broker/patron_request/db/prrepo.go @@ -120,7 +120,7 @@ func (r *PgPrRepo) FacetsPatronRequests(ctx common.ExtendedContext, facetParamet facetFields := strings.Split(*facetParameter, ",") var facets []Facet for _, field := range facetFields { - switch field { + switch strings.TrimSpace(field) { case "requester_symbol", "supplier_symbol": values, err := r.queries.FacetsPatronRequestsCql(ctx, r.GetConnOrTx(), field, cql) if err != nil { From 741f6201766bb0e8c15ca25e7446436080ccbeb0 Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Tue, 26 May 2026 17:58:52 +0200 Subject: [PATCH 08/33] Rename --- broker/patron_request/api/api-handler.go | 8 +++++--- broker/patron_request/db/prcql.go | 15 ++++++--------- broker/patron_request/db/prrepo.go | 7 +------ broker/sqlc/pr_query.sql | 2 +- 4 files changed, 13 insertions(+), 19 deletions(-) diff --git a/broker/patron_request/api/api-handler.go b/broker/patron_request/api/api-handler.go index 21508a11..6e5b167a 100644 --- a/broker/patron_request/api/api-handler.go +++ b/broker/patron_request/api/api-handler.go @@ -197,9 +197,11 @@ func (a *PatronRequestApiHandler) GetPatronRequests(w http.ResponseWriter, r *ht facetResults[i].Name = field.Field facetResults[i].Values = make([]proapi.FacetResultValue, len(field.Values)) for j, value := range field.Values { - facetResults[i].Values[j] = proapi.FacetResultValue{ - Value: value.Value, - Count: value.Count, + if value.Value.Valid { + facetResults[i].Values[j] = proapi.FacetResultValue{ + Value: value.Value.String, + Count: value.Count, + } } } } diff --git a/broker/patron_request/db/prcql.go b/broker/patron_request/db/prcql.go index e1cb5a6a..004a5c11 100644 --- a/broker/patron_request/db/prcql.go +++ b/broker/patron_request/db/prcql.go @@ -166,8 +166,8 @@ func handlePatronRequestsQuery(cqlString string, noBaseArgs int) (pgcql.Query, e return def.Parse(query, noBaseArgs+1) } -func (q *Queries) FacetsPatronRequestsCql(ctx context.Context, db DBTX, facetField string, cqlString *string) ([]FacetValue, error) { - sql := facetsRequesterSymbol +func (q *Queries) FacetsPatronRequestsCql(ctx context.Context, db DBTX, facetField string, cqlString *string) ([]FacetsRequesterRow, error) { + sql := facetsRequester sql = strings.Replace(sql, "requester_symbol", facetField, 1) idx := strings.Index(sql, "GROUP BY") @@ -190,21 +190,18 @@ 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 values []FacetValue + var items []FacetsRequesterRow for rows.Next() { - var i FacetsRequesterSymbolRow + var i FacetsRequesterRow if err := rows.Scan(&i.Value, &i.Count); err != nil { return nil, err } - values = append(values, FacetValue{ - Value: i.Value.String, - Count: i.Count, - }) + items = append(items, i) } if err := rows.Err(); err != nil { return nil, err } - return values, nil + return items, nil } func (q *Queries) ListPatronRequestsCql(ctx context.Context, db DBTX, arg ListPatronRequestsParams, diff --git a/broker/patron_request/db/prrepo.go b/broker/patron_request/db/prrepo.go index 96ab4a02..861c7773 100644 --- a/broker/patron_request/db/prrepo.go +++ b/broker/patron_request/db/prrepo.go @@ -38,12 +38,7 @@ type PrRepo interface { type Facet struct { Field string - Values []FacetValue -} - -type FacetValue struct { - Value string - Count int64 + Values []FacetsRequesterRow } type PgPrRepo struct { diff --git a/broker/sqlc/pr_query.sql b/broker/sqlc/pr_query.sql index fe5399ae..9830b1d5 100644 --- a/broker/sqlc/pr_query.sql +++ b/broker/sqlc/pr_query.sql @@ -24,7 +24,7 @@ WHERE ill_request IS NOT NULL ORDER BY created_at LIMIT $1 OFFSET $2; --- name: FacetsRequesterSymbol :many +-- name: FacetsRequester :many SELECT requester_symbol AS value, COUNT(*) AS count FROM patron_request_search_view WHERE ill_request IS NOT NULL From 8272b29b782edb1d94dbdd3be997a9cf15a82d16 Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Tue, 26 May 2026 19:02:46 +0200 Subject: [PATCH 09/33] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- broker/sqlc/pr_query.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/broker/sqlc/pr_query.sql b/broker/sqlc/pr_query.sql index 9830b1d5..87db54b6 100644 --- a/broker/sqlc/pr_query.sql +++ b/broker/sqlc/pr_query.sql @@ -28,7 +28,7 @@ LIMIT $1 OFFSET $2; SELECT requester_symbol AS value, COUNT(*) AS count FROM patron_request_search_view WHERE ill_request IS NOT NULL -GROUP BY value +GROUP BY 1 ORDER BY count DESC; -- name: UpdatePatronRequest :one From 29478d35589548a8f9b87d0a5ea1b1b96527230e Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Tue, 26 May 2026 19:03:52 +0200 Subject: [PATCH 10/33] trimmedField --- broker/patron_request/db/prrepo.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/broker/patron_request/db/prrepo.go b/broker/patron_request/db/prrepo.go index 861c7773..2a069fbf 100644 --- a/broker/patron_request/db/prrepo.go +++ b/broker/patron_request/db/prrepo.go @@ -115,9 +115,10 @@ func (r *PgPrRepo) FacetsPatronRequests(ctx common.ExtendedContext, facetParamet facetFields := strings.Split(*facetParameter, ",") var facets []Facet for _, field := range facetFields { - switch strings.TrimSpace(field) { + tField := strings.TrimSpace(field) + switch tField { case "requester_symbol", "supplier_symbol": - values, err := r.queries.FacetsPatronRequestsCql(ctx, r.GetConnOrTx(), field, cql) + values, err := r.queries.FacetsPatronRequestsCql(ctx, r.GetConnOrTx(), tField, cql) if err != nil { return nil, err } From 0abe8e28ccdfd8eb9102ca2d87172a40920164fb Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Tue, 26 May 2026 19:13:19 +0200 Subject: [PATCH 11/33] Limit facet results to patron requests in test --- .../patron_request/api/api-handler_test.go | 51 ++++++------------- 1 file changed, 16 insertions(+), 35 deletions(-) diff --git a/broker/test/patron_request/api/api-handler_test.go b/broker/test/patron_request/api/api-handler_test.go index 7b4958f0..607ea01d 100644 --- a/broker/test/patron_request/api/api-handler_test.go +++ b/broker/test/patron_request/api/api-handler_test.go @@ -841,59 +841,40 @@ func TestFacetsOK(t *testing.T) { } var foundPrs proapi.PatronRequests - respBytes := httpRequest(t, "GET", basePath+"?cql=service_type%3DCopy&offset=0&limit=1", []byte{}, 200) + respBytes := httpRequest(t, "GET", basePath+"?cql=title%3Dfacets%20title&offset=0&limit=1", []byte{}, 200) err := json.Unmarshal(respBytes, &foundPrs) assert.NoError(t, err) - // a little brittle as there are patron requests created in other tests, but should be ok as long as we create more than 5 in this test - assert.GreaterOrEqual(t, foundPrs.About.Count, int64(5)) + assert.Equal(t, int64(10), foundPrs.About.Count) assert.Len(t, foundPrs.Items, 1) assert.Nil(t, foundPrs.About.Facets) - respBytes = httpRequest(t, "GET", basePath+"?facets=requester_symbol&cql=service_type%3DCopy&offset=0&limit=0", []byte{}, 200) + respBytes = httpRequest(t, "GET", basePath+"?facets=requester_symbol&cql=service_type%3DCopy+and+title%3Dfacets%20title&offset=0&limit=0", []byte{}, 200) err = json.Unmarshal(respBytes, &foundPrs) assert.NoError(t, err) - // a little brittle as there are patron requests created in other tests, but should be ok as long as we create more than 5 in this test - assert.GreaterOrEqual(t, foundPrs.About.Count, int64(5)) + assert.Equal(t, int64(5), foundPrs.About.Count) assert.Len(t, foundPrs.Items, 0) assert.NotNil(t, foundPrs.About.Facets) assert.Len(t, *foundPrs.About.Facets, 1) assert.Equal(t, "requester_symbol", (*foundPrs.About.Facets)[0].Name) - assert.GreaterOrEqual(t, len((*foundPrs.About.Facets)[0].Values), 2) - var count0 int64 - var count1 int64 - for i := range (*foundPrs.About.Facets)[0].Values { - switch (*foundPrs.About.Facets)[0].Values[i].Value { - case requesterSymbols[0]: - count0 = (*foundPrs.About.Facets)[0].Values[i].Count - case requesterSymbols[1]: - count1 = (*foundPrs.About.Facets)[0].Values[i].Count - } - } - assert.Equal(t, count0, int64(3)) - assert.Equal(t, count1, int64(2)) + assert.Len(t, (*foundPrs.About.Facets)[0].Values, 2) + assert.Equal(t, requesterSymbols[0], (*foundPrs.About.Facets)[0].Values[0].Value) + assert.Equal(t, int64(3), (*foundPrs.About.Facets)[0].Values[0].Count) + assert.Equal(t, requesterSymbols[1], (*foundPrs.About.Facets)[0].Values[1].Value) + assert.Equal(t, int64(2), (*foundPrs.About.Facets)[0].Values[1].Count) - respBytes = httpRequest(t, "GET", basePath+"?facets=requester_symbol&cql=cql.allRecords%3Dtrue%20&offset=0&limit=0", []byte{}, 200) + respBytes = httpRequest(t, "GET", basePath+"?facets=requester_symbol&cql=title%3Dfacets%20title&offset=0&limit=0", []byte{}, 200) err = json.Unmarshal(respBytes, &foundPrs) assert.NoError(t, err) - // a little brittle as there are patron requests created in other tests, but should be ok as long as we create more than 5 in this test - assert.GreaterOrEqual(t, foundPrs.About.Count, int64(10)) + assert.Equal(t, int64(10), foundPrs.About.Count) assert.Len(t, foundPrs.Items, 0) assert.NotNil(t, foundPrs.About.Facets) assert.Len(t, *foundPrs.About.Facets, 1) assert.Equal(t, "requester_symbol", (*foundPrs.About.Facets)[0].Name) - assert.GreaterOrEqual(t, len((*foundPrs.About.Facets)[0].Values), 2) - count0 = 0 - count1 = 0 - for i := range (*foundPrs.About.Facets)[0].Values { - switch (*foundPrs.About.Facets)[0].Values[i].Value { - case requesterSymbols[0]: - count0 = (*foundPrs.About.Facets)[0].Values[i].Count - case requesterSymbols[1]: - count1 = (*foundPrs.About.Facets)[0].Values[i].Count - } - } - assert.Equal(t, count0, int64(7)) - assert.Equal(t, count1, int64(3)) + assert.Len(t, (*foundPrs.About.Facets)[0].Values, 2) + assert.Equal(t, requesterSymbols[0], (*foundPrs.About.Facets)[0].Values[0].Value) + assert.Equal(t, int64(7), (*foundPrs.About.Facets)[0].Values[0].Count) + assert.Equal(t, requesterSymbols[1], (*foundPrs.About.Facets)[0].Values[1].Value) + assert.Equal(t, int64(3), (*foundPrs.About.Facets)[0].Values[1].Count) } func TestFacetsBadRequest(t *testing.T) { From fe3775f773c873df4405155476c7d8f469957bd2 Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Tue, 26 May 2026 19:14:44 +0200 Subject: [PATCH 12/33] Fix description --- broker/oapi/open-api.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/broker/oapi/open-api.yaml b/broker/oapi/open-api.yaml index bda1a6cf..2b9cc561 100644 --- a/broker/oapi/open-api.yaml +++ b/broker/oapi/open-api.yaml @@ -1480,7 +1480,7 @@ paths: - $ref: '#/components/parameters/Facets' responses: '200': - description: Successful retrieval of events + description: Successful retrieval of patron requests content: application/json: schema: From e2c2c2bec853d9b312d526a29e088ff13a0b01c5 Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Tue, 26 May 2026 19:21:32 +0200 Subject: [PATCH 13/33] FacetValue, limit 100 --- broker/patron_request/api/api-handler.go | 8 +++----- broker/patron_request/db/prrepo.go | 18 ++++++++++++++++-- broker/sqlc/pr_query.sql | 3 ++- 3 files changed, 21 insertions(+), 8 deletions(-) diff --git a/broker/patron_request/api/api-handler.go b/broker/patron_request/api/api-handler.go index 6e5b167a..21508a11 100644 --- a/broker/patron_request/api/api-handler.go +++ b/broker/patron_request/api/api-handler.go @@ -197,11 +197,9 @@ func (a *PatronRequestApiHandler) GetPatronRequests(w http.ResponseWriter, r *ht facetResults[i].Name = field.Field facetResults[i].Values = make([]proapi.FacetResultValue, len(field.Values)) for j, value := range field.Values { - if value.Value.Valid { - facetResults[i].Values[j] = proapi.FacetResultValue{ - Value: value.Value.String, - Count: value.Count, - } + facetResults[i].Values[j] = proapi.FacetResultValue{ + Value: value.Value, + Count: value.Count, } } } diff --git a/broker/patron_request/db/prrepo.go b/broker/patron_request/db/prrepo.go index 2a069fbf..112b2a31 100644 --- a/broker/patron_request/db/prrepo.go +++ b/broker/patron_request/db/prrepo.go @@ -38,7 +38,12 @@ type PrRepo interface { type Facet struct { Field string - Values []FacetsRequesterRow + Values []FacetValue +} + +type FacetValue struct { + Value string + Count int64 } type PgPrRepo struct { @@ -118,10 +123,19 @@ func (r *PgPrRepo) FacetsPatronRequests(ctx common.ExtendedContext, facetParamet tField := strings.TrimSpace(field) switch tField { case "requester_symbol", "supplier_symbol": - values, err := r.queries.FacetsPatronRequestsCql(ctx, r.GetConnOrTx(), tField, cql) + rows, err := r.queries.FacetsPatronRequestsCql(ctx, r.GetConnOrTx(), tField, cql) if err != nil { return nil, err } + var values []FacetValue + for _, row := range rows { + if row.Value.Valid { + values = append(values, FacetValue{ + Value: row.Value.String, + Count: row.Count, + }) + } + } facets = append(facets, Facet{ Field: field, Values: values, diff --git a/broker/sqlc/pr_query.sql b/broker/sqlc/pr_query.sql index 87db54b6..d944945c 100644 --- a/broker/sqlc/pr_query.sql +++ b/broker/sqlc/pr_query.sql @@ -29,7 +29,8 @@ SELECT requester_symbol AS value, COUNT(*) AS count FROM patron_request_search_view WHERE ill_request IS NOT NULL GROUP BY 1 -ORDER BY count DESC; +ORDER BY count DESC +LIMIT 100; -- name: UpdatePatronRequest :one UPDATE patron_request From 3b9946366560c0ea0ed73f98cb070d9da90aa946 Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Tue, 26 May 2026 19:25:14 +0200 Subject: [PATCH 14/33] Fixes, test with supplier_symbol facet --- broker/patron_request/db/prrepo.go | 8 ++++---- .../test/patron_request/api/api-handler_test.go | 16 ++++++++++++++++ 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/broker/patron_request/db/prrepo.go b/broker/patron_request/db/prrepo.go index 112b2a31..adaf50e9 100644 --- a/broker/patron_request/db/prrepo.go +++ b/broker/patron_request/db/prrepo.go @@ -119,11 +119,11 @@ func (r *PgPrRepo) FacetsPatronRequests(ctx common.ExtendedContext, facetParamet } facetFields := strings.Split(*facetParameter, ",") var facets []Facet - for _, field := range facetFields { - tField := strings.TrimSpace(field) - switch tField { + for _, comp := range facetFields { + field := strings.TrimSpace(comp) + switch field { case "requester_symbol", "supplier_symbol": - rows, err := r.queries.FacetsPatronRequestsCql(ctx, r.GetConnOrTx(), tField, cql) + rows, err := r.queries.FacetsPatronRequestsCql(ctx, r.GetConnOrTx(), field, cql) if err != nil { return nil, err } diff --git a/broker/test/patron_request/api/api-handler_test.go b/broker/test/patron_request/api/api-handler_test.go index 607ea01d..ec103cf4 100644 --- a/broker/test/patron_request/api/api-handler_test.go +++ b/broker/test/patron_request/api/api-handler_test.go @@ -875,6 +875,22 @@ func TestFacetsOK(t *testing.T) { assert.Equal(t, int64(7), (*foundPrs.About.Facets)[0].Values[0].Count) assert.Equal(t, requesterSymbols[1], (*foundPrs.About.Facets)[0].Values[1].Value) assert.Equal(t, int64(3), (*foundPrs.About.Facets)[0].Values[1].Count) + + respBytes = httpRequest(t, "GET", basePath+"?facets=requester_symbol%2C%20supplier_symbol&cql=title%3Dfacets%20title&offset=0&limit=0", []byte{}, 200) + err = json.Unmarshal(respBytes, &foundPrs) + assert.NoError(t, err) + assert.Equal(t, int64(10), foundPrs.About.Count) + assert.Len(t, foundPrs.Items, 0) + assert.NotNil(t, foundPrs.About.Facets) + assert.Len(t, *foundPrs.About.Facets, 2) + assert.Equal(t, "requester_symbol", (*foundPrs.About.Facets)[0].Name) + assert.Len(t, (*foundPrs.About.Facets)[0].Values, 2) + assert.Equal(t, requesterSymbols[0], (*foundPrs.About.Facets)[0].Values[0].Value) + assert.Equal(t, int64(7), (*foundPrs.About.Facets)[0].Values[0].Count) + assert.Equal(t, requesterSymbols[1], (*foundPrs.About.Facets)[0].Values[1].Value) + assert.Equal(t, int64(3), (*foundPrs.About.Facets)[0].Values[1].Count) + assert.Equal(t, "supplier_symbol", (*foundPrs.About.Facets)[1].Name) + assert.Len(t, (*foundPrs.About.Facets)[1].Values, 0) } func TestFacetsBadRequest(t *testing.T) { From 4292f47277c29908feca5f764fa36664f2038b7e Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Tue, 26 May 2026 19:27:41 +0200 Subject: [PATCH 15/33] Cover case facets= --- broker/test/patron_request/api/api-handler_test.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/broker/test/patron_request/api/api-handler_test.go b/broker/test/patron_request/api/api-handler_test.go index ec103cf4..473bd60a 100644 --- a/broker/test/patron_request/api/api-handler_test.go +++ b/broker/test/patron_request/api/api-handler_test.go @@ -893,7 +893,13 @@ func TestFacetsOK(t *testing.T) { assert.Len(t, (*foundPrs.About.Facets)[1].Values, 0) } -func TestFacetsBadRequest(t *testing.T) { +func TestFacetsBadRequest1(t *testing.T) { respBytes := httpRequest(t, "GET", basePath+"?facets=nosuch", []byte{}, 400) assert.Contains(t, string(respBytes), "unsupported facet field: nosuch") + +} + +func TestFacetsBadRequest2(t *testing.T) { + respBytes := httpRequest(t, "GET", basePath+"?facets=", []byte{}, 400) + assert.Contains(t, string(respBytes), "unsupported facet field: ") } From 69a7a50338ced4a0231011d73b366d5c2c22cedb Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Tue, 26 May 2026 19:28:01 +0200 Subject: [PATCH 16/33] lint --- broker/test/patron_request/api/api-handler_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/broker/test/patron_request/api/api-handler_test.go b/broker/test/patron_request/api/api-handler_test.go index 473bd60a..2d9d6f84 100644 --- a/broker/test/patron_request/api/api-handler_test.go +++ b/broker/test/patron_request/api/api-handler_test.go @@ -896,7 +896,6 @@ func TestFacetsOK(t *testing.T) { func TestFacetsBadRequest1(t *testing.T) { respBytes := httpRequest(t, "GET", basePath+"?facets=nosuch", []byte{}, 400) assert.Contains(t, string(respBytes), "unsupported facet field: nosuch") - } func TestFacetsBadRequest2(t *testing.T) { From 55d9bf0489260360edc7a4a585a5c901bc911618 Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Tue, 26 May 2026 19:33:21 +0200 Subject: [PATCH 17/33] Ignore empty facet components --- broker/patron_request/db/prrepo.go | 1 + broker/test/patron_request/api/api-handler_test.go | 14 ++++++++------ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/broker/patron_request/db/prrepo.go b/broker/patron_request/db/prrepo.go index c188da2f..b81c9b43 100644 --- a/broker/patron_request/db/prrepo.go +++ b/broker/patron_request/db/prrepo.go @@ -141,6 +141,7 @@ func (r *PgPrRepo) FacetsPatronRequests(ctx common.ExtendedContext, facetParamet Field: field, Values: values, }) + case "": default: return nil, fmt.Errorf("unsupported facet field: %s", field) } diff --git a/broker/test/patron_request/api/api-handler_test.go b/broker/test/patron_request/api/api-handler_test.go index 2d9d6f84..ac720c7f 100644 --- a/broker/test/patron_request/api/api-handler_test.go +++ b/broker/test/patron_request/api/api-handler_test.go @@ -848,6 +848,13 @@ func TestFacetsOK(t *testing.T) { assert.Len(t, foundPrs.Items, 1) assert.Nil(t, foundPrs.About.Facets) + respBytes = httpRequest(t, "GET", basePath+"?cql=title%3Dfacets%20title&offset=0&limit=1&facets=", []byte{}, 200) + err = json.Unmarshal(respBytes, &foundPrs) + assert.NoError(t, err) + assert.Equal(t, int64(10), foundPrs.About.Count) + assert.Len(t, foundPrs.Items, 1) + assert.Nil(t, foundPrs.About.Facets) + respBytes = httpRequest(t, "GET", basePath+"?facets=requester_symbol&cql=service_type%3DCopy+and+title%3Dfacets%20title&offset=0&limit=0", []byte{}, 200) err = json.Unmarshal(respBytes, &foundPrs) assert.NoError(t, err) @@ -876,7 +883,7 @@ func TestFacetsOK(t *testing.T) { assert.Equal(t, requesterSymbols[1], (*foundPrs.About.Facets)[0].Values[1].Value) assert.Equal(t, int64(3), (*foundPrs.About.Facets)[0].Values[1].Count) - respBytes = httpRequest(t, "GET", basePath+"?facets=requester_symbol%2C%20supplier_symbol&cql=title%3Dfacets%20title&offset=0&limit=0", []byte{}, 200) + respBytes = httpRequest(t, "GET", basePath+"?facets=requester_symbol%2C%20supplier_symbol%2C&cql=title%3Dfacets%20title&offset=0&limit=0", []byte{}, 200) err = json.Unmarshal(respBytes, &foundPrs) assert.NoError(t, err) assert.Equal(t, int64(10), foundPrs.About.Count) @@ -897,8 +904,3 @@ func TestFacetsBadRequest1(t *testing.T) { respBytes := httpRequest(t, "GET", basePath+"?facets=nosuch", []byte{}, 400) assert.Contains(t, string(respBytes), "unsupported facet field: nosuch") } - -func TestFacetsBadRequest2(t *testing.T) { - respBytes := httpRequest(t, "GET", basePath+"?facets=", []byte{}, 400) - assert.Contains(t, string(respBytes), "unsupported facet field: ") -} From 0f99e2382970be8e0e518583be8a9deeb000087d Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Tue, 26 May 2026 19:36:04 +0200 Subject: [PATCH 18/33] sort 2nd on value --- broker/sqlc/pr_query.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/broker/sqlc/pr_query.sql b/broker/sqlc/pr_query.sql index fafcaf66..9e951029 100644 --- a/broker/sqlc/pr_query.sql +++ b/broker/sqlc/pr_query.sql @@ -29,7 +29,7 @@ SELECT requester_symbol AS value, COUNT(*) AS count FROM patron_request_search_view WHERE ill_request IS NOT NULL GROUP BY 1 -ORDER BY count DESC +ORDER BY count DESC, value ASC LIMIT 100; -- name: UpdatePatronRequest :one From 99e63f5d967aaa715aa5a59634052cf4f263e591 Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Tue, 26 May 2026 19:50:18 +0200 Subject: [PATCH 19/33] Yet another rename --- broker/patron_request/db/prcql.go | 8 ++++---- broker/sqlc/pr_query.sql | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/broker/patron_request/db/prcql.go b/broker/patron_request/db/prcql.go index 913113f5..dbf1a2b4 100644 --- a/broker/patron_request/db/prcql.go +++ b/broker/patron_request/db/prcql.go @@ -170,8 +170,8 @@ func handlePatronRequestsQuery(cqlString string, noBaseArgs int) (pgcql.Query, e return def.Parse(query, noBaseArgs+1) } -func (q *Queries) FacetsPatronRequestsCql(ctx context.Context, db DBTX, facetField string, cqlString *string) ([]FacetsRequesterRow, error) { - sql := facetsRequester +func (q *Queries) FacetsPatronRequestsCql(ctx context.Context, db DBTX, facetField string, cqlString *string) ([]FacetsPatronRequestsRow, error) { + sql := facetsPatronRequests sql = strings.Replace(sql, "requester_symbol", facetField, 1) idx := strings.Index(sql, "GROUP BY") @@ -194,9 +194,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 []FacetsRequesterRow + var items []FacetsPatronRequestsRow for rows.Next() { - var i FacetsRequesterRow + var i FacetsPatronRequestsRow if err := rows.Scan(&i.Value, &i.Count); err != nil { return nil, err } diff --git a/broker/sqlc/pr_query.sql b/broker/sqlc/pr_query.sql index 9e951029..7aa2dbdf 100644 --- a/broker/sqlc/pr_query.sql +++ b/broker/sqlc/pr_query.sql @@ -24,7 +24,7 @@ WHERE ill_request IS NOT NULL ORDER BY created_at LIMIT $1 OFFSET $2; --- name: FacetsRequester :many +-- name: FacetsPatronRequests :many SELECT requester_symbol AS value, COUNT(*) AS count FROM patron_request_search_view WHERE ill_request IS NOT NULL From 6941da35e2fe0dea521c7d5df3047bd895953dbf Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Wed, 27 May 2026 10:56:41 +0200 Subject: [PATCH 20/33] Rename --- broker/patron_request/api/api-handler.go | 2 +- broker/patron_request/db/prrepo.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/broker/patron_request/api/api-handler.go b/broker/patron_request/api/api-handler.go index c21c044b..bb014d6e 100644 --- a/broker/patron_request/api/api-handler.go +++ b/broker/patron_request/api/api-handler.go @@ -186,7 +186,7 @@ func (a *PatronRequestApiHandler) GetPatronRequests(w http.ResponseWriter, r *ht resp := proapi.PatronRequests{Items: responseItems} resp.About = toProAbout(api.CollectAboutData(count, offset, limit, r)) - facets, err := a.prRepo.FacetsPatronRequests(ctx, params.Facets, &cqlStr) + facets, err := a.prRepo.GetPatronRequestsFacets(ctx, params.Facets, &cqlStr) if err != nil { api.AddBadRequestError(ctx, w, err) return diff --git a/broker/patron_request/db/prrepo.go b/broker/patron_request/db/prrepo.go index b81c9b43..ab6beb5a 100644 --- a/broker/patron_request/db/prrepo.go +++ b/broker/patron_request/db/prrepo.go @@ -19,7 +19,7 @@ type PrRepo interface { 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) - FacetsPatronRequests(ctx common.ExtendedContext, facets *string, cql *string) ([]Facet, error) + GetPatronRequestsFacets(ctx common.ExtendedContext, facets *string, cql *string) ([]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) @@ -114,7 +114,7 @@ func (r *PgPrRepo) ListPatronRequests(ctx common.ExtendedContext, params ListPat return list, fullCount, nil } -func (r *PgPrRepo) FacetsPatronRequests(ctx common.ExtendedContext, facetParameter *string, cql *string) ([]Facet, error) { +func (r *PgPrRepo) GetPatronRequestsFacets(ctx common.ExtendedContext, facetParameter *string, cql *string) ([]Facet, error) { if facetParameter == nil { return nil, nil // no facets requested, return nil } From 89350b97dda4317496f01f78a8d8469b41bfd252 Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Wed, 27 May 2026 11:05:18 +0200 Subject: [PATCH 21/33] CQL query always present for facets --- broker/patron_request/api/api-handler.go | 2 +- broker/patron_request/db/prcql.go | 18 ++++++++---------- broker/patron_request/db/prrepo.go | 4 ++-- 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/broker/patron_request/api/api-handler.go b/broker/patron_request/api/api-handler.go index bb014d6e..368a285d 100644 --- a/broker/patron_request/api/api-handler.go +++ b/broker/patron_request/api/api-handler.go @@ -186,7 +186,7 @@ func (a *PatronRequestApiHandler) GetPatronRequests(w http.ResponseWriter, r *ht resp := proapi.PatronRequests{Items: responseItems} resp.About = toProAbout(api.CollectAboutData(count, offset, limit, r)) - facets, err := a.prRepo.GetPatronRequestsFacets(ctx, params.Facets, &cqlStr) + facets, err := a.prRepo.GetPatronRequestsFacets(ctx, params.Facets, 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 dbf1a2b4..59bbeeb1 100644 --- a/broker/patron_request/db/prcql.go +++ b/broker/patron_request/db/prcql.go @@ -170,7 +170,7 @@ func handlePatronRequestsQuery(cqlString string, noBaseArgs int) (pgcql.Query, e return def.Parse(query, noBaseArgs+1) } -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, cqlString string) ([]FacetsPatronRequestsRow, error) { sql := facetsPatronRequests sql = strings.Replace(sql, "requester_symbol", facetField, 1) @@ -179,15 +179,13 @@ func (q *Queries) FacetsPatronRequestsCql(ctx context.Context, db DBTX, facetFie return nil, fmt.Errorf("base SQL query missing GROUP BY clause") } var queryArguments []any - if cqlString != nil { - 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:] - queryArguments = res.GetQueryArguments() - } + 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:] + queryArguments = res.GetQueryArguments() } rows, err := db.Query(ctx, sql, queryArguments...) if err != nil { diff --git a/broker/patron_request/db/prrepo.go b/broker/patron_request/db/prrepo.go index ab6beb5a..3d4a4f37 100644 --- a/broker/patron_request/db/prrepo.go +++ b/broker/patron_request/db/prrepo.go @@ -19,7 +19,7 @@ type PrRepo interface { 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, facets *string, cql *string) ([]Facet, error) + GetPatronRequestsFacets(ctx common.ExtendedContext, facets *string, cql string) ([]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) @@ -114,7 +114,7 @@ func (r *PgPrRepo) ListPatronRequests(ctx common.ExtendedContext, params ListPat return list, fullCount, nil } -func (r *PgPrRepo) GetPatronRequestsFacets(ctx common.ExtendedContext, facetParameter *string, cql *string) ([]Facet, error) { +func (r *PgPrRepo) GetPatronRequestsFacets(ctx common.ExtendedContext, facetParameter *string, cql string) ([]Facet, error) { if facetParameter == nil { return nil, nil // no facets requested, return nil } From 08fca4c166f15da69481589908f6e81abba72cd3 Mon Sep 17 00:00:00 2001 From: Jakub Skoczen Date: Wed, 27 May 2026 11:23:05 +0200 Subject: [PATCH 22/33] Use OpenAPI param schema --- broker/oapi/open-api.yaml | 6 +++++- broker/patron_request/api/api-handler.go | 5 ++++- broker/patron_request/db/prrepo.go | 8 ++------ broker/test/patron_request/api/api-handler_test.go | 7 +------ 4 files changed, 12 insertions(+), 14 deletions(-) diff --git a/broker/oapi/open-api.yaml b/broker/oapi/open-api.yaml index 35fe875c..ffd7d656 100644 --- a/broker/oapi/open-api.yaml +++ b/broker/oapi/open-api.yaml @@ -97,8 +97,12 @@ components: name: facets in: query description: Comma separated list of facet names to include in the response, for example "requester_symbol,supplier_symbol" + style: form + explode: false schema: - type: string + type: array + items: + type: string schemas: Index: type: object diff --git a/broker/patron_request/api/api-handler.go b/broker/patron_request/api/api-handler.go index 368a285d..4e9b74a3 100644 --- a/broker/patron_request/api/api-handler.go +++ b/broker/patron_request/api/api-handler.go @@ -186,7 +186,10 @@ func (a *PatronRequestApiHandler) GetPatronRequests(w http.ResponseWriter, r *ht resp := proapi.PatronRequests{Items: responseItems} resp.About = toProAbout(api.CollectAboutData(count, offset, limit, r)) - facets, err := a.prRepo.GetPatronRequestsFacets(ctx, params.Facets, cqlStr) + var facets []pr_db.Facet + if params.Facets != nil { + facets, err = a.prRepo.GetPatronRequestsFacets(ctx, *params.Facets, cqlStr) + } if err != nil { api.AddBadRequestError(ctx, w, err) return diff --git a/broker/patron_request/db/prrepo.go b/broker/patron_request/db/prrepo.go index 3d4a4f37..bebe3968 100644 --- a/broker/patron_request/db/prrepo.go +++ b/broker/patron_request/db/prrepo.go @@ -19,7 +19,7 @@ type PrRepo interface { 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, facets *string, cql string) ([]Facet, error) + GetPatronRequestsFacets(ctx common.ExtendedContext, facetFields []string, cql string) ([]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) @@ -114,11 +114,7 @@ func (r *PgPrRepo) ListPatronRequests(ctx common.ExtendedContext, params ListPat return list, fullCount, nil } -func (r *PgPrRepo) GetPatronRequestsFacets(ctx common.ExtendedContext, facetParameter *string, cql string) ([]Facet, error) { - if facetParameter == nil { - return nil, nil // no facets requested, return nil - } - facetFields := strings.Split(*facetParameter, ",") +func (r *PgPrRepo) GetPatronRequestsFacets(ctx common.ExtendedContext, facetFields []string, cql string) ([]Facet, error) { var facets []Facet for _, comp := range facetFields { field := strings.TrimSpace(comp) diff --git a/broker/test/patron_request/api/api-handler_test.go b/broker/test/patron_request/api/api-handler_test.go index ac720c7f..768b6149 100644 --- a/broker/test/patron_request/api/api-handler_test.go +++ b/broker/test/patron_request/api/api-handler_test.go @@ -848,12 +848,7 @@ func TestFacetsOK(t *testing.T) { assert.Len(t, foundPrs.Items, 1) assert.Nil(t, foundPrs.About.Facets) - respBytes = httpRequest(t, "GET", basePath+"?cql=title%3Dfacets%20title&offset=0&limit=1&facets=", []byte{}, 200) - err = json.Unmarshal(respBytes, &foundPrs) - assert.NoError(t, err) - assert.Equal(t, int64(10), foundPrs.About.Count) - assert.Len(t, foundPrs.Items, 1) - assert.Nil(t, foundPrs.About.Facets) + httpRequest(t, "GET", basePath+"?cql=title%3Dfacets%20title&offset=0&limit=1&facets=", []byte{}, 400) respBytes = httpRequest(t, "GET", basePath+"?facets=requester_symbol&cql=service_type%3DCopy+and+title%3Dfacets%20title&offset=0&limit=0", []byte{}, 200) err = json.Unmarshal(respBytes, &foundPrs) From 372bc570d4164fe688ccdfdff72ec94700e91565 Mon Sep 17 00:00:00 2001 From: Jakub Skoczen Date: Wed, 27 May 2026 11:36:40 +0200 Subject: [PATCH 23/33] Use typed errors --- broker/patron_request/api/api-handler.go | 6 ++- broker/patron_request/api/api-handler_test.go | 39 +++++++++++++++++++ broker/patron_request/db/prrepo.go | 5 ++- 3 files changed, 48 insertions(+), 2 deletions(-) diff --git a/broker/patron_request/api/api-handler.go b/broker/patron_request/api/api-handler.go index 4e9b74a3..e84fa6e2 100644 --- a/broker/patron_request/api/api-handler.go +++ b/broker/patron_request/api/api-handler.go @@ -191,7 +191,11 @@ func (a *PatronRequestApiHandler) GetPatronRequests(w http.ResponseWriter, r *ht facets, err = a.prRepo.GetPatronRequestsFacets(ctx, *params.Facets, cqlStr) } if err != nil { - api.AddBadRequestError(ctx, w, err) + if errors.Is(err, pr_db.ErrUnsupportedFacet) { + api.AddBadRequestError(ctx, w, err) + } else { + api.AddInternalError(ctx, w, err) + } return } if len(facets) > 0 { diff --git a/broker/patron_request/api/api-handler_test.go b/broker/patron_request/api/api-handler_test.go index bca69538..6cd3f9da 100644 --- a/broker/patron_request/api/api-handler_test.go +++ b/broker/patron_request/api/api-handler_test.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "net/http" "net/http/httptest" "strconv" @@ -168,6 +169,32 @@ func TestGetPatronRequests(t *testing.T) { assert.Contains(t, rr.Body.String(), "DB error") } +func TestGetPatronRequestsFacetsDBError(t *testing.T) { + facets := proapi.Facets{"requester_symbol"} + handler := NewPrApiHandler(new(PrRepoError), mockEventBus, mockEventRepo, tenant.NewResolver(), nil, 10) + req, _ := http.NewRequest("GET", "/", nil) + rr := httptest.NewRecorder() + params := proapi.GetPatronRequestsParams{ + Facets: &facets, + } + handler.GetPatronRequests(rr, req, params) + assert.Equal(t, http.StatusInternalServerError, rr.Code) + assert.Contains(t, rr.Body.String(), "DB error") +} + +func TestGetPatronRequestsFacetsUnsupported(t *testing.T) { + facets := proapi.Facets{"nosuch"} + handler := NewPrApiHandler(new(PrRepoFacetsUnsupported), mockEventBus, mockEventRepo, tenant.NewResolver(), nil, 10) + req, _ := http.NewRequest("GET", "/", nil) + rr := httptest.NewRecorder() + params := proapi.GetPatronRequestsParams{ + Facets: &facets, + } + handler.GetPatronRequests(rr, req, params) + assert.Equal(t, http.StatusBadRequest, rr.Code) + assert.Contains(t, rr.Body.String(), "nosuch") +} + func TestGetPatronRequestsNoSymbol(t *testing.T) { repo := new(PrRepoCapture) handler := NewPrApiHandler(repo, mockEventBus, mockEventRepo, tenant.NewResolver(), nil, 10) @@ -1017,6 +1044,18 @@ func (r *PrRepoError) GetNotificationById(ctx common.ExtendedContext, id string) } } +func (r *PrRepoError) GetPatronRequestsFacets(_ common.ExtendedContext, _ []string, _ string) ([]pr_db.Facet, error) { + return nil, errors.New("DB error") +} + +type PrRepoFacetsUnsupported struct { + PrRepoCapture +} + +func (r *PrRepoFacetsUnsupported) GetPatronRequestsFacets(_ common.ExtendedContext, _ []string, _ string) ([]pr_db.Facet, error) { + return nil, fmt.Errorf("%w: nosuch", pr_db.ErrUnsupportedFacet) +} + type MockIso18626Handler struct { mock.Mock handler.Iso18626Handler diff --git a/broker/patron_request/db/prrepo.go b/broker/patron_request/db/prrepo.go index bebe3968..d69063b1 100644 --- a/broker/patron_request/db/prrepo.go +++ b/broker/patron_request/db/prrepo.go @@ -1,6 +1,7 @@ package pr_db import ( + "errors" "fmt" "strings" @@ -37,6 +38,8 @@ type PrRepo interface { DeleteItemById(ctx common.ExtendedContext, id string) error } +var ErrUnsupportedFacet = errors.New("unsupported facet field") + type Facet struct { Field string Values []FacetValue @@ -139,7 +142,7 @@ func (r *PgPrRepo) GetPatronRequestsFacets(ctx common.ExtendedContext, facetFiel }) case "": default: - return nil, fmt.Errorf("unsupported facet field: %s", field) + return nil, fmt.Errorf("%w: %s", ErrUnsupportedFacet, field) } } return facets, nil From 632cd52837cab6aed1088c12007c8cd6c71ee561 Mon Sep 17 00:00:00 2001 From: Jakub Skoczen Date: Wed, 27 May 2026 11:53:19 +0200 Subject: [PATCH 24/33] Comments and cosmetics --- broker/patron_request/db/prcql.go | 9 +++++++-- broker/sqlc/pr_query.sql | 3 +++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/broker/patron_request/db/prcql.go b/broker/patron_request/db/prcql.go index 59bbeeb1..479e3917 100644 --- a/broker/patron_request/db/prcql.go +++ b/broker/patron_request/db/prcql.go @@ -170,9 +170,14 @@ func handlePatronRequestsQuery(cqlString string, noBaseArgs int) (pgcql.Query, e return def.Parse(query, noBaseArgs+1) } +// facetFieldPlaceholder is the column name used in the facetsPatronRequests SQL template. +// 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) { - sql := facetsPatronRequests - sql = strings.Replace(sql, "requester_symbol", facetField, 1) + // 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) idx := strings.Index(sql, "GROUP BY") if idx == -1 { diff --git a/broker/sqlc/pr_query.sql b/broker/sqlc/pr_query.sql index 7aa2dbdf..5221481f 100644 --- a/broker/sqlc/pr_query.sql +++ b/broker/sqlc/pr_query.sql @@ -25,6 +25,9 @@ ORDER BY created_at LIMIT $1 OFFSET $2; -- name: FacetsPatronRequests :many +-- TEMPLATE: 'requester_symbol' is a placeholder column name. This query is not +-- meant to be called directly; use FacetsPatronRequestsCql 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 WHERE ill_request IS NOT NULL From 6630a6e681ce2203cf482f7513573e83962a2893 Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Wed, 27 May 2026 11:18:40 +0200 Subject: [PATCH 25/33] Simplify, test case without user-supplied cql --- broker/patron_request/db/prcql.go | 4 +--- broker/test/patron_request/api/api-handler_test.go | 9 +++++++++ 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/broker/patron_request/db/prcql.go b/broker/patron_request/db/prcql.go index 479e3917..da2d9d3d 100644 --- a/broker/patron_request/db/prcql.go +++ b/broker/patron_request/db/prcql.go @@ -183,16 +183,14 @@ func (q *Queries) FacetsPatronRequestsCql(ctx context.Context, db DBTX, facetFie if idx == -1 { return nil, fmt.Errorf("base SQL query missing GROUP BY clause") } - var queryArguments []any 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:] - queryArguments = res.GetQueryArguments() } - rows, err := db.Query(ctx, sql, queryArguments...) + rows, err := db.Query(ctx, sql, res.GetQueryArguments()...) if err != nil { return nil, fmt.Errorf("failed to execute facets query: %w", err) } diff --git a/broker/test/patron_request/api/api-handler_test.go b/broker/test/patron_request/api/api-handler_test.go index 768b6149..e5b34e9b 100644 --- a/broker/test/patron_request/api/api-handler_test.go +++ b/broker/test/patron_request/api/api-handler_test.go @@ -893,6 +893,15 @@ func TestFacetsOK(t *testing.T) { assert.Equal(t, int64(3), (*foundPrs.About.Facets)[0].Values[1].Count) assert.Equal(t, "supplier_symbol", (*foundPrs.About.Facets)[1].Name) assert.Len(t, (*foundPrs.About.Facets)[1].Values, 0) + + // omit CQL (all records), we might get more results than in earlier tests + respBytes = httpRequest(t, "GET", basePath+"?facets=requester_symbol%2Csupplier_symbol&offset=0&limit=0", []byte{}, 200) + err = json.Unmarshal(respBytes, &foundPrs) + assert.NoError(t, err) + assert.GreaterOrEqual(t, foundPrs.About.Count, int64(10)) + assert.Len(t, foundPrs.Items, 0) + assert.NotNil(t, foundPrs.About.Facets) + assert.GreaterOrEqual(t, len(*foundPrs.About.Facets), 2) } func TestFacetsBadRequest1(t *testing.T) { From d22b0071525f60d1b1edb0b54537cf00e4618ac3 Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Wed, 27 May 2026 12:55:54 +0200 Subject: [PATCH 26/33] AboutWithFacets --- broker/oapi/open-api.yaml | 23 ++++++++++++++++++++++- broker/patron_request/api/api-handler.go | 10 +++++----- broker/scheduler/api/api_handler.go | 16 +--------------- 3 files changed, 28 insertions(+), 21 deletions(-) diff --git a/broker/oapi/open-api.yaml b/broker/oapi/open-api.yaml index ffd7d656..5db030b9 100644 --- a/broker/oapi/open-api.yaml +++ b/broker/oapi/open-api.yaml @@ -238,6 +238,27 @@ components: format: int64 description: Count of items for this facet value About: + type: object + required: + - count + properties: + count: + type: integer + format: int64 + description: Total number of items in the result + nextLink: + type: string + description: Link to the next page of results + prevLink: + type: string + description: Link to the previous page of results + firstLink: + type: string + description: Link to the first page of results + lastLink: + type: string + description: Link to the last page of results + AboutWithFacets: type: object required: - count @@ -588,7 +609,7 @@ components: - about properties: about: - $ref: '#/components/schemas/About' + $ref: '#/components/schemas/AboutWithFacets' items: type: array description: List of patron request diff --git a/broker/patron_request/api/api-handler.go b/broker/patron_request/api/api-handler.go index e84fa6e2..c6cbc786 100644 --- a/broker/patron_request/api/api-handler.go +++ b/broker/patron_request/api/api-handler.go @@ -185,7 +185,7 @@ func (a *PatronRequestApiHandler) GetPatronRequests(w http.ResponseWriter, r *ht } resp := proapi.PatronRequests{Items: responseItems} - resp.About = toProAbout(api.CollectAboutData(count, offset, limit, r)) + 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) @@ -215,11 +215,11 @@ func (a *PatronRequestApiHandler) GetPatronRequests(w http.ResponseWriter, r *ht api.WriteJsonResponse(w, resp) } -// toProAbout converts an oapi.About to a proapi.About by copying the pagination fields. +// toProAbout converts an oapi.About to a proapi.AboutWithFacets by copying the pagination fields. // The two types are generated independently from identical OpenAPI schemas but are not // directly convertible because their Facets types differ. -func toProAbout(a oapi.About) proapi.About { - return proapi.About{ +func toProAboutWithFacets(a oapi.About) proapi.AboutWithFacets { + return proapi.AboutWithFacets{ Count: a.Count, FirstLink: a.FirstLink, LastLink: a.LastLink, @@ -733,7 +733,7 @@ func (a *PatronRequestApiHandler) GetPatronRequestsIdNotifications(w http.Respon responseList = append(responseList, apiN) } resp := proapi.PrNotifications{Items: responseList} - resp.About = toProAbout(api.CollectAboutData(fullCount, offset, limit, r)) + resp.About = proapi.About(api.CollectAboutData(fullCount, offset, limit, r)) api.WriteJsonResponse(w, resp) } diff --git a/broker/scheduler/api/api_handler.go b/broker/scheduler/api/api_handler.go index 8b88864e..4765b2fc 100644 --- a/broker/scheduler/api/api_handler.go +++ b/broker/scheduler/api/api_handler.go @@ -10,7 +10,6 @@ import ( brokerapi "github.com/indexdata/crosslink/broker/api" "github.com/indexdata/crosslink/broker/common" "github.com/indexdata/crosslink/broker/events" - "github.com/indexdata/crosslink/broker/oapi" sched_db "github.com/indexdata/crosslink/broker/scheduler/db" schedoapi "github.com/indexdata/crosslink/broker/scheduler/oapi" sched_service "github.com/indexdata/crosslink/broker/scheduler/service" @@ -65,25 +64,12 @@ func (h SchedulerApiHandler) GetBatchActions(w http.ResponseWriter, r *http.Requ } resp := schedoapi.BatchActions{ - About: toSchedAbout(brokerapi.CollectAboutData(count, offset, limit, r)), + About: schedoapi.About(brokerapi.CollectAboutData(count, offset, limit, r)), Items: toBatchActionList(items), } brokerapi.WriteJsonResponse(w, resp) } -// toSchedAbout converts an oapi.About to a schedoapi.About by copying the pagination fields. -// The two types are generated independently from identical OpenAPI schemas but are not -// directly convertible because their Facets types differ. -func toSchedAbout(a oapi.About) schedoapi.About { - return schedoapi.About{ - Count: a.Count, - FirstLink: a.FirstLink, - LastLink: a.LastLink, - NextLink: a.NextLink, - PrevLink: a.PrevLink, - } -} - // PostBatchActions creates a new batch action. func (h SchedulerApiHandler) PostBatchActions(w http.ResponseWriter, r *http.Request, params schedoapi.PostBatchActionsParams) { logParams := map[string]string{"method": "PostBatchActions"} From f8b34e0e5819d9630cf490df716e3edc47defb82 Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Wed, 27 May 2026 12:57:38 +0200 Subject: [PATCH 27/33] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- broker/oapi/open-api.yaml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/broker/oapi/open-api.yaml b/broker/oapi/open-api.yaml index 5db030b9..b97addf3 100644 --- a/broker/oapi/open-api.yaml +++ b/broker/oapi/open-api.yaml @@ -96,13 +96,16 @@ components: Facets: name: facets in: query - description: Comma separated list of facet names to include in the response, for example "requester_symbol,supplier_symbol" + description: Comma separated list of facet names to include in the response. Supported values are "requester_symbol" and "supplier_symbol"; unsupported facet names result in a 400 response. For example: "requester_symbol,supplier_symbol" style: form explode: false schema: type: array items: type: string + enum: + - requester_symbol + - supplier_symbol schemas: Index: type: object From 4f5bd89f784808d24e8e5562405201c8dca59b0b Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Wed, 27 May 2026 12:58:48 +0200 Subject: [PATCH 28/33] Reformat, avoid : --- broker/oapi/open-api.yaml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/broker/oapi/open-api.yaml b/broker/oapi/open-api.yaml index b97addf3..fccf2677 100644 --- a/broker/oapi/open-api.yaml +++ b/broker/oapi/open-api.yaml @@ -96,7 +96,10 @@ components: Facets: name: facets in: query - description: Comma separated list of facet names to include in the response. Supported values are "requester_symbol" and "supplier_symbol"; unsupported facet names result in a 400 response. For example: "requester_symbol,supplier_symbol" + description: Comma separated list of facet names to include in the response. + Supported values are "requester_symbol" and "supplier_symbol"; + unsupported facet names result in a 400 response. + For example "requester_symbol,supplier_symbol" style: form explode: false schema: From da8e7b726dd74f85251d8f129d9e4ad28a1555e7 Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Wed, 27 May 2026 13:00:58 +0200 Subject: [PATCH 29/33] OpenAPI rejects blanks in facets spec --- broker/test/patron_request/api/api-handler_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/broker/test/patron_request/api/api-handler_test.go b/broker/test/patron_request/api/api-handler_test.go index e5b34e9b..a580380d 100644 --- a/broker/test/patron_request/api/api-handler_test.go +++ b/broker/test/patron_request/api/api-handler_test.go @@ -878,7 +878,7 @@ func TestFacetsOK(t *testing.T) { assert.Equal(t, requesterSymbols[1], (*foundPrs.About.Facets)[0].Values[1].Value) assert.Equal(t, int64(3), (*foundPrs.About.Facets)[0].Values[1].Count) - respBytes = httpRequest(t, "GET", basePath+"?facets=requester_symbol%2C%20supplier_symbol%2C&cql=title%3Dfacets%20title&offset=0&limit=0", []byte{}, 200) + respBytes = httpRequest(t, "GET", basePath+"?facets=requester_symbol%2Csupplier_symbol&cql=title%3Dfacets%20title&offset=0&limit=0", []byte{}, 200) err = json.Unmarshal(respBytes, &foundPrs) assert.NoError(t, err) assert.Equal(t, int64(10), foundPrs.About.Count) From fc44e770d4f2b44e655a627ebd9dfcf294f0a7a8 Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Wed, 27 May 2026 13:06:30 +0200 Subject: [PATCH 30/33] Update test for OpenAPI validation --- broker/test/patron_request/api/api-handler_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/broker/test/patron_request/api/api-handler_test.go b/broker/test/patron_request/api/api-handler_test.go index a580380d..7c49d607 100644 --- a/broker/test/patron_request/api/api-handler_test.go +++ b/broker/test/patron_request/api/api-handler_test.go @@ -904,7 +904,7 @@ func TestFacetsOK(t *testing.T) { assert.GreaterOrEqual(t, len(*foundPrs.About.Facets), 2) } -func TestFacetsBadRequest1(t *testing.T) { +func TestFacetsBadRequest(t *testing.T) { respBytes := httpRequest(t, "GET", basePath+"?facets=nosuch", []byte{}, 400) - assert.Contains(t, string(respBytes), "unsupported facet field: nosuch") + assert.Contains(t, string(respBytes), "parameter \\\"facets\\\" in query") } From 31d5f47ef38fbeff820710cc0a8a3fd02f13faf9 Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Wed, 27 May 2026 13:10:46 +0200 Subject: [PATCH 31/33] Mention 100 entries for now --- broker/oapi/open-api.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/broker/oapi/open-api.yaml b/broker/oapi/open-api.yaml index fccf2677..e00469a1 100644 --- a/broker/oapi/open-api.yaml +++ b/broker/oapi/open-api.yaml @@ -227,7 +227,7 @@ components: description: Facet name values: type: array - description: List of facet values + description: List of facet values. At most 100 entries are returned. items: $ref: '#/components/schemas/FacetResultValue' FacetResultValue: From 413e363a17c0458ea99da9e6b22a5577514dcd64 Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Wed, 27 May 2026 13:15:25 +0200 Subject: [PATCH 32/33] Make GetPatronRequestsFacets strict Now that the OpenAPI spec is validated. In fact , one could argue that it an error now should be seena as Internal Error. --- broker/patron_request/db/prrepo.go | 4 +--- broker/test/patron_request/api/api-handler_test.go | 7 ++++++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/broker/patron_request/db/prrepo.go b/broker/patron_request/db/prrepo.go index d69063b1..39c71378 100644 --- a/broker/patron_request/db/prrepo.go +++ b/broker/patron_request/db/prrepo.go @@ -119,8 +119,7 @@ func (r *PgPrRepo) ListPatronRequests(ctx common.ExtendedContext, params ListPat func (r *PgPrRepo) GetPatronRequestsFacets(ctx common.ExtendedContext, facetFields []string, cql string) ([]Facet, error) { var facets []Facet - for _, comp := range facetFields { - field := strings.TrimSpace(comp) + for _, field := range facetFields { switch field { case "requester_symbol", "supplier_symbol": rows, err := r.queries.FacetsPatronRequestsCql(ctx, r.GetConnOrTx(), field, cql) @@ -140,7 +139,6 @@ func (r *PgPrRepo) GetPatronRequestsFacets(ctx common.ExtendedContext, facetFiel Field: field, Values: values, }) - case "": default: return nil, fmt.Errorf("%w: %s", ErrUnsupportedFacet, field) } diff --git a/broker/test/patron_request/api/api-handler_test.go b/broker/test/patron_request/api/api-handler_test.go index 7c49d607..21b9aa71 100644 --- a/broker/test/patron_request/api/api-handler_test.go +++ b/broker/test/patron_request/api/api-handler_test.go @@ -904,7 +904,12 @@ func TestFacetsOK(t *testing.T) { assert.GreaterOrEqual(t, len(*foundPrs.About.Facets), 2) } -func TestFacetsBadRequest(t *testing.T) { +func TestFacetsUnknownField(t *testing.T) { respBytes := httpRequest(t, "GET", basePath+"?facets=nosuch", []byte{}, 400) assert.Contains(t, string(respBytes), "parameter \\\"facets\\\" in query") } + +func TestFacetsEmptyField(t *testing.T) { + respBytes := httpRequest(t, "GET", basePath+"?facets=", []byte{}, 400) + assert.Contains(t, string(respBytes), "parameter \\\"facets\\\" in query") +} From 5fe8682b166eb211d5f40097d3f610a0ea0f1ec5 Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Wed, 27 May 2026 13:23:28 +0200 Subject: [PATCH 33/33] Useless comment now --- broker/patron_request/api/api-handler.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/broker/patron_request/api/api-handler.go b/broker/patron_request/api/api-handler.go index c6cbc786..e75a430f 100644 --- a/broker/patron_request/api/api-handler.go +++ b/broker/patron_request/api/api-handler.go @@ -215,9 +215,6 @@ func (a *PatronRequestApiHandler) GetPatronRequests(w http.ResponseWriter, r *ht api.WriteJsonResponse(w, resp) } -// toProAbout converts an oapi.About to a proapi.AboutWithFacets by copying the pagination fields. -// The two types are generated independently from identical OpenAPI schemas but are not -// directly convertible because their Facets types differ. func toProAboutWithFacets(a oapi.About) proapi.AboutWithFacets { return proapi.AboutWithFacets{ Count: a.Count,