Fix roundtripping of binary data, add unit test

plist-cil.test/BinaryPropertyListWriterTests.cs

@@ -0,0 +1,25 @@
+using Claunia.PropertyList;
+using System.IO;
+using Xunit;
+
+namespace plistcil.test
+{
+    public class BinaryPropertyListWriterTests
+    {
+        [Fact]
+        public void RoundtripTest()
+        {
+            byte[] data = File.ReadAllBytes("test-files/plist.bin");
+            NSObject root = PropertyListParser.Parse(data);
+
+            using (MemoryStream actualOutput = new MemoryStream())
+            using (Stream expectedOutput = File.OpenRead("test-files/plist.bin"))
+            using (ValidatingStream validatingStream = new ValidatingStream(actualOutput, expectedOutput))
+            {
+                BinaryPropertyListWriter writer = new BinaryPropertyListWriter(validatingStream);
+                writer.ReuseObjectIds = false;
+                writer.Write(root);
+            }
+        }
+    }
+}

plist-cil.test/ValidatingStream.cs

@@ -0,0 +1,134 @@
+// <copyright file="ValidatingCompositeStream.cs" company="Quamotion">
+// Copyright (c) Quamotion. All rights reserved.
+// </copyright>
+
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Linq;
+using System.Text;
+using System.Threading;
+using System.Threading.Tasks;
+using Xunit;
+
+namespace plistcil.test
+{
+    /// <summary>
+    /// A <see cref="Stream"/> which writes its output to a <see cref="Stream"/> and validates that the data which
+    /// is being written to the output stream matches the data in a reference stream.
+    /// </summary>
+    internal class ValidatingStream : Stream
+    {
+        private Stream output;
+        private Stream expectedOutput;
+
+        /// <summary>
+        /// Initializes a new instance of the <see cref="ValidatingCompositeStream"/> class.
+        /// </summary>
+        /// <param name="output">
+        /// The <see cref="Stream"/> to which to write data.
+        /// </param>
+        /// <param name="expectedOutput">
+        /// The reference stream for <paramref name="output"/>.
+        /// </param>
+        public ValidatingStream(Stream output, Stream expectedOutput)
+        {
+            this.output = output ?? throw new ArgumentNullException(nameof(output));
+            this.expectedOutput = expectedOutput ?? throw new ArgumentNullException(nameof(expectedOutput));
+        }
+
+        /// <inheritdoc/>
+        public override bool CanRead
+        {
+            get { return false; }
+        }
+
+        /// <inheritdoc/>
+        public override bool CanSeek
+        {
+            get { return false; }
+        }
+
+        /// <inheritdoc/>
+        public override bool CanWrite
+        {
+            get { return true; }
+        }
+
+        /// <inheritdoc/>
+        public override long Length
+        {
+            get { return this.output.Length; }
+        }
+
+        /// <inheritdoc/>
+        public override long Position
+        {
+            get { return this.output.Position; }
+            set { throw new NotImplementedException(); }
+        }
+
+        /// <inheritdoc/>
+        public override void Flush()
+        {
+            this.output.Flush();
+        }
+
+        /// <inheritdoc/>
+        public override int Read(byte[] buffer, int offset, int count)
+        {
+            throw new NotSupportedException();
+        }
+
+        /// <inheritdoc/>
+        public override Task<int> ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)
+        {
+            throw new NotSupportedException();
+        }
+
+        /// <inheritdoc/>
+        public override long Seek(long offset, SeekOrigin origin)
+        {
+            throw new NotImplementedException();
+        }
+
+        /// <inheritdoc/>
+        public override void SetLength(long value)
+        {
+            throw new NotImplementedException();
+        }
+
+        /// <inheritdoc/>
+        public override void Write(byte[] buffer, int offset, int count)
+        {
+            byte[] expected = new byte[buffer.Length];
+            this.expectedOutput.Read(expected, offset, count);
+
+            byte[] bufferChunk = buffer.Skip(offset).Take(count).ToArray();
+            byte[] expectedChunk = expected.Skip(offset).Take(count).ToArray();
+
+            // Make sure the data being writen matches the data which was written to the expected stream.
+            // This will detect any errors as the invalid data is being written out - as opposed to post-
+            // test binary validation.
+            Assert.Equal(expectedChunk, bufferChunk);
+            this.output.Write(buffer, offset, count);
+        }
+
+        /// <inheritdoc/>
+        public override async Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)
+        {
+            byte[] expected = new byte[buffer.Length];
+            await this.expectedOutput.ReadAsync(expected, offset, count, cancellationToken).ConfigureAwait(false);
+
+            byte[] bufferChunk = buffer.Skip(offset).Take(count).ToArray();
+            byte[] expectedChunk = expected.Skip(offset).Take(count).ToArray();
+
+            // Make sure the data being writen matches the data which was written to the expected stream.
+            // This will detect any errors as the invalid data is being written out - as opposed to post-
+            // test binary validation.
+            Assert.Equal(expectedChunk, bufferChunk);
+
+            await this.output.WriteAsync(buffer, offset, count, cancellationToken).ConfigureAwait(false);
+        }
+    }
+}

