CROSSLINK-277 GVI#581
Open
adamdickmeiss wants to merge 26 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the broker’s holdings lookup pipeline to propagate ISO18626 ServiceType into SRU/Z39.50 holdings parsing and adds a configurable SRU holdings parser (reservoir vs MARC-21plus-1) to support MARC 924-based routing.
Changes:
- Add
ServiceTypetoadapter.LookupParamsand update theHoldingsParserinterface to accept params during parsing. - Introduce
HOLDINGS_FORMATconfiguration and select the SRU holdings parser based on it. - Implement a new
Marc21Plus1HoldingsParserthat extracts holdings from MARC 924 and filters by requested service type.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| broker/service/supplierlocator.go | Populates LookupParams.ServiceType from the transaction’s ServiceInfo so downstream parsers can filter appropriately. |
| broker/README.md | Documents the new HOLDINGS_FORMAT configuration option. |
| broker/availability/availability_zoom.go | Passes LookupParams into holdings parsing for Z39.50 records. |
| broker/app/app.go | Reads HOLDINGS_FORMAT from env and wires it into adapter creation. |
| broker/adapter/sru_holdings.go | Threads LookupParams through SRU record parsing to the configured holdings parser. |
| broker/adapter/reservoir_holdings_parser.go | Updates parser signature to accept LookupParams (currently unused). |
| broker/adapter/opac_holdings_parser.go | Updates parser signature to accept LookupParams (currently unused). |
| broker/adapter/marc21_plus_holdings_parser.go | Adds new MARC 924-based parser with service-type-aware filtering. |
| broker/adapter/marc_holdings_parser.go | Updates parser signature to accept LookupParams (currently unused). |
| broker/adapter/holdings.go | Extends LookupParams and updates the HoldingsParser interface signature. |
| broker/adapter/create_holdings.go | Adds HOLDINGS_FORMAT and chooses parser implementation based on config. |
jakub-id
approved these changes
May 12, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (2)
broker/holdings/adapter_zoom.go:114
- The final return uses
pqfList[0]even when only CQL queries were built (i.e.,pqfListempty). If all CQL searches return no holdings, this will panic with index-out-of-range. Return an empty query string, or track/return the first attempted query from whichever list is non-empty.
broker/holdings/adapter_zoom.go:108 - This error wraps a Z39.50/ZOOM operation but says "failed to search SRU server query". Updating the message to refer to the Z39.50 server (or just "server") will avoid confusion when diagnosing failures.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (2)
broker/holdings/adapter_zoom.go:114
- If
pqfListis empty butcqlListis non-empty, the finalreturn nil, pqfList[0], nilwill panic with index-out-of-range. Return a query string based on whichever list was attempted (e.g. first CQL when PQF is empty) or return an empty query string when none were built.
broker/holdings/adapter_zoom.go:108 - Error message says "failed to search SRU server query" but this code path is executing a ZOOM/Z39.50 search using a CQL query. This is misleading for troubleshooting; update the message to reference the correct backend (ZOOM/Z39.50).
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
broker/holdings/adapter_zoom.go:114
- The final
return nil, pqfList[0], nilwill panic whenpqfListis empty butcqlListis non-empty and yields no results (valid withQueryConfig.type=cql). Track the last attempted query (PQF or CQL) and return that safely, or branch on which list is non-empty before indexing.
broker/holdings/adapter_zoom.go:108 - The error message on CQL search failure says "failed to search SRU server query" even though this adapter is named/used as a ZOOM/Z39.50 adapter (and may be used for either protocol depending on options). Consider making the message protocol-agnostic (e.g. "failed to search remote target") or using consistent wording with the earlier "Z39.50 server" message.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
broker/holdings/adapter_zoom.go:114
- Potential panic when only CQL queries are generated: if pqfList is empty (e.g. QueryConfig.type=cql), the function falls through and returns
pqfList[0], which will index an empty slice. Return the first query from whichever list is non-empty (or track the last attempted query) instead of always using pqfList[0].
broker/holdings/adapter_zoom.go:108 - The error message inside the CQL query loop says "failed to search SRU server query" but this code is executing a Z39.50/ZOOM search. This is misleading for debugging; update the message to refer to the correct backend (Z39.50/ZOOM).
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 37 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
broker/holdings/adapter_zoom.go:113
Lookupcan panic when only CQL queries are generated (e.g., QueryConfig.Type = cql): ifpqfListis empty, the finalreturn nil, pqfList[0], nilwill index out of range. Track the first attempted query (PQF or CQL) and return that, or return an empty query string when none were attempted.
broker/holdings/adapter_zoom.go:107- Error message says "failed to search SRU server query" but this code is executing a ZOOM/Z39.50 search with a CQL query. Update the message to reference the correct backend (Z39.50/ZOOM) to avoid confusing operators and tests.
Comment on lines
+11
to
+13
| type QueryBuilderPqf struct { | ||
| config directory.QueryConfig | ||
| } |
Comment on lines
+115
to
+118
| return events.LogErrorAndReturnResult(ctx, "failed to get adapter for consortia", err) | ||
| } | ||
| if lookupAdapter == nil { | ||
| return events.LogErrorAndReturnResult(ctx, "no holdings adapter available for consortia", fmt.Errorf("no adapter found")) |
| return events.LogErrorAndReturnResult(ctx, "failed to get adapter for consortia", err) | ||
| } | ||
| if lookupAdapter == nil { | ||
| return events.LogErrorAndReturnResult(ctx, "no holdings adapter available for consortia", fmt.Errorf("no adapter found")) |
Comment on lines
+63
to
+67
| func cqlEncode(value string) string { | ||
| // escape backslashes and double quotes | ||
| escaped := "\"" | ||
| for _, r := range value { | ||
| if r == '\\' || r == '"' { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://index-data.atlassian.net/browse/CROSSLINK-277