From 66055a4a4082b5612565f107833ba203c7d2ff37 Mon Sep 17 00:00:00 2001 From: Steffen Schaefer Date: Wed, 6 Dec 2017 09:56:51 +0100 Subject: [PATCH 1/5] Bug 4383: Added failing test --- .../AbstractSettingsSerializationTest.java | 65 ++++++++++++++++++- 1 file changed, 64 insertions(+), 1 deletion(-) 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..386a095a679 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 @@ -4,6 +4,7 @@ import static org.junit.Assert.assertEquals; import java.math.BigDecimal; import java.util.Arrays; +import java.util.Collection; import org.junit.Test; @@ -15,9 +16,11 @@ import com.sap.sse.common.settings.generic.BooleanSetting; import com.sap.sse.common.settings.generic.DecimalListSetting; import com.sap.sse.common.settings.generic.DecimalSetting; import com.sap.sse.common.settings.generic.EnumListSetting; +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.SettingsList; +import com.sap.sse.common.settings.generic.StringSetSetting; import com.sap.sse.common.settings.generic.StringSetting; import com.sap.sse.common.settings.generic.StringToEnumConverter; @@ -177,7 +180,49 @@ public abstract class AbstractSettingsSerializationTest { @Override protected void addChildSettings() { - disallowedKey= new StringSetting("disallowed.key", this); + disallowedKey = new StringSetting("disallowed.key", this); + } + } + + 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); } } @@ -292,4 +337,22 @@ public abstract class AbstractSettingsSerializationTest { final TestListSettings deserializedSettings = serializeAndDeserialize(settings); assertEquals(settings, deserializedSettings); } + + /** + * 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); + } } From 2c8784f1b9c7a769a4d05874aa77cf0138df7f55 Mon Sep 17 00:00:00 2001 From: Steffen Schaefer Date: Wed, 6 Dec 2017 09:59:01 +0100 Subject: [PATCH 2/5] Bug 4383: Improved error handling in settings deserialization if a value can not be converted to the target type --- .../AbstractSettingsToJsonSerializer.java | 19 ++++++++++++++++-- .../SettingsToStringMapSerializer.java | 20 +++++++++++++++++-- 2 files changed, 35 insertions(+), 4 deletions(-) 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..705b6b20200 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(AbstractSettingsToJsonSerializer.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; From ab54bcf3d5dc90a43c7864946979dcb70b4db7a2 Mon Sep 17 00:00:00 2001 From: Steffen Schaefer Date: Wed, 6 Dec 2017 13:42:26 +0100 Subject: [PATCH 3/5] Bug 4383: Adjusted URL serialization of Boolean values to be consistent to the behavior of JSON serialization --- .../settings/generic/converter/BooleanConverter.java | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) 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 From ba4e9cd37834997e8b002a933f7df183cf1efb2b Mon Sep 17 00:00:00 2001 From: Steffen Schaefer Date: Wed, 6 Dec 2017 13:42:51 +0100 Subject: [PATCH 4/5] Bug 4383: Using correct logger --- .../settings/serializer/SettingsToStringMapSerializer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 705b6b20200..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 @@ -31,7 +31,7 @@ import com.sap.sse.common.settings.generic.ValueSetting; */ public class SettingsToStringMapSerializer { - private static final Logger LOG = Logger.getLogger(AbstractSettingsToJsonSerializer.class.getName()); + 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; From cd313c598843f92b557d2f9246612360ffad56bc Mon Sep 17 00:00:00 2001 From: Steffen Schaefer Date: Wed, 6 Dec 2017 13:44:27 +0100 Subject: [PATCH 5/5] Bug 4383: Moved the new test because it isn't compatible with Java serialization and the problem won't occur there --- .../AbstractSettingsSerializationTest.java | 63 ---------------- ...tingsSerializationTestWithSettingsMap.java | 72 ++++++++++++++++++- 2 files changed, 71 insertions(+), 64 deletions(-) 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 386a095a679..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 @@ -4,7 +4,6 @@ import static org.junit.Assert.assertEquals; import java.math.BigDecimal; import java.util.Arrays; -import java.util.Collection; import org.junit.Test; @@ -16,11 +15,9 @@ import com.sap.sse.common.settings.generic.BooleanSetting; import com.sap.sse.common.settings.generic.DecimalListSetting; import com.sap.sse.common.settings.generic.DecimalSetting; import com.sap.sse.common.settings.generic.EnumListSetting; -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.SettingsList; -import com.sap.sse.common.settings.generic.StringSetSetting; import com.sap.sse.common.settings.generic.StringSetting; import com.sap.sse.common.settings.generic.StringToEnumConverter; @@ -183,48 +180,6 @@ public abstract class AbstractSettingsSerializationTest { disallowedKey = new StringSetting("disallowed.key", this); } } - - 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); - } - } protected abstract SOT serialize(T settings) throws Exception; @@ -337,22 +292,4 @@ public abstract class AbstractSettingsSerializationTest { final TestListSettings deserializedSettings = serializeAndDeserialize(settings); assertEquals(settings, deserializedSettings); } - - /** - * 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); - } } 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); + } }