From 42fe78e8eb0efa607dd35e980b9df92e669a1027 Mon Sep 17 00:00:00 2001 From: Adam Dickmeiss Date: Tue, 26 May 2026 13:58:44 +0200 Subject: [PATCH] CROSSLINK-274 patron request facets --- broker/oapi/open-api.yaml | 58 ++++++++- broker/patron_request/api/api-handler.go | 38 +++++- broker/patron_request/api/api-handler_test.go | 39 ++++++ broker/patron_request/db/prcql.go | 39 ++++++ broker/patron_request/db/prrepo.go | 44 +++++++ broker/sqlc/pr_query.sql | 11 ++ .../patron_request/api/api-handler_test.go | 123 ++++++++++++++++++ 7 files changed, 349 insertions(+), 3 deletions(-) diff --git a/broker/oapi/open-api.yaml b/broker/oapi/open-api.yaml index 10015b61..4fa47b36 100644 --- a/broker/oapi/open-api.yaml +++ b/broker/oapi/open-api.yaml @@ -93,6 +93,22 @@ 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. + 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 @@ -197,6 +213,36 @@ 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. At most 100 entries are returned. + items: + $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: @@ -218,6 +264,13 @@ components: lastLink: type: string description: Link to the last page of results + AboutWithFacets: + allOf: + - $ref: '#/components/schemas/About' + - type: object + properties: + facets: + $ref: '#/components/schemas/FacetsResult' Event: type: object properties: @@ -546,7 +599,7 @@ components: - about properties: about: - $ref: '#/components/schemas/About' + $ref: '#/components/schemas/AboutWithFacets' items: type: array description: List of patron request @@ -1453,9 +1506,10 @@ paths: - $ref: '#/components/parameters/Offset' - $ref: '#/components/parameters/Side' - $ref: '#/components/parameters/Symbol' + - $ref: '#/components/parameters/Facets' responses: '200': - description: Successful retrieval of events + description: Successful retrieval of patron requests content: application/json: schema: diff --git a/broker/patron_request/api/api-handler.go b/broker/patron_request/api/api-handler.go index 025d47be..e75a430f 100644 --- a/broker/patron_request/api/api-handler.go +++ b/broker/patron_request/api/api-handler.go @@ -185,10 +185,46 @@ 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 = 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) + } + if err != nil { + if errors.Is(err, pr_db.ErrUnsupportedFacet) { + api.AddBadRequestError(ctx, w, err) + } else { + api.AddInternalError(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) } +func toProAboutWithFacets(a oapi.About) proapi.AboutWithFacets { + return proapi.AboutWithFacets{ + 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 { 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/prcql.go b/broker/patron_request/db/prcql.go index a13ee954..da2d9d3d 100644 --- a/broker/patron_request/db/prcql.go +++ b/broker/patron_request/db/prcql.go @@ -170,6 +170,45 @@ 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) { + // 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 { + 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:] + } + rows, err := db.Query(ctx, sql, res.GetQueryArguments()...) + if err != nil { + return nil, fmt.Errorf("failed to execute facets query: %w", err) + } + defer rows.Close() + var items []FacetsPatronRequestsRow + for rows.Next() { + var i FacetsPatronRequestsRow + if err := rows.Scan(&i.Value, &i.Count); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, 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 90f454ad..39c71378 100644 --- a/broker/patron_request/db/prrepo.go +++ b/broker/patron_request/db/prrepo.go @@ -1,6 +1,8 @@ package pr_db import ( + "errors" + "fmt" "strings" "github.com/indexdata/crosslink/broker/common" @@ -18,6 +20,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, 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) @@ -35,6 +38,18 @@ type PrRepo interface { DeleteItemById(ctx common.ExtendedContext, id string) error } +var ErrUnsupportedFacet = errors.New("unsupported facet field") + +type Facet struct { + Field string + Values []FacetValue +} + +type FacetValue struct { + Value string + Count int64 +} + type PgPrRepo struct { repo.PgBaseRepo[PrRepo] queries Queries @@ -102,6 +117,35 @@ 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) { + var facets []Facet + for _, field := range facetFields { + switch field { + case "requester_symbol", "supplier_symbol": + rows, err := r.queries.FacetsPatronRequestsCql(ctx, r.GetConnOrTx(), field, 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, + }) + default: + return nil, fmt.Errorf("%w: %s", ErrUnsupportedFacet, 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 26fecf13..5221481f 100644 --- a/broker/sqlc/pr_query.sql +++ b/broker/sqlc/pr_query.sql @@ -24,6 +24,17 @@ WHERE ill_request IS NOT NULL 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 +GROUP BY 1 +ORDER BY count DESC, value ASC +LIMIT 100; + -- name: UpdatePatronRequest :one -- internal_note ($20) is a pass-through to keep PatronRequest <-> UpdatePatronRequestParams -- convertible; edits go through UpdatePatronRequestInternalNote. diff --git a/broker/test/patron_request/api/api-handler_test.go b/broker/test/patron_request/api/api-handler_test.go index 54530730..21b9aa71 100644 --- a/broker/test/patron_request/api/api-handler_test.go +++ b/broker/test/patron_request/api/api-handler_test.go @@ -790,3 +790,126 @@ 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=title%3Dfacets%20title&offset=0&limit=1", []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) + assert.NoError(t, err) + 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.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=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, 1) + 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) + + 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) + 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) + + // 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 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") +}