diff --git a/src/main/java/org/breedinginsight/api/v1/controller/BreedingMethodController.java b/src/main/java/org/breedinginsight/api/v1/controller/BreedingMethodController.java index 6e89bd7cd..9d80f9115 100644 --- a/src/main/java/org/breedinginsight/api/v1/controller/BreedingMethodController.java +++ b/src/main/java/org/breedinginsight/api/v1/controller/BreedingMethodController.java @@ -7,7 +7,6 @@ import io.micronaut.security.annotation.Secured; import io.micronaut.security.rules.SecurityRule; import lombok.extern.slf4j.Slf4j; -import org.apache.commons.lang3.StringUtils; import org.brapi.client.v2.model.exceptions.ApiException; import org.breedinginsight.api.auth.*; import org.breedinginsight.api.model.v1.response.DataResponse; @@ -17,18 +16,14 @@ import org.breedinginsight.api.model.v1.response.metadata.Status; import org.breedinginsight.api.model.v1.response.metadata.StatusCode; import org.breedinginsight.api.v1.controller.metadata.AddMetadata; -import org.breedinginsight.dao.db.tables.pojos.BreedingMethodEntity; import org.breedinginsight.dao.db.tables.pojos.ProgramBreedingMethodEntity; -import org.breedinginsight.daos.BreedingMethodDAO; import org.breedinginsight.services.BreedingMethodService; import org.breedinginsight.services.exceptions.BadRequestException; import javax.inject.Inject; -import javax.validation.Valid; import java.util.ArrayList; import java.util.List; import java.util.UUID; -import java.util.stream.Collectors; @Slf4j @Controller("/${micronaut.bi.api.version}") @@ -64,7 +59,7 @@ public HttpResponse>> getSyst @Post("programs/{programId}/breeding-methods") @Produces(MediaType.APPLICATION_JSON) @ProgramSecured(roles = {ProgramSecuredRole.BREEDER}) - public HttpResponse> createProgramBreedingMethod(@PathVariable UUID programId, @Body ProgramBreedingMethodEntity breedingMethod) throws BadRequestException, ApiException { + public HttpResponse createProgramBreedingMethod(@PathVariable UUID programId, @Body ProgramBreedingMethodEntity breedingMethod) throws ApiException{ log.debug("Saving new program breeding method"); try { @@ -79,6 +74,8 @@ public HttpResponse> createProgramBreeding response.metadata = metadata; return HttpResponse.ok(response); + } catch (BadRequestException ex) { + return HttpResponse.badRequest(ex.getMessage()); } catch (Exception e) { log.error("Error creating program breeding method", e); throw e; @@ -110,7 +107,7 @@ public HttpResponse>> getProg @Put("programs/{programId}/breeding-methods/{breedingMethodId}") @Produces(MediaType.APPLICATION_JSON) @ProgramSecured(roles = {ProgramSecuredRole.BREEDER}) - public HttpResponse> updateProgramBreedingMethod(@PathVariable UUID programId, @PathVariable UUID breedingMethodId, @Body ProgramBreedingMethodEntity breedingMethod) throws BadRequestException, ApiException { + public HttpResponse updateProgramBreedingMethod(@PathVariable UUID programId, @PathVariable UUID breedingMethodId, @Body ProgramBreedingMethodEntity breedingMethod) throws ApiException { log.debug("Saving new program breeding method"); try { @@ -128,6 +125,8 @@ public HttpResponse> updateProgramBreeding response.metadata = metadata; return HttpResponse.ok(response); + } catch (BadRequestException ex) { + return HttpResponse.badRequest(ex.getMessage()); } catch (Exception e) { log.error("Error updating program breeding method", e); throw e; diff --git a/src/main/java/org/breedinginsight/services/impl/BreedingMethodServiceImpl.java b/src/main/java/org/breedinginsight/services/impl/BreedingMethodServiceImpl.java index 9b5c9fab6..19e880309 100644 --- a/src/main/java/org/breedinginsight/services/impl/BreedingMethodServiceImpl.java +++ b/src/main/java/org/breedinginsight/services/impl/BreedingMethodServiceImpl.java @@ -1,6 +1,5 @@ package org.breedinginsight.services.impl; -import lombok.extern.slf4j.Slf4j; import org.apache.commons.lang3.StringUtils; import org.brapi.client.v2.model.exceptions.ApiException; import org.breedinginsight.brapi.v2.services.BrAPIGermplasmService; @@ -14,8 +13,9 @@ import java.util.*; import java.util.stream.Collectors; +import static java.lang.String.format; + @Singleton -@Slf4j public class BreedingMethodServiceImpl implements BreedingMethodService { private final BreedingMethodDAO breedingMethodDAO; @@ -60,28 +60,39 @@ public List fetchBreedingMethodsInUse(UUID programI } @Override - public ProgramBreedingMethodEntity createBreedingMethod(ProgramBreedingMethodEntity breedingMethod, UUID programId, UUID userId) throws BadRequestException, ApiException { - if(!validateBreedingMethod(breedingMethod)) { - throw new BadRequestException("Missing required data"); - } + public ProgramBreedingMethodEntity createBreedingMethod(ProgramBreedingMethodEntity breedingMethod, UUID programId, UUID userId) throws BadRequestException { + validate(breedingMethod, programId); return dsl.transactionResult(() -> breedingMethodDAO.createProgramMethod(breedingMethod, programId, userId)); } + @Override public ProgramBreedingMethodEntity updateBreedingMethod(ProgramBreedingMethodEntity breedingMethod, UUID programId, UUID userId) throws BadRequestException, ApiException { - if(!validateBreedingMethod(breedingMethod)) { - throw new BadRequestException("Missing required data"); - } - List inUseMethods = fetchBreedingMethodsInUse(programId); if(inUseMethods.stream().anyMatch(method -> method.getId().equals(breedingMethod.getId()))) { throw new BadRequestException("Breeding method is not allowed to be edited"); } + validate(breedingMethod, programId); return dsl.transactionResult(() -> breedingMethodDAO.updateProgramMethod(breedingMethod, programId, userId)); } + private void validate(ProgramBreedingMethodEntity breedingMethod, UUID programId) throws BadRequestException { + if (isMissingRequiredFields(breedingMethod)) { + throw new BadRequestException("Missing required data"); + } + + List programAndSystemMethods = getBreedingMethods(programId); + if( isDuplicateMethodNameFoundOnList(breedingMethod, programAndSystemMethods)){ + throw new BadRequestException(format("A breeding method with the name '%s' is already defined in the system.", breedingMethod.getName())); + } + if( isDuplicateMethodAbbreviationFoundOnList(breedingMethod, programAndSystemMethods)){ + throw new BadRequestException(format("A breeding method with the abbreviation '%s' is already defined in the system.", breedingMethod.getAbbreviation())); + } + } + + @Override public void enableSystemMethods(List systemBreedingMethods, UUID programId, UUID userId) throws ApiException, BadRequestException { List inUseMethods = fetchBreedingMethodsInUse(programId); @@ -111,10 +122,70 @@ public void deleteBreedingMethod(UUID programId, UUID breedingMethodId) throws A dsl.transaction(() -> breedingMethodDAO.deleteProgramMethod(programId, breedingMethodId)); } - private boolean validateBreedingMethod(ProgramBreedingMethodEntity method) { - return StringUtils.isNotBlank(method.getName()) - && StringUtils.isNotBlank(method.getAbbreviation()) - && StringUtils.isNotBlank(method.getCategory()) - && StringUtils.isNotBlank(method.getGeneticDiversity()); + //'protected' instead of 'private' so it is accessible to unit test. + protected boolean isMissingRequiredFields(ProgramBreedingMethodEntity method) { + return StringUtils.isBlank(method.getName()) + || StringUtils.isBlank(method.getAbbreviation()) + || StringUtils.isBlank(method.getCategory()) + || StringUtils.isBlank(method.getGeneticDiversity()); + } + + //'protected' instead of 'private' so it is accessible to unit test. + protected boolean isDuplicateMethodNameFoundOnList(ProgramBreedingMethodEntity method, List programBreedingMethodEntityList) { + boolean foundDup = false; + for (ProgramBreedingMethodEntity testMethod: programBreedingMethodEntityList) { + if(isDuplicateName(testMethod, method)){ + foundDup = true; + break; + } + } + return foundDup; + } + + //'protected' instead of 'private' so it is accessible to unit test. + protected boolean isDuplicateMethodAbbreviationFoundOnList(ProgramBreedingMethodEntity method, List programBreedingMethodEntityList) { + boolean foundDup = false; + for (ProgramBreedingMethodEntity testMethod: programBreedingMethodEntityList) { + if(isDuplicateAbbreviation(testMethod, method)){ + foundDup = true; + break; + } + } + return foundDup; + } + + //'protected' instead of 'private' so it is accessible to unit test. + protected boolean isDuplicateName(ProgramBreedingMethodEntity testMethod, ProgramBreedingMethodEntity method) { + // SPECIAL CASE: If the two methods are the same method, then they are not duplicates + if( (testMethod.getId()!=null) && testMethod.getId().equals(method.getId()) ){ + return false; + } + + boolean isDup = false; + if(testMethod.getName()!= null && testMethod.getName().equalsIgnoreCase(method.getName())){ + isDup = true; + } + else if(testMethod.getName()==null && method.getName()==null ){ + isDup = true; + } + return isDup; + } + + //'protected' instead of 'private' so it is accessible to unit test. + protected boolean isDuplicateAbbreviation(ProgramBreedingMethodEntity testMethod, ProgramBreedingMethodEntity method) { + // SPECIAL CASE: If the two methods are the same method, then they are not duplicates + if( (testMethod.getId()!=null) && testMethod.getId().equals(method.getId()) ){ + return false; + } + + boolean isDup = false; + if(testMethod.getAbbreviation()!= null && testMethod.getAbbreviation().equalsIgnoreCase(method.getAbbreviation())){ + isDup = true; + } + else if(testMethod.getAbbreviation()==null && method.getAbbreviation()==null ){ + isDup = true; + } + return isDup; } + } diff --git a/src/test/java/org/breedinginsight/services/impl/BreedingMethodServiceImplTest.java b/src/test/java/org/breedinginsight/services/impl/BreedingMethodServiceImplTest.java new file mode 100644 index 000000000..cc59fd6ae --- /dev/null +++ b/src/test/java/org/breedinginsight/services/impl/BreedingMethodServiceImplTest.java @@ -0,0 +1,156 @@ +package org.breedinginsight.services.impl; + +import lombok.SneakyThrows; +import org.breedinginsight.brapi.v2.services.BrAPIGermplasmService; +import org.breedinginsight.dao.db.tables.pojos.ProgramBreedingMethodEntity; +import org.breedinginsight.daos.BreedingMethodDAO; +import org.jooq.DSLContext; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInstance; + +import java.util.ArrayList; +import java.util.List; +import java.util.UUID; + +import static org.junit.jupiter.api.Assertions.*; + +@TestInstance(TestInstance.Lifecycle.PER_CLASS) +public class BreedingMethodServiceImplTest { + BreedingMethodServiceImpl breedingMethodService; + BreedingMethodDAO breedingMethodDAO = null; + BrAPIGermplasmService germplasmService = null; + DSLContext dsl = null; + + @BeforeAll + public void setup() { + breedingMethodService = new BreedingMethodServiceImpl(breedingMethodDAO, germplasmService, dsl); + } + + @Test + public void hasRequiredFields(){ + ProgramBreedingMethodEntity method= new ProgramBreedingMethodEntity(); + assertTrue(breedingMethodService.isMissingRequiredFields(method),"the method has blanks"); + + method.setName("Dave"); + method.setAbbreviation("DRP"); + method.setCategory("human"); + method.setGeneticDiversity("none"); + assertFalse(breedingMethodService.isMissingRequiredFields(method),"the method has all required fields"); + } + + @Test + @SneakyThrows + public void isDuplicateMethodNameFoundOnList() { + List programBreedingMethodEntityList = new ArrayList<>(); + programBreedingMethodEntityList.add(makeMethod("1", null)); + programBreedingMethodEntityList.add(makeMethod("2", null)); + + boolean isDup = breedingMethodService.isDuplicateMethodNameFoundOnList(makeMethod("1", null), programBreedingMethodEntityList); + assertTrue(isDup, "Duplicate name found in list."); + } + + @Test + @SneakyThrows + public void isDuplicateMethodAbbreviationFoundOnList() { + List programBreedingMethodEntityList = new ArrayList<>(); + programBreedingMethodEntityList.add(makeMethod(null, "1")); + programBreedingMethodEntityList.add(makeMethod(null, "2")); + + boolean isDup = breedingMethodService.isDuplicateMethodAbbreviationFoundOnList(makeMethod(null, "1"), programBreedingMethodEntityList); + assertTrue(isDup, "Duplicate abbreviation found in list."); + isDup = breedingMethodService.isDuplicateMethodAbbreviationFoundOnList(makeMethod(null, "unique"), programBreedingMethodEntityList); + assertFalse(isDup, "Duplicate abbreviation NOT found in list."); + } + + + + @Test + @SneakyThrows + public void isDuplicateName() { + ProgramBreedingMethodEntity method = makeMethod("ABC", null); + ProgramBreedingMethodEntity testMethod = makeMethod("ABC", null); + ProgramBreedingMethodEntity testMethodLowerCase = makeMethod("abc", null); + + + + assertTrue( + breedingMethodService.isDuplicateName(testMethod, method), + "Method Names are Equal" + ); + assertTrue( + breedingMethodService.isDuplicateName(method, testMethod), + "Method Names are Equal (switched order)" + ); + assertTrue( + breedingMethodService.isDuplicateName(method, testMethodLowerCase), + "Method Names are Equal (one is LowerCase)" + ); + testMethod.setId(method.getId()); + assertFalse( + breedingMethodService.isDuplicateName(testMethod, method), + "The methods are the same method (not duplicate data)" + ); + + + testMethod.setName("misatch Name"); + testMethod.setId(UUID.randomUUID()); + assertFalse( + breedingMethodService.isDuplicateName(testMethod, method), + "Method Names are not Equal" + ); + testMethod.setName(null); + assertFalse( + breedingMethodService.isDuplicateName(testMethod, method), + "Method Names are not Equal (one is null)" + ); + + testMethod.setName("name"); + method.setName(null); + assertFalse( + breedingMethodService.isDuplicateName(testMethod, method), + "Method Names are not Equal (the other is null)" + ); + } + + @Test + @SneakyThrows + public void isDuplicateAbbreviation() { + ProgramBreedingMethodEntity method = makeMethod("ABC", "ABC"); + ProgramBreedingMethodEntity testMethod = makeMethod("mismatch_name", "ABC"); + ProgramBreedingMethodEntity testMethodLowerCase = makeMethod("mismatch_name", "abc"); + assertTrue( + breedingMethodService.isDuplicateAbbreviation(testMethod, method), + "Method Abbreviations are Equal" + ); + assertTrue( + breedingMethodService.isDuplicateAbbreviation(method, testMethod), + "Method Abbreviations are Equal (switched order)" + ); + assertTrue( + breedingMethodService.isDuplicateAbbreviation(testMethodLowerCase, method), + "Method Abbreviations are Equal (one is lower case)" + ); + testMethod.setAbbreviation("misatch_abbreviation"); + assertFalse( + breedingMethodService.isDuplicateAbbreviation(method, testMethod), + "Method Abbreviations are not equal." + ); + testMethod.setAbbreviation(null); + assertFalse( + breedingMethodService.isDuplicateAbbreviation(method, testMethod), + "Method Abbreviations are not equal (one is null)." + ); + } + + // Helper Methods // + private ProgramBreedingMethodEntity makeMethod(String nameSuffix, String abbrevSuffix){ + ProgramBreedingMethodEntity method = new ProgramBreedingMethodEntity(); + + method.setName("Name" + ( nameSuffix != null? nameSuffix : "junk" )); + method.setAbbreviation("Abbreviation" + ( abbrevSuffix != null? abbrevSuffix : "junk" )); + method.setId(UUID.randomUUID()); + return method; + } + +} diff --git a/src/test/java/org/breedinginsight/services/validators/TraitValidatorUnitTest.java b/src/test/java/org/breedinginsight/services/validators/TraitValidatorUnitTest.java index f40151f19..a7fa4507b 100644 --- a/src/test/java/org/breedinginsight/services/validators/TraitValidatorUnitTest.java +++ b/src/test/java/org/breedinginsight/services/validators/TraitValidatorUnitTest.java @@ -86,7 +86,6 @@ public void missingMethod() { ValidationErrors validationErrors = traitValidatorService.checkRequiredTraitFields(List.of(trait), new TraitValidatorError()); - assertEquals(1, validationErrors.getRowErrors().size(), "Wrong number of row errors returned"); RowValidationErrors rowValidationErrors = validationErrors.getRowErrors().get(0); assertEquals(1, rowValidationErrors.getErrors().size(), "Wrong number of errors for row");