XSTR-773 implementation

from Groostav/XStream-GG#2

xstream/src/java/com/thoughtworks/xstream/XStream.java

@@ -870,40 +870,39 @@ protected void setupImmutableTypes() {
             return;
         }
 
-        // primitives are always immutable
-        addImmutableType(boolean.class);
-        addImmutableType(Boolean.class);
-        addImmutableType(byte.class);
-        addImmutableType(Byte.class);
-        addImmutableType(char.class);
-        addImmutableType(Character.class);
-        addImmutableType(double.class);
-        addImmutableType(Double.class);
-        addImmutableType(float.class);
-        addImmutableType(Float.class);
-        addImmutableType(int.class);
-        addImmutableType(Integer.class);
-        addImmutableType(long.class);
-        addImmutableType(Long.class);
-        addImmutableType(short.class);
-        addImmutableType(Short.class);
+        // primitives are always immutable                  //retainPathsOnDeserialization
+        addImmutableType(boolean.class,                     false);
+        addImmutableType(Boolean.class,                     false);
+        addImmutableType(byte.class,                        false);
+        addImmutableType(Byte.class,                        false);
+        addImmutableType(char.class,                        false);
+        addImmutableType(double.class,                      false);
+        addImmutableType(Double.class,                      false);
+        addImmutableType(float.class,                       false);
+        addImmutableType(Float.class,                       false);
+        addImmutableType(int.class,                         false);
+        addImmutableType(Integer.class,                     false);
+        addImmutableType(long.class,                        false);
+        addImmutableType(Long.class,                        false);
+        addImmutableType(short.class,                       false);
+        addImmutableType(Short.class,                       false);
 
         // additional types
