[Port to dtq-dev] Issue dspace-customers#903: stop ItemConverter writing to the DB on every item GET

[jr-rk]

Problem

Every REST GET of an item triggered database writes. ItemConverter.convert() — invoked during read-only serialization — called ClarinItemService.updateItemDatesMetadata(), which does clearMetadata/addMetadata. This caused Hibernate dirty-checking and DB writes on read (rolled back by DSpaceRequestContextFilter.abort()), plus per-request log noise, on every item GET.

Port of dataquest-dev/dspace-customers#903 (item 1 of a 6-item umbrella). Source: customer/zcu-data 24f31c330d — cherry-pick conflicted (see below), applied as an adapted port.

Root cause

Date-derivation (turn local.approximateDate.issued into dc.date.issued) is a write that was being run from a read path so that stale DB values would still display correctly. The write belongs in update(); the display correction belongs on the DTO.

Change set

Split derivation (pure) from persistence (write):

• ClarinItemService (interface) — add deriveDateIssuedFromApproximateDate(Item).
• ClarinItemServiceImpl — extract the parsing into deriveDateIssuedFromApproximateDate() (returns the derived year, "0000", or null); updateItemDatesMetadata() delegates to it and writes only when the stored value differs — skipping the write only when dc.date.issued already holds exactly one value equal to the derived one (multi-valued fields are still normalized to the single derived value). Kept dtq-dev's log.debug for the empty-approximate-date case (the WARN→DEBUG change already landed on dtq-dev via 84e9f3a2c2; the source commit still had log.warn, which is why the cherry-pick conflicted here).
• ItemServiceImpl.update() — call clarinItemService.updateItemDatesMetadata() inside the existing isMetadataModified() guard, so the date is derived and persisted only when metadata actually changes (PATCH/PUT).
• ItemConverter.convert() — remove the write-on-read call; instead override dc.date.issued on the ItemRest DTO from the derived value via MetadataRest#put (display-only, honouring metadataExposureService.isHidden). No entity mutation, no DB write.

Review updates (b238a0b397)

Both Copilot review comments addressed and unit coverage added:

• Skip-guard (ClarinItemServiceImpl) — skip the write only for a single already-derived value; a multi-valued dc.date.issued is still collapsed to the single derived value (was skipping on a first-value match).
• DTO write (ItemConverter) — build the derived value via MetadataRest#put (place normalization + Arrays.asList semantics, consistent with other fields) instead of getMap().put(Collections.singletonList(...)); dropped the manual setPlace(0) and the now-unused Collections import.
• Tests — new ClarinItemServiceImplTest (9 pure-Mockito unit tests) covering derivation (empty / blank / non-numeric / sequence / single) and the skip/normalize guard.

Validation

mvn -pl dspace-api           checkstyle:check                            -> 0 violations
mvn -pl dspace-server-webapp checkstyle:check                            -> 0 violations
mvn -pl dspace-server-webapp -am compile                                 -> BUILD SUCCESS
mvn -pl dspace-api -DskipUnitTests=false -Dtest=ClarinItemServiceImplTest test
                                                                         -> Tests run: 9, Failures: 0

The no-write property is covered structurally (the converter no longer calls updateItemDatesMetadata) plus the unit-level skip/normalize guard. Full-context runtime (docker / DSpace test-kernel) is not bootstrappable in this environment. CI exercises the approximate-date display path via ItemRestRepositoryIT — note those ITs assert the displayed value; they are not a dedicated "no DB write on GET" assertion.

Risk & rollback

Touches core CLARIN item serialization and ItemServiceImpl.update(). For the standard read/display path, behaviour is preserved: items with local.approximateDate.issued still show the derived dc.date.issued in REST responses, and the value is now persisted on metadata change (previously the read-path write was always rolled back, so nothing was ever persisted). Regression focus: approximate-date handling. Revert = single commit.

Behaviour notes

• With local.approximateDate.issued present it is authoritative: any metadata-modifying update() (PATCH/PUT) derives and overwrites dc.date.issued, so a client-supplied dc.date.issued is replaced. This matches what the REST API already displayed.
• Non-numeric approximate dates normalize to "0000".
• Existing items are normalized on their next metadata edit only (no backfill migration); non-REST consumers (OAI-PMH, Solr) read the stored value — unchanged from before this PR.

Notes / assumptions

Conflict resolution kept dtq-dev-side changes (the log.debug downgrade). The four-file split matches the source commit's intent; the display-only override reuses the shared derivation rather than duplicating parsing.

[Copilot]

Pull request overview

This PR ports dspace-customers#903 to dtq-dev to eliminate unintended database writes during REST GET /items/{id} serialization by removing entity mutation from the read/DTO conversion path and relocating persistence to the item update path.

Changes:

• Removed the ItemConverter.convert() write-on-read call to updateItemDatesMetadata() and replaced it with a DTO-only override of dc.date.issued derived from local.approximateDate.issued.
• Added a pure derivation method deriveDateIssuedFromApproximateDate(Item) to ClarinItemService / ClarinItemServiceImpl, and updated updateItemDatesMetadata() to only write when the stored value differs.
• Updated ItemServiceImpl.update() to persist the derived date only when item metadata is modified (PATCH/PUT).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
dspace-server-webapp/src/main/java/org/dspace/app/rest/converter/ItemConverter.java	Removes write-on-read and derives dc.date.issued for display on the REST DTO only.
dspace-api/src/main/java/org/dspace/content/service/clarin/ClarinItemService.java	Adds a pure derivation API to support read-only DTO overrides.
dspace-api/src/main/java/org/dspace/content/ItemServiceImpl.java	Persists derived date only during update flows when metadata changes.
dspace-api/src/main/java/org/dspace/content/clarin/ClarinItemServiceImpl.java	Extracts derivation logic and avoids unnecessary metadata writes when already up-to-date.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[jr-rk]

Addressed both Copilot review comments in b238a0b397:

• ClarinItemServiceImpl — skip the dc.date.issued write only when the field holds exactly one already-derived value; multi-valued fields are still normalized to the single derived value.
• ItemConverter — build the derived DTO value via MetadataRest#put (place normalization + consistent Arrays.asList semantics) instead of getMap().put(singletonList(...)).

Added ClarinItemServiceImplTest (9 unit tests) covering deriveDateIssuedFromApproximateDate and the normalize/skip guard. checkstyle:check (both modules) and mvn -pl dspace-server-webapp -am compile are green.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

dspace-server-webapp/src/main/java/org/dspace/app/rest/converter/ItemConverter.java:97

• Avoid the magic number -1 for metadata confidence. DSpace defines Choices.CF_UNSET for the default/unset confidence (and MetadataValue defaults confidence to -1), so using the constant improves readability and reduces the chance of inconsistency if the value ever changes.

        MetadataValueRest dateRest = new MetadataValueRest(derivedValue);
        dateRest.setConfidence(-1);
        // MetadataRest#put normalizes place and keeps Arrays.asList semantics consistent with other fields
        target.getMetadata().put("dc.date.issued", dateRest);