plist-cil.test/plist-cil.test.csproj

@@ -19,6 +19,9 @@
   </ItemGroup>
 
   <ItemGroup>
+    <None Include="..\plist-cil.benchmark\plist.bin" Link="test-files\plist.bin">
+      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
+    </None>
     <None Include="..\test-files\test1-ascii-gnustep.plist">
       <Link>test-files\test1-ascii-gnustep.plist</Link>
       <Gettext-ScanForTranslations>False</Gettext-ScanForTranslations>

plist-cil/BinaryPropertyListWriter.AddObjectEqualityComparer.cs

@@ -0,0 +1,38 @@
+using System;
+using System.Collections.Generic;
+
+namespace Claunia.PropertyList
+{
+    public partial class BinaryPropertyListWriter
+    {
+        /// <summary>
+        /// The equality comparer which is used when adding an object to the <see cref="BinaryPropertyListWriter.idMap" />. In most cases,
+        /// objects are always added. The only exception are very specific strings, which are only added once.
+        /// </summary>
+        private class AddObjectEqualityComparer : EqualityComparer<NSObject>
+        {
+            public override bool Equals(NSObject x, NSObject y)
+            {
+                var a = x as NSString;
+                var b = y as NSString;
+
+                if (a == null || b == null)
+                {
+                    return object.ReferenceEquals(x, y);
+                }
+
+                if (!BinaryPropertyListWriter.IsSerializationPrimitive(a) || !BinaryPropertyListWriter.IsSerializationPrimitive(b))
+                {
+                    return object.ReferenceEquals(x, y);
+                }
+
+                return string.Equals(a.Content, b.Content, StringComparison.Ordinal);
+            }
+
+            public override int GetHashCode(NSObject obj)
+            {
+                throw new System.NotImplementedException();
+            }
+        }
+    }
+}

plist-cil/BinaryPropertyListWriter.GetObjectEqualityComparer.cs

@@ -0,0 +1,44 @@
+using System;
+using System.Collections.Generic;
+
+namespace Claunia.PropertyList
+{
+    public partial class BinaryPropertyListWriter
+    {
+        /// <summary>
+        /// The equality comparer which is used when retrieving objects in the <see cref="BinaryPropertyListWriter.idMap"/>.
+        /// The logic is slightly different from <see cref="AddObjectEqualityComparer"/>, results in two equivalent objects
+        /// (UIDs mainly) being added to the <see cref="BinaryPropertyListWriter.idMap"/>. Whenever the ID for one of
+        /// those equivalent objects is requested, the first ID is always returned.
+        /// This means that there are "orphan" objects in binary property lists - duplicate objects which are never referenced -;
+        /// this logic exists purely to maintain binary compatibility with Apple's format.
+        /// </summary>
+        private class GetObjectEqualityComparer : EqualityComparer<NSObject>
+        {
+            public override bool Equals(NSObject x, NSObject y)
+            {
+                // By default, use reference equality. Even if there are two objects - say a NSString - with the same
+                // value, do not consider them equal unless they are the same instance of NSString.
+                // The exceptions are UIDs, where we always compare by value, and "primitive" strings (a list of well-known
+                // strings), which are treaded specially and "recycled".
+                if (x is UID)
+                {
+                    return x.Equals(y);
+                }
+                else if (x is NSString && BinaryPropertyListWriter.IsSerializationPrimitive((NSString)x))
+                {
+                    return x.Equals(y);
+                }
+                else
+                {
+                    return object.ReferenceEquals(x, y);
+                }
+            }
+
+            public override int GetHashCode(NSObject obj)
+            {
+                throw new NotImplementedException();
+            }
+        }
+    }
+}

