From 7415219fde4a35cb77ae7cb43df46498d27e4ebb Mon Sep 17 00:00:00 2001 From: Alex Nikonov Date: Fri, 11 Oct 2024 09:34:35 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20unknown=20values=20must=20raise=20error?= =?UTF-8?q?=20for=20boolean=20fields=20instead=20of=20be=E2=80=A6=20(#51)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: unknown values must raise error for boolean fields instead of being treated as 'false' Refs: #43 --- .../excel_importer/service/ImportService.java | 38 ++++++--- .../service/ImportServiceTest.java | 85 ++++++++++++++++++- 2 files changed, 107 insertions(+), 16 deletions(-) diff --git a/src/main/java/ch/sbb/polarion/extension/excel_importer/service/ImportService.java b/src/main/java/ch/sbb/polarion/extension/excel_importer/service/ImportService.java index ffd90a8..2ee3b25 100644 --- a/src/main/java/ch/sbb/polarion/extension/excel_importer/service/ImportService.java +++ b/src/main/java/ch/sbb/polarion/extension/excel_importer/service/ImportService.java @@ -12,6 +12,7 @@ import com.polarion.core.util.StringUtils; import com.polarion.subterra.base.data.model.IType; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.VisibleForTesting; import java.util.ArrayList; import java.util.List; @@ -122,6 +123,7 @@ private void fillWorkItemFields(IWorkItem workItem, Map mappingR // However, it must be saved to the newly created work item otherwise sequential imports will produce several objects. if (fieldId != null && !fieldId.equals(IUniqueObject.KEY_ID) && (!fieldId.equals(linkColumnId) || !workItem.isPersisted()) && (model.isOverwriteWithEmpty() || !isEmpty(value)) && + ensureValidValue(fieldId, value, fieldMetadataSet) && existingValueDiffers(workItem, fieldId, value, fieldMetadataSet)) { polarionServiceExt.setFieldValue(workItem, fieldId, value, model.getEnumsMapping()); } else if (fieldId != null && fieldId.equals(IUniqueObject.KEY_ID) && !fieldId.equals(linkColumnId)) { @@ -131,18 +133,25 @@ private void fillWorkItemFields(IWorkItem workItem, Map mappingR }); } + @VisibleForTesting + boolean ensureValidValue(String fieldId, Object value, Set fieldMetadataSet) { + FieldMetadata fieldMetadata = getFieldMetadataForField(fieldMetadataSet, fieldId); + if (FieldType.BOOLEAN.getType().equals(fieldMetadata.getType()) && + (!(value instanceof String) || !("true".equalsIgnoreCase((String) value) || "false".equalsIgnoreCase((String) value)))) { + throw new IllegalArgumentException(String.format("'%s' isn't a valid boolean value", value == null ? "" : value)); + } + return true; + } + /** * Here wy try to make a preliminary check whether the value differs with the existing one. * This can be useful coz Polarion sometimes makes update in case when it isn't needed (e.g. if you try to * set false to the boolean field which already has this value Polarion will rewrite the value and increment revision). */ - @SuppressWarnings("java:S1125") //will be improve later - private boolean existingValueDiffers(IWorkItem workItem, String fieldId, Object newValue, Set fieldMetadataSet) { - FieldMetadata fieldMetadata = fieldMetadataSet.stream() - .filter(m -> m.getId().equals(fieldId)) - .findFirst() - .orElseThrow(() -> new IllegalArgumentException("Cannot find field metadata for ID '%s'".formatted(fieldId))); - + @SuppressWarnings("java:S1125") //will be improved later + @VisibleForTesting + boolean existingValueDiffers(IWorkItem workItem, String fieldId, Object newValue, Set fieldMetadataSet) { + FieldMetadata fieldMetadata = getFieldMetadataForField(fieldMetadataSet, fieldId); Object existingValue = polarionServiceExt.getFieldValue(workItem, fieldId); if (existingValue == null && newValue == null) { return false; @@ -150,12 +159,8 @@ private boolean existingValueDiffers(IWorkItem workItem, String fieldId, Object IType fieldType = fieldMetadata.getType(); if (FieldType.BOOLEAN.getType().equals(fieldType)) { - if (newValue instanceof String value) { - //later generic will treat all nonsense values (e.g. 'qwe', 'yes' etc.) as false so we can do it here in advance - newValue = Boolean.valueOf(value); - } - //treat nulls as false values for both new and existing values - return !Objects.equals(existingValue == null ? false : existingValue, newValue == null ? false : newValue); + newValue = Boolean.valueOf((String) newValue); // validator that had been running before must ensure that the new value is a proper string + return !Objects.equals(existingValue == null ? false : existingValue, newValue); } else if (FieldType.FLOAT.getType().equals(fieldType)) { //WORKAROUND: converting to string helps to find same values even between different types (Float, Double etc.) return !Objects.equals(String.valueOf(newValue), String.valueOf(existingValue)); @@ -164,6 +169,13 @@ private boolean existingValueDiffers(IWorkItem workItem, String fieldId, Object } } + private FieldMetadata getFieldMetadataForField(Set fieldMetadataSet, String fieldId) { + return fieldMetadataSet.stream() + .filter(m -> m.getId().equals(fieldId)) + .findFirst() + .orElseThrow(() -> new IllegalArgumentException("Cannot find field metadata for ID '%s'".formatted(fieldId))); + } + private boolean isEmpty(Object value) { return value == null || (value instanceof String stringValue && StringUtils.isEmptyTrimmed(stringValue)); } diff --git a/src/test/java/ch/sbb/polarion/extension/excel_importer/service/ImportServiceTest.java b/src/test/java/ch/sbb/polarion/extension/excel_importer/service/ImportServiceTest.java index 8169f38..ce8659d 100644 --- a/src/test/java/ch/sbb/polarion/extension/excel_importer/service/ImportServiceTest.java +++ b/src/test/java/ch/sbb/polarion/extension/excel_importer/service/ImportServiceTest.java @@ -2,6 +2,7 @@ import ch.sbb.polarion.extension.excel_importer.settings.ExcelSheetMappingSettingsModel; import ch.sbb.polarion.extension.generic.fields.FieldType; +import ch.sbb.polarion.extension.generic.fields.model.FieldMetadata; import com.polarion.alm.projects.IProjectService; import com.polarion.alm.shared.api.transaction.RunnableInWriteTransaction; import com.polarion.alm.shared.api.transaction.TransactionalExecutor; @@ -33,10 +34,10 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.stream.Stream; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.*; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.*; @@ -144,7 +145,7 @@ void testSuccessfulImport() { Map map = new HashMap<>(); map.put("A", "a1"); map.put("B", "b1"); - map.put("C", "c1"); + map.put("C", "false"); map.put("D", null); //test using disabled 'overwriteWithEmpty' @@ -259,6 +260,84 @@ void testImportViaId() { } } + @Test + void testEnsureValidValue() { + ImportService service = new ImportService(mock(PolarionServiceExt.class)); + + IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, () -> service.ensureValidValue("someField", "true", Set.of())); + assertEquals("Cannot find field metadata for ID 'someField'", exception.getMessage()); + + Set metadataSet = Set.of(FieldMetadata.builder() + .id("someField") + .type(FieldType.BOOLEAN.getType()) + .build()); + assertTrue(service.ensureValidValue("someField", "true", metadataSet)); + assertTrue(service.ensureValidValue("someField", "True", metadataSet)); + assertTrue(service.ensureValidValue("someField", "FALSE", metadataSet)); + + exception = assertThrows(IllegalArgumentException.class, () -> service.ensureValidValue("someField", "FALSE ", metadataSet)); + assertEquals("'FALSE ' isn't a valid boolean value", exception.getMessage()); + + exception = assertThrows(IllegalArgumentException.class, () -> service.ensureValidValue("someField", 42, metadataSet)); + assertEquals("'42' isn't a valid boolean value", exception.getMessage()); + + exception = assertThrows(IllegalArgumentException.class, () -> service.ensureValidValue("someField", null, metadataSet)); + assertEquals("'' isn't a valid boolean value", exception.getMessage()); + } + + @Test + void testExistingValueDiffers() { + PolarionServiceExt polarionService = mock(PolarionServiceExt.class); + ImportService service = new ImportService(polarionService); + IWorkItem workItem = mock(IWorkItem.class); + + IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, + () -> service.existingValueDiffers(workItem, "unknownFieldId", "someValue", Set.of())); + assertEquals("Cannot find field metadata for ID 'unknownFieldId'", exception.getMessage()); + + Set stringMetadata = Set.of(FieldMetadata.builder().id("fieldId").type(FieldType.STRING.getType()).build()); + + when(polarionService.getFieldValue(workItem, "fieldId")).thenReturn(null); + assertFalse(service.existingValueDiffers(workItem, "fieldId", null, stringMetadata)); + assertTrue(service.existingValueDiffers(workItem, "fieldId", "someValue", stringMetadata)); + assertTrue(service.existingValueDiffers(workItem, "fieldId", "", stringMetadata)); + + when(polarionService.getFieldValue(workItem, "fieldId")).thenReturn(""); + assertTrue(service.existingValueDiffers(workItem, "fieldId", null, stringMetadata)); + + when(polarionService.getFieldValue(workItem, "fieldId")).thenReturn("someValue"); + assertFalse(service.existingValueDiffers(workItem, "fieldId", "someValue", stringMetadata)); + + when(polarionService.getFieldValue(workItem, "fieldId")).thenReturn("someValue"); + assertTrue(service.existingValueDiffers(workItem, "fieldId", "someValue ", stringMetadata)); + + Set booleanMetadata = Set.of(FieldMetadata.builder().id("fieldId").type(FieldType.BOOLEAN.getType()).build()); + when(polarionService.getFieldValue(workItem, "fieldId")).thenReturn(null); + assertFalse(service.existingValueDiffers(workItem, "fieldId", null, booleanMetadata)); + assertFalse(service.existingValueDiffers(workItem, "fieldId", "someValue", booleanMetadata)); // unrealistic scenario + assertFalse(service.existingValueDiffers(workItem, "fieldId", "", booleanMetadata)); // unrealistic scenario + + when(polarionService.getFieldValue(workItem, "fieldId")).thenReturn(false); + assertTrue(service.existingValueDiffers(workItem, "fieldId", "true", booleanMetadata)); + assertFalse(service.existingValueDiffers(workItem, "fieldId", null, booleanMetadata)); + assertFalse(service.existingValueDiffers(workItem, "fieldId", "FALSE", booleanMetadata)); + + // boolean fields can hold null values, they are treated as having 'false' value + when(polarionService.getFieldValue(workItem, "fieldId")).thenReturn(null); + assertTrue(service.existingValueDiffers(workItem, "fieldId", "true", booleanMetadata)); + assertFalse(service.existingValueDiffers(workItem, "fieldId", "false", booleanMetadata)); + + Set floatMetadata = Set.of(FieldMetadata.builder().id("fieldId").type(FieldType.FLOAT.getType()).build()); + when(polarionService.getFieldValue(workItem, "fieldId")).thenReturn(null); + assertFalse(service.existingValueDiffers(workItem, "fieldId", null, floatMetadata)); + assertTrue(service.existingValueDiffers(workItem, "fieldId", 0f, floatMetadata)); + + when(polarionService.getFieldValue(workItem, "fieldId")).thenReturn(42f); + assertFalse(service.existingValueDiffers(workItem, "fieldId", "42.0", floatMetadata)); + assertTrue(service.existingValueDiffers(workItem, "fieldId", "42", floatMetadata)); + assertTrue(service.existingValueDiffers(workItem, "fieldId", null, floatMetadata)); + } + private ExcelSheetMappingSettingsModel generateSettings(boolean overwriteWithEmpty) { ExcelSheetMappingSettingsModel model = new ExcelSheetMappingSettingsModel(); model.setLinkColumn("B");