-        addImmutableType(Mapper.Null.class);
-        addImmutableType(BigDecimal.class);
-        addImmutableType(BigInteger.class);
-        addImmutableType(String.class);
-        addImmutableType(Charset.class);
-        addImmutableType(Currency.class);
-        addImmutableType(URI.class);
-        addImmutableType(URL.class);
-        addImmutableType(File.class);
-        addImmutableType(Class.class);
-        addImmutableType(UUID.class);
-
-        addImmutableType(Collections.EMPTY_LIST.getClass());
-        addImmutableType(Collections.EMPTY_SET.getClass());
-        addImmutableType(Collections.EMPTY_MAP.getClass());
+        addImmutableType(Mapper.Null.class,                 false);
+        addImmutableType(BigDecimal.class,                  false);
+        addImmutableType(BigInteger.class,                  false);
+        addImmutableType(String.class,                      false);
+        addImmutableType(Charset.class,                     true);
+        addImmutableType(Currency.class,                    true);
+        addImmutableType(URI.class,                         false);
+        addImmutableType(URL.class,                         false);
+        addImmutableType(File.class,                        false);
+        addImmutableType(Class.class,                       false);
+        addImmutableType(UUID.class,                        true);
+
+        addImmutableType(Collections.EMPTY_LIST.getClass(), false);
+        addImmutableType(Collections.EMPTY_SET.getClass(),  false);
+        addImmutableType(Collections.EMPTY_MAP.getClass(),  false);
 
         if (JVM.isAWTAvailable()) {
             addImmutableTypeDynamically("java.awt.font.TextAttribute");
@@ -1328,16 +1327,42 @@ public void addDefaultImplementation(final Class<?> defaultImplementation, final
     }
 
     /**
-     * Add immutable types. The value of the instances of these types will always be written into the stream even if
-     * they appear multiple times.
-     * 
+     * Register an immutable type. Instances of immutable types will always be written
+     * into the stream (as a full document instead of a reference) even if they appear multiple times.
+     *
+     * <p>A reference map will be built at unmarshalling time anyways so that documents of an
+     * earlier version (where the <tt>type</tt> was <i>not</i> registered as immutable) will
+     * still be deserialized properly. Use {@link #addImmutableType(Class, boolean)}
+     * if this is not desired.
+     *
+     * @throws InitializationException if no {@link ImmutableTypesMapper} is available
+     * @throws IllegalArgumentException if <code>type</code> is <tt>null</tt>
+     */
+    public void addImmutableType(Class type) {
+        addImmutableType(type, true);
+    }
+
+    /**
+     * Register an immutable type. Instances of immutable types will always be written
+     * into the stream (as a full document instead of a reference) even if they appear multiple times.
+     *
+     * <p>Documents containing reference-paths to the specified immutable type will continue to
+     * deserialize if <code>canBeReferencedByPath</code> is <tt>true</tt>, else no reference paths
+     * will be kept as the document is unmarshalled,
+     * saving memory but breaking those existing documents.</p>
+     *
      * @throws InitializationException if no {@link ImmutableTypesMapper} is available
+     * @throws IllegalArgumentException if <code>type</code> is <tt>null</tt>
+     * @since 1.4.9
      */
-    public void addImmutableType(final Class<?> type) {
+    public void addImmutableType(Class type, boolean canBeReferencedByPath) {
+        if(type == null) { throw new IllegalArgumentException("type"); }
         if (immutableTypesMapper == null) {
-            throw new InitializationException("No " + ImmutableTypesMapper.class.getName() + " available");
+            throw new com.thoughtworks.xstream.InitializationException("No "
+                    + ImmutableTypesMapper.class.getName()
+                    + " available");
         }
-        immutableTypesMapper.addImmutableType(type);
+        immutableTypesMapper.addImmutableType(type, canBeReferencedByPath);
     }
 
     /**

xstream/src/java/com/thoughtworks/xstream/core/AbstractReferenceMarshaller.java

@@ -10,8 +10,6 @@
  */
 package com.thoughtworks.xstream.core;
 
-import java.util.Iterator;
-
 import com.thoughtworks.xstream.converters.ConversionException;
 import com.thoughtworks.xstream.converters.Converter;
 import com.thoughtworks.xstream.converters.ConverterLookup;
@@ -23,6 +21,8 @@
 import com.thoughtworks.xstream.io.path.PathTrackingWriter;
 import com.thoughtworks.xstream.mapper.Mapper;
 
+import java.util.Iterator;
+
 
 /**
  * Abstract base class for a TreeMarshaller, that can build references.
@@ -34,7 +34,7 @@
  */
 public abstract class AbstractReferenceMarshaller<R> extends TreeMarshaller implements MarshallingContext {
 
-    private final ObjectIdDictionary<Id<R>> references = new ObjectIdDictionary<Id<R>>();
+    /*Visible for testing*/ final ObjectIdDictionary<Id<R>> references = new ObjectIdDictionary<Id<R>>();
     private final ObjectIdDictionary<Object> implicitElements = new ObjectIdDictionary<Object>();
     private final PathTracker pathTracker = new PathTracker();
     private Path lastPath;

xstream/src/java/com/thoughtworks/xstream/core/AbstractReferenceUnmarshaller.java

@@ -10,16 +10,16 @@
  */
 package com.thoughtworks.xstream.core;
 
-import java.util.HashMap;
-import java.util.Map;
-
 import com.thoughtworks.xstream.converters.ConversionException;
 import com.thoughtworks.xstream.converters.Converter;
 import com.thoughtworks.xstream.converters.ConverterLookup;
 import com.thoughtworks.xstream.core.util.FastStack;
 import com.thoughtworks.xstream.io.HierarchicalStreamReader;
 import com.thoughtworks.xstream.mapper.Mapper;
 
+import java.util.HashMap;
+import java.util.Map;
+
 
 /**
  * Abstract base class for a TreeUnmarshaller, that resolves references.
@@ -32,7 +32,8 @@
 public abstract class AbstractReferenceUnmarshaller<R> extends TreeUnmarshaller {
 
     private static final Object NULL = new Object();
-    private final Map<R, Object> values = new HashMap<R, Object>();
+
+    /*Visible For Testing*/ final Map<R, Object> instancesByDocPath = new HashMap<R, Object>();
     private final FastStack<R> parentStack = new FastStack<R>(16);
 
     public AbstractReferenceUnmarshaller(
@@ -47,34 +48,56 @@ protected Object convert(final Object parent, final Class<?> type, final Convert
             final R parentReferenceKey = parentStack.peek();
             if (parentReferenceKey != null) {
                 // see AbstractCircularReferenceTest.testWeirdCircularReference()
-                if (!values.containsKey(parentReferenceKey)) {
-                    values.put(parentReferenceKey, parent);
+                if (!instancesByDocPath.containsKey(parentReferenceKey)) {
+                    instancesByDocPath.put(parentReferenceKey, parent);
                 }
             }
         }
         final Object result;
-        final String attributeName = getMapper().aliasForSystemAttribute("reference");
-        final String reference = attributeName == null ? null : reader.getAttribute(attributeName);
-        if (reference != null) {
-            final Object cache = values.get(getReferenceKey(reference));
-            if (cache == null) {
-                final ConversionException ex = new ConversionException("Invalid reference");
-                ex.add("reference", reference);
-                throw ex;
-            }
+        String referenceAttrName = getMapper().aliasForSystemAttribute("reference");
+        String reference = referenceAttrName == null ? null : reader.getAttribute(referenceAttrName);
+        boolean needsReference = type == null
+                || ! getMapper().isImmutableValueType(type)
+                || getMapper().canBeReferencedByPath(type);
+
+        if (reference == null && ! needsReference){
+            //if reference is not null but type is declared as not needing refs,
+            // its feasible that the ref is to a subclass instance somewhere else on the document,
+            // so treat it normally.
+            result = super.convert(parent, type, converter);
+        } else if (reference != null) {
+            Object cache = instancesByDocPath.get(getReferenceKey(reference));
+            throwIfReferenceIsBad(type, reference, ! needsReference, cache);
             result = cache == NULL ? null : cache;
         } else {
             final R currentReferenceKey = getCurrentReferenceKey();
             parentStack.push(currentReferenceKey);
             result = super.convert(parent, type, converter);
             if (currentReferenceKey != null) {
-                values.put(currentReferenceKey, result == null ? NULL : result);
+                instancesByDocPath.put(currentReferenceKey, result == null ? NULL : result);
             }
             parentStack.popSilently();
         }
         return result;
     }
 
+    private void throwIfReferenceIsBad(Class type, String reference, boolean isImmutable, Object referant) {
+        if (referant != null) {
+            return;
+        }
+
+        String msg = "Invalid reference";
+        msg = isImmutable
+                ? msg + ", no references to any instances of that class are kept because it is immutable"
+                : msg;
+
+        final ConversionException ex = new ConversionException(msg);
+        ex.add("class", type == null ? "not available" : type.getCanonicalName());
+        ex.add("reference", reference);
+
+        throw ex;
+    }
+
     protected abstract R getReferenceKey(String reference);
 
     protected abstract R getCurrentReferenceKey();

xstream/src/java/com/thoughtworks/xstream/core/ReferenceByXPathMarshallingStrategy.java

@@ -29,13 +29,13 @@ public ReferenceByXPathMarshallingStrategy(final int mode) {
     }
 
     @Override
-    protected TreeUnmarshaller createUnmarshallingContext(final Object root, final HierarchicalStreamReader reader,
+    protected ReferenceByXPathUnmarshaller createUnmarshallingContext(final Object root, final HierarchicalStreamReader reader,
             final ConverterLookup converterLookup, final Mapper mapper) {
         return new ReferenceByXPathUnmarshaller(root, reader, converterLookup, mapper);
     }
 
     @Override
-    protected TreeMarshaller createMarshallingContext(final HierarchicalStreamWriter writer,
+    protected ReferenceByXPathMarshaller createMarshallingContext(final HierarchicalStreamWriter writer,
             final ConverterLookup converterLookup, final Mapper mapper) {
         return new ReferenceByXPathMarshaller(writer, converterLookup, mapper, mode);
     }

xstream/src/java/com/thoughtworks/xstream/mapper/DefaultMapper.java

@@ -109,6 +109,11 @@ public boolean isImmutableValueType(final Class<?> type) {
         return false;
     }
 
+    @Override
+    public boolean canBeReferencedByPath(Class type){
+        return false;
+    }
+
     @Override
     public String getFieldNameForItemTypeAndName(final Class<?> definedIn, final Class<?> itemType,
             final String itemFieldName) {

xstream/src/java/com/thoughtworks/xstream/mapper/ImmutableTypesMapper.java

@@ -23,23 +23,25 @@
  */
 public class ImmutableTypesMapper extends MapperWrapper {
 
-    private final Set<Class<?>> immutableTypes = new HashSet<Class<?>>();
+    private final Set<Class> referentTypes = new HashSet<>();
+    private final Set<Class> immutableTypes = new HashSet<>();
 
-    public ImmutableTypesMapper(final Mapper wrapped) {
+    public ImmutableTypesMapper(Mapper wrapped) {
         super(wrapped);
     }
 
-    public void addImmutableType(final Class<?> type) {
+    public void addImmutableType(Class type, boolean retainPathsOnDeserialization) {
         immutableTypes.add(type);
+        if(retainPathsOnDeserialization) { referentTypes.add(type); }
     }
 
     @Override
-    public boolean isImmutableValueType(final Class<?> type) {
-        if (immutableTypes.contains(type)) {
-            return true;
-        } else {
-            return super.isImmutableValueType(type);
-        }
+    public boolean isImmutableValueType(Class type) {
+        return immutableTypes.contains(type) || super.isImmutableValueType(type);
     }
 
+    @Override
+    public boolean canBeReferencedByPath(Class type) {
+        return referentTypes.contains(type);
+    }
 }

xstream/src/java/com/thoughtworks/xstream/mapper/Mapper.java

@@ -47,6 +47,14 @@ class Null {}
      */
     boolean isImmutableValueType(Class<?> type);
 
+    /**
+     * Whether this type is an immutable type whose references must be kept anyways
+     * (for compatibility) at deserialization.
+     *
+     * @since 1.4.9
+     */
+    boolean canBeReferencedByPath(Class type);
+
     Class<?> defaultImplementationOf(Class<?> type);
 
     /**

xstream/src/java/com/thoughtworks/xstream/mapper/MapperWrapper.java

@@ -48,6 +48,11 @@ public boolean isImmutableValueType(final Class<?> type) {
         return wrapped.isImmutableValueType(type);
     }
 
+    @Override
+    public boolean canBeReferencedByPath(Class type) {
+        return wrapped.canBeReferencedByPath(type);
+    }
+
     @Override
     public Class<?> defaultImplementationOf(final Class<?> type) {
         return wrapped.defaultImplementationOf(type);