Skip to content

Commit 5b3a1ec

Browse files
JanisSaldabolsJanisSaldabols
committed
CROSSLINK-266 Check incoming duplication messages
1 parent 5a2c9c4 commit 5b3a1ec

10 files changed

Lines changed: 300 additions & 3 deletions

File tree

‎broker/handler/iso18626-handler.go‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ const (
4747
InvalidAction ErrorValue = "invalid action"
4848
InvalidStatus ErrorValue = "invalid status"
4949
InvalidReason ErrorValue = "invalid reason"
50+
ReqIsDuplicate ErrorValue = "duplicate request: an ILL request with the same patron, item, and service type was already submitted within the configured time window"
5051
)
5152

5253
const PublicFailedToProcessReqMsg = "failed to process request"
@@ -64,6 +65,7 @@ var ErrReqAgencyNotFound = errors.New(string(ReqAgencyNotFound))
6465
var ErrInvalidAction = errors.New(string(InvalidAction))
6566
var ErrInvalidStatus = errors.New(string(InvalidStatus))
6667
var ErrInvalidReason = errors.New(string(InvalidReason))
68+
var ErrDuplicateRequest = errors.New(string(ReqIsDuplicate))
6769

6870
var waitingReqs = map[string]RequestWait{}
6971

@@ -139,6 +141,10 @@ func Iso18626PostHandler(repo ill_db.IllRepo, eventBus events.EventBus, dirAdapt
139141
}
140142

