Fix collection typed property which has non-public setter are not handled correctly. Issue #62.

yfakariya · yfakariya · commit 2246e8084535 · 2015-02-01T18:03:09.000+09:00
This commit changes filter in non-annotated classes to allow properties which have non-public setters. In addition, same issue exists in code generation even if classes in annotated, so unnecessary reflection based accesses are emitted.
This commit also includes additional unit testing cases.
diff --git a/CHANGES.txt b/CHANGES.txt
@@ -218,3 +218,4 @@ Relase 0.5.9 - 2015/2/01
 
   BUG FIXES
   * Fix NullReferenceException is thrown for annotated non-public members serialization/deserialization in WinRT.
+  * Fix collection typed property which has non-public setter are not handled correctly. Issue #62.
diff --git a/src/MsgPack/ReflectionAbstractions.cs b/src/MsgPack/ReflectionAbstractions.cs
@@ -203,28 +203,26 @@ public static IEnumerable<Type> GetInterfaces( this Type source )
 
 		public static MethodInfo GetGetMethod( this PropertyInfo source )
 		{
-			return source.GetMethod;
+			return GetGetMethod( source, false );
 		}
 
 		[System.Diagnostics.CodeAnalysis.SuppressMessage( "Microsoft.Usage", "CA1801:ReviewUnusedParameters", MessageId = "containsNonPublic", Justification = "For API compabitility" )]
 		public static MethodInfo GetGetMethod( this PropertyInfo source, bool containsNonPublic )
 		{
-			Contract.Assert( containsNonPublic ); // false is not supported now.
-
-			return source.GetMethod;
+			var getter = source.GetMethod;
+			return ( containsNonPublic || getter.IsPublic ) ? getter : null;
 		}
 
 		public static MethodInfo GetSetMethod( this PropertyInfo source )
 		{
-			return source.SetMethod;
+			return GetSetMethod( source, false );
 		}
 
 		[System.Diagnostics.CodeAnalysis.SuppressMessage( "Microsoft.Usage", "CA1801:ReviewUnusedParameters", MessageId = "containsNonPublic", Justification = "For API compabitility" )]
 		public static MethodInfo GetSetMethod( this PropertyInfo source, bool containsNonPublic )
 		{
-			Contract.Assert( containsNonPublic ); // false is not supported now.
-
-			return source.SetMethod;
+			var setter = source.SetMethod;
+			return ( containsNonPublic || setter.IsPublic ) ? setter : null;
 		}
 
 		public static IEnumerable<Type> FindInterfaces( this Type source, Func<Type, object, bool> filter, object filterCriteria )
diff --git a/src/MsgPack/Serialization/AbstractSerializers/SerializerBuilder`3.CommonConstructs.cs b/src/MsgPack/Serialization/AbstractSerializers/SerializerBuilder`3.CommonConstructs.cs
@@ -2,7 +2,7 @@
 //
 // MessagePack for CLI
 //
-// Copyright (C) 2010-2013 FUJIWARA, Yusuke
+// Copyright (C) 2010-2015 FUJIWARA, Yusuke
 //
 //    Licensed under the Apache License, Version 2.0 (the "License");
 //    you may not use this file except in compliance with the License.
@@ -315,15 +315,15 @@ private TConstruct EmitGetMemberValueExpression( TContext context, TConstruct in
 			FieldInfo asField;
 			if ( ( asField = member as FieldInfo ) != null )
 			{
-				return this.EmitGetField( context, instance, asField, !asField.GetIsPublic() );
+				return this.EmitGetField( context, instance, asField, !asField.GetHasPublicGetter() );
 			}
 			else
 			{
 				var asProperty = member as PropertyInfo;
 #if DEBUG
 				Contract.Assert( asProperty != null, member.GetType().FullName );
 #endif
-				return this.EmitGetProperty( context, instance, asProperty, !asProperty.GetIsPublic() );
+				return this.EmitGetProperty( context, instance, asProperty, !asProperty.GetHasPublicGetter() );
 			}
 		}
 
@@ -416,7 +416,7 @@ private TConstruct EmitSetMemberValueStatement( TContext context, TConstruct ins
 					return this.EmitSetField( context, instance, asField, value, false );
 				}
 
