IonSystem.clone not doing deep copy as the doc indicates

[r0b0ji]

The Javadoc on IonSystem indicates to use IonSystem.clone to create a copy of value for use by a different system.

ion-java/src/main/java/com/amazon/ion/IonSystem.java

Lines 33 to 43 in 9e6c570

	* In general, {@link IonValue} instances returned from one system instance
	* are not interoperable with those returned by other instances.
	* The intended usage pattern is for an application to construct a single
	* <code>IonSystem</code> instance and use it throughout,
	* rather than constructing multiple systems and intermingling their use.
	* To create a copy of a value for use by a different system, use
	* {@link #clone(IonValue)}.
	* <p>
	* To create an {@code IonSystem},
	* see {@link com.amazon.ion.system.IonSystemBuilder}.
	* <p>

Similarly, the Javadoc on ValueFactory which IonSystems extends mentions that clone does the deep copy and it allows you to shift data from one factory instance to another.

ion-java/src/main/java/com/amazon/ion/ValueFactory.java

Lines 570 to 589 in 9e6c570

	/**
	* Creates a deep copy of an Ion value. This method can properly clone
	* {@link IonDatagram}s.
	* <p>
	* The given value can be in the context of any {@code ValueFactory},
	* and the result will be in the context of this one. This allows you to
	* shift data from one factory instance to another.
	*
	* @param value the value to copy.
	*
	* @return a deep copy of value, with no container.
	*
	* @throws NullPointerException if {@code value} is null.
	* @throws IonException if there's a problem creating the clone.
	* @throws UnknownSymbolException
	* if any part of this value has unknown text but known Sid for
	* its field name, annotation or symbol.
	*
	* @see IonValue#clone()
	*/

So, I'd expect that when I do ionSystem.clone(ionValue) then result is a deep copy with everything with ionSystem, but I found this test fails indicating it didn't do that.

@Test
    public void testIonSystem() {
        IonSystem ionSystem1 = IonSystemBuilder.standard().build();
        IonSystem ionSystem2 = IonSystemBuilder.standard().build();
        IonSystem ionSystem3 = IonSystemBuilder.standard().build();

        IonStruct requestStruct = (IonStruct) ionSystem1.singleValue("{" +
                "request_id:1234," +
                "item_id:\"item_1\"" +
                "}");
        IonStruct responseStruct = (IonStruct) ionSystem2.singleValue("{" +
                "status:success," +
                "request_id:1234," +
                "item_id:\"item_1\"" +
                "}");
        IonStruct container = ionSystem3.newEmptyStruct();
        container.add("request", requestStruct);
        container.add("response", responseStruct);

        IonStruct finalStruct = ionSystem3.clone(container);
        assertEquals(finalStruct.get("request").getSystem(), finalStruct.get("response").getSystem())  ;
    }

[tgregg]

The test as written should be expected to fail due to the following guidance in the IonSystem documentation:

In general, {@link IonValue} instances returned from one system instance are not interoperable with those returned by other instances. 

Ideally it would fail in the location indicated below:

    @Test
    public void testIonSystem() {
        IonSystem ionSystem1 = IonSystemBuilder.standard().build();
        IonSystem ionSystem2 = IonSystemBuilder.standard().build();
        IonSystem ionSystem3 = IonSystemBuilder.standard().build();

        IonStruct requestStruct = (IonStruct) ionSystem1.singleValue("{" +
                "request_id:1234," +
                "item_id:\"item_1\"" +
                "}");
        IonStruct responseStruct = (IonStruct) ionSystem2.singleValue("{" +
                "status:success," +
                "request_id:1234," +
                "item_id:\"item_1\"" +
                "}");
        IonStruct container = ionSystem3.newEmptyStruct();
        container.add("request", requestStruct); // SHOULD FAIL HERE: 'requestStruct' from 'ionSystem1' is not interoperable with 'container' from 'ionSystem3'
        container.add("response", responseStruct);

        IonStruct finalStruct = ionSystem3.clone(container);
        assertEquals(finalStruct.get("request").getSystem(), finalStruct.get("response").getSystem())  ;
    }

Unfortunately it does not, and it's highly likely that fixing this would break existing users who are getting away with doing this. As a result, we may be stuck with the current behavior in this major version of IonJava, namely that doing this may fail in some subsequent location, or may not.

The following works:

    @Test
    public void testIonSystem() {
        IonSystem ionSystem1 = IonSystemBuilder.standard().build();
        IonSystem ionSystem2 = IonSystemBuilder.standard().build();
        IonSystem ionSystem3 = IonSystemBuilder.standard().build();

        IonStruct requestStruct = (IonStruct) ionSystem1.singleValue("{" +
            "request_id:1234," +
            "item_id:\"item_1\"" +
            "}");
        IonStruct responseStruct = (IonStruct) ionSystem2.singleValue("{" +
            "status:success," +
            "request_id:1234," +
            "item_id:\"item_1\"" +
            "}");
        IonStruct container = ionSystem3.newEmptyStruct();
        container.add("request", ionSystem3.clone(requestStruct)); // NOTE: clone before adding
        container.add("response", ionSystem3.clone(responseStruct));

        assertEquals(container.get("request").getSystem(), container.get("response").getSystem());
    }

[r0b0ji]

I understand that it should have failed when adding an IonValue to another container created by other IonSystem, however it doesn't and changing this behavior is current major version is not safe and dangerous.

But I'd expect when I do ionSystem1.clone(ionValue), the resulting IonValue is fully in ionSystem1.

The documentation of clone does indicate that, this is a pattern to use when intended to create IonValue for another system.

* ...
 * The intended usage pattern is for an application to construct a single 
  * <code>IonSystem</code> instance and use it throughout, 
  * rather than constructing multiple systems and intermingling their use. 
  * To create a copy of a value for use by a different system, use 
  * {@link #clone(IonValue)}. 
  * ...

And similarly here

     /** 
      * Creates a deep copy of an Ion value.  This method can properly clone 
      * {@link IonDatagram}s. 
      * <p> 
      * The given value can be in the context of any {@code ValueFactory}, 
      * and the result will be in the context of this one. This allows you to 
      * shift data from one factory instance to another. 
      * 
      * @param value the value to copy. 
      * 
      * @return a deep copy of value, with no container. 
      * 
      * ... 
      */ 

While changing the above behavior is dangerous, changing the clone behavior should be safe so that it does what it says it does, a deep copy and once I get a deep copy with new IonSystem I can expect it to have just one IonSystem I cloned with.

[r0b0ji]

Also, with current behavior even this is not fullproof. Now, imagine requestStruct and responseStruct itself a deeply nested, then it is still possible that some part of those requestStruct is with different IonSystem and then in assert if I try to assert those, those will still be different.

Only solution I think will work is to make ionSystem.clone return a deep clone.

    @Test
    public void testIonSystem() {
        IonSystem ionSystem1 = IonSystemBuilder.standard().build();
        IonSystem ionSystem2 = IonSystemBuilder.standard().build();
        IonSystem ionSystem3 = IonSystemBuilder.standard().build();

        IonStruct requestStruct = (IonStruct) ionSystem1.singleValue("{" +
            "request_id:1234," +
            "item_id:\"item_1\"" +
            "}");
        IonStruct responseStruct = (IonStruct) ionSystem2.singleValue("{" +
            "status:success," +
            "request_id:1234," +
            "item_id:\"item_1\"" +
            "}");
        IonStruct container = ionSystem3.newEmptyStruct();
        container.add("request", ionSystem3.clone(requestStruct)); // NOTE: clone before adding
        container.add("response", ionSystem3.clone(responseStruct));

        assertEquals(container.get("request").getSystem(), container.get("response").getSystem());
    }