141143
func handleNewRequest(ctx common.ExtendedContext, request *iso18626.Request, repo ill_db.IllRepo, requesterSymbol pgtype.Text, peers []ill_db.Peer) (string, error) {
144+
if err := checkDuplicateRequest(ctx, request, repo, requesterSymbol.String, peers[0]); err != nil {
145+
return "", err
146+
}
147+
142148
supplierSymbol := createPgText(request.Header.SupplyingAgencyId.AgencyIdType.Text + ":" + request.Header.SupplyingAgencyId.AgencyIdValue)
143149
requesterRequestId := createPgText(request.Header.RequestingAgencyRequestId)
144150
supplierRequestId := createPgText(request.Header.SupplyingAgencyRequestId)
@@ -173,6 +179,42 @@ func handleNewRequest(ctx common.ExtendedContext, request *iso18626.Request, rep
173179
return id, err
174180
}
175181

182+
func checkDuplicateRequest(ctx common.ExtendedContext, request *iso18626.Request, repo ill_db.IllRepo, requesterSymbol string, peer ill_db.Peer) error {
183+
windowHours := peer.CustomData.DuplicateCheckWindowHours
184+
if windowHours == nil || *windowHours <= 0 {
185+
return nil
186+
}
187+
188+
patronId := ""
189+
if request.PatronInfo != nil {
190+
patronId = request.PatronInfo.PatronId
191+
}
192+
193+
lookupParams := service.CreateHoldingsParams(ill_db.IllTransactionData{
194+
BibliographicInfo: request.BibliographicInfo,
195+
ServiceInfo: request.ServiceInfo,
196+
})
197+
198+
_, err := repo.FindDuplicateIllTransaction(ctx, ill_db.FindDuplicateIllTransactionParams{
199+
RequesterSymbol: createPgText(requesterSymbol),
200+
Hours: service.ToInt32(*windowHours),
201+
PatronID: patronId,
202+
Identifier: lookupParams.Identifier,
203+
Title: lookupParams.Title,
204+
ServiceType: lookupParams.ServiceType,
205+
Isbn: lookupParams.Isbn,
206+
Issn: lookupParams.Issn,
207+
})
208+
if err != nil {
209+
if errors.Is(err, pgx.ErrNoRows) {
210+
return nil // no duplicate found
211+
}
212+
ctx.Logger().Warn("failed to check for duplicate requests, proceeding", "error", err)
213+
return nil // fail open
214+
}
215+
return ErrDuplicateRequest
216+
}
217+
176218
func handleRetryRequest(ctx common.ExtendedContext, request *iso18626.Request, repo ill_db.IllRepo) (string, bool, error) {
177219
// ServiceInfo already nil checked in handleIso18626Request
178220
prevReqId := createPgText(request.ServiceInfo.RequestingAgencyPreviousRequestId)
@@ -268,6 +310,8 @@ func handleRequest(ctx common.ExtendedContext, illMessage *iso18626.ISO18626Mess
268310
handleRequestError(ctx, w, request, iso18626.TypeErrorTypeUnrecognisedDataValue, ReqIdAlreadyExists)
269311
} else if errors.Is(err, ErrRetryNotPossible) {
270312
handleRequestError(ctx, w, request, iso18626.TypeErrorTypeUnrecognisedDataValue, RetryNotPossible)
313+
} else if errors.Is(err, ErrDuplicateRequest) {
314+
handleRequestError(ctx, w, request, iso18626.TypeErrorTypeUnrecognisedDataValue, ReqIsDuplicate)
271315
} else {
272316
ctx.Logger().Error(InternalFailedToSaveTx, "error", err)
273317
http.Error(w, PublicFailedToProcessReqMsg, http.StatusInternalServerError)

‎broker/handler/iso18626-handler_test.go‎

Lines changed: 188 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,3 +155,191 @@ type MockIllRepositoryNoSelectedSupplier struct {
155155
func (r *MockIllRepositoryNoSelectedSupplier) GetSelectedSupplierForIllTransaction(ctx common.ExtendedContext, illTransId string) (ill_db.LocatedSupplier, error) {
156156
return ill_db.LocatedSupplier{}, pgx.ErrNoRows
157157
}
158+
159+
// mockDuplicateCheckRepo overrides FindDuplicateIllTransaction to return
160+
// configurable results for duplicate-check testing.
161+
type mockDuplicateCheckRepo struct {
162+
mocks.MockIllRepositorySuccess
163+
duplicateId string
164+
err error
165+
called bool
166+
params ill_db.FindDuplicateIllTransactionParams
167+
}
168+
169+
func (r *mockDuplicateCheckRepo) FindDuplicateIllTransaction(ctx common.ExtendedContext, params ill_db.FindDuplicateIllTransactionParams) (string, error) {
170+
r.called = true
171+
r.params = params
172+
return r.duplicateId, r.err
173+
}
174+
175+
func TestCheckDuplicateRequest(t *testing.T) {
176+
window1 := 1
177+
window0 := 0
178+
windowNeg := -1
179+
180+
baseRequest := &iso18626.Request{
181+
BibliographicInfo: iso18626.BibliographicInfo{
182+
SupplierUniqueRecordId: "rec-1",
183+
Title: "Test Title",
184+
},
185+
ServiceInfo: &iso18626.ServiceInfo{
186+
ServiceType: iso18626.TypeServiceTypeLoan,
187+
},
188+
PatronInfo: &iso18626.PatronInfo{
189+
PatronId: "patron-1",
190+
},
191+
}
192+
193+
isbnRequest := &iso18626.Request{
194+
BibliographicInfo: iso18626.BibliographicInfo{
195+
BibliographicItemId: []iso18626.BibliographicItemId{
196+
{
197+
BibliographicItemIdentifier: "978-1234",
198+
BibliographicItemIdentifierCode: iso18626.TypeSchemeValuePair{Text: "ISBN"},
199+
},
200+
},
201+
},
202+
ServiceInfo: &iso18626.ServiceInfo{ServiceType: iso18626.TypeServiceTypeCopy},
203+
PatronInfo: &iso18626.PatronInfo{PatronId: "patron-2"},
204+
}
205+
206+
tests := []struct {
207+
name string
208+
request *iso18626.Request
209+
peer ill_db.Peer
210+
duplicateId string
211+
repoErr error
212+
wantErr error
213+
wantRepoCalled bool
214+
wantPatronId string
215+
wantWindowHrs int32
216+
wantIdentifier string
217+
wantIsbn string
218+
wantIssn string
219+
wantTitle string
220+
wantSvcType string
221+
}{
222+
{
223+
name: "no DuplicateCheckWindowHours configured - skips check",
224+
request: baseRequest,
225+
peer: ill_db.Peer{},
226+
wantErr: nil,
227+
wantRepoCalled: false,
228+
},
229+
{
230+
name: "window is zero - skips check",
231+
request: baseRequest,
232+
peer: ill_db.Peer{CustomData: directory.Entry{DuplicateCheckWindowHours: &window0}},
233+
wantErr: nil,
234+
wantRepoCalled: false,
235+
},
236+
{
237+
name: "window is negative - skips check",
238+
request: baseRequest,
239+
peer: ill_db.Peer{CustomData: directory.Entry{DuplicateCheckWindowHours: &windowNeg}},
240+
wantErr: nil,
241+
wantRepoCalled: false,
242+
},
243+
{
244+
name: "db error - fails open, allows request through",
245+
request: baseRequest,
246+
peer: ill_db.Peer{CustomData: directory.Entry{DuplicateCheckWindowHours: &window1}},
247+
repoErr: errors.New("db connection error"),
248+
wantErr: nil,
249+
wantRepoCalled: true,
250+
wantPatronId: "patron-1",
251+
wantWindowHrs: 1,
252+
wantIdentifier: "rec-1",
253+
wantTitle: "Test Title",
254+
wantSvcType: "Loan",
255+
},
256+
{
257+
name: "no duplicate found (ErrNoRows) - not a duplicate",
258+
request: baseRequest,
259+
peer: ill_db.Peer{CustomData: directory.Entry{DuplicateCheckWindowHours: &window1}},
260+
repoErr: pgx.ErrNoRows,
261+
wantErr: nil,
262+
wantRepoCalled: true,
263+
wantPatronId: "patron-1",
264+
wantWindowHrs: 1,
265+
wantIdentifier: "rec-1",
266+
wantTitle: "Test Title",
267+
wantSvcType: "Loan",
268+
},
269+
{
270+
name: "duplicate found - returns ErrDuplicateRequest",
271+
request: baseRequest,
272+
peer: ill_db.Peer{CustomData: directory.Entry{DuplicateCheckWindowHours: &window1}},
273+
duplicateId: "existing-tx-id",
274+
wantErr: ErrDuplicateRequest,
275+
wantRepoCalled: true,
276+
wantPatronId: "patron-1",
277+
wantWindowHrs: 1,
278+
wantIdentifier: "rec-1",
279+
wantTitle: "Test Title",
280+
wantSvcType: "Loan",
281+
},
282+
{
283+
name: "nil PatronInfo - uses empty patron ID in query",
284+
request: &iso18626.Request{
285+
BibliographicInfo: iso18626.BibliographicInfo{SupplierUniqueRecordId: "rec-1"},
286+
ServiceInfo: &iso18626.ServiceInfo{ServiceType: iso18626.TypeServiceTypeLoan},
287+
},
288+
peer: ill_db.Peer{CustomData: directory.Entry{DuplicateCheckWindowHours: &window1}},
289+
repoErr: pgx.ErrNoRows,
290+
wantErr: nil,
291+
wantRepoCalled: true,
292+
wantPatronId: "",
293+
wantWindowHrs: 1,
294+
wantIdentifier: "rec-1",
295+
wantSvcType: "Loan",
296+
},
297+
{
298+
name: "isbn passed as parameter to DB query",
299+
request: isbnRequest,
300+
peer: ill_db.Peer{CustomData: directory.Entry{DuplicateCheckWindowHours: &window1}},
301+
repoErr: pgx.ErrNoRows,
302+
wantErr: nil,
303+
wantRepoCalled: true,
304+
wantPatronId: "patron-2",
305+
wantWindowHrs: 1,
306+
wantIsbn: "978-1234",
307+
wantSvcType: "Copy",
308+
},
309+
{
310+
name: "duplicate found via isbn - returns ErrDuplicateRequest",
311+
request: isbnRequest,
312+
peer: ill_db.Peer{CustomData: directory.Entry{DuplicateCheckWindowHours: &window1}},
313+
duplicateId: "existing-tx-isbn",
314+
wantErr: ErrDuplicateRequest,
315+
wantRepoCalled: true,
316+
wantPatronId: "patron-2",
317+
wantWindowHrs: 1,
318+
wantIsbn: "978-1234",
319+
wantSvcType: "Copy",
320+
},
321+
}
322+
323+
for _, tt := range tests {
324+
t.Run(tt.name, func(t *testing.T) {
325+
appCtx := common.CreateExtCtxWithArgs(context.Background(), nil)
326+
mockRepo := &mockDuplicateCheckRepo{
327+
duplicateId: tt.duplicateId,
328+
err: tt.repoErr,
329+
}
330+
err := checkDuplicateRequest(appCtx, tt.request, mockRepo, "ISIL:REQ1", tt.peer)
331+
assert.Equal(t, tt.wantErr, err)
332+
assert.Equal(t, tt.wantRepoCalled, mockRepo.called)
333+
if tt.wantRepoCalled {
334+
assert.Equal(t, "ISIL:REQ1", mockRepo.params.RequesterSymbol.String)
335+
assert.Equal(t, tt.wantPatronId, mockRepo.params.PatronID)
336+
assert.Equal(t, tt.wantWindowHrs, mockRepo.params.Hours)
337+
assert.Equal(t, tt.wantIdentifier, mockRepo.params.Identifier)
338+
assert.Equal(t, tt.wantIsbn, mockRepo.params.Isbn)
339+
assert.Equal(t, tt.wantIssn, mockRepo.params.Issn)
340+
assert.Equal(t, tt.wantTitle, mockRepo.params.Title)
341+
assert.Equal(t, tt.wantSvcType, mockRepo.params.ServiceType)
342+
}
343+
})
344+
}
345+
}

‎broker/ill_db/illrepo.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ type IllRepo interface {
5050
GetExclusiveBranchSymbolsByPeerId(ctx common.ExtendedContext, peerId string) ([]BranchSymbol, error)
5151
DeleteBranchSymbolByPeerId(ctx common.ExtendedContext, peerId string) error
5252
CallArchiveIllTransactionByDateAndStatus(ctx common.ExtendedContext, toDate time.Time, statuses []string) error
53+
FindDuplicateIllTransaction(ctx common.ExtendedContext, params FindDuplicateIllTransactionParams) (string, error)
5354
}
5455

5556
type PgIllRepo struct {
@@ -477,6 +478,10 @@ func (r *PgIllRepo) CallArchiveIllTransactionByDateAndStatus(ctx common.Extended
477478
return err
478479
}
479480

481+
func (r *PgIllRepo) FindDuplicateIllTransaction(ctx common.ExtendedContext, params FindDuplicateIllTransactionParams) (string, error) {
482+
return r.queries.FindDuplicateIllTransaction(ctx, r.GetConnOrTx(), params)
483+
}
484+
480485
func (r *PgIllRepo) GetExclusiveBranchSymbolsByPeerId(ctx common.ExtendedContext, peerId string) ([]BranchSymbol, error) {
481486
rows, err := r.queries.GetExclusiveBranchSymbolsByPeerId(ctx, r.GetConnOrTx(), peerId)
482487
var symbols []BranchSymbol

‎broker/patron_request/service/statemodel_capabilities.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ const (
4242
BorrowerStateRetryAccepted pr_db.PatronRequestState = "RETRY_ACCEPTED"
4343
BorrowerStateRetryRejected pr_db.PatronRequestState = "RETRY_REJECTED"
4444
BorrowerStateManuallyClosed pr_db.PatronRequestState = "MANUALLY_CLOSED"
45+
BorrowerStateClosedDuplicate pr_db.PatronRequestState = "CLOSED_DUPLICATE"
4546
LenderStateNew pr_db.PatronRequestState = "NEW"
4647
LenderStateValidated pr_db.PatronRequestState = "VALIDATED"
4748
LenderStateWillSupply pr_db.PatronRequestState = "WILL_SUPPLY"
@@ -124,6 +125,7 @@ func requesterBuiltInStates() []string {
124125
string(BorrowerStateRetryRejected),
125126
string(BorrowerStateRetryPending),
126127
string(BorrowerStateManuallyClosed),
128+
string(BorrowerStateClosedDuplicate),
127129
})
128130
}
129131

‎broker/patron_request/service/statemodels/state-models.json‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,8 @@
3636
"name": "send-request",
3737
"desc": "Send ISO18626 request to the supplier or broker",
3838
"transitions": {
39-
"success": "SENT"
39+
"success": "SENT",
40+
"failure": "CLOSED_DUPLICATE"
4041
},
4142
"trigger": "auto"
4243
}
@@ -54,7 +55,8 @@
5455
"name": "send-request",
5556
"desc": "Send ISO18626 request to the supplier or broker",
5657
"transitions": {
57-
"success": "SENT"
58+
"success": "SENT",
59+
"failure": "CLOSED_DUPLICATE"
5860
}
5961
}
6062
]
@@ -395,6 +397,13 @@
395397
"terminal": true,
396398
"manualClose": true
397399
},
400+
{
401+
"name": "CLOSED_DUPLICATE",
402+
"display": "Closed Duplicate",
403+
"desc": "Closed due to duplicate request",
404+
"side": "REQUESTER",
405+
"terminal": true
406+
},
398407
{
399408
"name": "NEW",
400409
"display": "New",

‎broker/sqlc/ill_query.sql‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,3 +234,30 @@ SELECT archive_ill_transaction_by_date_and_status($1, $2);
234234
SELECT sqlc.embed(branch_symbol)
235235
FROM branch_symbol b
236236
WHERE b.peer_id = $1 AND b.symbol_value not in (SELECT s.symbol_value FROM symbol s);
237+
238+
-- name: FindDuplicateIllTransaction :one
239+
SELECT ill_transaction.id
240+
FROM ill_transaction
241+
WHERE requester_symbol = $1
242+
AND timestamp > NOW() - make_interval(hours => sqlc.arg(hours)::int4)
243+
AND COALESCE(ill_transaction_data->'patronInfo'->>'patronId', '') = sqlc.arg(patron_id)::text
244+
AND COALESCE(ill_transaction_data->'bibliographicInfo'->>'supplierUniqueRecordId', '') = sqlc.arg(identifier)::text
245+
AND COALESCE(ill_transaction_data->'bibliographicInfo'->>'title', '') = sqlc.arg(title)::text
246+
AND COALESCE(ill_transaction_data->'serviceInfo'->>'serviceType', '') = sqlc.arg(service_type)::text
247+
AND COALESCE((
248+
SELECT elem->>'bibliographicItemIdentifier'
249+
FROM jsonb_array_elements(
250+
COALESCE(ill_transaction_data->'bibliographicInfo'->'bibliographicItemId', '[]'::jsonb)
251+
) AS elem
252+
WHERE elem->'bibliographicItemIdentifierCode'->>'#text' = 'ISBN'
253+
LIMIT 1
254+
), '') = sqlc.arg(isbn)::text
255+
AND COALESCE((
256+
SELECT elem->>'bibliographicItemIdentifier'
257+
FROM jsonb_array_elements(
258+
COALESCE(ill_transaction_data->'bibliographicInfo'->'bibliographicItemId', '[]'::jsonb)
259+
) AS elem
260+
WHERE elem->'bibliographicItemIdentifierCode'->>'#text' = 'ISSN'
261+
LIMIT 1
262+
), '') = sqlc.arg(issn)::text
263+
LIMIT 1;

‎broker/test/mocks/mock_illrepo.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99

1010
"github.com/indexdata/crosslink/broker/common"
1111
"github.com/indexdata/crosslink/broker/ill_db"
12+
"github.com/jackc/pgx/v5"
1213
"github.com/jackc/pgx/v5/pgtype"
1314
"github.com/stretchr/testify/mock"
1415
)
@@ -197,6 +198,10 @@ func (r *MockIllRepositorySuccess) GetLocatedSupplierByIdForUpdate(ctx common.Ex
197198
return ill_db.LocatedSupplier{ID: id}, nil
198199
}
199200

201+
func (r *MockIllRepositorySuccess) FindDuplicateIllTransaction(ctx common.ExtendedContext, params ill_db.FindDuplicateIllTransactionParams) (string, error) {
202+
return "", pgx.ErrNoRows
203+
}
204+
200205
type MockIllRepositoryError struct {
201206
mock.Mock
202207
}
@@ -338,3 +343,7 @@ func (r *MockIllRepositoryError) GetExclusiveBranchSymbolsByPeerId(ctx common.Ex
338343
func (r *MockIllRepositoryError) GetLocatedSupplierByIdForUpdate(ctx common.ExtendedContext, id string) (ill_db.LocatedSupplier, error) {
339344
return ill_db.LocatedSupplier{}, errors.New("DB error")
340345
}
346+
347+
func (r *MockIllRepositoryError) FindDuplicateIllTransaction(ctx common.ExtendedContext, params ill_db.FindDuplicateIllTransactionParams) (string, error) {
348+
return "", errors.New("DB error")
349+
}

0 commit comments

Comments
 (0)