-				getCollection = this.EmitGetField( context, instance, asField, !asField.GetIsPublic() );
+				getCollection = this.EmitGetField( context, instance, asField, !asField.GetHasPublicGetter() );
 				traits = asField.FieldType.GetCollectionTraits();
 			}
 			else
@@ -431,7 +431,7 @@ private TConstruct EmitSetMemberValueStatement( TContext context, TConstruct ins
 					return this.EmitSetProprety( context, instance, asProperty, value, false );
 				}
 
-				getCollection = this.EmitGetProperty( context, instance, asProperty, !asProperty.GetIsPublic() );
+				getCollection = this.EmitGetProperty( context, instance, asProperty, !asProperty.GetHasPublicGetter() );
 				traits = asProperty.PropertyType.GetCollectionTraits();
 			}
 
diff --git a/src/MsgPack/Serialization/ReflectionExtensions.cs b/src/MsgPack/Serialization/ReflectionExtensions.cs
@@ -2,7 +2,7 @@
 //
 // MessagePack for CLI
 //
-// Copyright (C) 2010-2014 FUJIWARA, Yusuke
+// Copyright (C) 2010-2015 FUJIWARA, Yusuke
 //
 //    Licensed under the Apache License, Version 2.0 (the "License");
 //    you may not use this file except in compliance with the License.
@@ -484,6 +484,28 @@ public static IEnumerable<Type> FindInterfaces( this Type source, Func<Type, obj
 		}
 #endif
 
+		public static bool GetHasPublicGetter( this MemberInfo source )
+		{
+			PropertyInfo asProperty;
+			FieldInfo asField;
+			if ( ( asProperty = source as PropertyInfo ) != null )
+			{
+#if !NETFX_CORE
+				return asProperty.GetGetMethod() != null;
+#else
+				return ( asProperty.GetMethod != null && asProperty.GetMethod.IsPublic );
+#endif
+			}
+			else if ( ( asField = source as FieldInfo ) != null )
+			{
+				return asField.IsPublic;
+			}
+			else
+			{
+				throw new NotSupportedException( source.GetType() + " is not supported." );
+			}
+		}
+
 		public static bool GetIsPublic( this MemberInfo source )
 		{
 			PropertyInfo asProperty;
diff --git a/src/MsgPack/Serialization/SerializationTarget.cs b/src/MsgPack/Serialization/SerializationTarget.cs
@@ -2,7 +2,7 @@
 //
 // MessagePack for CLI
 //
-// Copyright (C) 2014 FUJIWARA, Yusuke
+// Copyright (C) 2014-2015 FUJIWARA, Yusuke
 //
 //    Licensed under the Apache License, Version 2.0 (the "License");
 //    you may not use this file except in compliance with the License.
@@ -188,10 +188,10 @@ internal static IEnumerable<SerializingMember> GetTargetMembers( Type type )
 					);
 			}
 #if SILVERLIGHT || NETFX_CORE
-			return members.Where( member => member.GetIsPublic() ).Select( member => new SerializingMember( member, new DataMemberContract( member ) ) );
+			return members.Where( member => member.GetHasPublicGetter() ).Select( member => new SerializingMember( member, new DataMemberContract( member ) ) );
 #else
 			return
-				members.Where( item => item.GetIsPublic() && !Attribute.IsDefined( item, typeof( NonSerializedAttribute ) ) )
+				members.Where( item => item.GetHasPublicGetter() && !Attribute.IsDefined( item, typeof( NonSerializedAttribute ) ) )
 				.Select( member => new SerializingMember( member, new DataMemberContract( member ) ) );
 #endif
 		}
diff --git a/test/MsgPack.UnitTest.CodeDom/Serialization/ArrayCodeDomBasedAutoMessagePackSerializerTest.cs b/test/MsgPack.UnitTest.CodeDom/Serialization/ArrayCodeDomBasedAutoMessagePackSerializerTest.cs
@@ -233,6 +233,7 @@ private void TestComplexObjectCore( SerializationContext context )
 		{
 			var target = new ComplexType() { Source = new Uri( "http://www.exambple.com" ), TimeStamp = DateTime.Now, Data = new byte[] { 0x1, 0x2, 0x3, 0x4 } };
 			target.History.Add( DateTime.Now.Subtract( TimeSpan.FromDays( 1 ) ), "Create New" );
+			target.Points.Add( 123 );
 			TestCoreWithVerify( target, context );
 		}
 
