From f85788a8411076300875c55699b9c98ae89e5e3a Mon Sep 17 00:00:00 2001 From: jr-rk <95219754+jr-rk@users.noreply.github.com> Date: Tue, 11 Aug 2026 16:19:05 +0200 Subject: [PATCH 1/2] fix(rest): stop ItemConverter writing to the DB on every item GET ItemConverter.convert() -- invoked during read-only REST serialization -- called ClarinItemService.updateItemDatesMetadata(), which runs clearMetadata/addMetadata. That triggered Hibernate dirty-checking and DB writes on every item GET (rolled back by DSpaceRequestContextFilter), plus per-request log noise. Split derivation from persistence: - ClarinItemService/Impl: extract deriveDateIssuedFromApproximateDate(Item) (pure, returns the derived value or null); updateItemDatesMetadata() delegates to it and only writes when the value actually changes. dtq-dev's log.debug for the empty approximate-date case is kept (the WARN->DEBUG change already landed via 84e9f3a2c2), which is why the source cherry-pick conflicted here. - ItemServiceImpl.update(): derive + persist inside the existing isMetadataModified guard, so the write happens on PATCH/PUT, not on read. - ItemConverter.convert(): drop the write-on-read call; instead override dc.date.issued on the REST DTO from the derived value (display-only, honouring metadataExposureService.isHidden) so stale DB values still display correctly. Port of dataquest-dev/dspace-customers#903 (item 1). Source: customer/zcu-data 24f31c330d (adapted; cherry-pick conflicted on the WARN->DEBUG divergence). Co-Authored-By: Claude Fable 5 --- .../org/dspace/content/ItemServiceImpl.java | 9 ++++ .../content/clarin/ClarinItemServiceImpl.java | 44 +++++++++++------- .../service/clarin/ClarinItemService.java | 12 +++++ .../app/rest/converter/ItemConverter.java | 46 +++++++++++++------ 4 files changed, 81 insertions(+), 30 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/content/ItemServiceImpl.java b/dspace-api/src/main/java/org/dspace/content/ItemServiceImpl.java index 9c853613a94..1925b1d48df 100644 --- a/dspace-api/src/main/java/org/dspace/content/ItemServiceImpl.java +++ b/dspace-api/src/main/java/org/dspace/content/ItemServiceImpl.java @@ -50,6 +50,7 @@ import org.dspace.content.service.MetadataSchemaService; import org.dspace.content.service.RelationshipService; import org.dspace.content.service.WorkspaceItemService; +import org.dspace.content.service.clarin.ClarinItemService; import org.dspace.content.virtual.VirtualMetadataPopulator; import org.dspace.core.Constants; import org.dspace.core.Context; @@ -180,6 +181,9 @@ public class ItemServiceImpl extends DSpaceObjectServiceImpl implements It @Autowired private VersionHistoryService versionHistoryService; + @Autowired(required = true) + private ClarinItemService clarinItemService; + @Autowired(required = true) ClarinMatomoBitstreamTracker matomoBitstreamTracker; @@ -686,6 +690,11 @@ public void update(Context context, Item item) throws SQLException, AuthorizeExc } if (item.isMetadataModified() || item.isModified()) { + // Derive dc.date.issued from local.approximateDate.issued when metadata changes + if (item.isMetadataModified()) { + clarinItemService.updateItemDatesMetadata(context, item); + } + // Set the last modified date item.setLastModified(new Date()); diff --git a/dspace-api/src/main/java/org/dspace/content/clarin/ClarinItemServiceImpl.java b/dspace-api/src/main/java/org/dspace/content/clarin/ClarinItemServiceImpl.java index 018964b4cbf..1689cfadea1 100644 --- a/dspace-api/src/main/java/org/dspace/content/clarin/ClarinItemServiceImpl.java +++ b/dspace-api/src/main/java/org/dspace/content/clarin/ClarinItemServiceImpl.java @@ -223,12 +223,32 @@ public void updateItemDatesMetadata(Context context, Item item) throws SQLExcept return; } + String derivedDate = deriveDateIssuedFromApproximateDate(item); + if (derivedDate == null) { + log.debug("Cannot update item dates metadata because the approximate date is empty."); + return; + } + + // Skip the write when dc.date.issued already holds the derived value + List currentDateIssued = + itemService.getMetadata(item, "dc", "date", "issued", Item.ANY, false); + if (CollectionUtils.isNotEmpty(currentDateIssued) + && derivedDate.equals(currentDateIssued.get(0).getValue())) { + return; + } + + // Clear the current `dc.date.issued` metadata and set it to the derived value + itemService.clearMetadata(context, item, "dc", "date", "issued", Item.ANY); + itemService.addMetadata(context, item, "dc", "date", "issued", Item.ANY, derivedDate); + } + + @Override + public String deriveDateIssuedFromApproximateDate(Item item) { List approximatedDates = itemService.getMetadata(item, "local", "approximateDate", "issued", Item.ANY, false); if (CollectionUtils.isEmpty(approximatedDates) || StringUtils.isBlank(approximatedDates.get(0).getValue())) { - log.debug("Cannot update item dates metadata because the approximate date is empty."); - return; + return null; } // Get the approximate date value from the metadata @@ -239,21 +259,11 @@ public void updateItemDatesMetadata(Context context, Item item) throws SQLExcept // Trim the list of years - remove leading and trailing whitespaces listOfYearValues.replaceAll(String::trim); - try { - // Clear the current `dc.date.issued` metadata - itemService.clearMetadata(context, item, "dc", "date", "issued", Item.ANY); - - // Update the `dc.date.issued` metadata with a new value: `0000` or the last year from the sequence - if (CollectionUtils.isNotEmpty(listOfYearValues) && isListOfNumbers(listOfYearValues)) { - // Take the last year from the list of years and add it to the `dc.date.issued` metadata - itemService.addMetadata(context, item, "dc", "date", "issued", Item.ANY, - getLastNumber(listOfYearValues)); - } else { - // Add the `0000` value to the `dc.date.issued` metadata - itemService.addMetadata(context, item, "dc", "date", "issued", Item.ANY, NO_YEAR); - } - } catch (SQLException e) { - log.error("Cannot remove `dc.date.issued` metadata because: {}", e.getMessage()); + // `0000` when the approximate date is not a list of numbers, otherwise the last year in the sequence + if (CollectionUtils.isNotEmpty(listOfYearValues) && isListOfNumbers(listOfYearValues)) { + return getLastNumber(listOfYearValues); + } else { + return NO_YEAR; } } diff --git a/dspace-api/src/main/java/org/dspace/content/service/clarin/ClarinItemService.java b/dspace-api/src/main/java/org/dspace/content/service/clarin/ClarinItemService.java index 0559b10e137..6009cbf94cc 100644 --- a/dspace-api/src/main/java/org/dspace/content/service/clarin/ClarinItemService.java +++ b/dspace-api/src/main/java/org/dspace/content/service/clarin/ClarinItemService.java @@ -102,4 +102,16 @@ public interface ClarinItemService { */ void updateItemDatesMetadata(Context context, Item item) throws SQLException; + /** + * Derive the display value for {@code dc.date.issued} from the item's + * {@code local.approximateDate.issued} metadata, without touching the database. + * Returns the last year for a numeric sequence (e.g. "1938, 1945" -> "1945"), + * {@code "0000"} for a non-numeric approximate value, or {@code null} when no + * approximate date is present. + * + * @param item the item to derive the date from + * @return the derived {@code dc.date.issued} value, or {@code null} if none applies + */ + String deriveDateIssuedFromApproximateDate(Item item); + } diff --git a/dspace-server-webapp/src/main/java/org/dspace/app/rest/converter/ItemConverter.java b/dspace-server-webapp/src/main/java/org/dspace/app/rest/converter/ItemConverter.java index d4513db54ca..23f7d2190f2 100644 --- a/dspace-server-webapp/src/main/java/org/dspace/app/rest/converter/ItemConverter.java +++ b/dspace-server-webapp/src/main/java/org/dspace/app/rest/converter/ItemConverter.java @@ -9,6 +9,7 @@ import java.sql.SQLException; import java.util.ArrayList; +import java.util.Collections; import java.util.LinkedList; import java.util.List; import java.util.Objects; @@ -18,6 +19,7 @@ import org.apache.logging.log4j.Logger; import org.dspace.app.rest.model.ItemRest; import org.dspace.app.rest.model.MetadataValueList; +import org.dspace.app.rest.model.MetadataValueRest; import org.dspace.app.rest.projection.Projection; import org.dspace.app.rest.utils.ContextUtil; import org.dspace.content.Item; @@ -27,7 +29,6 @@ import org.dspace.content.service.clarin.ClarinItemService; import org.dspace.core.Context; import org.dspace.discovery.IndexableObject; -import org.dspace.services.model.Request; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Component; import org.springframework.util.ObjectUtils; @@ -53,18 +54,6 @@ public class ItemConverter @Override public ItemRest convert(Item obj, Projection projection) { - Context context = null; - Request currentRequest = requestService.getCurrentRequest(); - if (currentRequest != null) { - context = ContextUtil.obtainContext(currentRequest.getHttpServletRequest()); - } - try { - clarinItemService.updateItemDatesMetadata(context, obj); - } catch (SQLException e) { - log.error("Error updating item dates metadata", e); - throw new RuntimeException(e); - } - ItemRest item = super.convert(obj, projection); item.setInArchive(obj.isArchived()); item.setDiscoverable(obj.isDiscoverable()); @@ -77,9 +66,40 @@ public ItemRest convert(Item obj, Projection projection) { item.setEntityType(entityTypes.get(0).getValue()); } + // Override dc.date.issued on the REST DTO with the value derived from + // local.approximateDate.issued. Display-only: it does not modify the entity or the database. + overrideDateIssuedFromApproximateDate(obj, item); + return item; } + /** + * If the item has a {@code local.approximateDate.issued} value, override {@code dc.date.issued} + * on the REST DTO using {@link ClarinItemService#deriveDateIssuedFromApproximateDate(Item)}. + * Display-only (no database writes); skipped when {@code dc.date.issued} is hidden. + */ + private void overrideDateIssuedFromApproximateDate(Item source, ItemRest target) { + String derivedValue = clarinItemService.deriveDateIssuedFromApproximateDate(source); + if (derivedValue == null) { + return; + } + + Context context = ContextUtil.obtainCurrentRequestContext(); + try { + if (metadataExposureService.isHidden(context, "dc", "date", "issued", source)) { + return; + } + } catch (SQLException e) { + log.error("Error checking metadata visibility for dc.date.issued", e); + return; + } + + MetadataValueRest dateRest = new MetadataValueRest(derivedValue); + dateRest.setConfidence(-1); + dateRest.setPlace(0); + target.getMetadata().getMap().put("dc.date.issued", Collections.singletonList(dateRest)); + } + /** * Retrieves the metadata list filtered according to the hidden metadata configuration * When the context is null, it will return the metadatalist as for an anonymous user From b238a0b3978b53810319395601426e6785349c9a Mon Sep 17 00:00:00 2001 From: jr-rk <95219754+jr-rk@users.noreply.github.com> Date: Wed, 12 Aug 2026 16:31:01 +0200 Subject: [PATCH 2/2] fix(rest): address PR #1411 review on item-converter-no-db-writes - ClarinItemServiceImpl.updateItemDatesMetadata(): skip the dc.date.issued write only when the field holds exactly one value equal to the derived value. A multi-valued field is now still normalized to the single derived value instead of being skipped on a first-value match (Copilot review #2). - ItemConverter: set the derived dc.date.issued DTO value via MetadataRest#put instead of getMap().put(Collections.singletonList(...)), so place is normalized and list semantics stay consistent with other fields; drops the now-unused Collections import and manual setPlace(0) (Copilot review #1). - Add ClarinItemServiceImplTest: pure Mockito unit tests covering deriveDateIssuedFromApproximateDate (empty/blank/non-numeric/sequence) and the skip/normalize guard (single-equal skip, multi-value normalize, differ). Co-Authored-By: Claude Opus 4.8 --- .../content/clarin/ClarinItemServiceImpl.java | 5 +- .../clarin/ClarinItemServiceImplTest.java | 146 ++++++++++++++++++ .../app/rest/converter/ItemConverter.java | 5 +- 3 files changed, 151 insertions(+), 5 deletions(-) create mode 100644 dspace-api/src/test/java/org/dspace/content/clarin/ClarinItemServiceImplTest.java diff --git a/dspace-api/src/main/java/org/dspace/content/clarin/ClarinItemServiceImpl.java b/dspace-api/src/main/java/org/dspace/content/clarin/ClarinItemServiceImpl.java index 1689cfadea1..faeb0dacce8 100644 --- a/dspace-api/src/main/java/org/dspace/content/clarin/ClarinItemServiceImpl.java +++ b/dspace-api/src/main/java/org/dspace/content/clarin/ClarinItemServiceImpl.java @@ -229,10 +229,11 @@ public void updateItemDatesMetadata(Context context, Item item) throws SQLExcept return; } - // Skip the write when dc.date.issued already holds the derived value + // Skip the write only when dc.date.issued already holds exactly the single derived value. + // A multi-valued field must still be normalized down to the single derived value. List currentDateIssued = itemService.getMetadata(item, "dc", "date", "issued", Item.ANY, false); - if (CollectionUtils.isNotEmpty(currentDateIssued) + if (currentDateIssued.size() == 1 && derivedDate.equals(currentDateIssued.get(0).getValue())) { return; } diff --git a/dspace-api/src/test/java/org/dspace/content/clarin/ClarinItemServiceImplTest.java b/dspace-api/src/test/java/org/dspace/content/clarin/ClarinItemServiceImplTest.java new file mode 100644 index 00000000000..52f065c922e --- /dev/null +++ b/dspace-api/src/test/java/org/dspace/content/clarin/ClarinItemServiceImplTest.java @@ -0,0 +1,146 @@ +/** + * The contents of this file are subject to the license and copyright + * detailed in the LICENSE and NOTICE files at the root of the source + * tree and available online at + * + * http://www.dspace.org/license/ + */ +package org.dspace.content.clarin; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.sql.SQLException; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; + +import org.dspace.content.Item; +import org.dspace.content.MetadataValue; +import org.dspace.content.service.ItemService; +import org.dspace.core.Context; +import org.junit.Before; +import org.junit.Test; +import org.springframework.test.util.ReflectionTestUtils; + +/** + * Pure unit tests for {@link ClarinItemServiceImpl}: the {@code dc.date.issued} derivation + * from {@code local.approximateDate.issued}, and the normalization guard in + * {@code updateItemDatesMetadata}. Fully mocked — no DSpace kernel or database. + * + * @author dataquest + */ +public class ClarinItemServiceImplTest { + + private ItemService itemService; + private ClarinItemServiceImpl clarinItemService; + private Item item; + private Context context; + + @Before + public void setUp() { + itemService = mock(ItemService.class); + item = mock(Item.class); + context = mock(Context.class); + clarinItemService = new ClarinItemServiceImpl(); + ReflectionTestUtils.setField(clarinItemService, "itemService", itemService); + } + + private MetadataValue mv(String value) { + MetadataValue metadataValue = mock(MetadataValue.class); + when(metadataValue.getValue()).thenReturn(value); + return metadataValue; + } + + private void mockApproximateDate(String value) { + List values = value == null ? Collections.emptyList() : Collections.singletonList(mv(value)); + when(itemService.getMetadata(item, "local", "approximateDate", "issued", Item.ANY, false)) + .thenReturn(values); + } + + private void mockCurrentDateIssued(List values) { + when(itemService.getMetadata(item, "dc", "date", "issued", Item.ANY, false)).thenReturn(values); + } + + // ---- deriveDateIssuedFromApproximateDate (pure, no DB) ---- + + @Test + public void derive_returnsNull_whenNoApproximateDate() { + mockApproximateDate(null); + assertNull(clarinItemService.deriveDateIssuedFromApproximateDate(item)); + } + + @Test + public void derive_returnsNull_whenApproximateDateBlank() { + mockApproximateDate(" "); + assertNull(clarinItemService.deriveDateIssuedFromApproximateDate(item)); + } + + @Test + public void derive_returnsNoYear_whenNonNumeric() { + mockApproximateDate("spring 1945"); + assertEquals("0000", clarinItemService.deriveDateIssuedFromApproximateDate(item)); + } + + @Test + public void derive_returnsLastYear_whenNumericSequence() { + mockApproximateDate("1938, 1945, 2022"); + assertEquals("2022", clarinItemService.deriveDateIssuedFromApproximateDate(item)); + } + + @Test + public void derive_returnsSingleYear() { + mockApproximateDate("1990"); + assertEquals("1990", clarinItemService.deriveDateIssuedFromApproximateDate(item)); + } + + // ---- updateItemDatesMetadata: skip-write / normalization guard ---- + + @Test + public void update_skipsWrite_whenSingleValueAlreadyDerived() throws SQLException { + mockApproximateDate("2022"); + mockCurrentDateIssued(Collections.singletonList(mv("2022"))); + + clarinItemService.updateItemDatesMetadata(context, item); + + verify(itemService, never()).clearMetadata(context, item, "dc", "date", "issued", Item.ANY); + verify(itemService, never()).addMetadata(context, item, "dc", "date", "issued", Item.ANY, "2022"); + } + + @Test + public void update_normalizesMultiValue_evenWhenFirstMatchesDerived() throws SQLException { + // Regression guard: a multi-valued dc.date.issued must still be collapsed to the single derived value, + // even if the first stored value already equals the derived one. + mockApproximateDate("2022"); + mockCurrentDateIssued(Arrays.asList(mv("2022"), mv("1999"))); + + clarinItemService.updateItemDatesMetadata(context, item); + + verify(itemService).clearMetadata(context, item, "dc", "date", "issued", Item.ANY); + verify(itemService).addMetadata(context, item, "dc", "date", "issued", Item.ANY, "2022"); + } + + @Test + public void update_writes_whenSingleValueDiffers() throws SQLException { + mockApproximateDate("2022"); + mockCurrentDateIssued(Collections.singletonList(mv("1900"))); + + clarinItemService.updateItemDatesMetadata(context, item); + + verify(itemService).clearMetadata(context, item, "dc", "date", "issued", Item.ANY); + verify(itemService).addMetadata(context, item, "dc", "date", "issued", Item.ANY, "2022"); + } + + @Test + public void update_skips_whenApproximateDateEmpty() throws SQLException { + mockApproximateDate(null); + + clarinItemService.updateItemDatesMetadata(context, item); + + verify(itemService, never()).clearMetadata(context, item, "dc", "date", "issued", Item.ANY); + } +} diff --git a/dspace-server-webapp/src/main/java/org/dspace/app/rest/converter/ItemConverter.java b/dspace-server-webapp/src/main/java/org/dspace/app/rest/converter/ItemConverter.java index 23f7d2190f2..e6059db9f6f 100644 --- a/dspace-server-webapp/src/main/java/org/dspace/app/rest/converter/ItemConverter.java +++ b/dspace-server-webapp/src/main/java/org/dspace/app/rest/converter/ItemConverter.java @@ -9,7 +9,6 @@ import java.sql.SQLException; import java.util.ArrayList; -import java.util.Collections; import java.util.LinkedList; import java.util.List; import java.util.Objects; @@ -96,8 +95,8 @@ private void overrideDateIssuedFromApproximateDate(Item source, ItemRest target) MetadataValueRest dateRest = new MetadataValueRest(derivedValue); dateRest.setConfidence(-1); - dateRest.setPlace(0); - target.getMetadata().getMap().put("dc.date.issued", Collections.singletonList(dateRest)); + // MetadataRest#put normalizes place and keeps Arrays.asList semantics consistent with other fields + target.getMetadata().put("dc.date.issued", dateRest); } /**