diff --git a/broker/migrations/034_add_item_cql_indexes.down.sql b/broker/migrations/034_add_item_cql_indexes.down.sql new file mode 100644 index 00000000..6d94be36 --- /dev/null +++ b/broker/migrations/034_add_item_cql_indexes.down.sql @@ -0,0 +1,3 @@ +DROP INDEX IF EXISTS idx_item_call_number_pr_id; +DROP INDEX IF EXISTS idx_item_item_id_pr_id; +DROP INDEX IF EXISTS idx_item_barcode_pr_id; diff --git a/broker/migrations/034_add_item_cql_indexes.up.sql b/broker/migrations/034_add_item_cql_indexes.up.sql new file mode 100644 index 00000000..7e173548 --- /dev/null +++ b/broker/migrations/034_add_item_cql_indexes.up.sql @@ -0,0 +1,10 @@ +CREATE INDEX idx_item_barcode_pr_id + ON item (barcode, pr_id); + +CREATE INDEX idx_item_item_id_pr_id + ON item (item_id, pr_id) + WHERE item_id IS NOT NULL; + +CREATE INDEX idx_item_call_number_pr_id + ON item (call_number, pr_id) + WHERE call_number IS NOT NULL; diff --git a/broker/oapi/open-api.yaml b/broker/oapi/open-api.yaml index 55829da5..21b24562 100644 --- a/broker/oapi/open-api.yaml +++ b/broker/oapi/open-api.yaml @@ -1359,7 +1359,8 @@ paths: Query parameter cql can be used to filter the results. With cql you can use these fields state, side, requester_symbol, supplier_symbol, needs_attention, has_notification, has_cost, has_unread_notification, service_type, service_level, created_at, needed_at, - requester_req_id, title, author, patron, terminal_state, updated_at, cql.serverChoice. + requester_req_id, title, author, isbn, issn, item_id, barcode, call_number, patron, terminal_state, + updated_at, cql.serverChoice. tags: - patron-requests-api parameters: diff --git a/broker/patron_request/db/field_exists_string.go b/broker/patron_request/db/field_exists_string.go new file mode 100644 index 00000000..3844fa36 --- /dev/null +++ b/broker/patron_request/db/field_exists_string.go @@ -0,0 +1,91 @@ +package pr_db + +import ( + "fmt" + + "github.com/indexdata/cql-go/cql" + "github.com/indexdata/cql-go/pgcql" +) + +type FieldExistsString struct { + column string + table string + alias string + correlation string + valueExpression string + sortExpression string + field *pgcql.FieldString +} + +func NewFieldExistsString(table, alias, correlation, valueExpression string) *FieldExistsString { + field := pgcql.NewFieldString().WithLikeOps() + field.SetColumn(valueExpression) + return &FieldExistsString{ + table: table, + alias: alias, + correlation: correlation, + valueExpression: valueExpression, + field: field, + } +} + +func (f *FieldExistsString) GetColumn() string { + return f.column +} + +func (f *FieldExistsString) SetColumn(column string) { + f.column = column +} + +func (f *FieldExistsString) Sort() string { + return f.sortExpression +} + +func (f *FieldExistsString) WithSortExpression(sortExpression string) *FieldExistsString { + f.sortExpression = sortExpression + return f +} + +func (f *FieldExistsString) WithField(field *pgcql.FieldString) *FieldExistsString { + f.field = field + f.field.SetColumn(f.valueExpression) + return f +} + +func (f *FieldExistsString) Generate(sc cql.SearchClause, queryArgumentIndex int) (string, []any, error) { + switch { + case sc.Term == "" && isPositiveStringRelation(sc.Relation): + return "NOT " + f.existsSql(f.nonEmptyValuePredicate()), nil, nil + case sc.Term == "" && sc.Relation == cql.NE: + return f.existsSql(f.nonEmptyValuePredicate()), nil, nil + case sc.Relation == cql.NE: + sc.Relation = cql.EQ + predicate, args, err := f.field.Generate(sc, queryArgumentIndex) + if err != nil { + return "", nil, err + } + return "NOT " + f.existsSql(predicate), args, nil + default: + predicate, args, err := f.field.Generate(sc, queryArgumentIndex) + if err != nil { + return "", nil, err + } + return f.existsSql(predicate), args, nil + } +} + +func isPositiveStringRelation(relation cql.Relation) bool { + return relation == cql.EQ || relation == cql.EXACT || relation == "==" +} + +func (f *FieldExistsString) existsSql(predicate string) string { + source := f.table + if f.alias != "" { + source += " " + f.alias + } + return fmt.Sprintf("EXISTS (SELECT 1 FROM %s WHERE %s AND %s)", source, f.correlation, predicate) +} + +func (f *FieldExistsString) nonEmptyValuePredicate() string { + return fmt.Sprintf("COALESCE(%s, '') <> ''", f.valueExpression) +} diff --git a/broker/patron_request/db/prcql.go b/broker/patron_request/db/prcql.go index b61db410..8d432e67 100644 --- a/broker/patron_request/db/prcql.go +++ b/broker/patron_request/db/prcql.go @@ -143,6 +143,11 @@ func handlePatronRequestsQuery(cqlString string, noBaseArgs int) (pgcql.Query, e def.AddField("isbn", NewFieldTextArrayContains("bibliographic_item_identifiers(ill_request, 'ISBN')").WithFunction("norm_isxn")) def.AddField("issn", NewFieldTextArrayContains("bibliographic_item_identifiers(ill_request, 'ISSN')").WithFunction("norm_isxn")) + itemCorrelation := "cql_item.pr_id = patron_request_search_view.id" + def.AddField("item_id", NewFieldExistsString("item", "cql_item", itemCorrelation, "cql_item.item_id")) + def.AddField("barcode", NewFieldExistsString("item", "cql_item", itemCorrelation, "cql_item.barcode")) + def.AddField("call_number", NewFieldExistsString("item", "cql_item", itemCorrelation, "cql_item.call_number")) + f = pgcql.NewFieldString().WithLikeOps() def.AddField("patron", f) diff --git a/broker/patron_request/db/prcql_test.go b/broker/patron_request/db/prcql_test.go index 895a3c0b..7e55a8cd 100644 --- a/broker/patron_request/db/prcql_test.go +++ b/broker/patron_request/db/prcql_test.go @@ -98,6 +98,121 @@ func TestHandlePatronRequestsQueryIsbnUsesNormIsxn(t *testing.T) { } } +func TestFieldExistsStringGenerate(t *testing.T) { + f := NewFieldExistsString("item", "i", "i.pr_id = pr.id", "i.barcode") + + t.Run("eq uses exists with string field predicate", func(t *testing.T) { + sc := searchClauseForTest("abc", "=") + gotSQL, gotArgs, err := f.Generate(sc, 3) + if err != nil { + t.Fatalf("Generate() error = %v", err) + } + wantSQL := "EXISTS (SELECT 1 FROM item i WHERE i.pr_id = pr.id AND i.barcode = $3)" + if gotSQL != wantSQL { + t.Fatalf("sql = %q, want %q", gotSQL, wantSQL) + } + if len(gotArgs) != 1 || gotArgs[0] != "abc" { + t.Fatalf("args = %#v, want one raw term arg", gotArgs) + } + }) + + t.Run("wildcard eq uses exists with like predicate", func(t *testing.T) { + sc := searchClauseForTest("abc*", "=") + gotSQL, gotArgs, err := f.Generate(sc, 4) + if err != nil { + t.Fatalf("Generate() error = %v", err) + } + wantSQL := "EXISTS (SELECT 1 FROM item i WHERE i.pr_id = pr.id AND i.barcode LIKE $4)" + if gotSQL != wantSQL { + t.Fatalf("sql = %q, want %q", gotSQL, wantSQL) + } + if len(gotArgs) != 1 || gotArgs[0] != "abc%" { + t.Fatalf("args = %#v, want wildcard-converted term arg", gotArgs) + } + }) + + t.Run("ne uses not exists with positive string field predicate", func(t *testing.T) { + sc := searchClauseForTest("abc*", "<>") + gotSQL, gotArgs, err := f.Generate(sc, 5) + if err != nil { + t.Fatalf("Generate() error = %v", err) + } + wantSQL := "NOT EXISTS (SELECT 1 FROM item i WHERE i.pr_id = pr.id AND i.barcode LIKE $5)" + if gotSQL != wantSQL { + t.Fatalf("sql = %q, want %q", gotSQL, wantSQL) + } + if len(gotArgs) != 1 || gotArgs[0] != "abc%" { + t.Fatalf("args = %#v, want wildcard-converted term arg", gotArgs) + } + }) + + t.Run("empty eq checks no non-empty related value", func(t *testing.T) { + sc := searchClauseForTest("", "=") + gotSQL, gotArgs, err := f.Generate(sc, 6) + if err != nil { + t.Fatalf("Generate() error = %v", err) + } + wantSQL := "NOT EXISTS (SELECT 1 FROM item i WHERE i.pr_id = pr.id AND COALESCE(i.barcode, '') <> '')" + if gotSQL != wantSQL { + t.Fatalf("sql = %q, want %q", gotSQL, wantSQL) + } + if len(gotArgs) != 0 { + t.Fatalf("args = %#v, want empty args", gotArgs) + } + }) + + t.Run("empty ne checks at least one non-empty related value", func(t *testing.T) { + sc := searchClauseForTest("", "<>") + gotSQL, gotArgs, err := f.Generate(sc, 7) + if err != nil { + t.Fatalf("Generate() error = %v", err) + } + wantSQL := "EXISTS (SELECT 1 FROM item i WHERE i.pr_id = pr.id AND COALESCE(i.barcode, '') <> '')" + if gotSQL != wantSQL { + t.Fatalf("sql = %q, want %q", gotSQL, wantSQL) + } + if len(gotArgs) != 0 { + t.Fatalf("args = %#v, want empty args", gotArgs) + } + }) +} + +func TestHandlePatronRequestsQueryItemFieldsUseExists(t *testing.T) { + tests := []struct { + name string + cql string + wantWhere string + }{ + { + name: "item_id exact", + cql: `item_id = "item-123"`, + wantWhere: "EXISTS (SELECT 1 FROM item cql_item WHERE cql_item.pr_id = patron_request_search_view.id AND cql_item.item_id = $3)", + }, + { + name: "barcode wildcard", + cql: `barcode = "abc*"`, + wantWhere: "EXISTS (SELECT 1 FROM item cql_item WHERE cql_item.pr_id = patron_request_search_view.id AND cql_item.barcode LIKE $3)", + }, + { + name: "call_number empty", + cql: `call_number = ""`, + wantWhere: "NOT EXISTS (SELECT 1 FROM item cql_item WHERE cql_item.pr_id = patron_request_search_view.id AND COALESCE(cql_item.call_number, '') <> '')", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + query, err := handlePatronRequestsQuery(tc.cql, 2) + if err != nil { + t.Fatalf("handlePatronRequestsQuery() error = %v", err) + } + if got := query.GetWhereClause(); got != tc.wantWhere { + t.Fatalf("where clause = %q, want %q", got, tc.wantWhere) + } + }) + } +} + func searchClauseForTest(term, relation string) cql.SearchClause { return cql.SearchClause{Term: term, Relation: cql.Relation(relation)} } diff --git a/broker/sqlc/pr_schema.sql b/broker/sqlc/pr_schema.sql index 66f39149..877a3249 100644 --- a/broker/sqlc/pr_schema.sql +++ b/broker/sqlc/pr_schema.sql @@ -40,6 +40,17 @@ CREATE TABLE item created_at TIMESTAMP NOT NULL DEFAULT now() ); +CREATE INDEX idx_item_barcode_pr_id + ON item (barcode, pr_id); + +CREATE INDEX idx_item_item_id_pr_id + ON item (item_id, pr_id) + WHERE item_id IS NOT NULL; + +CREATE INDEX idx_item_call_number_pr_id + ON item (call_number, pr_id) + WHERE call_number IS NOT NULL; + CREATE TABLE notification ( id VARCHAR PRIMARY KEY, diff --git a/broker/test/patron_request/db/prrepo_test.go b/broker/test/patron_request/db/prrepo_test.go index 9e776412..a95c6b46 100644 --- a/broker/test/patron_request/db/prrepo_test.go +++ b/broker/test/patron_request/db/prrepo_test.go @@ -420,6 +420,7 @@ func TestMarkConditionNotificationsReceipt(t *testing.T) { func TestListPatronRequests(t *testing.T) { prIds := []string{} + itemIds := []string{} // Create 2 requests for i := 0; i < 2; i++ { @@ -486,6 +487,48 @@ func TestListPatronRequests(t *testing.T) { }) assert.NoError(t, err) } + firstItemId := uuid.NewString() + itemIds = append(itemIds, firstItemId) + _, err := prRepo.SaveItem(appCtx, pr_db.SaveItemParams{ + ID: firstItemId, + PrID: prIds[0], + Barcode: "barcode-123", + CallNumber: pgtype.Text{ + String: "call-123", + Valid: true, + }, + Title: pgtype.Text{ + String: "Item title", + Valid: true, + }, + ItemID: pgtype.Text{ + String: "item-123", + Valid: true, + }, + CreatedAt: pgtype.Timestamp{ + Time: time.Now(), + Valid: true, + }, + }) + assert.NoError(t, err) + + secondItemId := uuid.NewString() + itemIds = append(itemIds, secondItemId) + _, err = prRepo.SaveItem(appCtx, pr_db.SaveItemParams{ + ID: secondItemId, + PrID: prIds[0], + Barcode: "barcode-456", + Title: pgtype.Text{ + String: "Second item title", + Valid: true, + }, + CreatedAt: pgtype.Timestamp{ + Time: time.Now(), + Valid: true, + }, + }) + assert.NoError(t, err) + cql := "title = Androids" list, fullCount, err := prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ Limit: 1, @@ -568,6 +611,76 @@ func TestListPatronRequests(t *testing.T) { assert.Len(t, list, 2) assert.Equal(t, int64(2), fullCount) + cql = `item_id = "item-123"` + list, fullCount, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ + Limit: 10, + Offset: 0, + }, &cql) + assert.NoError(t, err) + assert.Len(t, list, 1) + assert.Equal(t, prIds[0], list[0].ID) + assert.Equal(t, int64(1), fullCount) + + cql = `barcode = "barcode-123"` + list, fullCount, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ + Limit: 10, + Offset: 0, + }, &cql) + assert.NoError(t, err) + assert.Len(t, list, 1) + assert.Equal(t, prIds[0], list[0].ID) + assert.Equal(t, int64(1), fullCount) + + cql = `barcode = "barcode-*"` + list, fullCount, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ + Limit: 10, + Offset: 0, + }, &cql) + assert.NoError(t, err) + assert.Len(t, list, 1) + assert.Equal(t, prIds[0], list[0].ID) + assert.Equal(t, int64(1), fullCount) + + cql = `call_number = "call-123"` + list, fullCount, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ + Limit: 10, + Offset: 0, + }, &cql) + assert.NoError(t, err) + assert.Len(t, list, 1) + assert.Equal(t, prIds[0], list[0].ID) + assert.Equal(t, int64(1), fullCount) + + cql = `barcode <> "barcode-123"` + list, fullCount, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ + Limit: 10, + Offset: 0, + }, &cql) + assert.NoError(t, err) + assert.Len(t, list, 1) + assert.Equal(t, prIds[1], list[0].ID) + assert.Equal(t, int64(1), fullCount) + + cql = `call_number = ""` + list, fullCount, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ + Limit: 10, + Offset: 0, + }, &cql) + assert.NoError(t, err) + assert.Len(t, list, 1) + assert.Equal(t, prIds[1], list[0].ID) + assert.Equal(t, int64(1), fullCount) + + cql = `call_number <> ""` + list, fullCount, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ + Limit: 10, + Offset: 0, + }, &cql) + assert.NoError(t, err) + assert.Len(t, list, 1) + assert.Equal(t, prIds[0], list[0].ID) + assert.Equal(t, int64(1), fullCount) + // not found cql = "title = banners" list, fullCount, err = prRepo.ListPatronRequests(appCtx, pr_db.ListPatronRequestsParams{ @@ -597,6 +710,10 @@ func TestListPatronRequests(t *testing.T) { assert.Len(t, list, 1) assert.Equal(t, int64(2), fullCount) + for _, itemId := range itemIds { + err = prRepo.DeleteItemById(appCtx, itemId) + assert.NoError(t, err) + } for _, prId := range prIds { err = prRepo.DeletePatronRequest(appCtx, prId) assert.NoError(t, err) diff --git a/bruno/crosslink/PR Happy flow/Lending side create pull slip.bru b/bruno/crosslink/PR Happy flow/Lending side create pull slip.bru index 416daa28..5da641c4 100644 Binary files a/bruno/crosslink/PR Happy flow/Lending side create pull slip.bru and b/bruno/crosslink/PR Happy flow/Lending side create pull slip.bru differ