diff --git a/test/MsgPack.UnitTest.CodeDom/Serialization/MapCodeDomBasedAutoMessagePackSerializerTest.cs b/test/MsgPack.UnitTest.CodeDom/Serialization/MapCodeDomBasedAutoMessagePackSerializerTest.cs
@@ -233,6 +233,7 @@ private void TestComplexObjectCore( SerializationContext context )
 		{
 			var target = new ComplexType() { Source = new Uri( "http://www.exambple.com" ), TimeStamp = DateTime.Now, Data = new byte[] { 0x1, 0x2, 0x3, 0x4 } };
 			target.History.Add( DateTime.Now.Subtract( TimeSpan.FromDays( 1 ) ), "Create New" );
+			target.Points.Add( 123 );
 			TestCoreWithVerify( target, context );
 		}
 
diff --git a/test/MsgPack.UnitTest/Serialization/ArrayContextBasedAutoMessagePackSerializerTest.cs b/test/MsgPack.UnitTest/Serialization/ArrayContextBasedAutoMessagePackSerializerTest.cs
@@ -233,6 +233,7 @@ private void TestComplexObjectCore( SerializationContext context )
 		{
 			var target = new ComplexType() { Source = new Uri( "http://www.exambple.com" ), TimeStamp = DateTime.Now, Data = new byte[] { 0x1, 0x2, 0x3, 0x4 } };
 			target.History.Add( DateTime.Now.Subtract( TimeSpan.FromDays( 1 ) ), "Create New" );
+			target.Points.Add( 123 );
 			TestCoreWithVerify( target, context );
 		}
 
diff --git a/test/MsgPack.UnitTest/Serialization/ArrayExpressionBasedAutoMessagePackSerializerTest.cs b/test/MsgPack.UnitTest/Serialization/ArrayExpressionBasedAutoMessagePackSerializerTest.cs
@@ -233,6 +233,7 @@ private void TestComplexObjectCore( SerializationContext context )
 		{
 			var target = new ComplexType() { Source = new Uri( "http://www.exambple.com" ), TimeStamp = DateTime.Now, Data = new byte[] { 0x1, 0x2, 0x3, 0x4 } };
 			target.History.Add( DateTime.Now.Subtract( TimeSpan.FromDays( 1 ) ), "Create New" );
+			target.Points.Add( 123 );
 			TestCoreWithVerify( target, context );
 		}
 
diff --git a/test/MsgPack.UnitTest/Serialization/ArrayFieldBasedAutoMessagePackSerializerTest.cs b/test/MsgPack.UnitTest/Serialization/ArrayFieldBasedAutoMessagePackSerializerTest.cs
@@ -233,6 +233,7 @@ private void TestComplexObjectCore( SerializationContext context )
 		{
 			var target = new ComplexType() { Source = new Uri( "http://www.exambple.com" ), TimeStamp = DateTime.Now, Data = new byte[] { 0x1, 0x2, 0x3, 0x4 } };
 			target.History.Add( DateTime.Now.Subtract( TimeSpan.FromDays( 1 ) ), "Create New" );
+			target.Points.Add( 123 );
 			TestCoreWithVerify( target, context );
 		}
 
diff --git a/test/MsgPack.UnitTest/Serialization/ArrayReflectionBasedAutoMessagePackSerializerTest.cs b/test/MsgPack.UnitTest/Serialization/ArrayReflectionBasedAutoMessagePackSerializerTest.cs
@@ -233,6 +233,7 @@ private void TestComplexObjectCore( SerializationContext context )
 		{
 			var target = new ComplexType() { Source = new Uri( "http://www.exambple.com" ), TimeStamp = DateTime.Now, Data = new byte[] { 0x1, 0x2, 0x3, 0x4 } };
 			target.History.Add( DateTime.Now.Subtract( TimeSpan.FromDays( 1 ) ), "Create New" );
+			target.Points.Add( 123 );
 			TestCoreWithVerify( target, context );
 		}
 
