-
Notifications
You must be signed in to change notification settings - Fork 0
RSRVR-31 matchkey matcher args #163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
The matchkey config has a new property "args" which controls how arguments are passed to the matcher method. If value is "payload" (default behavior), only the payload content is passed. If value is "full" then the global record entry is passed. This has "localId", "sourceId", and "payload" properties at the very least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Adds a new args property to matchKey configuration to control whether matcher modules receive only the record payload (default) or the full global record object.
Changes:
- Extend matchKey OpenAPI schema with
args(payload|full) and add tests for invalid values. - Persist
argsin storage (schema migration + CRUD + config retrieval) and wire it throughReservoirService. - Adjust matcher invocation so matchers can receive the full global record object when configured.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/test/java/com/indexdata/reservoir/server/MainVerticleTest.java | Adds validation tests for invalid args values and updates expected matchKey GET response to include args. |
| server/src/main/resources/openapi/schemas/matchKey.json | Adds args enum to the matchKey schema (and modifies matcher description). |
| server/src/main/java/com/indexdata/reservoir/server/Storage.java | Adds DB column + CRUD plumbing for args; changes matcher invocation to optionally pass full global record. |
| server/src/main/java/com/indexdata/reservoir/server/ReservoirService.java | Threads args from request into storage insert/update; refactors async flow to use compose. |
| server/src/main/java/com/indexdata/reservoir/server/OaiPmhClientService.java | Populates global record fields using ClusterBuilder labels and adds sourceId to the global record. |
| server/src/main/java/com/indexdata/reservoir/server/IngestMatcher.java | Adds onlyPayload flag to control matcher argument shape. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
server/src/main/java/com/indexdata/reservoir/server/Storage.java
Outdated
Show resolved
Hide resolved
| List<Future<MatcherResult>> futures = new ArrayList<>(); | ||
| for (IngestMatcher ingestMatcher : ingestMatchers) { | ||
| futures.add(runMatcher(ingestMatcher, ingestMetrics, payload)); | ||
| futures.add(runMatcher(ingestMatcher, ingestMetrics, globalRecord)); | ||
| } |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In args=full mode, the matcher receives the globalRecord object as-is. For ingest paths like file upload (IngestWriteStream) and bulk update (updateGlobalRecords), globalRecord typically contains localId/payload but not sourceId (and not sourceVersion), even though the PR description says the full record passed to matchers includes those fields. Consider enriching the object passed to the matcher (e.g., add sourceId/sourceVersion from the method parameters, or build a dedicated matcher input object) so args=full is consistent across all ingestion paths.
| @Test | ||
| public void testMatchkeyBadArgs() { | ||
| JsonObject matchKey = new JsonObject() | ||
| .put("id", "10a") | ||
| .put("matcher", "matcher-10a") |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new args behavior adds a second execution mode (full vs payload), but the tests added here only cover invalid values / null. Consider adding an integration test that sets args to "full" and asserts the matcher can read top-level fields (like localId/sourceId) and behaves differently than the default payload-only mode.
server/src/main/java/com/indexdata/reservoir/server/Storage.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
The matchkey config has a new property "args" which controls how arguments are passed to the matcher method. If value is "payload" (default behavior), only the payload content is passed. If value is "full" then the global record entry is passed. This has "localId", "sourceId", and "payload" properties at the very least.
https://index-data.atlassian.net/browse/RSRVR-31