XMLExporter has several bugs/inconsistencies with BinaryExporter

[JosiahGoeman]

I've been struggling a bit with the XMLExporter recently in my game and this prompted me to write some unit tests for the JmeExporter/JmeImporter implementations. You can see the tests on my fork here:
https://github.com/JosiahGoeman/jmonkeyengine/blob/master/jme3-plugins/src/test/java/com/jme3/export/InputOutputCapsuleTest.java.
All write* and read* methods in OutputCapsule and InputCapsule respectively are tested here. I put in all the edge cases I could think of. BinaryExporter/BinaryImporter pass all tests no problemo, but XMLExporter/XMLImporter do not.

Here's the problems I've discovered so far:

1. ☑Write an empty string -> Reads defVal as if attribute was not present
2. ☑Write a string containing an apostrophe/single quote -> throws IOException when reading
3. ☑Write a string containing tab, newline, or carriage return -> Reads string with these characters replaced with spaces
4. ☑ Write any BitSet object -> Reads BitSet containing a single zero
5. ☑Write String[] containing a null string -> Reads array with null string having been replaced with an empty string
6. ☑Write a 2d array of any type except int[][] containing a null element -> Throws NullPointerException when writing
7. ☑Write an ArrayList containing a null element -> Throws IOException when reading
8. ☑Write an ArrayList<ByteBuffer> -> Reads ArrayList with same number of entries, but all are null

XMLExporterMREs.java

package com.mygame;

import com.jme3.asset.AssetInfo;
import com.jme3.export.InputCapsule;
import com.jme3.export.JmeExporter;
import com.jme3.export.JmeImporter;
import com.jme3.export.OutputCapsule;
import com.jme3.export.Savable;
import com.jme3.export.binary.BinaryExporter;
import com.jme3.export.binary.BinaryImporter;
import com.jme3.export.xml.XMLExporter;
import com.jme3.export.xml.XMLImporter;
import com.jme3.math.Vector3f;
import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.nio.ByteBuffer;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.BitSet;
import java.util.Objects;
import org.lwjgl.BufferUtils;

public class XMLExporterMREs
{
	static class TestString implements Savable
	{
		static String testString;

		@Override
		public void write(JmeExporter je) throws IOException
		{
			OutputCapsule capsule = je.getCapsule(this);

			capsule.write(testString, "test_string", null);
		}

		@Override
		public void read(JmeImporter ji) throws IOException
		{
			InputCapsule capsule = ji.getCapsule(this);

			String readString = capsule.readString("test_string", null);
			if(!Objects.equals(testString, readString))
				throw new IOException("Expected " + makeWhitespaceExplicit(testString) + " but got " + makeWhitespaceExplicit(readString));
		}
	}

	static class TestBitSet implements Savable
	{
		private static final BitSet testBitSet = BitSet.valueOf("BitSet".getBytes());

		@Override
		public void write(JmeExporter je) throws IOException
		{
			OutputCapsule capsule = je.getCapsule(this);

			capsule.write(testBitSet, "test_bit_set", null);
		}

		@Override
		public void read(JmeImporter ji) throws IOException
		{
			InputCapsule capsule = ji.getCapsule(this);

			BitSet bs = capsule.readBitSet("test_bit_set", null);

			if(!testBitSet.equals(bs))
				throw new IOException("Expected " + testBitSet + " but got " + bs);
		}
	}

	static class TestStringArrayWithNull implements Savable
	{
		private static final String[] testStringArray = new String[]
		{
			"hello",
			null,
			"world"
		};

		@Override
		public void write(JmeExporter je) throws IOException
		{
			OutputCapsule capsule = je.getCapsule(this);

			try
			{
				capsule.write(testStringArray, "test_string_array", null);
			} catch(Exception e)
			{
				System.out.println(e);
			}
		}

		@Override
		public void read(JmeImporter ji) throws IOException
		{
			InputCapsule capsule = ji.getCapsule(this);

			String[] readArray = capsule.readStringArray("test_string_array", null);

			if(!Arrays.equals(testStringArray, readArray))
				throw new IOException("Expected " + Arrays.toString(testStringArray) + " but got " + Arrays.toString(readArray));
		}
	}

	static class Test2DArrayWithNull implements Savable
	{
		private static final byte[][] testByteArray2d = new byte[][]
		{
			new byte[]
			{
				0, 1, 2
			},
			null,
			new byte[]
			{
				0, 1, 2
			}
		};

		@Override
		public void write(JmeExporter je) throws IOException
		{
			OutputCapsule capsule = je.getCapsule(this);

			capsule.write(testByteArray2d, "test_array_2d", null);
		}

