MODLD-599: fix review remarks

folio-org · Nov 25, 2024 · b5d336b · b5d336b
1 parent 664efc3
commit b5d336b
Show file tree

Hide file tree

Showing 11 changed files with 112 additions and 94 deletions.
diff --git a/.run/LinkedDataApplication [Local-Standalone].run.xml b/.run/LinkedDataApplication [Local-Standalone].run.xml
@@ -13,4 +13,4 @@
       <option name="Make" enabled="true" />
     </method>
   </configuration>
-</component>
+</component>
diff --git a/README.md b/README.md
@@ -61,6 +61,7 @@ To run mod-linked-data in standalone mode, set the value of the environment vari
 | KAFKA_WORK_SEARCH_INDEX_TOPIC_PARTITIONS`*`         | 1                               | Custom Work Search Index topic partitions number                                                                                                                                      |
 | KAFKA_WORK_SEARCH_INDEX_TOPIC_REPLICATION_FACTOR`*` | -                               | Custom Work Search Index topic replication factor                                                                                                                                     |
 | KAFKA_INVENTORY_INSTANCE_INGRESS_EVENT_TOPIC`*`     | inventory.instance_ingress      | Custom Inventory Instance Ingress Event topic name                                                                                                                                    |
+| CACHE_TTL_SPEC_RULES_MS                             | 18000000                        | Specifies time to live for `spec-rules` cache                                                                                                                                           |
 * Applicable only in FOLIO mode
 ## REST API
 Full list of APIs are documented in [src/main/resources/swagger.api/mod-linked-data.yaml](https://github.com/folio-org/mod-linked-data/blob/master/src/main/resources/swagger.api/mod-linked-data.yaml).

diff --git a/src/main/java/org/folio/linked/data/client/SpecClient.java b/src/main/java/org/folio/linked/data/client/SpecClient.java
@@ -1,8 +1,11 @@
 package org.folio.linked.data.client;
 
+import static org.folio.linked.data.util.Constants.Cache.SPEC_RULES;
+
 import java.util.UUID;
 import org.folio.rspec.domain.dto.SpecificationDtoCollection;
 import org.folio.rspec.domain.dto.SpecificationRuleDtoCollection;
+import org.springframework.cache.annotation.Cacheable;
 import org.springframework.cloud.openfeign.FeignClient;
 import org.springframework.http.ResponseEntity;
 import org.springframework.web.bind.annotation.GetMapping;
@@ -11,9 +14,11 @@
 @FeignClient(name = "specification-storage")
 public interface SpecClient {
 
+  @Cacheable(cacheNames = "bib-marc-specs")
   @GetMapping(value = "/specifications?profile=bibliographic&family=MARC")
   ResponseEntity<SpecificationDtoCollection> getBibMarcSpecs();
 
+  @Cacheable(cacheNames = SPEC_RULES, key = "#specId")
   @GetMapping(value = "/specifications/{specId}/rules")
   ResponseEntity<SpecificationRuleDtoCollection> getSpecRules(@PathVariable("specId") UUID specId);
 }
diff --git a/src/main/java/org/folio/linked/data/configuration/CacheConfiguration.java b/src/main/java/org/folio/linked/data/configuration/CacheConfiguration.java
diff --git a/src/main/java/org/folio/linked/data/job/CacheCleaningJob.java b/src/main/java/org/folio/linked/data/job/CacheCleaningJob.java
@@ -0,0 +1,19 @@
+package org.folio.linked.data.job;
+
+import static org.folio.linked.data.util.Constants.Cache.SPEC_RULES;
+
+import lombok.extern.slf4j.Slf4j;
+import org.springframework.cache.annotation.CacheEvict;
+import org.springframework.scheduling.annotation.Scheduled;
+import org.springframework.stereotype.Component;
+
+@Component
+@Slf4j
+public class CacheCleaningJob {
+
+  @CacheEvict(value = SPEC_RULES, allEntries = true)
+  @Scheduled(fixedRateString = "${mod-linked-data.cache.ttl.spec-rules}")
+  public void emptySpecRules() {
+    log.info("Emptying {} cache", SPEC_RULES);
+  }
+}
diff --git a/src/main/java/org/folio/linked/data/util/Constants.java b/src/main/java/org/folio/linked/data/util/Constants.java
@@ -21,4 +21,8 @@ public class Constants {
   public static final String MSG_NOT_FOUND_IN = "{} with {}: {} was not found in {}";
   public static final String LINKED_DATA_STORAGE = "Linked Data storage";
 
+  @UtilityClass
+  public static class Cache {
+    public static final String SPEC_RULES = "spec-rules";
+  }
 }
diff --git a/src/main/java/org/folio/linked/data/validation/dto/LccnPatternValidator.java b/src/main/java/org/folio/linked/data/validation/dto/LccnPatternValidator.java
@@ -1,6 +1,5 @@
 package org.folio.linked.data.validation.dto;
 
-import static java.lang.Boolean.TRUE;
 import static org.apache.commons.collections4.CollectionUtils.isEmpty;
 
 import jakarta.validation.ConstraintValidator;
@@ -15,21 +14,16 @@
 @RequiredArgsConstructor
 public class LccnPatternValidator implements ConstraintValidator<LccnPatternConstraint, LccnRequest> {
 
+  public static final String CODE = "invalidLccnSubfieldValue";
+
   private static final Pattern LCCN_STRUCTURE_A_PATTERN = Pattern.compile("( {3}|[a-z][ |a-z]{2})\\d{8} ");
   private static final Pattern LCCN_STRUCTURE_B_PATTERN = Pattern.compile("( {2}|[a-z][ |a-z])\\d{10}");
 
   private final SpecProvider specProvider;
 
   @Override
   public boolean isValid(LccnRequest lccnRequest, ConstraintValidatorContext constraintValidatorContext) {
-    var isEnabled = specProvider.getSpecRules()
-      .stream()
-      .filter(rule -> rule.getCode().equals("invalidLccnSubfieldValue"))
-      .findFirst()
-      .map(SpecificationRuleDto::getEnabled)
-      .orElse(false);
-
-    if (TRUE.equals(isEnabled) && isCurrent(lccnRequest)) {
+    if (isCurrent(lccnRequest) && isLccnFormatValidationEnabled()) {
       return lccnRequest.getValue()
         .stream()
         .anyMatch(this::hasValidPattern);
@@ -45,6 +39,15 @@ private boolean isCurrent(LccnRequest lccnRequest) {
       .anyMatch(link -> link.endsWith("current"));
   }
 
+  private boolean isLccnFormatValidationEnabled() {
+    return specProvider.getSpecRules()
+      .stream()
+      .filter(rule -> CODE.equals(rule.getCode()))
+      .findFirst()
+      .map(SpecificationRuleDto::getEnabled)
+      .orElse(false);
+  }
+
   private boolean hasValidPattern(String lccnValue) {
     return LCCN_STRUCTURE_A_PATTERN.matcher(lccnValue).matches()
       || LCCN_STRUCTURE_B_PATTERN.matcher(lccnValue).matches();

diff --git a/src/main/java/org/folio/linked/data/validation/spec/impl/SpecProviderImpl.java b/src/main/java/org/folio/linked/data/validation/spec/impl/SpecProviderImpl.java
@@ -1,7 +1,5 @@
 package org.folio.linked.data.validation.spec.impl;
 
-import static org.folio.linked.data.configuration.CacheConfiguration.SPEC_RULES;
-
 import feign.FeignException;
 import java.util.Collection;
 import java.util.Collections;
@@ -15,10 +13,7 @@
 import org.folio.rspec.domain.dto.SpecificationDtoCollection;
 import org.folio.rspec.domain.dto.SpecificationRuleDto;
 import org.folio.rspec.domain.dto.SpecificationRuleDtoCollection;
-import org.springframework.cache.annotation.CacheEvict;
-import org.springframework.cache.annotation.Cacheable;
 import org.springframework.http.ResponseEntity;
-import org.springframework.scheduling.annotation.Scheduled;
 import org.springframework.stereotype.Component;
 
 @Component
@@ -28,7 +23,6 @@ public class SpecProviderImpl implements SpecProvider {
 
   private final SpecClient client;
 
-  @Cacheable(value = SPEC_RULES)
   @Override
   public List<SpecificationRuleDto> getSpecRules() {
     try {
@@ -49,10 +43,4 @@ public List<SpecificationRuleDto> getSpecRules() {
       return Collections.emptyList();
     }
   }
-
-  @CacheEvict(value = SPEC_RULES)
-  @Scheduled(fixedRateString = "${mod-linked-data.caching.ttl.specRules}")
-  public void emptyCache() {
-    log.info("Emptying {} cache", SPEC_RULES);
-  }
 }
diff --git a/src/main/resources/application.yml b/src/main/resources/application.yml
@@ -79,9 +79,9 @@ mod-linked-data:
     topic:
       work-search-index: ${KAFKA_WORK_SEARCH_INDEX_TOPIC:linked-data.work}
       instance-ingress: ${KAFKA_INVENTORY_INSTANCE_INGRESS_EVENT_TOPIC:inventory.instance_ingress}
-  caching:
+  cache:
     ttl:
-      specRules: 18000000
+      spec-rules: ${CACHE_TTL_SPEC_RULES:18000000}
 
 management:
   endpoints:

diff --git a/src/test/java/org/folio/linked/data/e2e/resource/ResourceControllerITBase.java b/src/test/java/org/folio/linked/data/e2e/resource/ResourceControllerITBase.java
@@ -169,6 +169,7 @@
 import org.folio.linked.data.service.resource.hash.HashService;
 import org.folio.linked.data.test.ResourceTestService;
 import org.folio.linked.data.test.TestUtil;
+import org.folio.linked.data.validation.dto.LccnPatternValidator;
 import org.folio.rest.jaxrs.model.ParsedRecord;
 import org.folio.rest.jaxrs.model.Record;
 import org.folio.rspec.domain.dto.SpecificationDto;
@@ -264,6 +265,10 @@ void createInstanceWithWorkRef_shouldSaveEntityCorrectly() throws Exception {
   @Test
   void createInstanceWithWorkRef_shouldReturn400_ifLccnIsInvalid() throws Exception {
     // given
+    var specRuleId = randomUUID();
+    when(specClient.getBibMarcSpecs()).thenReturn(ResponseEntity.ok().body(createSpecifications(specRuleId)));
+    when(specClient.getSpecRules(specRuleId)).thenReturn(ResponseEntity.ok().body(createSpecRules()));
+
     var work = getSampleWork(null);
     setExistingResourcesIds(work);
     resourceTestService.saveGraph(work);
@@ -384,16 +389,9 @@ void createWorkWithInstanceRef_shouldSaveEntityCorrectly() throws Exception {
   @Test
   void update_shouldReturnCorrectlyUpdatedInstanceWithWorkRef_deleteOldOne_sendMessages() throws Exception {
     // given
-    var specifications = new SpecificationDtoCollection();
     var specRuleId = randomUUID();
-    specifications.setSpecifications(List.of(new SpecificationDto().id(specRuleId)));
-    var specRules = new SpecificationRuleDtoCollection();
-    var specRule = new SpecificationRuleDto();
-    specRule.setCode("invalidLccnSubfieldValue");
-    specRule.setEnabled(true);
-    specRules.setRules(List.of(specRule));
-    when(specClient.getBibMarcSpecs()).thenReturn(ResponseEntity.ok().body(specifications));
-    when(specClient.getSpecRules(specRuleId)).thenReturn(ResponseEntity.ok().body(specRules));
+    when(specClient.getBibMarcSpecs()).thenReturn(ResponseEntity.ok().body(createSpecifications(specRuleId)));
+    when(specClient.getSpecRules(specRuleId)).thenReturn(ResponseEntity.ok().body(createSpecRules()));
 
     var work = getSampleWork(null);
     var originalInstance = resourceTestService.saveGraph(getSampleInstanceResource(null, work));
@@ -449,6 +447,10 @@ void update_shouldReturnCorrectlyUpdatedInstanceWithWorkRef_deleteOldOne_sendMes
   @Test
   void update_shouldReturn400_ifLccnIsInvalid() throws Exception {
     // given
+    var specRuleId = randomUUID();
+    when(specClient.getBibMarcSpecs()).thenReturn(ResponseEntity.ok().body(createSpecifications(specRuleId)));
+    when(specClient.getSpecRules(specRuleId)).thenReturn(ResponseEntity.ok().body(createSpecRules()));
+
     var updateDto = getSampleInstanceDtoMap();
     var instance = (LinkedHashMap) ((LinkedHashMap) updateDto.get("resource")).get(INSTANCE.getUri());
     instance.remove("inventoryId");
@@ -2083,6 +2085,21 @@ private ArrayList getStatus(LinkedHashMap instance) {
     return (ArrayList) lccn.get(STATUS.getUri());
   }
 
+  private SpecificationDtoCollection createSpecifications(UUID specRuleId) {
+    var specifications = new SpecificationDtoCollection();
+    specifications.setSpecifications(List.of(new SpecificationDto().id(specRuleId)));
+    return specifications;
+  }
+
+  private SpecificationRuleDtoCollection createSpecRules() {
+    var specRules = new SpecificationRuleDtoCollection();
+    var specRule = new SpecificationRuleDto();
+    specRule.setCode(LccnPatternValidator.CODE);
+    specRule.setEnabled(true);
+    specRules.setRules(List.of(specRule));
+    return specRules;
+  }
+
   private record LookupResources(
     List<Resource> subjects,
     List<Resource> geographicCoverages,