Skip to content

Commit

Permalink
fix: unknown values must raise error for boolean fields instead of be… (
Browse files Browse the repository at this point in the history 
#51)

* fix: unknown values must raise error for boolean fields instead of being treated as 'false'

Refs: #43
  • Loading branch information
@Jumas
Jumas authored Oct 11, 2024
1 parent dbc370d commit 7415219
Show file tree
Hide file tree
Showing 2 changed files with 107 additions and 16 deletions.
38 changes: 25 additions & 13 deletions src/main/java/ch/sbb/polarion/extension/excel_importer/service/ImportService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -122,6 +123,7 @@ private void fillWorkItemFields(IWorkItem workItem, Map<String, Object> 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)) {
Expand All @@ -131,31 +133,34 @@ private void fillWorkItemFields(IWorkItem workItem, Map<String, Object> mappingR
});
}

@VisibleForTesting
boolean ensureValidValue(String fieldId, Object value, Set<FieldMetadata> 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<FieldMetadata> 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<FieldMetadata> fieldMetadataSet) {
FieldMetadata fieldMetadata = getFieldMetadataForField(fieldMetadataSet, fieldId);
Object existingValue = polarionServiceExt.getFieldValue(workItem, fieldId);
if (existingValue == null && newValue == null) {
return false;
}

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));
Expand All @@ -164,6 +169,13 @@ private boolean existingValueDiffers(IWorkItem workItem, String fieldId, Object
}
}

private FieldMetadata getFieldMetadataForField(Set<FieldMetadata> 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));
}
Expand Down
85 changes: 82 additions & 3 deletions src/test/java/ch/sbb/polarion/extension/excel_importer/service/ImportServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.*;
Expand Down Expand Up @@ -144,7 +145,7 @@ void testSuccessfulImport() {
Map<String, Object> 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'
Expand Down Expand Up @@ -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<FieldMetadata> 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<FieldMetadata> 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<FieldMetadata> 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<FieldMetadata> 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");
Expand Down

0 comments on commit 7415219

Please sign in to comment.