		@Override
		public void read(JmeImporter ji) throws IOException
		{
			InputCapsule capsule = ji.getCapsule(this);

			byte[][] readArray = capsule.readByteArray2D("test_array_2d", null);

			if(!Arrays.deepEquals(testByteArray2d, readArray))
				throw new IOException("Expected " + Arrays.toString(testByteArray2d) + " but got " + Arrays.toString(readArray));
		}
	}

	static class TestListWithNull implements Savable
	{
		static final ArrayList<Savable> testArrayList = new ArrayList();

		static
		{
			testArrayList.add(new Vector3f());
			testArrayList.add(null);
			testArrayList.add(new Vector3f());
		}

		@Override
		public void write(JmeExporter je) throws IOException
		{
			OutputCapsule capsule = je.getCapsule(this);

			capsule.writeSavableArrayList(testArrayList, "test_array_list", null);
		}

		@Override
		public void read(JmeImporter ji) throws IOException
		{
			InputCapsule capsule = ji.getCapsule(this);

			ArrayList<Savable> readArrayList = capsule.readSavableArrayList("test_array_list", null);

			if(!testArrayList.equals(readArrayList))
				throw new IOException("Expected " + testArrayList + " but got " + readArrayList);
		}
	}

	static class TestByteBufferList implements Savable
	{
		static final ArrayList<ByteBuffer> testArrayList = new ArrayList();

		static
		{
			testArrayList.add(BufferUtils.createByteBuffer(3).put(new byte[]
			{
				Byte.MIN_VALUE, Byte.MAX_VALUE
			}).rewind());
			testArrayList.add(BufferUtils.createByteBuffer(3).put(new byte[]
			{
				Byte.MIN_VALUE, Byte.MAX_VALUE
			}).rewind());
		}

		@Override
		public void write(JmeExporter je) throws IOException
		{
			OutputCapsule capsule = je.getCapsule(this);

			capsule.writeByteBufferArrayList(testArrayList, "test_array_list", null);
		}

		@Override
		public void read(JmeImporter ji) throws IOException
		{
			InputCapsule capsule = ji.getCapsule(this);

			ArrayList<ByteBuffer> readArrayList = capsule.readByteBufferArrayList("test_array_list", null);

			if(!testArrayList.equals(readArrayList))
				throw new IOException("Expected " + testArrayList + " but got " + readArrayList);
		}
	}

	public static void main(String[] args)
	{
		String[] problemStrings = new String[]
		{
			"",
			"\'",
			"\t",
			"\n",
			"\r"
		};

		for(String s : problemStrings)
		{
			TestString.testString = s;

			System.out.println("Testing string: " + makeWhitespaceExplicit(TestString.testString) + "");
			saveAndLoad(new BinaryExporter(), new BinaryImporter(), new TestString());
			saveAndLoad(new XMLExporter(), new XMLImporter(), new TestString());
			System.out.println();
		}

		System.out.println("Testing BitSet");
		saveAndLoad(new BinaryExporter(), new BinaryImporter(), new TestBitSet());
		saveAndLoad(new XMLExporter(), new XMLImporter(), new TestBitSet());
		System.out.println();

		System.out.println("Testing String[] with null element");
		saveAndLoad(new BinaryExporter(), new BinaryImporter(), new TestStringArrayWithNull());
		saveAndLoad(new XMLExporter(), new XMLImporter(), new TestStringArrayWithNull());
		System.out.println();

		//for the sake of brevity, I'm only showing this on write(byte[][]),
		//but it happens on all the other write() overloads that accept a 2d array, except int[][] curiously.
		System.out.println("Testing byte[][] with null element");
		saveAndLoad(new BinaryExporter(), new BinaryImporter(), new Test2DArrayWithNull());
		try
		{
			saveAndLoad(new XMLExporter(), new XMLImporter(), new Test2DArrayWithNull());
		} catch(NullPointerException e)
		{
			System.out.println("\n\t" + e);
		}
		System.out.println();

		System.out.println("Testing ArrayList<Savable> with null element");
		saveAndLoad(new BinaryExporter(), new BinaryImporter(), new TestListWithNull());
		try
		{
			saveAndLoad(new XMLExporter(), new XMLImporter(), new TestListWithNull());
		} catch(NullPointerException e)
		{
			System.out.println("\n\t" + e);
		}
		System.out.println();

		System.out.println("Testing ArrayList<ByteBuffer>");
		saveAndLoad(new BinaryExporter(), new BinaryImporter(), new TestByteBufferList());
		saveAndLoad(new XMLExporter(), new XMLImporter(), new TestByteBufferList());
		System.out.println();
	}

