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

Add External Validation Rules for Analysis Properties Based on Dynamic Schema #877

Open
wants to merge 9 commits into
base: feat/refactor-analyis-data-model
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,5 @@
@AllArgsConstructor
public class AnalysisTypeOptions {
private List<String> fileTypes;
private List<ExternalValidation> externalValidation;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package bio.overture.song.core.model;

import lombok.AllArgsConstructor;
import lombok.Builder;
import lombok.Data;
import lombok.NoArgsConstructor;

@Data
@Builder
@NoArgsConstructor
@AllArgsConstructor
public class ExternalValidation {
private String jsonPath;
private String url;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package bio.overture.song.server.config;

import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.web.client.RestTemplate;

@Configuration
public class RestTemplateConfig {
@Bean
public RestTemplate restTemplate() {
return new RestTemplate();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,24 +4,18 @@
import static bio.overture.song.server.repository.CustomJsonType.CUSTOM_JSON_TYPE_PKG_PATH;
import static com.google.common.collect.Sets.newHashSet;

import bio.overture.song.core.model.AnalysisTypeOptions;
import bio.overture.song.server.model.analysis.Analysis;
import bio.overture.song.server.model.enums.ModelAttributeNames;
import bio.overture.song.server.model.enums.TableAttributeNames;
import bio.overture.song.server.model.enums.TableNames;
import bio.overture.song.server.utils.StringListConverter;
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.databind.JsonNode;
import java.time.LocalDateTime;
import java.util.List;
import java.util.Set;
import javax.persistence.*;
import javax.validation.constraints.NotNull;
import lombok.AllArgsConstructor;
import lombok.Builder;
import lombok.Data;
import lombok.EqualsAndHashCode;
import lombok.NoArgsConstructor;
import lombok.ToString;
import lombok.*;
import org.hibernate.annotations.CreationTimestamp;
import org.hibernate.annotations.Type;

Expand Down Expand Up @@ -54,9 +48,10 @@ public class AnalysisSchema {
@Type(type = CUSTOM_JSON_TYPE_PKG_PATH)
private JsonNode schema;

@Column(name = FILE_TYPES, columnDefinition = "text[]")
@Convert(converter = StringListConverter.class)
private List<String> fileTypes;
@NotNull
@Column(name = OPTIONS)
@Type(type = CUSTOM_JSON_TYPE_PKG_PATH)
private AnalysisTypeOptions options;

@JsonIgnore
@Builder.Default
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ public class TableAttributeNames {
public static final String MATCHED_NORMAL_SUBMITTER_SAMPLE_ID =
"matched_normal_submitter_sample_id";
public static final String SCHEMA = "schema";
public static final String OPTIONS = "options";
public static final String VERSION = "version";
public static final String ANALYSIS_SCHEMA_ID = "analysis_schema_id";
public static final String DATA = "data";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,9 +155,11 @@ public AnalysisType getAnalysisType(
val resolvedSchemaJson =
resolveSchemaJsonView(analysisSchema.getSchema(), unrenderedOnly, false);

AnalysisTypeOptions options = analysisSchema.getOptions();

List<String> fileTypes =
(analysisSchema.getFileTypes() != null && !analysisSchema.getFileTypes().isEmpty())
? analysisSchema.getFileTypes()
(options.getFileTypes() != null && !options.getFileTypes().isEmpty())
? options.getFileTypes()
: new ArrayList<>();
return AnalysisType.builder()
.name(analysisTypeId.getName())
Expand Down Expand Up @@ -265,11 +267,33 @@ private AnalysisType commitAnalysisType(
if (options != null && CollectionUtils.isNotEmpty(options.getFileTypes())) {
fileTypes = options.getFileTypes();
}

// checking if file types is empty
// if the version is new version of the schema , we are checking the previous version allowed
// file types
// if it is new then it is empty
Azher2Ali marked this conversation as resolved.
Show resolved Hide resolved
if (fileTypes.isEmpty()) {
List<AnalysisSchema> analysisSchemaList =
analysisSchemaRepository.findAllByName(analysisTypeName);

if (!analysisSchemaList.isEmpty()) {
Optional<AnalysisSchema> latestSchemaOptional =
analysisSchemaList.stream()
.filter(schema -> schema.getVersion() != null)
.max(Comparator.comparingInt(AnalysisSchema::getVersion));

if (latestSchemaOptional.isPresent()) {
AnalysisTypeOptions optionsFromDb = latestSchemaOptional.get().getOptions();
fileTypes = optionsFromDb.getFileTypes();
Azher2Ali marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Azher2Ali marked this conversation as resolved.
Show resolved Hide resolved

val analysisSchema =
AnalysisSchema.builder()
.name(analysisTypeName)
.schema(analysisTypeSchema)
.fileTypes(fileTypes)
.options(options)
.build();

log.debug("Creating analysisSchema with file types: {} " + fileTypes);
Expand Down Expand Up @@ -319,7 +343,10 @@ public static AnalysisTypeId resolveAnalysisTypeId(@NonNull AnalysisType analysi
private AnalysisType convertToAnalysisType(
AnalysisSchema analysisSchema, boolean hideSchema, boolean unrenderedOnly) {
AnalysisTypeOptions options = new AnalysisTypeOptions();
options.setFileTypes(analysisSchema.getFileTypes());
if (analysisSchema.getOptions() != null) {
options.setFileTypes(analysisSchema.getOptions().getFileTypes());
options.setExternalValidation(analysisSchema.getOptions().getExternalValidation());
}
Azher2Ali marked this conversation as resolved.
Show resolved Hide resolved
return AnalysisType.builder()
.name(analysisSchema.getName())
.version(analysisSchema.getVersion())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,15 +58,16 @@ public SubmitService(
this.studyService = studyService;
}

public SubmitResponse submit(@NonNull String studyId, String payloadString, boolean allowDuplicates) {
public SubmitResponse submit(
@NonNull String studyId, String payloadString, boolean allowDuplicates) {
// Check study exists
studyService.checkStudyExist(studyId);

// Parse JSON payload
val payloadJson = parsePayload(payloadString);

// Validate JSON Payload
validatePayload(payloadJson);
validatePayload(payloadJson, studyId);

// Deserialize JSON payload to Payload DTO
val payload = fromJson(payloadJson, Payload.class);
Expand All @@ -75,7 +76,7 @@ public SubmitResponse submit(@NonNull String studyId, String payloadString, bool
checkStudyInPayload(studyId, payload);

// check duplicate analysis
if(!allowDuplicates){
if (!allowDuplicates) {
analysisService.checkDuplicateAnalysis(payload);
}

Expand Down Expand Up @@ -119,9 +120,9 @@ private AnalysisTypeId parseAnalysisTypeId(@NonNull JsonNode payloadJson) {
return fromJson(analysisTypePath, AnalysisTypeId.class);
}

private void validatePayload(JsonNode payloadJson) {
private void validatePayload(JsonNode payloadJson, String studyId) {
// Validate payload format and content
val error = validator.validate(payloadJson);
val error = validator.validate(payloadJson, studyId);
if (error.isPresent()) {
val message = error.get();
throw buildServerException(getClass(), SCHEMA_VIOLATION, message);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,26 +29,32 @@
import static org.apache.commons.lang.StringUtils.isBlank;

import bio.overture.song.core.model.AnalysisTypeId;
import bio.overture.song.core.model.ExternalValidation;
import bio.overture.song.core.model.FileData;
import bio.overture.song.server.model.enums.UploadStates;
import bio.overture.song.server.repository.UploadRepository;
import bio.overture.song.server.validation.SchemaValidator;
import bio.overture.song.server.validation.ValidationResponse;
import com.fasterxml.jackson.databind.JsonNode;
import com.jayway.jsonpath.JsonPath;
import java.util.ArrayList;
import java.util.List;
import java.util.Objects;
import java.util.Optional;
import java.util.function.Supplier;
import lombok.NonNull;
import lombok.SneakyThrows;
import lombok.extern.slf4j.Slf4j;
import lombok.val;
import org.apache.commons.collections.CollectionUtils;
import org.apache.http.client.utils.URIBuilder;
import org.everit.json.schema.Schema;
import org.everit.json.schema.ValidationException;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.http.ResponseEntity;
import org.springframework.stereotype.Service;
import org.springframework.web.client.RestTemplate;

@Slf4j
@Service
Expand All @@ -62,23 +68,26 @@ public class ValidationService {
private final UploadRepository uploadRepository;
private final boolean enforceLatest;
private final Schema analysisTypeIdSchema;
private final RestTemplate restTemplate;
Azher2Ali marked this conversation as resolved.
Show resolved Hide resolved

@Autowired
public ValidationService(
@Value("${schemas.enforceLatest}") boolean enforceLatest,
@NonNull SchemaValidator validator,
@NonNull AnalysisTypeService analysisTypeService,
@NonNull Supplier<Schema> analysisTypeIdSchemaSupplier,
@NonNull UploadRepository uploadRepository) {
@NonNull UploadRepository uploadRepository,
@NonNull RestTemplate restTemplate) {
this.validator = validator;
this.analysisTypeService = analysisTypeService;
this.uploadRepository = uploadRepository;
this.enforceLatest = enforceLatest;
this.analysisTypeIdSchema = analysisTypeIdSchemaSupplier.get();
this.restTemplate = restTemplate;
}

@SneakyThrows
public Optional<String> validate(@NonNull JsonNode payload) {
public Optional<String> validate(@NonNull JsonNode payload, @NonNull String studyId) {
Azher2Ali marked this conversation as resolved.
Show resolved Hide resolved
String errors = null;
try {
validateWithSchema(analysisTypeIdSchema, payload);
Expand Down Expand Up @@ -109,6 +118,43 @@ public Optional<String> validate(@NonNull JsonNode payload) {
return Optional.ofNullable(errors);
}

private void externalValidations(
List<ExternalValidation> externalValidations, @NonNull JsonNode payload) {

for (ExternalValidation externalValidation : externalValidations) {
String jsonPath = externalValidation.getJsonPath();
String externalUrl = externalValidation.getUrl();

// Extract the value from the uploaded JSON using the jsonPath
String value = JsonPath.read(payload, "$." + jsonPath);

if(Objects.isNull(value) || value.isEmpty())
return;

// call the external url using restTemplate
}
}

public boolean invokeExternalUrl(String studyId, String url, String value) {
Azher2Ali marked this conversation as resolved.
Show resolved Hide resolved
// will change the logic to make it parameterised
try {

// create URL
URIBuilder uriBuilder = new URIBuilder(url);
uriBuilder.addParameter("studyId", studyId);
uriBuilder.addParameter("value",value);
// Make the HTTP GET request
ResponseEntity<Void> response = restTemplate.getForEntity(uriBuilder.build().toURL().toString(), Void.class);

// Return true if the response status is 200 OK
return response.getStatusCode().is2xxSuccessful();
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

} catch (Exception e) {
// Handle exceptions (e.g., log them)
System.err.println("Validation failed " + e.getMessage());
return false;
}
}

private void validateFileType(List<String> fileTypes, @NonNull JsonNode payload) {

if (CollectionUtils.isNotEmpty(fileTypes)) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@

-- Remove the file_types column
ALTER TABLE public.analysis_schema
DROP COLUMN file_types;
Copy link
Contributor

Choose a reason for hiding this comment

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

Have we published this version with file types yet? We would need to migrate all the file_types that have already been set into the new analysis schema format.

Did you consider storign analysis_schema in a separate column instead of storing options as a JSON?


-- Add the options column as jsonb (nullable by default)
ALTER TABLE public.analysis_schema
ADD COLUMN options jsonb;
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ public void submit_StudyDne_StudyIdDoesNotExist() {
// Verify
assertSongError(
() -> submitService.submit("anyStudy", "anyAnalysisId", false), STUDY_ID_DOES_NOT_EXIST);
verify(validationService, never()).validate(isA(JsonNode.class));
verify(validationService, never()).validate(isA(JsonNode.class),"anyStudy");
Azher2Ali marked this conversation as resolved.
Show resolved Hide resolved
verify(analysisService, never()).create(anyString(), isA(Payload.class));
}

Expand All @@ -113,7 +113,7 @@ public void submit_malformedPayload_PayloadParsingError() {
// Verify
assertSongError(
() -> submitService.submit("anyStudy", "non json format", false), PAYLOAD_PARSING);
verify(validationService, never()).validate(isA(JsonNode.class));
verify(validationService, never()).validate(isA(JsonNode.class),"anyStudy");
verify(analysisService, never()).create(anyString(), isA(Payload.class));
}

Expand All @@ -122,7 +122,7 @@ public void submit_invalidPayload_SchemaViolation() {
// Setup
val studyId = "anyStudy";
doNothing().when(studyService).checkStudyExist(anyString());
when(validationService.validate(isA(JsonNode.class)))
when(validationService.validate(isA(JsonNode.class), "anyStudy"))
.thenReturn(Optional.of("there was an error"));

// Create an invalid payload and not a malformed one
Expand All @@ -136,7 +136,7 @@ public void submit_invalidPayload_SchemaViolation() {

// Verify
assertSongError(() -> submitService.submit(studyId, invalidPayload, false), SCHEMA_VIOLATION);
verify(validationService, times(1)).validate(isA(JsonNode.class));
verify(validationService, times(1)).validate(isA(JsonNode.class),"anyStudy");
verify(analysisService, never()).create(anyString(), isA(Payload.class));
}

Expand All @@ -146,7 +146,7 @@ public void submit_mismatchingStudies_StudyIdMismatch() {
val study1 = "study1";
val study2 = "study2";
doNothing().when(studyService).checkStudyExist(anyString());
when(validationService.validate(isA(JsonNode.class))).thenReturn(Optional.empty());
when(validationService.validate(isA(JsonNode.class), "anyStudy")).thenReturn(Optional.empty());
val payloadString =
toJson(
Payload.builder()
Expand All @@ -158,7 +158,7 @@ public void submit_mismatchingStudies_StudyIdMismatch() {
// Verify
assertNotEquals(study1, study2);
assertSongError(() -> submitService.submit(study2, payloadString, false), STUDY_ID_MISMATCH);
verify(validationService, times(1)).validate(isA(JsonNode.class));
verify(validationService, times(1)).validate(isA(JsonNode.class), "anyStudy");
verify(analysisService, never()).create(anyString(), isA(Payload.class));
}

Expand All @@ -174,7 +174,7 @@ public void submit_validPayload_Success() {
.build();

doNothing().when(studyService).checkStudyExist(anyString());
when(validationService.validate(isA(JsonNode.class))).thenReturn(Optional.empty());
when(validationService.validate(isA(JsonNode.class), "anyStudy")).thenReturn(Optional.empty());

val payloadString = toJson(payload);
when(analysisService.create(study, payload))
Expand All @@ -184,7 +184,7 @@ public void submit_validPayload_Success() {
// Verify
val actualSubmitResponse = submitService.submit(study, payloadString, false);
assertEquals(expectedSubmitResponse, actualSubmitResponse);
verify(validationService, times(1)).validate(isA(JsonNode.class));
verify(validationService, times(1)).validate(isA(JsonNode.class), "anyStudy");
verify(analysisService, times(1)).create(anyString(), isA(Payload.class));
}
}
Loading