From b9fae80d22564061c73178105656d46ec958582b Mon Sep 17 00:00:00 2001 From: Fernando Bellas Permuy Date: Wed, 13 Jan 2010 17:59:06 +0100 Subject: [PATCH] ItEr43S05ValidacionEProbasFuncionaisItEr42S05: Minor improvements to criterion REST service. The following improvements have been implemented: (1) criterion type and criterion names are trimmed with StringUtils.trim, (2) case is ignored for criterion type and criterion names, (3) just one constraint violation is reported for a criterion type with a name already used by another criterion type being imported, (4) CriterionType::checkConstraintNonRepeatedCriterionNames now discards criterions with names fulfilling StringUtils.isBlank(criterionName), (5) Criterion::createUnvalidated and CriterionType::createUnvalidated now use BaseEntity::create (to avoid to call setNewObject and other possible future actions), and (6) assertTrue/assertFalse JUnit methods in CriterionServiceTest now make use of the toString method provided by constraint violations related DTOs (to make debugging easier). --- .../resources/entities/Criterion.java | 3 +- .../resources/entities/CriterionType.java | 12 +- .../criterion/impl/CriterionConverter.java | 11 +- .../criterion/impl/CriterionServiceREST.java | 104 +++++++++--------- .../criterion/api/CriterionServiceTest.java | 49 +++++---- .../rest-clients/criterion-types-sample.xml | 15 ++- 6 files changed, 103 insertions(+), 91 deletions(-) diff --git a/navalplanner-business/src/main/java/org/navalplanner/business/resources/entities/Criterion.java b/navalplanner-business/src/main/java/org/navalplanner/business/resources/entities/Criterion.java index 64fcb5418..d6fb55f83 100644 --- a/navalplanner-business/src/main/java/org/navalplanner/business/resources/entities/Criterion.java +++ b/navalplanner-business/src/main/java/org/navalplanner/business/resources/entities/Criterion.java @@ -45,13 +45,12 @@ public class Criterion extends BaseEntity implements ICriterion { public static Criterion createUnvalidated(String name, CriterionType type, Criterion parent, boolean active) { - Criterion criterion = new Criterion(); + Criterion criterion = create(new Criterion()); criterion.name = name; criterion.type = type; criterion.parent = parent; criterion.active = active; - criterion.setNewObject(true); return criterion; diff --git a/navalplanner-business/src/main/java/org/navalplanner/business/resources/entities/CriterionType.java b/navalplanner-business/src/main/java/org/navalplanner/business/resources/entities/CriterionType.java index 0f865126c..95113ffea 100644 --- a/navalplanner-business/src/main/java/org/navalplanner/business/resources/entities/CriterionType.java +++ b/navalplanner-business/src/main/java/org/navalplanner/business/resources/entities/CriterionType.java @@ -56,7 +56,7 @@ public class CriterionType extends BaseEntity implements Boolean allowSimultaneousCriterionsPerResource, Boolean enabled, ResourceEnum resource) { - CriterionType criterionType = new CriterionType(); + CriterionType criterionType = create(new CriterionType()); criterionType.name = name; criterionType.description = description; @@ -65,7 +65,6 @@ public class CriterionType extends BaseEntity implements allowSimultaneousCriterionsPerResource; criterionType.enabled = enabled; criterionType.resource = resource; - criterionType.setNewObject(true); return criterionType; @@ -306,10 +305,13 @@ public class CriterionType extends BaseEntity implements Set criterionNames = new HashSet(); for (Criterion c : criterions) { - if (criterionNames.contains(c.getName())) { - return false; + if (!StringUtils.isBlank(c.getName())) { + if (criterionNames.contains(c.getName().toLowerCase())) { + return false; + } else { + criterionNames.add(c.getName().toLowerCase()); + } } - criterionNames.add(c.getName()); } return true; diff --git a/navalplanner-webapp/src/main/java/org/navalplanner/ws/resources/criterion/impl/CriterionConverter.java b/navalplanner-webapp/src/main/java/org/navalplanner/ws/resources/criterion/impl/CriterionConverter.java index ef32aae77..00ef6d99b 100644 --- a/navalplanner-webapp/src/main/java/org/navalplanner/ws/resources/criterion/impl/CriterionConverter.java +++ b/navalplanner-webapp/src/main/java/org/navalplanner/ws/resources/criterion/impl/CriterionConverter.java @@ -24,6 +24,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.List; +import org.apache.commons.lang.StringUtils; import org.navalplanner.business.resources.entities.Criterion; import org.navalplanner.business.resources.entities.CriterionType; import org.navalplanner.ws.common.impl.ResourceEnumConverter; @@ -101,7 +102,8 @@ public final class CriterionConverter { CriterionTypeDTO criterionTypeDTO) { CriterionType criterionType = CriterionType.createUnvalidated( - trim(criterionTypeDTO.name), trim(criterionTypeDTO.description), + StringUtils.trim(criterionTypeDTO.name), + StringUtils.trim(criterionTypeDTO.description), criterionTypeDTO.allowHierarchy, criterionTypeDTO.allowSimultaneousCriterionsPerResource, criterionTypeDTO.enabled, @@ -136,15 +138,12 @@ public final class CriterionConverter { CriterionDTO childDTO, CriterionType criterionType, Criterion criterionParent) { - Criterion criterion = Criterion.createUnvalidated(trim(childDTO.name), + Criterion criterion = Criterion.createUnvalidated( + StringUtils.trim(childDTO.name), criterionType, criterionParent, childDTO.active); return criterion; } - private static String trim(String s) { - return s == null ? null : s.trim(); - } - } diff --git a/navalplanner-webapp/src/main/java/org/navalplanner/ws/resources/criterion/impl/CriterionServiceREST.java b/navalplanner-webapp/src/main/java/org/navalplanner/ws/resources/criterion/impl/CriterionServiceREST.java index 53c20467a..808163236 100644 --- a/navalplanner-webapp/src/main/java/org/navalplanner/ws/resources/criterion/impl/CriterionServiceREST.java +++ b/navalplanner-webapp/src/main/java/org/navalplanner/ws/resources/criterion/impl/CriterionServiceREST.java @@ -36,7 +36,6 @@ import javax.ws.rs.Produces; import org.navalplanner.business.common.exceptions.ValidationException; import org.navalplanner.business.resources.daos.ICriterionTypeDAO; import org.navalplanner.business.resources.entities.CriterionType; -import org.navalplanner.ws.common.api.ConstraintViolationDTO; import org.navalplanner.ws.common.api.InstanceConstraintViolationsDTO; import org.navalplanner.ws.common.api.InstanceConstraintViolationsListDTO; import org.navalplanner.ws.common.impl.ConstraintViolationConverter; @@ -80,74 +79,73 @@ public class CriterionServiceREST implements ICriterionService { int instanceNumber = 1; Set criterionTypeNames = new HashSet(); + /* Process criterion types. */ for (CriterionTypeDTO criterionTypeDTO : criterionTypes.criterionTypes) { - CriterionType criterionType = - CriterionConverter.toEntity(criterionTypeDTO); + /* Convert DTO to entity. */ InstanceConstraintViolationsDTO instanceConstraintViolationsDTO = null; - boolean criterionTypeNameRepeated = - criterionTypeNames.contains(criterionType.getName()); + CriterionType criterionType = + CriterionConverter.toEntity(criterionTypeDTO); + + /* + * Check if the criterion type name is used by another criterion + * type being imported. + */ + if (criterionType.getName() != null && criterionTypeNames.contains( + criterionType.getName().toLowerCase())) { - try { - /* - * "validate" is executed before "save", since "save" first - * adds the object to the underlying ORM session and then - * validates. So, if "validate" method is not called - * explicitly before "save", an invalid criterion type - * would be added to the underlying ORM session, causing - * the invalid criterion type to be added to the database - * when the ORM commits the transaction. As a side effect, - * validations are executed twice. Note also, that - * "CriterionType::checkConstraintUniqueCriterionTypeName" - * only checks if a criterion type with the same name already - * exists in the *database*, and that the criterion types - * being imported are inserted in the database when the - * transaction is committed. In consequence, we can only call - * "save" if the criterion type is valid according to "validate" - * method and its name is not used by another previously - * *imported* (not in the database yet) criterion type. - */ - criterionType.validate(); - if (!criterionTypeNameRepeated) { - criterionTypeDAO.save(criterionType); - } - } catch (ValidationException e) { instanceConstraintViolationsDTO = - ConstraintViolationConverter.toDTO( + InstanceConstraintViolationsDTO.create( Util.generateInstanceId(instanceNumber, criterionTypeDTO.name), - e.getInvalidValues()); - } + _("criterion type name is used by another criterion " + + "type being imported")); - /* - * If criterion type name is repeated, add it to the list of - * constraint violations. - */ - if (criterionTypeNameRepeated) { - if (instanceConstraintViolationsDTO == null) { + } else { + + /* Validate criterion type. */ + try { + + /* + * "validate" is executed before "save", since "save" first + * adds the object to the underlying ORM session and then + * validates. So, if "validate" method is not called + * explicitly before "save", an invalid criterion type + * would be added to the underlying ORM session, causing + * the invalid criterion type to be added to the database + * when the ORM commits the transaction. As a side effect, + * validations are executed twice. Note also, that + * "CriterionType::checkConstraintUniqueCriterionTypeName" + * only checks if a criterion type with the same name + * already exists in the *database*, and that the criterion + * types being imported are inserted in the database when + * the transaction is committed. In consequence, we can only + * call "save" if the criterion type is valid according to + * "validate" method and its name is not used by another + * previously *imported* (not in the database yet) criterion + * type. + */ + criterionType.validate(); + criterionTypeDAO.save(criterionType); + + if (criterionType.getName() != null) { + criterionTypeNames.add(criterionType.getName(). + toLowerCase()); + } + + } catch (ValidationException e) { instanceConstraintViolationsDTO = - new InstanceConstraintViolationsDTO( + ConstraintViolationConverter.toDTO( Util.generateInstanceId(instanceNumber, criterionTypeDTO.name), - new ArrayList()); - } - instanceConstraintViolationsDTO.constraintViolations.add( - new ConstraintViolationDTO( - CriterionType.class.getSimpleName() + ":name", - _("criterion type name is used by another criterion " + - "type being imported"))); - } else { - if (criterionType.getName() != null) { - criterionTypeNames.add(criterionType.getName()); + e.getInvalidValues()); } + } - /* - * Add constraint violations of this criterion type to the returned - * list of constraint violations. - */ + /* Add constraint violations (if any). */ if (instanceConstraintViolationsDTO != null) { instanceConstraintViolationsList.add( instanceConstraintViolationsDTO); diff --git a/navalplanner-webapp/src/test/java/org/navalplanner/web/test/ws/resources/criterion/api/CriterionServiceTest.java b/navalplanner-webapp/src/test/java/org/navalplanner/web/test/ws/resources/criterion/api/CriterionServiceTest.java index 15450ef28..715e95026 100644 --- a/navalplanner-webapp/src/test/java/org/navalplanner/web/test/ws/resources/criterion/api/CriterionServiceTest.java +++ b/navalplanner-webapp/src/test/java/org/navalplanner/web/test/ws/resources/criterion/api/CriterionServiceTest.java @@ -84,7 +84,7 @@ public class CriterionServiceTest { ct1c2Criterions); CriterionDTO ct1c3 = new CriterionDTO("c3", true, new ArrayList()); - CriterionDTO ct1c4 = new CriterionDTO("c3", true, + CriterionDTO ct1c4 = new CriterionDTO(" C3 ", true, new ArrayList()); // Repeated criterion name. List ct1Criterions = new ArrayList(); ct1Criterions.add(ct1c1); @@ -129,18 +129,12 @@ public class CriterionServiceTest { CriterionTypeDTO ct3 = new CriterionTypeDTO(ct3Name, "desc", true, true, true, ResourceEnumDTO.RESOURCE, ct3Criterions); - /* Build criterion type "ct4" (2 constraint violations). */ - CriterionDTO ct4c1 = new CriterionDTO(null, true, // Missing criterion - new ArrayList()); // name. - CriterionDTO ct4c2 = new CriterionDTO("c2", true, - new ArrayList()); - List ct4Criterions = new ArrayList(); - ct4Criterions.add(ct4c1); - ct4Criterions.add(ct4c2); - CriterionTypeDTO ct4 = // Repeated criterion - new CriterionTypeDTO(ct3Name, // type name (see previous - "desc", true, true, true, // criterion type). - ResourceEnumDTO.RESOURCE, ct4Criterions); + /* Build criterion type "ct4" (1 constraint violation). */ + CriterionTypeDTO ct4 = + new CriterionTypeDTO( // Repeated criterion + ' ' + ct3Name.toUpperCase() + ' ', // type name. + "desc", true, true, true, + ResourceEnumDTO.RESOURCE, new ArrayList()); /* Criterion type list. */ List criterionTypes = @@ -155,13 +149,24 @@ public class CriterionServiceTest { new CriterionTypeListDTO(criterionTypes)). instanceConstraintViolationsList; - assertTrue(instanceConstraintViolationsList.size() == 3); - assertTrue(instanceConstraintViolationsList.get(0). + assertTrue( + instanceConstraintViolationsList.toString(), + instanceConstraintViolationsList.size() == 3); + assertTrue( + instanceConstraintViolationsList.get(0). + constraintViolations.toString(), + instanceConstraintViolationsList.get(0). constraintViolations.size() == 4); - assertTrue(instanceConstraintViolationsList.get(1). - constraintViolations.size() == 2); - assertTrue(instanceConstraintViolationsList.get(2). + assertTrue( + instanceConstraintViolationsList.get(1). + constraintViolations.toString(), + instanceConstraintViolationsList.get(1). constraintViolations.size() == 2); + assertTrue( + instanceConstraintViolationsList.get(2). + constraintViolations.toString(), + instanceConstraintViolationsList.get(2). + constraintViolations.size() == 1); /* Find criterion types. */ List returnedCriterionTypes = @@ -205,13 +210,17 @@ public class CriterionServiceTest { transactionService.runOnTransaction( createCriterionType). instanceConstraintViolationsList; - assertTrue(instanceConstraintViolationsList.size() == 0); + assertTrue( + instanceConstraintViolationsList.toString(), + instanceConstraintViolationsList.size() == 0); instanceConstraintViolationsList = transactionService.runOnTransaction( createCriterionType). instanceConstraintViolationsList; - assertTrue(instanceConstraintViolationsList.size() == 1); + assertTrue( + instanceConstraintViolationsList.toString(), + instanceConstraintViolationsList.size() == 1); } diff --git a/scripts/rest-clients/criterion-types-sample.xml b/scripts/rest-clients/criterion-types-sample.xml index 337f946d6..4006f58f3 100644 --- a/scripts/rest-clients/criterion-types-sample.xml +++ b/scripts/rest-clients/criterion-types-sample.xml @@ -21,7 +21,7 @@ - + @@ -59,14 +59,17 @@ - - + + allow-hierarchy="false" + allow-simultaneous-criterions-per-resource="false" enabled="false" + resource="WORKER"/>