Github user HansBrende commented on a diff in the pull request:
https://github.com/apache/any23/pull/122#discussion_r226702275
--- Diff: api/src/main/java/org/apache/any23/configuration/Setting.java ---
@@ -118,7 +128,22 @@ private Type getValueType() {
}
}
- protected abstract V checkedValue(Setting<V> original, V newValue)
throws Exception;
+ /**
+ * Subclasses may override this method to check that new settings
for this key are valid,
+ * and/or to decorate new setting values, using, for example,
{@link Collections#unmodifiableList(List)}.
+ * The default implementation of this method throws a {@link
NullPointerException} if the new value is null and the initial value was
non-null.
+ *
+ * @param initial the setting containing the initial value for
this key, or null if the setting has not yet been initialized
+ * @param newValue the new value for this setting
+ * @return the new value for this setting
+ * @throws Exception if the new value for this setting was invalid
+ */
+ protected V checkedValue(Setting<V> initial, V newValue) throws
Exception {
+ if (newValue == null && initial != null && initial.value !=
null) {
+ throw new NullPointerException();
+ }
+ return newValue;
+ }
--- End diff --
Actually, we should not allow keys to decorate values. Consider the
following scenario: user copies the value from one setting into another
setting. Now the key is decorating a value that has *already been decorated*.
This could lead to an unfortunate chain of, e.g.,
```
Collections.unmodifiableList(Collections.unmodifiableList(Collections.unmodifiableList(...
)))
```
Therefore, any decorating should happen *before* the setting is created,
and if the value is not appropriately decorated, the key should throw an
exception in the value check.
Also we should change this method's signature to:
```
protected void checkValue(Setting<V> initial, V newValue) throws Exception
```
---