diff --git a/test/MsgPack.UnitTest/Serialization/AutoMessagePackSerializerTest.ttinclude b/test/MsgPack.UnitTest/Serialization/AutoMessagePackSerializerTest.ttinclude
@@ -284,6 +284,7 @@ namespace MsgPack.Serialization
 		{
 			var target = new ComplexType() { Source = new Uri( "http://www.exambple.com" ), TimeStamp = DateTime.Now, Data = new byte[] { 0x1, 0x2, 0x3, 0x4 } };
 			target.History.Add( DateTime.Now.Subtract( TimeSpan.FromDays( 1 ) ), "Create New" );
+			target.Points.Add( 123 );
 			TestCoreWithVerify( target, context );
 		}
 
diff --git a/test/MsgPack.UnitTest/Serialization/ComplexType.cs b/test/MsgPack.UnitTest/Serialization/ComplexType.cs
@@ -2,7 +2,7 @@
 //
 // MessagePack for CLI
 //
-// Copyright (C) 2010-2012 FUJIWARA, Yusuke
+// Copyright (C) 2010-2015 FUJIWARA, Yusuke
 //
 //    Licensed under the Apache License, Version 2.0 (the "License");
 //    you may not use this file except in compliance with the License.
@@ -42,14 +42,28 @@ public class ComplexType : IVerifiable
 		[MessagePackMember( 1 )]
 		public byte[] Data { get; set; }
 
-		private readonly Dictionary<DateTime, string> _history = new Dictionary<DateTime, string>();
+		private readonly Dictionary<DateTime, string> _history;
 
 		[MessagePackMember( 3 )]
 		public Dictionary<DateTime, string> History
 		{
 			get { return this._history; }
 		}
 
+		// Issue #62
+		[MessagePackMember( 4 )]
+		public List<int> Points
+		{
+			get;
+			private set;
+		}
+
+		public ComplexType()
+		{
+			this._history = new Dictionary<DateTime, string>();
+			this.Points = new List<int>();
+		}
+
 		public void Verify( Stream stream )
 		{
 			stream.Position = 0;
@@ -67,6 +81,8 @@ public void Verify( Stream stream )
 				NUnit.Framework.Assert.That( map[ "Data" ].AsBinary(), Is.EqualTo( this.Data ) );
 				NUnit.Framework.Assert.That( map.ContainsKey( "History" ) );
 				NUnit.Framework.Assert.That( map[ "History" ].AsDictionary().Count, Is.EqualTo( this.History.Count ) );
+				NUnit.Framework.Assert.That( map.ContainsKey( "Points" ) );
+				NUnit.Framework.Assert.That( map[ "Points" ].AsList().Count, Is.EqualTo( this.Points.Count ) );
 			}
 			else
 			{
@@ -76,6 +92,7 @@ public void Verify( Stream stream )
 				NUnit.Framework.Assert.That( MessagePackConvert.ToDateTime( array[ 2 ].AsInt64() ), Is.EqualTo( this.TimeStamp ) );
 				NUnit.Framework.Assert.That( array[ 1 ].AsBinary(), Is.EqualTo( this.Data ) );
 				NUnit.Framework.Assert.That( array[ 3 ].AsDictionary().Count, Is.EqualTo( this.History.Count ) );
+				NUnit.Framework.Assert.That( array[ 4 ].AsList().Count, Is.EqualTo( this.Points.Count ) );
 			}
 		}
 	}
diff --git a/test/MsgPack.UnitTest/Serialization/MapContextBasedAutoMessagePackSerializerTest.cs b/test/MsgPack.UnitTest/Serialization/MapContextBasedAutoMessagePackSerializerTest.cs
@@ -233,6 +233,7 @@ private void TestComplexObjectCore( SerializationContext context )
 		{
 			var target = new ComplexType() { Source = new Uri( "http://www.exambple.com" ), TimeStamp = DateTime.Now, Data = new byte[] { 0x1, 0x2, 0x3, 0x4 } };
 			target.History.Add( DateTime.Now.Subtract( TimeSpan.FromDays( 1 ) ), "Create New" );
+			target.Points.Add( 123 );
 			TestCoreWithVerify( target, context );
 		}
 
