Skip to content

Commit 6e14337

Browse files
yfakariyayfakariya
committed
Fix Unpacker.ReadObject does not handle collection. #143
The bug causes UnpackSingleObject failes to deserialize collections. This fix adds new flag which indicates ReadSubtreeObject should unpack collections recursively instead of just unpacking their length. For compatibility to Read() implementation, ReadSubtreeObject just unpack lengthes if the flag is false.
1 parent 6e723c1 commit 6e14337

File tree

9 files changed

+829
-21
lines changed

9 files changed

+829
-21
lines changed

‎src/MsgPack/ItemsUnpacker.Read.cs‎

Lines changed: 55 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
#endif
2626

2727
using System;
28+
using System.Collections.Generic;
2829

2930
namespace MsgPack
3031
{
@@ -1579,10 +1580,10 @@ public override bool ReadObject( out MessagePackObject result )
15791580
this.EnsureNotInSubtreeMode();
15801581
#endif // !UNITY
15811582

1582-
return this.ReadSubtreeObject( out result );
1583+
return this.ReadSubtreeObject( /* isDeep */true, out result );
15831584
}
15841585

1585-
internal bool ReadSubtreeObject( out MessagePackObject result )
1586+
internal bool ReadSubtreeObject( bool isDeep, out MessagePackObject result )
15861587
{
15871588
byte header;
15881589
long integral;
@@ -1682,14 +1683,62 @@ internal bool ReadSubtreeObject( out MessagePackObject result )
16821683
}
16831684
case ReadValueResult.ArrayLength:
16841685
{
1685-
result = unchecked( ( UInt32 )this.ReadArrayLengthCore( integral ) );
1686+
var length = unchecked( ( UInt32 )this.ReadArrayLengthCore( integral ) );
1687+
if ( !isDeep )
1688+
{
1689+
result = length;
1690+
return true;
1691+
}
1692+
1693+
this.CheckLength( length, ReadValueResult.ArrayLength );
1694+
var collection = new List<MessagePackObject>( unchecked( ( int ) length ) );
1695+
for( var i = 0; i < length; i++ )
1696+
{
1697+
MessagePackObject item;
1698+
if( !this.ReadSubtreeObject( /* isDeep */true, out item ) )
1699+
{
1700+
result = default( MessagePackObject );
1701+
return false;
1702+
}
1703+
1704+
collection.Add( item );
1705+
}
1706+
result = new MessagePackObject( collection, /* isImmutable */true );
16861707
return true;
16871708
}
16881709
case ReadValueResult.MapLength:
16891710
{
1690-
result = unchecked( ( UInt32 )this.ReadMapLengthCore( integral ) );
1691-
return true;
1692-
}
1711+
var length = unchecked( ( UInt32 )this.ReadMapLengthCore( integral ) );
1712+
if ( !isDeep )
1713+
{
1714+
result = length;
1715+
return true;
1716+
}
1717+
1718+
this.CheckLength( length, ReadValueResult.MapLength );
1719+
var collection = new MessagePackObjectDictionary( unchecked( ( int ) length ) );
1720+
for( var i = 0; i < length; i++ )
1721+
{
1722+
MessagePackObject key;
1723+
if( !this.ReadSubtreeObject( /* isDeep */true, out key ) )
1724+
{
1725+
result = default( MessagePackObject );
1726+
return false;
1727+
}
1728+
1729+
MessagePackObject value;
1730+
if( !this.ReadSubtreeObject( /* isDeep */true, out value ) )
1731+
{
1732+
result = default( MessagePackObject );
1733+
return false;
1734+
}
1735+
1736+
collection.Add( key, value );
1737+
}
1738+
result = new MessagePackObject( collection, /* isImmutable */true );
1739+
return true;
1740+
}
1741+
16931742
case ReadValueResult.String:
16941743
{
16951744
result = new MessagePackObject( new MessagePackString( this.ReadBinaryCore( integral ), false ) );

‎src/MsgPack/ItemsUnpacker.Read.tt‎

Lines changed: 63 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
#endif
3636

3737
using System;
38+
using System.Collections.Generic;
3839

3940
namespace MsgPack
4041
{
@@ -115,8 +116,10 @@ private static readonly TypeCodeMapping[] TypeCodeMappings =
115116

116117
private void WriteReadBody( string typeName, string fullTypeName, bool isForSubtree, Action bodyWriter )
117118
{
119+
var firstParameter = ( isForSubtree && typeName == "Object" ) ? "bool isDeep, " : String.Empty;
120+
var firstArgument = ( !isForSubtree && typeName == "Object" ) ? "/* isDeep */true, " : String.Empty;
118121
#>
119-
<#= isForSubtree ? "internal" : "public override" #> bool Read<#= isForSubtree ? "Subtree" : String.Empty #><#= typeName #>( out <#= fullTypeName #> result )
122+
<#= isForSubtree ? "internal" : "public override" #> bool Read<#= isForSubtree ? "Subtree" : String.Empty #><#= typeName #>( <#= firstParameter #>out <#= fullTypeName #> result )
120123
{
121124
<#+
122125
if( !isForSubtree )
@@ -126,7 +129,7 @@ private void WriteReadBody( string typeName, string fullTypeName, bool isForSubt
126129
this.EnsureNotInSubtreeMode();
127130
#endif // !UNITY
128131

129-
return this.ReadSubtree<#= typeName #>( out result );
132+
return this.ReadSubtree<#= typeName #>( <#= firstArgument #>out result );
130133
<#+
131134
}
132135
else
@@ -385,16 +388,69 @@ case ReadValueResult.<#= entry.Code #>:
385388
<#+
386389
} // foreach -- scalar
387390

388-
foreach( var collection in new [] { "Array", "Map" } )
389-
{
391+
// Array/Map
390392
#>
391-
case ReadValueResult.<#= collection #>Length:
393+
case ReadValueResult.ArrayLength:
392394
{
393-
<#= resultVariable #> = <#= CastIfNecessary( typeof( long ), typeof( uint ), false, "this.Read"+ collection + "LengthCore( " + valueVariable.Integral + " )" ) #>;
395+
var length = <#= CastIfNecessary( typeof( long ), typeof( uint ), false, "this.ReadArrayLengthCore( " + valueVariable.Integral + " )", false ) #>;
396+
if ( !isDeep )
397+
{
398+
<#= resultVariable #> = length;
399+
return true;
400+
}
401+
402+
this.CheckLength( length, ReadValueResult.ArrayLength );
403+
var collection = new List<MessagePackObject>( unchecked( ( int ) length ) );
404+
for( var i = 0; i < length; i++ )
405+
{
406+
MessagePackObject item;
407+
if( !this.ReadSubtreeObject( /* isDeep */true, out item ) )
408+
{
409+
<#= resultVariable #> = default( MessagePackObject );
410+
return false;
411+
}
412+
413+
collection.Add( item );
414+
}
415+
<#= resultVariable #> = new MessagePackObject( collection, /* isImmutable */true );
394416
return true;
395417
}
418+
case ReadValueResult.MapLength:
419+
{
420+
var length = <#= CastIfNecessary( typeof( long ), typeof( uint ), false, "this.ReadMapLengthCore( " + valueVariable.Integral + " )", false ) #>;
421+
if ( !isDeep )
422+
{
423+
<#= resultVariable #> = length;
424+
return true;
425+
}
426+
427+
this.CheckLength( length, ReadValueResult.MapLength );
428+
var collection = new MessagePackObjectDictionary( unchecked( ( int ) length ) );
429+
for( var i = 0; i < length; i++ )
430+
{
431+
MessagePackObject key;
432+
if( !this.ReadSubtreeObject( /* isDeep */true, out key ) )
433+
{
434+
<#= resultVariable #> = default( MessagePackObject );
435+
return false;
436+
}
437+
438+
MessagePackObject value;
439+
if( !this.ReadSubtreeObject( /* isDeep */true, out value ) )
440+
{
441+
<#= resultVariable #> = default( MessagePackObject );
442+
return false;
443+
}
444+
445+
collection.Add( key, value );
446+
}
447+
<#= resultVariable #> = new MessagePackObject( collection, /* isImmutable */true );
448+
return true;
449+
}
450+
396451
<#+
397-
} // foreach -- collection
452+
// Array/Map
453+
398454
#>
399455
case ReadValueResult.String:
400456
{

‎src/MsgPack/ItemsUnpacker.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
//
33
// MessagePack for CLI
44
//
5-
// Copyright (C) 2010-2014 FUJIWARA, Yusuke
5+
// Copyright (C) 2010-2016 FUJIWARA, Yusuke
66
//
77
// Licensed under the Apache License, Version 2.0 (the "License");
88
// you may not use this file except in compliance with the License.
@@ -121,7 +121,7 @@ protected override void Dispose( bool disposing )
121121
protected override bool ReadCore()
122122
{
123123
MessagePackObject value;
124-
var success = this.ReadSubtreeObject( out value );
124+
var success = this.ReadSubtreeObject( false, out value );
125125
if ( success )
126126
{
127127
this.InternalData = value;

‎src/MsgPack/SubtreeUnpacker.Unpacking.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
//
44
// MessagePack for CLI
55
//
6-
// Copyright (C) 2010-2012 FUJIWARA, Yusuke
6+
// Copyright (C) 2010-2016 FUJIWARA, Yusuke
77
//
88
// Licensed under the Apache License, Version 2.0 (the "License");
99
// you may not use this file except in compliance with the License.
@@ -1146,7 +1146,7 @@ public override bool ReadObject( out MessagePackObject result )
11461146
return false;
11471147
}
11481148

1149-
if ( !this._root.ReadSubtreeObject( out result ) )
1149+
if ( !this._root.ReadSubtreeObject( /* isDeep */true, out result ) )
11501150
{
11511151
return false;
11521152
}

‎src/MsgPack/SubtreeUnpacker.Unpacking.tt‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
//
1313
// MessagePack for CLI
1414
//
15-
// Copyright (C) 2010-2012 FUJIWARA, Yusuke
15+
// Copyright (C) 2010-2016 FUJIWARA, Yusuke
1616
//
1717
// Licensed under the Apache License, Version 2.0 (the "License");
1818
// you may not use this file except in compliance with the License.
@@ -122,14 +122,14 @@ this.PopIndent();
122122
{
123123
<#
124124
this.PushIndent( 3 );
125-
this.WriteRead( "Object", "MessagePackObject" );
125+
this.WriteRead( "Object", "MessagePackObject", "/* isDeep */true, " );
126126
this.PopIndent();
127127
#>
128128
}
129129
}
130130
}
131131
<#+
132-
private void WriteRead( string typeName, string fullTypeName )
132+
private void WriteRead( string typeName, string fullTypeName, string isDeep = "" )
133133
{
134134
#>
135135
this.DiscardCompletedStacks();
@@ -140,7 +140,7 @@ if ( this._itemsCount.Count == 0 )
140140
return false;
141141
}
142142

143-
if ( !this._root.ReadSubtree<#= typeName #>( out result ) )
143+
if ( !this._root.ReadSubtree<#= typeName #>( <#= isDeep #>out result ) )
144144
{
145145
return false;
146146
}

‎test/MsgPack.UnitTest/MsgPack.UnitTest.csproj‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1329,6 +1329,11 @@
13291329
<DependentUpon>UnpackerTest.Ext.tt</DependentUpon>
13301330
</Compile>
13311331
<Compile Include="UnpackerTest.cs" />
1332+
<Compile Include="UnpackerTest.Object.cs">
1333+
<AutoGen>True</AutoGen>
1334+
<DesignTime>True</DesignTime>
1335+
<DependentUpon>UnpackerTest.Object.tt</DependentUpon>
1336+
</Compile>
13321337
<Compile Include="UnpackerTest.Raw.cs">
13331338
<AutoGen>True</AutoGen>
13341339
<DesignTime>True</DesignTime>
@@ -1759,6 +1764,10 @@
17591764
<ItemGroup />
17601765
<ItemGroup>
17611766
<Content Include="gen\ReadMe.txt" />
1767+
<Content Include="UnpackerTest.Object.tt">
1768+
<Generator>TextTemplatingFileGenerator</Generator>
1769+
<LastGenOutput>UnpackerTest.Object.cs</LastGenOutput>
1770+
</Content>
17621771
</ItemGroup>
17631772
<Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />
17641773
<Import Project="$(SolutionDir)\.nuget\nuget.targets" />

‎test/MsgPack.UnitTest/Serialization/RegressionTests.cs‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424

2525
using System;
2626
using System.Collections.Generic;
27+
using System.Diagnostics;
2728
using System.IO;
2829
using System.Linq;
2930
using System.Runtime.Serialization;
@@ -253,5 +254,30 @@ protected internal override TestValueTypeWrapper UnpackFromCore( Unpacker unpack
253254
return new TestValueTypeWrapper { Value = this._serializer0.UnpackFrom( unpacker ) };
254255
}
255256
}
257+
258+
[Test]
259+
public void Issue143()
260+
{
261+
var array =
262+
new object[]
263+
{
264+
"111",
265+
32432,
266+
new int[] { 9, 8 },
267+
909
268+
};
269+
var serializer = MessagePackSerializer.Get<object>();
270+
var packedBinary = serializer.PackSingleObject( array );
271+
var unpacked = serializer.UnpackSingleObject( packedBinary );
272+
var unpackedList = ( ( MessagePackObject )unpacked ).AsList();
273+
Assert.That( unpackedList.Count, Is.EqualTo( 4 ) );
274+
Assert.That( unpackedList[ 0 ] == "111" );
275+
Assert.That( unpackedList[ 1 ] == 32432 );
276+
Assert.That( unpackedList[ 2 ].IsList );
277+
Assert.That( unpackedList[ 2 ].AsList().Count, Is.EqualTo( 2 ) );
278+
Assert.That( unpackedList[ 2 ].AsList()[ 0 ] == 9 );
279+
Assert.That( unpackedList[ 2 ].AsList()[ 1 ] == 8 );
280+
Assert.That( unpackedList[ 3 ] == 909 );
281+
}
256282
}
257283
}

0 commit comments

Comments
 (0)