Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MODLD-599: LCCN validation configuration #51

Merged
merged 3 commits into from
Nov 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .run/LinkedDataApplication [Local-Standalone].run.xml
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,4 @@
<option name="Make" enabled="true" />
</method>
</configuration>
</component>
</component>
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let us define this in the module descriptor file also.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

* 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).
Expand Down
15 changes: 12 additions & 3 deletions descriptors/ModuleDescriptor-template.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@
"source-storage.snapshots.post",
"source-storage.records.formatted.item.get",
"source-storage.records.post",
"source-storage.records.put"
"source-storage.records.put",
"specification-storage.specifications.collection.get",
"specification-storage.specification.rules.collection.get"
]
},
{
Expand Down Expand Up @@ -65,7 +67,9 @@
"source-storage.snapshots.post",
"source-storage.records.formatted.item.get",
"source-storage.records.post",
"source-storage.records.put"
"source-storage.records.put",
"specification-storage.specifications.collection.get",
"specification-storage.specification.rules.collection.get"
]
},
{
Expand Down Expand Up @@ -172,6 +176,10 @@
{
"id": "source-storage-records",
"version": "3.2 3.3"
},
{
"id": "specification-storage",
"version": "1.0"
}
],
"permissionSets": [
Expand Down Expand Up @@ -277,7 +285,8 @@
{ "name": "KAFKA_WORK_SEARCH_INDEX_TOPIC", "value": "linked-data.work" },
{ "name": "KAFKA_WORK_SEARCH_INDEX_TOPIC_PARTITIONS", "value": "1" },
{ "name": "KAFKA_WORK_SEARCH_INDEX_TOPIC_REPLICATION_FACTOR", "value": "" },
{ "name": "KAFKA_INVENTORY_INSTANCE_INGRESS_EVENT_TOPIC", "value": "" }
{ "name": "KAFKA_INVENTORY_INSTANCE_INGRESS_EVENT_TOPIC", "value": "" },
{ "name": "CACHE_TTL_SPEC_RULES_MS", "value": "18000000" }
]
}
}
6 changes: 6 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
<lib-linked-data-marc4ld.version>1.0.2-SNAPSHOT</lib-linked-data-marc4ld.version>
<lib-linked-data-fingerprint.version>1.0.1-SNAPSHOT</lib-linked-data-fingerprint.version>
<mod-source-record-storage-client.version>5.9.1</mod-source-record-storage-client.version>
<mod-record-specifications-dto.version>1.1.0-SNAPSHOT</mod-record-specifications-dto.version>
<maven-checkstyle-plugin.checkstyle.version>10.20.1</maven-checkstyle-plugin.checkstyle.version>

<!-- Test dependencies versions -->
Expand Down Expand Up @@ -102,6 +103,11 @@
<artifactId>mod-source-record-storage-client</artifactId>
<version>${mod-source-record-storage-client.version}</version>
</dependency>
<dependency>
<groupId>org.folio</groupId>
<artifactId>mod-record-specifications-dto</artifactId>
<version>${mod-record-specifications-dto.version}</version>
</dependency>
<dependency>
<groupId>org.folio</groupId>
<artifactId>folio-kafka-wrapper</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@
import org.springframework.cloud.openfeign.EnableFeignClients;
import org.springframework.context.annotation.ComponentScan;
import org.springframework.scheduling.annotation.EnableAsync;
import org.springframework.scheduling.annotation.EnableScheduling;

@EnableCaching
@EnableAsync
@EnableFeignClients
@EnableScheduling
@SpringBootApplication
@ComponentScan(value = "org.folio", excludeFilters = @Filter(type = REGEX, pattern =
{"org.folio.spring.tools.systemuser.*", "org.folio.spring.tools.batch.*"}))
Expand Down
24 changes: 24 additions & 0 deletions src/main/java/org/folio/linked/data/client/SpecClient.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
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;
import org.springframework.web.bind.annotation.PathVariable;

@FeignClient(name = "specification-storage")
public interface SpecClient {

@Cacheable(cacheNames = "bib-marc-specs")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this cache is not evicted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but for this one it's not necessary is it?

@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);
}
19 changes: 19 additions & 0 deletions src/main/java/org/folio/linked/data/job/CacheCleaningJob.java
Original file line number Diff line number Diff line change
@@ -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);
}
}
4 changes: 4 additions & 0 deletions src/main/java/org/folio/linked/data/util/Constants.java
Original file line number Diff line number Diff line change
Expand Up @@ -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";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,25 @@
import jakarta.validation.ConstraintValidator;
import jakarta.validation.ConstraintValidatorContext;
import java.util.regex.Pattern;
import lombok.RequiredArgsConstructor;
import org.folio.linked.data.domain.dto.LccnRequest;
import org.folio.linked.data.validation.LccnPatternConstraint;
import org.folio.linked.data.validation.spec.SpecProvider;
import org.folio.rspec.domain.dto.SpecificationRuleDto;