diff --git a/test/MsgPack.UnitTest/Serialization/MapExpressionBasedAutoMessagePackSerializerTest.cs b/test/MsgPack.UnitTest/Serialization/MapExpressionBasedAutoMessagePackSerializerTest.cs
@@ -233,6 +233,7 @@ private void TestComplexObjectCore( SerializationContext context )
 		{
 			var target = new ComplexType() { Source = new Uri( "http://www.exambple.com" ), TimeStamp = DateTime.Now, Data = new byte[] { 0x1, 0x2, 0x3, 0x4 } };
 			target.History.Add( DateTime.Now.Subtract( TimeSpan.FromDays( 1 ) ), "Create New" );
+			target.Points.Add( 123 );
 			TestCoreWithVerify( target, context );
 		}
 
diff --git a/test/MsgPack.UnitTest/Serialization/MapFieldBasedAutoMessagePackSerializerTest.cs b/test/MsgPack.UnitTest/Serialization/MapFieldBasedAutoMessagePackSerializerTest.cs
@@ -233,6 +233,7 @@ private void TestComplexObjectCore( SerializationContext context )
 		{
 			var target = new ComplexType() { Source = new Uri( "http://www.exambple.com" ), TimeStamp = DateTime.Now, Data = new byte[] { 0x1, 0x2, 0x3, 0x4 } };
 			target.History.Add( DateTime.Now.Subtract( TimeSpan.FromDays( 1 ) ), "Create New" );
+			target.Points.Add( 123 );
 			TestCoreWithVerify( target, context );
 		}
 
diff --git a/test/MsgPack.UnitTest/Serialization/MapReflectionBasedAutoMessagePackSerializerTest.cs b/test/MsgPack.UnitTest/Serialization/MapReflectionBasedAutoMessagePackSerializerTest.cs
@@ -233,6 +233,7 @@ private void TestComplexObjectCore( SerializationContext context )
 		{
 			var target = new ComplexType() { Source = new Uri( "http://www.exambple.com" ), TimeStamp = DateTime.Now, Data = new byte[] { 0x1, 0x2, 0x3, 0x4 } };
 			target.History.Add( DateTime.Now.Subtract( TimeSpan.FromDays( 1 ) ), "Create New" );
+			target.Points.Add( 123 );
 			TestCoreWithVerify( target, context );
 		}
 

Original file line number	Diff line number	Diff line change
`@@ -218,3 +218,4 @@ Relase 0.5.9 - 2015/2/01`
`218`	`218`
`219`	`219`	`BUG FIXES`
`220`	`220`	`* Fix NullReferenceException is thrown for annotated non-public members serialization/deserialization in WinRT.`
	`221`	`+ * Fix collection typed property which has non-public setter are not handled correctly. Issue #62.`
Original file line number	Diff line number	Diff line change
`@@ -203,28 +203,26 @@ public static IEnumerable<Type> GetInterfaces( this Type source )`
`203`	`203`
`204`	`204`	`public static MethodInfo GetGetMethod( this PropertyInfo source )`
`205`	`205`	`{`
`206`		`- return source.GetMethod;`
	`206`	`+ return GetGetMethod( source, false );`
`207`	`207`	`}`
`208`	`208`
`209`	`209`	`[System.Diagnostics.CodeAnalysis.SuppressMessage( "Microsoft.Usage", "CA1801:ReviewUnusedParameters", MessageId = "containsNonPublic", Justification = "For API compabitility" )]`
`210`	`210`	`public static MethodInfo GetGetMethod( this PropertyInfo source, bool containsNonPublic )`
`211`	`211`	`{`
`212`		`- Contract.Assert( containsNonPublic ); // false is not supported now.`
`213`		`-`
`214`		`- return source.GetMethod;`
	`212`	`+ var getter = source.GetMethod;`
	`213`	`+ return ( containsNonPublic \|\| getter.IsPublic ) ? getter : null;`
`215`	`214`	`}`
`216`	`215`
`217`	`216`	`public static MethodInfo GetSetMethod( this PropertyInfo source )`
`218`	`217`	`{`
`219`		`- return source.SetMethod;`
	`218`	`+ return GetSetMethod( source, false );`
`220`	`219`	`}`
`221`	`220`
`222`	`221`	`[System.Diagnostics.CodeAnalysis.SuppressMessage( "Microsoft.Usage", "CA1801:ReviewUnusedParameters", MessageId = "containsNonPublic", Justification = "For API compabitility" )]`
`223`	`222`	`public static MethodInfo GetSetMethod( this PropertyInfo source, bool containsNonPublic )`
`224`	`223`	`{`
`225`		`- Contract.Assert( containsNonPublic ); // false is not supported now.`
`226`		`-`
`227`		`- return source.SetMethod;`
	`224`	`+ var setter = source.SetMethod;`
	`225`	`+ return ( containsNonPublic \|\| setter.IsPublic ) ? setter : null;`