plist-cil/BinaryPropertyListWriter.cs

@@ -42,7 +42,7 @@ namespace Claunia.PropertyList
     /// </summary>
     /// @author Keith Randall
     /// @author Natalia Portillo
-    public class BinaryPropertyListWriter
+    public partial class BinaryPropertyListWriter
     {
         /// <summary>
         /// Binary property list version 0.0
@@ -61,6 +61,9 @@ namespace Claunia.PropertyList
         /// </summary>
         public const int VERSION_20 = 20;
 
+        private readonly AddObjectEqualityComparer addObjectComparer = new AddObjectEqualityComparer();
+        private readonly GetObjectEqualityComparer getObjectComparer = new GetObjectEqualityComparer();
+
         /// <summary>
         /// Gets or sets a value indicating whether two equivalent objects should be serialized once in the binary property list file, or whether
         /// the value should be stored multiple times in the binary property list file. The default is <see langword="false"/>.
@@ -284,59 +287,48 @@ namespace Claunia.PropertyList
 
         internal void AssignID(NSObject obj)
         {
-            // If binary compatibility with the Apple format is required,
-            // UID, NSArray and NSString objects are assigned a new ID,
-            // even if they already exist in the file.
-            if (!this.ReuseObjectIds && (obj is UID || obj is NSNumber || obj is NSArray))
+            if (this.ReuseObjectIds)
             {
-                idMap.Add(obj);
+                if (!this.idMap.Contains(obj))
+                {
+                    idMap.Add(obj);
+                }
             }
-            else if (!this.ReuseObjectIds && obj is NSString && !IsSerializationPrimitive((NSString)obj))
+            else
             {
-                idMap.Add(obj);
+                if (!this.idMap.Contains(obj, addObjectComparer))
+                {
+                    idMap.Add(obj);
+                }
             }
-            else if (!idMap.Contains(obj))
-            {
-                idMap.Add(obj);
-            }
-        }
-
-        internal bool IsSerializationPrimitive(NSString obj)
-        {
-            return obj != null && obj.Content.StartsWith("$") || obj.Content.StartsWith("NS");
         }
 
         internal int GetID(NSObject obj)
         {
-            if (!this.ReuseObjectIds && obj is UID)
+            if (this.ReuseObjectIds)
             {
-                var uid = obj as UID;
-                var first = idMap.OfType<UID>().First(v => NSObject.ArrayEquals(v.Bytes, uid.Bytes));
-                return idMap.IndexOf(first);
-            }
-            else if (!this.ReuseObjectIds && (obj is NSArray || (obj is NSString && !IsSerializationPrimitive((NSString)obj))))
-            {
-                int index = -1;
-
-                for (int i = 0; i < idMap.Count; i++)
-                {
-                    if (idMap[i] == obj)
-                    {
-                        index = i;
-                        break;
-                    }
-                }
-
-                if (index == -1)
-                {
-                    throw new InvalidOperationException();
-                }
-
-                return index;
+                return idMap.IndexOf(obj);
             }
             else
             {
-                return idMap.IndexOf(obj);
+                if (obj is UID)
+                {
+                    var uid = obj as UID;
+                    var first = idMap.OfType<UID>().First(v => NSObject.ArrayEquals(v.Bytes, uid.Bytes));
+                    return idMap.IndexOf(first);
+                }
+                else
+                {
+                    for (int i = 0; i < idMap.Count; i++)
+                    {
+                        if (this.getObjectComparer.Equals(idMap[i], obj))
+                        {
+                            return i;
+                        }
+                    }
+
+                    throw new InvalidOperationException();
+                }
             }
         }
 
@@ -422,6 +414,24 @@ namespace Claunia.PropertyList
         {
             WriteLong(BitConverter.DoubleToInt64Bits(value));
         }
+
+        internal static bool IsSerializationPrimitive(NSString obj)
+        {
+            var content = obj.Content;
+
+            // This is a list of "special" values which are only added once to a binary property
+            // list, and can be referenced multiple times.
+            return content == "$class"
+                || content == "$classes"
+                || content == "$classname"
+                || content == "NS.objects"
+                || content == "NS.keys"
+                || content == "NSDictionary"
+                || content == "NSObject"
+                || content == "NSMutableDictionary"
+                || content == "NSMutableArray"
+                || content == "NSArray";
+        }
     }
 }