Skip to content

Commit 06e2c24

Browse files
viacheslavkolviacheslavkol
authored
fix(indexing): Update instance_call_number tenantId on Instance becoming shared (#904)
* fix(indexing): Update instance_call_number tenantId on Instance becoming shared - add instance sharing events identification to PopulateInstanceBatchInterceptor when consumed in one batch - Remove redundant sharing logic from extractors - Remove redundant shadow copy logic from InstanceChildrenResourceService - Add call number tenant update to InstanceChildrenResourceService for shared new Instance events Closes: MSEARCH-1168
1 parent 8392e24 commit 06e2c24

File tree

12 files changed

+342
-63
lines changed

12 files changed

+342
-63
lines changed

‎NEWS.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@
7171
* Add error handling on upload range processing ([MSEARCH-1151](https://folio-org.atlassian.net/browse/MSEARCH-1151))
7272
* Ignore shadow locations and location units while indexing domain events ([MSEARCH-1154](https://folio-org.atlassian.net/browse/MSEARCH-1154))
7373
* Add support for exact match on isbn, honor '*' in IsbnSearchTermProcessor ([MSEARCH-1011](https://folio-org.atlassian.net/browse/MSEARCH-1011))
74+
* Update instance_call_number tenantId on Instance becoming shared ([MSEARCH-1168](https://folio-org.atlassian.net/browse/MSEARCH-1168))
7475

7576
### Tech Dept
7677
* Migrate to Opensearch 3.0.0 ([MSEARCH-1033](https://folio-org.atlassian.net/browse/MSEARCH-1033))

‎src/main/java/org/folio/search/integration/message/interceptor/PopulateInstanceBatchInterceptor.java‎

Lines changed: 27 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ public ConsumerRecords<String, ResourceEvent> intercept(ConsumerRecords<String,
6565
if (list.size() > 1) {
6666
list.sort(Comparator.comparingLong(ConsumerRecord::timestamp));
6767
}
68-
if (isUpdateOwnershipEvents(list)) {
68+
if (processAll(list)) {
6969
consumerRecords.addAll(list.stream().map(ConsumerRecord::value).toList());
7070
} else {
7171
consumerRecords.add(list.getLast().value());
@@ -75,6 +75,15 @@ public ConsumerRecords<String, ResourceEvent> intercept(ConsumerRecords<String,
7575
return records;
7676
}
7777

78+
private boolean processAll(List<ConsumerRecord<String, ResourceEvent>> records) {
79+
if (records.size() != 2
80+
|| Objects.equals(records.getFirst().value().getTenant(), records.getLast().value().getTenant())) {
81+
return false;
82+
}
83+
return isUpdateOwnershipEvents(records)
84+
|| isInstanceSharingEvents(records);
85+
}
86+
7887
/**
7988
* Needed in case 2 item events with same id come in 1 batch on update ownership case.
8089
* When mod-inventory-storage send CREATE event for new tenant and DELETE event for old tenant.
@@ -83,10 +92,6 @@ public ConsumerRecords<String, ResourceEvent> intercept(ConsumerRecords<String,
8392
* This method helps identify such case.
8493
*/
8594
private boolean isUpdateOwnershipEvents(List<ConsumerRecord<String, ResourceEvent>> records) {
86-
if (records.size() != 2
87-
|| Objects.equals(records.getFirst().value().getTenant(), records.getLast().value().getTenant())) {
88-
return false;
89-
}
9095
var eventTypes = records.stream()
9196
.map(consumerRecord -> consumerRecord.value().getType())
9297
.toList();
@@ -95,6 +100,23 @@ private boolean isUpdateOwnershipEvents(List<ConsumerRecord<String, ResourceEven
95100
&& eventTypes.contains(ResourceEventType.DELETE);
96101
}
97102

103+
/**
104+
* Needed in case 2 instance events with same id come in 1 batch on instance sharing case.
105+
* When mod-inventory-storage send CREATE event for central tenant and UPDATE event for member tenant.
106+
* UPDATE event in such case could have higher timestamp value and
107+
* caller method (intercept) logic would filter out the CREATE event since both events have same id.
108+
* This method helps identify such case.
109+
* UPDATE event would be filtered out later in process method since source of such event would have consortium prefix.
110+
* */
111+
private boolean isInstanceSharingEvents(List<ConsumerRecord<String, ResourceEvent>> records) {
112+
var eventTypes = records.stream()
113+
.map(consumerRecord -> consumerRecord.value().getType())
114+
.toList();
115+
116+
return eventTypes.contains(ResourceEventType.CREATE)
117+
&& eventTypes.contains(ResourceEventType.UPDATE);
118+
}
119+
98120
private void populate(List<ResourceEvent> records) {
99121
var batchByTenant = records.stream().collect(Collectors.groupingBy(ResourceEvent::getTenant));
100122
batchByTenant.forEach((tenant, batch) -> {

‎src/main/java/org/folio/search/service/InstanceChildrenResourceService.java‎

Lines changed: 42 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,19 @@
11
package org.folio.search.service;
22

3-
import static org.folio.search.utils.SearchConverterUtils.getResourceSource;
4-
import static org.folio.search.utils.SearchUtils.SOURCE_CONSORTIUM_PREFIX;
3+
import static org.folio.search.utils.SearchConverterUtils.getMapValueByPath;
4+
import static org.folio.search.utils.SearchConverterUtils.getNewAsMap;
55

66
import java.util.List;
77
import java.util.Map;
8+
import java.util.Objects;
89
import java.util.stream.Collectors;
910
import lombok.extern.log4j.Log4j2;
10-
import org.apache.commons.lang3.Strings;
1111
import org.folio.search.domain.dto.ResourceEvent;
1212
import org.folio.search.domain.dto.ResourceEventType;
1313
import org.folio.search.model.types.ResourceType;
1414
import org.folio.search.service.consortium.ConsortiumTenantProvider;
1515
import org.folio.search.service.converter.preprocessor.extractor.ChildResourceExtractor;
16-
import org.folio.search.utils.SearchConverterUtils;
16+
import org.folio.search.service.reindex.jdbc.CallNumberRepository;
1717
import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
1818
import org.springframework.stereotype.Component;
1919

@@ -28,12 +28,15 @@ public class InstanceChildrenResourceService {
2828

2929
private final Map<ResourceType, List<ChildResourceExtractor>> resourceExtractors;
3030
private final ConsortiumTenantProvider consortiumTenantProvider;
31+
private final CallNumberRepository callNumberRepository;
3132

3233
public InstanceChildrenResourceService(List<ChildResourceExtractor> resourceExtractors,
33-
ConsortiumTenantProvider consortiumTenantProvider) {
34+
ConsortiumTenantProvider consortiumTenantProvider,
35+
CallNumberRepository callNumberRepository) {
3436
this.resourceExtractors = resourceExtractors.stream()
3537
.collect(Collectors.groupingBy(ChildResourceExtractor::resourceType));
3638
this.consortiumTenantProvider = consortiumTenantProvider;
39+
this.callNumberRepository = callNumberRepository;
3740
}
3841

3942
public void persistChildren(String tenantId, ResourceType resourceType, List<ResourceEvent> events) {
@@ -43,21 +46,41 @@ public void persistChildren(String tenantId, ResourceType resourceType, List<Res
4346
}
4447

4548
var shared = consortiumTenantProvider.isCentralTenant(tenantId);
46-
var noShadowCopiesInstanceEvents = events.stream()
47-
.filter(resourceEvent -> {
48-
if (ResourceType.INSTANCE.getName().equals(resourceEvent.getResourceName())) {
49-
return !Strings.CS.startsWith(getResourceSource(resourceEvent), SOURCE_CONSORTIUM_PREFIX);
50-
}
51-
return true;
52-
})
53-
.toList();
54-
var eventsForResourceSharing = events.stream()
55-
.filter(SearchConverterUtils::isUpdateEventForResourceSharing)
56-
.toList();
57-
extractors.forEach(resourceExtractor ->
58-
resourceExtractor.persistChildren(tenantId, shared, noShadowCopiesInstanceEvents));
49+
50+
// Process child resources normally
5951
extractors.forEach(resourceExtractor ->
60-
resourceExtractor.persistChildrenForResourceSharing(shared, eventsForResourceSharing));
52+
resourceExtractor.persistChildren(tenantId, shared, events));
53+
54+
// When background job processes new instances in central tenant, update call numbers
55+
// that may still be pointing to member tenant. Covers sharing instance case.
56+
if (shared && resourceType == ResourceType.INSTANCE && !events.isEmpty()) {
57+
var instanceIds = events.stream()
58+
.filter(this::isNewInstance)
59+
.map(ResourceEvent::getId)
60+
.toList();
61+
log.info("persistChildren: Updating call number tenant_id for {} instances in central tenant {}",
62+
instanceIds.size(), tenantId);
63+
callNumberRepository.updateTenantIdForCentralInstances(instanceIds, tenantId);
64+
}
65+
}
66+
67+
/**
68+
* Checks if the instance is newly created by comparing metadata dates.
69+
* An instance is considered new if its createdDate equals its updatedDate.
70+
*
71+
* @param event the resource event to check
72+
* @return true if the instance is newly created, false otherwise
73+
*/
74+
private Boolean isNewInstance(ResourceEvent event) {
75+
var instanceData = getNewAsMap(event);
76+
if (instanceData.isEmpty()) {
77+
return false;
78+
}
79+
80+
var createdDate = getMapValueByPath("metadata.createdDate", instanceData);
81+
var updatedDate = getMapValueByPath("metadata.updatedDate", instanceData);
82+
83+
return Objects.equals(createdDate, updatedDate);
6184
}
6285

6386
public void persistChildrenOnReindex(String tenantId, ResourceType resourceType,

‎src/main/java/org/folio/search/service/converter/preprocessor/extractor/ChildResourceExtractor.java‎

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,6 @@
22

33
import static java.util.Collections.emptyList;
44
import static java.util.Collections.emptySet;
5-
import static java.util.stream.Collectors.groupingBy;
6-
import static java.util.stream.Collectors.mapping;
7-
import static java.util.stream.Collectors.toList;
85
import static org.apache.commons.collections4.MapUtils.getObject;
96
import static org.folio.search.utils.SearchConverterUtils.getNewAsMap;
107

@@ -51,18 +48,6 @@ public void persistChildren(String tenantId, boolean shared, List<ResourceEvent>
5148
repository.saveAll(new ChildResourceEntityBatch(new ArrayList<>(entities), relations));
5249
}
5350

54-
public void persistChildrenForResourceSharing(boolean shared, List<ResourceEvent> events) {
55-
var eventsForSharingByTenant = events.stream()
56-
.collect(groupingBy(ResourceEvent::getTenant, mapping(ResourceEvent::getId, toList())));
57-
eventsForSharingByTenant.forEach((tenant, instanceIds) -> {
58-
if (shared) {
59-
log.warn("Update event for instance sharing is supposed to be for member tenant,"
60-
+ " but received for central tenant: {}, eventId: {}", tenant, String.join(",", instanceIds));
61-
}
62-
repository.deleteByInstanceIds(instanceIds, tenant);
63-
});
64-
}
65-
6651
protected abstract List<Map<String, Object>> constructRelations(boolean shared, ResourceEvent event,
6752
List<Map<String, Object>> entities);
6853

‎src/main/java/org/folio/search/service/reindex/jdbc/CallNumberRepository.java‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,19 @@ WHERE item_id IN (%2$s) %3$s
7575
WHERE id IN (SELECT * FROM deleted_ids);
7676
""";
7777

78+
private static final String UPDATE_TENANT_FOR_CENTRAL_QUERY = """
79+
WITH updated_call_numbers AS (
80+
UPDATE %1$s.instance_call_number
81+
SET tenant_id = ?
82+
WHERE instance_id IN (%2$s)
83+
AND tenant_id != ?
84+
RETURNING call_number_id
85+
)
86+
UPDATE %1$s.call_number
87+
SET last_updated_date = CURRENT_TIMESTAMP
88+
WHERE id IN (SELECT call_number_id FROM updated_call_numbers);
89+
""";
90+
7891
private static final String SELECT_BY_UPDATED_QUERY = """
7992
WITH cte AS (
8093
SELECT
@@ -165,6 +178,7 @@ protected CallNumberRepository(JdbcTemplate jdbcTemplate, JsonConverter jsonConv
165178
}
166179

167180
@Override
181+
@SuppressWarnings("java:S2077")
168182
public void deleteByInstanceIds(List<String> itemIds, String tenantId) {
169183
var sql = DELETE_QUERY.formatted(JdbcUtils.getSchemaName(context), getParamPlaceholderForUuid(itemIds.size()),
170184
tenantId == null ? "" : "AND tenant_id = ?");
@@ -174,6 +188,25 @@ public void deleteByInstanceIds(List<String> itemIds, String tenantId) {
174188
jdbcTemplate.update(sql, params);
175189
}
176190

191+
/**
192+
* Updates tenant_id in instance_call_number records for the given instances to the central tenant.
193+
* Used when an instance is shared to the central tenant - the background job processes the newly created
194+
* central instance and this method updates any existing call number relations that still point to the member tenant.
195+
* For the updated relations - also updates last_updated_date in call_number table
196+
* to trigger reindexing of those call numbers.
197+
*
198+
* @param instanceIds list of instance IDs whose call number relations should be updated
199+
* @param centralTenantId the central tenant ID to set
200+
*/
201+
public void updateTenantIdForCentralInstances(List<String> instanceIds, String centralTenantId) {
202+
var sql = UPDATE_TENANT_FOR_CENTRAL_QUERY.formatted(JdbcUtils.getSchemaName(context),
203+
getParamPlaceholderForUuid(instanceIds.size()));
204+
var params = Stream.of(List.of(centralTenantId), instanceIds, List.of(centralTenantId))
205+
.flatMap(List::stream)
206+
.toArray();
207+
jdbcTemplate.update(sql, params);
208+
}
209+
177210
@Override
178211
public void saveAll(ChildResourceEntityBatch entityBatch) {
179212
saveResourceEntities(entityBatch);

‎src/main/java/org/folio/search/utils/SearchConverterUtils.java‎

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,7 @@
33
import static java.util.Collections.emptyMap;
44
import static org.apache.commons.collections4.MapUtils.getString;
55
import static org.apache.commons.lang3.BooleanUtils.isTrue;
6-
import static org.folio.search.domain.dto.ResourceEventType.UPDATE;
76
import static org.folio.search.utils.SearchUtils.ID_FIELD;
8-
import static org.folio.search.utils.SearchUtils.SOURCE_CONSORTIUM_PREFIX;
97
import static org.folio.search.utils.SearchUtils.SOURCE_FIELD;
108

119
import java.util.Arrays;
@@ -17,7 +15,6 @@
1715
import lombok.NoArgsConstructor;
1816
import org.apache.commons.collections4.CollectionUtils;
1917
import org.apache.commons.collections4.MapUtils;
20-
import org.apache.commons.lang3.Strings;
2118
import org.folio.search.domain.dto.ResourceEvent;
2219
import org.springframework.stereotype.Component;
2320

@@ -186,14 +183,6 @@ public static void copyEntityFields(Map<String, Object> source, Map<String, Obje
186183
}
187184
}
188185

189-
public static boolean isUpdateEventForResourceSharing(ResourceEvent event) {
190-
var newSource = getResourceSource(getNewAsMap(event));
191-
return event.getType() == UPDATE
192-
&& Strings.CS.startsWith(newSource, SOURCE_CONSORTIUM_PREFIX)
193-
&& Objects.equals(getResourceSource(getOldAsMap(event)),
194-
Strings.CS.removeStart(newSource, SOURCE_CONSORTIUM_PREFIX));
195-
}
196-
197186
/**
198187
* Checks if the given {@link ResourceEvent} represents a shadow location or unit.
199188
*

‎src/test/java/org/folio/indexing/IndexingInstanceCallNumberConsortiumIT.java‎

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
package org.folio.indexing;
2+
3+
import static java.util.Collections.emptyList;
4+
import static org.assertj.core.api.AssertionsForInterfaceTypes.assertThat;
5+
import static org.folio.search.model.types.ResourceType.INSTANCE_CALL_NUMBER;
6+
import static org.folio.search.service.reindex.ReindexConstants.CALL_NUMBER_TABLE;
7+
import static org.folio.search.service.reindex.ReindexConstants.HOLDING_TABLE;
8+
import static org.folio.search.service.reindex.ReindexConstants.INSTANCE_CALL_NUMBER_TABLE;
9+
import static org.folio.search.service.reindex.ReindexConstants.INSTANCE_TABLE;
10+
import static org.folio.search.service.reindex.ReindexConstants.ITEM_TABLE;
11+
import static org.folio.support.TestConstants.CENTRAL_TENANT_ID;
12+
import static org.folio.support.TestConstants.MEMBER_TENANT_ID;
13+
import static org.folio.support.base.ApiEndpoints.instanceSearchPath;
14+
import static org.folio.support.utils.TestUtils.randomId;
15+
16+
import java.util.List;
17+
import java.util.Map;
18+
import org.folio.search.domain.dto.Holding;
19+
import org.folio.search.domain.dto.Instance;
20+
import org.folio.search.domain.dto.Item;
21+
import org.folio.search.domain.dto.ItemEffectiveCallNumberComponents;
22+
import org.folio.search.domain.dto.TenantConfiguredFeature;
23+
import org.folio.spring.testing.extension.DatabaseCleanup;
24+
import org.folio.spring.testing.type.IntegrationTest;
25+
import org.folio.support.base.BaseIntegrationTest;
26+
import org.junit.jupiter.api.AfterAll;
27+
import org.junit.jupiter.api.AfterEach;
28+
import org.junit.jupiter.api.BeforeAll;
29+
import org.junit.jupiter.api.Test;
30+
import org.springframework.test.context.TestPropertySource;
31+
32+
@IntegrationTest
33+
@TestPropertySource(properties = "folio.search-config.indexing.instance-children-index-enabled=true")
34+
@DatabaseCleanup(tenants = CENTRAL_TENANT_ID,
35+
tables = {CALL_NUMBER_TABLE, INSTANCE_CALL_NUMBER_TABLE, ITEM_TABLE, HOLDING_TABLE, INSTANCE_TABLE})
36+
class IndexingInstanceCallNumberConsortiumIT extends BaseIntegrationTest {
37+
38+
private static final String INSTANCE_ID = randomId();
39+
private static final String INSTANCE_TITLE = "title";
40+
private static final String CALL_NUMBER = "test";
41+
42+
@BeforeAll
43+
static void prepare() {
44+
setUpTenant(CENTRAL_TENANT_ID);
45+
setUpTenant(MEMBER_TENANT_ID);
46+
47+
enableFeature(CENTRAL_TENANT_ID, TenantConfiguredFeature.BROWSE_CALL_NUMBERS);
48+
49+
setUpTestData();
50+
51+
// fetch call number documents for the instance and check that tenant field contains member tenant id
52+
awaitAssertion(() -> assertInstanceCallNumberTenantId(MEMBER_TENANT_ID, false));
53+
}
54+
55+
@AfterAll
56+
static void cleanUp() {
57+
removeTenant(MEMBER_TENANT_ID);
58+
removeTenant(CENTRAL_TENANT_ID);
59+
}
60+
61+
@AfterEach
62+
void tearDown() {
63+
deleteAllDocuments(INSTANCE_CALL_NUMBER, CENTRAL_TENANT_ID);
64+
}
65+
66+
@Test
67+
void shouldUpdateInstanceCallNumber_onInstanceSharing() {
68+
// when - create instance in central tenant with the same instance id/title
69+
var centralInstance = new Instance().id(INSTANCE_ID).title(INSTANCE_TITLE).source("FOLIO");
70+
inventoryApi.createInstance(CENTRAL_TENANT_ID, centralInstance);
71+
72+
// and - update member tenant instance to change source to have consortium prefix
73+
var memberInstance = new Instance().id(INSTANCE_ID).title(INSTANCE_TITLE).source("CONSORTIUM-FOLIO");
74+
inventoryApi.updateInstance(MEMBER_TENANT_ID, memberInstance);
75+
76+
// then - fetch call number documents for the instance and check if tenant field changed to central
77+
awaitAssertion(() -> assertInstanceCallNumberTenantId(CENTRAL_TENANT_ID, true));
78+
}
79+
80+
private static void assertInstanceCallNumberTenantId(String expectedTenantId, boolean shared) {
81+
var hits = fetchAllDocuments(INSTANCE_CALL_NUMBER, CENTRAL_TENANT_ID);
82+
assertThat(hits).hasSize(1);
83+
84+
var sourceAsMap = hits[0].getSourceAsMap();
85+
@SuppressWarnings("unchecked")
86+
var instances = (List<Map<String, Object>>) sourceAsMap.get("instances");
87+
assertThat(instances)
88+
.hasSize(1)
89+
.allSatisfy(map -> assertThat(map)
90+
.containsEntry("tenantId", expectedTenantId)
91+
.containsEntry("shared", shared));
92+
}
93+
94+
private static void setUpTestData() {
95+
var holdings = new Holding().id(randomId());
96+
var item = new Item().id(randomId()).holdingsRecordId(holdings.getId())
97+
.effectiveCallNumberComponents(new ItemEffectiveCallNumberComponents().callNumber(CALL_NUMBER));
98+
var instance = new Instance().id(INSTANCE_ID).title(INSTANCE_TITLE).source("FOLIO")
99+
.holdings(List.of(holdings))
100+
.items(List.of(item));
101+
saveRecords(MEMBER_TENANT_ID, instanceSearchPath(), List.of(instance), 1, emptyList(),
102+
i -> inventoryApi.createInstance(MEMBER_TENANT_ID, i));
103+
}
104+
}

0 commit comments

Comments
 (0)