diff --git a/java/com.sap.sse.common/src/com/sap/sse/common/settings/generic/converter/BooleanConverter.java b/java/com.sap.sse.common/src/com/sap/sse/common/settings/generic/converter/BooleanConverter.java index e7ac0e80900..2f27088e1df 100644 --- a/java/com.sap.sse.common/src/com/sap/sse/common/settings/generic/converter/BooleanConverter.java +++ b/java/com.sap.sse.common/src/com/sap/sse/common/settings/generic/converter/BooleanConverter.java @@ -28,7 +28,14 @@ public class BooleanConverter implements ValueConverter { @Override public Boolean fromStringValue(String stringValue) { - return stringValue == null ? null : Boolean.valueOf(stringValue); + // Boolean.valueOf(stringValue) is explicitly not used here + // to ensure consistency to JSON value handling + if (Boolean.TRUE.toString().equalsIgnoreCase(stringValue)) { + return true; + } else if (Boolean.FALSE.toString().equalsIgnoreCase(stringValue)) { + return false; + } + throw new IllegalArgumentException("\"" + stringValue + "\" can not be parsed as boolean value"); } @Override diff --git a/java/com.sap.sse.common/src/com/sap/sse/common/settings/serializer/AbstractSettingsToJsonSerializer.java b/java/com.sap.sse.common/src/com/sap/sse/common/settings/serializer/AbstractSettingsToJsonSerializer.java index 828a0ffcec5..fe74be0457c 100644 --- a/java/com.sap.sse.common/src/com/sap/sse/common/settings/serializer/AbstractSettingsToJsonSerializer.java +++ b/java/com.sap.sse.common/src/com/sap/sse/common/settings/serializer/AbstractSettingsToJsonSerializer.java @@ -4,6 +4,8 @@ import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Map; +import java.util.logging.Level; +import java.util.logging.Logger; import com.sap.sse.common.Util; import com.sap.sse.common.settings.Settings; @@ -28,6 +30,8 @@ import com.sap.sse.common.settings.generic.ValueSetting; * JSON Array type */ public abstract class AbstractSettingsToJsonSerializer { + + private static final Logger LOG = Logger.getLogger(AbstractSettingsToJsonSerializer.class.getName()); protected abstract OBJECT newOBJECT(); @@ -241,7 +245,13 @@ public abstract class AbstractSettingsToJsonSerializer { } private void deserializeValueSetting(Object jsonValue, ValueSetting valueSetting) { - valueSetting.setValue(valueSetting.getValueConverter().fromJSONValue(jsonValue)); + final ValueConverter converter = valueSetting.getValueConverter(); + try { + valueSetting.setValue(converter.fromJSONValue(jsonValue)); + } catch(Exception e) { + LOG.log(Level.WARNING, "Error while converting JSON value \"" + jsonValue + "\" using converter \"" + + converter.getClass().getSimpleName() + "\"", e); + } } private void deserializeListSetting(Object jsonValue, CollectionSetting listSetting) { @@ -295,7 +305,12 @@ public abstract class AbstractSettingsToJsonSerializer { List values = new ArrayList<>(); if(jsonArray != null) { for (Object value : fromJsonArray(jsonArray)) { - values.add(converter.fromJSONValue(value)); + try { + values.add(converter.fromJSONValue(value)); + } catch(Exception e) { + LOG.log(Level.WARNING, "Error while converting JSON value \"" + value + "\" using converter \"" + + converter.getClass().getSimpleName() + "\"", e); + } } } return values; diff --git a/java/com.sap.sse.common/src/com/sap/sse/common/settings/serializer/SettingsToStringMapSerializer.java b/java/com.sap.sse.common/src/com/sap/sse/common/settings/serializer/SettingsToStringMapSerializer.java index b5f0222c39f..995b3777666 100644 --- a/java/com.sap.sse.common/src/com/sap/sse/common/settings/serializer/SettingsToStringMapSerializer.java +++ b/java/com.sap.sse.common/src/com/sap/sse/common/settings/serializer/SettingsToStringMapSerializer.java @@ -8,6 +8,8 @@ import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Map.Entry; +import java.util.logging.Level; +import java.util.logging.Logger; import java.util.Set; import java.util.TreeMap; @@ -29,6 +31,8 @@ import com.sap.sse.common.settings.generic.ValueSetting; */ public class SettingsToStringMapSerializer { + private static final Logger LOG = Logger.getLogger(SettingsToStringMapSerializer.class.getName()); + private static final String ADDED_SUFFIX = GenericSerializableSettings.PATH_SEPARATOR + GenericSerializableSettings.ADDED_TOKEN; private static final String REMOVED_SUFFIX = GenericSerializableSettings.PATH_SEPARATOR + GenericSerializableSettings.REMOVED_TOKEN; @@ -177,7 +181,14 @@ public class SettingsToStringMapSerializer { // Additional values are currently silently skipped. // This should only happen if somebody manipulates the data or a List setting is changed to a single setting // value. - valueSetting.setValue(valueSetting.getValueConverter().fromStringValue(iterator.next())); + final ValueConverter valueConverter = valueSetting.getValueConverter(); + final String stringValue = iterator.next(); + try { + valueSetting.setValue(valueConverter.fromStringValue(stringValue)); + } catch(Exception e) { + LOG.log(Level.WARNING, "Error while converting String value \"" + stringValue + "\" using converter \"" + + valueConverter.getClass().getSimpleName() + "\"", e); + } } } @@ -197,7 +208,12 @@ public class SettingsToStringMapSerializer { List deserializedValues = new ArrayList<>(); if(values != null) { for (String stringValue : values) { - deserializedValues.add(valueConverter.fromStringValue(stringValue)); + try { + deserializedValues.add(valueConverter.fromStringValue(stringValue)); + } catch(Exception e) { + LOG.log(Level.WARNING, "Error while converting String value \"" + stringValue + "\" using converter \"" + + valueConverter.getClass().getSimpleName() + "\"", e); + } } } return deserializedValues; diff --git a/java/com.sap.sse.shared.android.test/src/com/sap/sse/shared/android/test/AbstractSettingsSerializationTest.java b/java/com.sap.sse.shared.android.test/src/com/sap/sse/shared/android/test/AbstractSettingsSerializationTest.java index 8e1276bf8d0..fe0c73d962b 100644 --- a/java/com.sap.sse.shared.android.test/src/com/sap/sse/shared/android/test/AbstractSettingsSerializationTest.java +++ b/java/com.sap.sse.shared.android.test/src/com/sap/sse/shared/android/test/AbstractSettingsSerializationTest.java @@ -177,7 +177,7 @@ public abstract class AbstractSettingsSerializationTest { @Override protected void addChildSettings() { - disallowedKey= new StringSetting("disallowed.key", this); + disallowedKey = new StringSetting("disallowed.key", this); } } diff --git a/java/com.sap.sse.shared.android.test/src/com/sap/sse/shared/android/test/AbstractSettingsSerializationTestWithSettingsMap.java b/java/com.sap.sse.shared.android.test/src/com/sap/sse/shared/android/test/AbstractSettingsSerializationTestWithSettingsMap.java index 1c640993a4c..85420049558 100644 --- a/java/com.sap.sse.shared.android.test/src/com/sap/sse/shared/android/test/AbstractSettingsSerializationTestWithSettingsMap.java +++ b/java/com.sap.sse.shared.android.test/src/com/sap/sse/shared/android/test/AbstractSettingsSerializationTestWithSettingsMap.java @@ -2,17 +2,69 @@ package com.sap.sse.shared.android.test; import static org.junit.Assert.assertEquals; +import java.util.Arrays; +import java.util.Collection; import java.util.HashMap; import java.util.Map; import org.junit.Test; +import com.sap.sse.common.filter.TextOperator; +import com.sap.sse.common.filter.TextOperator.Operators; import com.sap.sse.common.settings.Settings; +import com.sap.sse.common.settings.generic.AbstractGenericSerializableSettings; +import com.sap.sse.common.settings.generic.BooleanSetting; +import com.sap.sse.common.settings.generic.DecimalSetting; +import com.sap.sse.common.settings.generic.EnumSetSetting; +import com.sap.sse.common.settings.generic.EnumSetting; import com.sap.sse.common.settings.generic.GenericSerializableSettings; import com.sap.sse.common.settings.generic.SettingsMap; +import com.sap.sse.common.settings.generic.StringSetSetting; +import com.sap.sse.common.settings.generic.StringSetting; public abstract class AbstractSettingsSerializationTestWithSettingsMap extends AbstractSettingsSerializationTest { - + private static class UntypedSettings extends AbstractGenericSerializableSettings { + private static final long serialVersionUID = -2265305217290147424L; + private transient StringSetting enumSetting; + private transient StringSetSetting enumSetSetting; + private transient StringSetting numberSetting; + private transient StringSetting booleanSetting; + + public UntypedSettings(String enumValue, Collection enumValues, String numberValue, String booleanValue) { + enumSetting.setValue(enumValue); + enumSetSetting.setValues(enumValues); + numberSetting.setValue(numberValue); + booleanSetting.setValue(booleanValue); + } + + @Override + protected void addChildSettings() { + enumSetting = new StringSetting("enumSetting", this); + enumSetSetting = new StringSetSetting("enumSetSetting", this); + numberSetting = new StringSetting("numberSetting", this); + booleanSetting = new StringSetting("booleanSetting", this); + } + } + + private static class TypedSettings extends AbstractGenericSerializableSettings { + private static final long serialVersionUID = -2265305217290147424L; + @SuppressWarnings("unused") + private transient EnumSetting enumSetting; + private transient EnumSetSetting enumSetSetting; + @SuppressWarnings("unused") + private transient DecimalSetting numberSetting; + @SuppressWarnings("unused") + private transient BooleanSetting booleanSetting; + + @Override + protected void addChildSettings() { + enumSetting = new EnumSetting<>("enumSetting", this, TextOperator.Operators::valueOf); + enumSetSetting = new EnumSetSetting<>("enumSetSetting", this, TextOperator.Operators::valueOf); + numberSetting = new DecimalSetting("numberSetting", this); + booleanSetting = new BooleanSetting("booleanSetting", this); + } + } + @Test public void testSettingsMapWithOneSerializableSettings() throws Exception { final GenericSerializableSettings settingsValues = createTestSettingsWithValues(); @@ -245,4 +297,22 @@ public abstract class AbstractSettingsSerializationTestWithSettingsMap exte } return new SettingsMapImpl(innerMap); } + + /** + * Regression test for Bug 4383. + * When the serialized settings contain values that can't be converted to the setting + * type, those values may not break the deserialization. If a user e.g. manipulates the URL, the page may not stay + * empty because of an internal exception. In this case, the wrong values should be ignored instead but all other + * values must be deserialized. + */ + @Test + public void testIllegalValuesInSerializedSettings() throws Exception { + UntypedSettings untypedSettings = new UntypedSettings("WrongEnumValue", Arrays.asList("Contains", "WrongEnumValue"), "noNumber", "noBoolean"); + SOT serialized = serialize(untypedSettings); + TypedSettings deserialized = deserialize(serialized, new TypedSettings()); + TypedSettings expected = new TypedSettings(); + // this is the only value that will be correctly deserialized + expected.enumSetSetting.addValue(Operators.Contains); + assertEquals(expected, deserialized); + } }