`228`	`226`	`}`
`229`	`227`
`230`	`228`	`public static IEnumerable<Type> FindInterfaces( this Type source, Func<Type, object, bool> filter, object filterCriteria )`
Original file line number	Diff line number	Diff line change
`@@ -2,7 +2,7 @@`
`2`	`2`	`//`
`3`	`3`	`// MessagePack for CLI`
`4`	`4`	`//`
`5`		`-// Copyright (C) 2010-2013 FUJIWARA, Yusuke`
	`5`	`+// Copyright (C) 2010-2015 FUJIWARA, Yusuke`
`6`	`6`	`//`
`7`	`7`	`// Licensed under the Apache License, Version 2.0 (the "License");`
`8`	`8`	`// you may not use this file except in compliance with the License.`
`@@ -315,15 +315,15 @@ private TConstruct EmitGetMemberValueExpression( TContext context, TConstruct in`
`315`	`315`	`FieldInfo asField;`
`316`	`316`	`if ( ( asField = member as FieldInfo ) != null )`
`317`	`317`	`{`
`318`		`- return this.EmitGetField( context, instance, asField, !asField.GetIsPublic() );`
	`318`	`+ return this.EmitGetField( context, instance, asField, !asField.GetHasPublicGetter() );`
`319`	`319`	`}`
`320`	`320`	`else`
`321`	`321`	`{`
`322`	`322`	`var asProperty = member as PropertyInfo;`
`323`	`323`	`#if DEBUG`
`324`	`324`	`Contract.Assert( asProperty != null, member.GetType().FullName );`
`325`	`325`	`#endif`
`326`		`- return this.EmitGetProperty( context, instance, asProperty, !asProperty.GetIsPublic() );`
	`326`	`+ return this.EmitGetProperty( context, instance, asProperty, !asProperty.GetHasPublicGetter() );`
`327`	`327`	`}`
`328`	`328`	`}`
`329`	`329`
`@@ -416,7 +416,7 @@ private TConstruct EmitSetMemberValueStatement( TContext context, TConstruct ins`
`416`	`416`	`return this.EmitSetField( context, instance, asField, value, false );`
`417`	`417`	`}`
`418`	`418`
`419`		`- getCollection = this.EmitGetField( context, instance, asField, !asField.GetIsPublic() );`
	`419`	`+ getCollection = this.EmitGetField( context, instance, asField, !asField.GetHasPublicGetter() );`
`420`	`420`	`traits = asField.FieldType.GetCollectionTraits();`
`421`	`421`	`}`
`422`	`422`	`else`
`@@ -431,7 +431,7 @@ private TConstruct EmitSetMemberValueStatement( TContext context, TConstruct ins`
`431`	`431`	`return this.EmitSetProprety( context, instance, asProperty, value, false );`
`432`	`432`	`}`
`433`	`433`
`434`		`- getCollection = this.EmitGetProperty( context, instance, asProperty, !asProperty.GetIsPublic() );`
	`434`	`+ getCollection = this.EmitGetProperty( context, instance, asProperty, !asProperty.GetHasPublicGetter() );`
`435`	`435`	`traits = asProperty.PropertyType.GetCollectionTraits();`
`436`	`436`	`}`
`437`	`437`
Original file line number	Diff line number	Diff line change
`@@ -233,6 +233,7 @@ private void TestComplexObjectCore( SerializationContext context )`
`233`	`233`	`{`
`234`	`234`	`var target = new ComplexType() { Source = new Uri( "http://www.exambple.com" ), TimeStamp = DateTime.Now, Data = new byte[] { 0x1, 0x2, 0x3, 0x4 } };`
`235`	`235`	`target.History.Add( DateTime.Now.Subtract( TimeSpan.FromDays( 1 ) ), "Create New" );`
	`236`	`+ target.Points.Add( 123 );`
`236`	`237`	`TestCoreWithVerify( target, context );`
`237`	`238`	`}`
`238`	`239`