	private static String makeWhitespaceExplicit(String s)
	{
		if(s == null)
			return "null";

		return "\"" + s.replaceAll("\t", "\\\\t").replaceAll("\n", "\\\\n").replaceAll("\r", "\\\\r").replaceAll("\s", "\\\\s") + "\"";
	}

	private static void saveAndLoad(JmeExporter exporter, JmeImporter importer, Savable savable)
	{
		System.out.print("Testing implementation: " + exporter.getClass().getSimpleName() + "...");

		// export
		byte[] exportedBytes = null;
		try(ByteArrayOutputStream outStream = new ByteArrayOutputStream())
		{
			exporter.save(savable, outStream);
			exportedBytes = outStream.toByteArray();
		} catch(IOException e)
		{
			System.out.println("\n\t" + e);
			return;
		}

		//if(exporter instanceof XMLExporter)
		//	System.out.println(new String(exportedBytes));
		// import
		try(ByteArrayInputStream inStream = new ByteArrayInputStream(exportedBytes))
		{
			AssetInfo info = new AssetInfo(null, null)
			{
				@Override
				public InputStream openStream()
				{
					return inStream;
				}
			};
			importer.load(info);    // this is where assertions will fail if loaded data does not match saved data.
		} catch(IOException e)
		{
			System.out.println("\n\t" + e);
			return;
		}

		System.out.println("ok.");
	}
}

I intend to continue investigating and hopefully resolve these problems.

[JosiahGoeman]

I figured out the problem with BitSets. It looks like DOMOutputCapsule was writing the indices of each set bit in the BitSet similar to how BitSet.toString() works, but DOMInputCapsule was reading a "1" or "0" for each bit in the set. I think having a bunch of 1s and 0s in the file seems more readable, so I went with that method and changed DOMOutputCapsule to match the behavior. Since this functionality was completely broken previously, backwards compatibility with old XML files shouldn't be an issue. I can also change it to use the indexing method if you guys think that would be better.
cd0f922
I'm thinking for the sake of simplicity, I'll just submit one PR that fixes all the listed bugs when I'm finished.

[JosiahGoeman]

Also figured out why readString() was returning defVal for empty strings. Element.getAttribute() returns an empty string when the attribute doesn't exist. It looks like DOMInputCapsule was interpreting an empty string as always meaning the attribute wasn't found, so intentionally empty strings were treated the same way as null. It now explicitly checks whether or not the attribute exists.
c1ce3e3

[stephengold]

Thanks for investigating these issues. Please keep up the good work!

[JosiahGoeman]

Is there any reason I shouldn't deprecate DOMSerializer in favor of using Transformer? DOMSerializer is only used by XMLExporter and it doesn't appear to be doing anything that can't be done with the more robust standard library classes. The apostrophe and whitespace normalization issues can sort of be solved by adding extra cases to encodeString() and the corresponding decodeString(), but that's creating another issue I haven't looked into, and anyway those methods are really just a workaround for DOMSerializer being fragile. I think the cleanest solution is to just let java's battle-tested library to the heavy lifting.
5f20e04

[pspeed42]

I don't have specific insights here but I agree in general. JME suffers from a lot of "not invented here" syndrome, especially some of the older stuff.

[JosiahGoeman]

For the time being, I'm going to assume its OK so long as XML files written with the old version still load properly. Easy enough to revert if needed.

More progress!
444b1a1
I fixed the issue with String arrays containing null values. Also fixed the NullPointerException for 2d arrays of primitives and strings, but it's still broken for Savables at this time. This involved some major refactoring. Main change is the use of helper methods to clean up duplicate code.

Edit: also now fixed Savables 9341624

[JosiahGoeman]

One discrepancy I noticed is that when writing buffers, BinaryExporter clobbers the buffer's position with rewind(). XMLExporter didn't used to, but I've changed it to match the BinaryExporter behavior. Really this seems like a bad behavior with BinaryExporter, but that's outside the scope of this issue, and I'd be scared to change something like that since it's used in so many different places. Is this worth making a separate issue about, or just leave it be?

[stephengold]

I think the rewind() invocations there were a mistake. It would be better for exporters to leave those buffer positions as they found them, either by using get(x) in place of get() or else by saving and restoring buffer positions. Go ahead and open an issue.

[JosiahGoeman]

Ok #2312

[JosiahGoeman]

2d21700
That's all tests passing! Next I'll try to come up with a way to check for backwards compatibility with XML generated by the old version and fix anything that arises from that.

[JosiahGoeman]

#2313