@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) {
if (isCurrent(lccnRequest)) {
if (isCurrent(lccnRequest) && isLccnFormatValidationEnabled()) {
return lccnRequest.getValue()
.stream()
.anyMatch(this::hasValidPattern);
Expand All @@ -31,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();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package org.folio.linked.data.validation.spec;

import java.util.List;
import org.folio.rspec.domain.dto.SpecificationRuleDto;

public interface SpecProvider {

List<SpecificationRuleDto> getSpecRules();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package org.folio.linked.data.validation.spec.impl;

import feign.FeignException;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Optional;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.folio.linked.data.client.SpecClient;
import org.folio.linked.data.validation.spec.SpecProvider;
import org.folio.rspec.domain.dto.SpecificationDto;
import org.folio.rspec.domain.dto.SpecificationDtoCollection;
import org.folio.rspec.domain.dto.SpecificationRuleDto;
import org.folio.rspec.domain.dto.SpecificationRuleDtoCollection;
import org.springframework.http.ResponseEntity;
import org.springframework.stereotype.Component;

@Component
@RequiredArgsConstructor
@Slf4j
public class SpecProviderImpl implements SpecProvider {

private final SpecClient client;

@Override
public List<SpecificationRuleDto> getSpecRules() {
try {
return Optional.ofNullable(client.getBibMarcSpecs().getBody())
.map(SpecificationDtoCollection::getSpecifications)
.stream()
.flatMap(Collection::stream)
.findFirst()
.map(SpecificationDto::getId)
.map(client::getSpecRules)
.map(ResponseEntity::getBody)
.map(SpecificationRuleDtoCollection::getRules)
.stream()
.flatMap(Collection::stream)
.toList();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do .filter(rule -> rule.getCode().equals("invalidLccnSubfieldValue")) here so that we are caching only the required rule.

(This may not be possible if we implement the suggestion that @PBobylev provided. Not a big deal as the number of rules is not that big)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not actual (caching is on Feign client now)

} catch (FeignException e) {
log.error("Unexpected exception during specification rules retrieval", e);
return Collections.emptyList();
Copy link
Contributor

@pkjacob pkjacob Nov 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problem with returning empty list here is that it will get cached for the next XXX hours if there is a network glitch when this call is made.

Let us check with Doug what we need to do in this case. These are the options I can think about -

  1. Fail the request. ie, throw an exception here. Nothing will get cached in this case.
  2. Process this request assuming that the LCCN format validation is not enabled. However, do not cache the empty list so that we will check the configuration again for the next request.
  3. Process this request assuming that the LCCN format validation is enabled. However, do not cache the empty list so that we will check the configuration again for the next request.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not actual (caching is on Feign client now)

Copy link
Contributor

@pkjacob pkjacob Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let us ask Doug on Monday what we should do in this case
Fail the request or process the request assuming that lccn validation is disabled.

Update:
Never mind. I see the following line in ticket
If the microservice is down, assume that the value is disabled / turned off.

}
}
}
3 changes: 3 additions & 0 deletions src/main/resources/application.yml
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +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}
cache:
ttl:
spec-rules: ${CACHE_TTL_SPEC_RULES:18000000}

management:
endpoints:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import static java.lang.String.join;
import static java.util.Spliterator.ORDERED;
import static java.util.Spliterators.spliteratorUnknownSize;
import static java.util.UUID.randomUUID;
import static java.util.stream.StreamSupport.stream;
import static org.assertj.core.api.Assertions.assertThat;
import static org.folio.ld.dictionary.PredicateDictionary.ACCESS_LOCATION;
Expand Down Expand Up @@ -154,6 +155,7 @@
import org.folio.ld.dictionary.PredicateDictionary;
import org.folio.ld.dictionary.PropertyDictionary;
import org.folio.ld.dictionary.ResourceTypeDictionary;
import org.folio.linked.data.client.SpecClient;
import org.folio.linked.data.client.SrsClient;
import org.folio.linked.data.domain.dto.InstanceResponseField;
import org.folio.linked.data.domain.dto.ResourceIndexEventType;
Expand All @@ -167,8 +169,13 @@
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;
import org.folio.rspec.domain.dto.SpecificationDtoCollection;
import org.folio.rspec.domain.dto.SpecificationRuleDto;
import org.folio.rspec.domain.dto.SpecificationRuleDtoCollection;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
Expand Down Expand Up @@ -214,6 +221,8 @@ public abstract class ResourceControllerITBase {
private HashService hashService;
@MockBean
private SrsClient srsClient;
@MockBean
private SpecClient specClient;

@BeforeEach
public void beforeEach() {
Expand Down Expand Up @@ -256,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);
Expand Down Expand Up @@ -376,6 +389,10 @@ void createWorkWithInstanceRef_shouldSaveEntityCorrectly() throws Exception {
@Test
void update_shouldReturnCorrectlyUpdatedInstanceWithWorkRef_deleteOldOne_sendMessages() 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);
var originalInstance = resourceTestService.saveGraph(getSampleInstanceResource(null, work));
var updateDto = getSampleInstanceDtoMap();
Expand Down Expand Up @@ -430,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");
Expand Down Expand Up @@ -2064,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,
Expand Down
Loading