Tweaks and more tests for maps - Change the default message hash code to 1 to be consistent with other code - Change the empty list/map hash code to 0 as "empty map" is equivalent to "no map" - Removed map fields from unittest_proto3.proto - Created map_unittest_proto3.proto which is like map_unittest.proto but proto3-only - Fixed factory methods in FieldCodec highlighted by using all field types :) - Added tests for map serialization: - Extra fields within entries - Entries with value then key - Non-contiguous entries for the same map - Multiple entries for the same key Changes to generated code coming in next commit
diff --git a/csharp/generate_protos.sh b/csharp/generate_protos.sh index 60bc281..e861f16 100755 --- a/csharp/generate_protos.sh +++ b/csharp/generate_protos.sh
@@ -43,10 +43,12 @@ rm src/google/protobuf/descriptor_proto_file.proto $PROTOC -Isrc --csharp_out=csharp/src/ProtocolBuffers.Test/TestProtos \ + src/google/protobuf/map_unittest_proto3.proto \ src/google/protobuf/unittest_proto3.proto \ src/google/protobuf/unittest_import_proto3.proto \ src/google/protobuf/unittest_import_public_proto3.proto + $PROTOC -Icsharp/protos/extest --csharp_out=csharp/src/ProtocolBuffers.Test/TestProtos \ csharp/protos/extest/unittest_issues.proto
diff --git a/csharp/src/ProtocolBuffers.Test/GeneratedMessageTest.cs b/csharp/src/ProtocolBuffers.Test/GeneratedMessageTest.cs index efeb7a9..f866866 100644 --- a/csharp/src/ProtocolBuffers.Test/GeneratedMessageTest.cs +++ b/csharp/src/ProtocolBuffers.Test/GeneratedMessageTest.cs
@@ -1,5 +1,4 @@ using System; -using System.Configuration; using System.IO; using Google.Protobuf.TestProtos; using NUnit.Framework; @@ -157,63 +156,204 @@ Assert.AreEqual(message, parsed); } + // Note that not every map within map_unittest_proto3 is used. They all go through very + // similar code paths. The fact that all maps are present is validation that we have codecs + // for every type. [Test] public void RoundTrip_Maps() { - var message = new TestAllTypes + var message = new TestMap { - MapBoolToEnum = { - { false, TestAllTypes.Types.NestedEnum.BAR}, - { true, TestAllTypes.Types.NestedEnum.BAZ} + MapBoolBool = { + { false, true }, + { true, false } }, - MapInt32ToBytes = { + MapInt32Bytes = { { 5, ByteString.CopyFrom(6, 7, 8) }, { 25, ByteString.CopyFrom(1, 2, 3, 4, 5) }, { 10, ByteString.Empty } }, - MapStringToNestedMessage = { - { "", new TestAllTypes.Types.NestedMessage { Bb = 10 } }, - { "null value", null }, + MapInt32ForeignMessage = { + { 0, new ForeignMessage { C = 10 } }, + { 5, null }, + }, + MapInt32Enum = { + { 1, MapEnum.MAP_ENUM_BAR }, + { 2000, MapEnum.MAP_ENUM_FOO } } }; byte[] bytes = message.ToByteArray(); - TestAllTypes parsed = TestAllTypes.Parser.ParseFrom(bytes); + TestMap parsed = TestMap.Parser.ParseFrom(bytes); Assert.AreEqual(message, parsed); } [Test] public void MapWithEmptyEntry() { - var message = new TestAllTypes + var message = new TestMap { - MapInt32ToBytes = { { 0, ByteString.Empty } } + MapInt32Bytes = { { 0, ByteString.Empty } } }; byte[] bytes = message.ToByteArray(); - Assert.AreEqual(3, bytes.Length); // Tag for field entry (2 bytes), length of entry (0; 1 byte) + Assert.AreEqual(2, bytes.Length); // Tag for field entry (1 byte), length of entry (0; 1 byte) - var parsed = TestAllTypes.Parser.ParseFrom(bytes); - Assert.AreEqual(1, parsed.MapInt32ToBytes.Count); - Assert.AreEqual(ByteString.Empty, parsed.MapInt32ToBytes[0]); + var parsed = TestMap.Parser.ParseFrom(bytes); + Assert.AreEqual(1, parsed.MapInt32Bytes.Count); + Assert.AreEqual(ByteString.Empty, parsed.MapInt32Bytes[0]); } - + [Test] public void MapWithOnlyValue() { // Hand-craft the stream to contain a single entry with just a value. var memoryStream = new MemoryStream(); var output = CodedOutputStream.CreateInstance(memoryStream); - output.WriteTag(TestAllTypes.MapStringToNestedMessageFieldNumber, WireFormat.WireType.LengthDelimited); - var nestedMessage = new TestAllTypes.Types.NestedMessage { Bb = 20 }; + output.WriteTag(TestMap.MapInt32ForeignMessageFieldNumber, WireFormat.WireType.LengthDelimited); + var nestedMessage = new ForeignMessage { C = 20 }; // Size of the entry (tag, size written by WriteMessage, data written by WriteMessage) output.WriteRawVarint32((uint)(nestedMessage.CalculateSize() + 3)); output.WriteTag(2, WireFormat.WireType.LengthDelimited); output.WriteMessage(nestedMessage); output.Flush(); - var parsed = TestAllTypes.Parser.ParseFrom(memoryStream.ToArray()); - Assert.AreEqual(nestedMessage, parsed.MapStringToNestedMessage[""]); + var parsed = TestMap.Parser.ParseFrom(memoryStream.ToArray()); + Assert.AreEqual(nestedMessage, parsed.MapInt32ForeignMessage[0]); + } + + [Test] + public void MapIgnoresExtraFieldsWithinEntryMessages() + { + // Hand-craft the stream to contain a single entry with three fields + var memoryStream = new MemoryStream(); + var output = CodedOutputStream.CreateInstance(memoryStream); + + output.WriteTag(TestMap.MapInt32Int32FieldNumber, WireFormat.WireType.LengthDelimited); + + var key = 10; // Field 1 + var value = 20; // Field 2 + var extra = 30; // Field 3 + + // Each field can be represented in a single byte, with a single byte tag. + // Total message size: 6 bytes. + output.WriteRawVarint32(6); + output.WriteTag(1, WireFormat.WireType.Varint); + output.WriteInt32(key); + output.WriteTag(2, WireFormat.WireType.Varint); + output.WriteInt32(value); + output.WriteTag(3, WireFormat.WireType.Varint); + output.WriteInt32(extra); + output.Flush(); + + var parsed = TestMap.Parser.ParseFrom(memoryStream.ToArray()); + Assert.AreEqual(value, parsed.MapInt32Int32[key]); + } + + [Test] + public void MapFieldOrderIsIrrelevant() + { + var memoryStream = new MemoryStream(); + var output = CodedOutputStream.CreateInstance(memoryStream); + + output.WriteTag(TestMap.MapInt32Int32FieldNumber, WireFormat.WireType.LengthDelimited); + + var key = 10; + var value = 20; + + // Each field can be represented in a single byte, with a single byte tag. + // Total message size: 4 bytes. + output.WriteRawVarint32(4); + output.WriteTag(2, WireFormat.WireType.Varint); + output.WriteInt32(value); + output.WriteTag(1, WireFormat.WireType.Varint); + output.WriteInt32(key); + output.Flush(); + + var parsed = TestMap.Parser.ParseFrom(memoryStream.ToArray()); + Assert.AreEqual(value, parsed.MapInt32Int32[key]); + } + + [Test] + public void MapNonContiguousEntries() + { + var memoryStream = new MemoryStream(); + var output = CodedOutputStream.CreateInstance(memoryStream); + + // Message structure: + // Entry for MapInt32Int32 + // Entry for MapStringString + // Entry for MapInt32Int32 + + // First entry + var key1 = 10; + var value1 = 20; + output.WriteTag(TestMap.MapInt32Int32FieldNumber, WireFormat.WireType.LengthDelimited); + output.WriteRawVarint32(4); + output.WriteTag(1, WireFormat.WireType.Varint); + output.WriteInt32(key1); + output.WriteTag(2, WireFormat.WireType.Varint); + output.WriteInt32(value1); + + // Second entry + var key2 = "a"; + var value2 = "b"; + output.WriteTag(TestMap.MapStringStringFieldNumber, WireFormat.WireType.LengthDelimited); + output.WriteRawVarint32(6); // 3 bytes per entry: tag, size, character + output.WriteTag(1, WireFormat.WireType.LengthDelimited); + output.WriteString(key2); + output.WriteTag(2, WireFormat.WireType.LengthDelimited); + output.WriteString(value2); + + // Third entry + var key3 = 15; + var value3 = 25; + output.WriteTag(TestMap.MapInt32Int32FieldNumber, WireFormat.WireType.LengthDelimited); + output.WriteRawVarint32(4); + output.WriteTag(1, WireFormat.WireType.Varint); + output.WriteInt32(key3); + output.WriteTag(2, WireFormat.WireType.Varint); + output.WriteInt32(value3); + + output.Flush(); + var parsed = TestMap.Parser.ParseFrom(memoryStream.ToArray()); + var expected = new TestMap + { + MapInt32Int32 = { { key1, value1 }, { key3, value3 } }, + MapStringString = { { key2, value2 } } + }; + Assert.AreEqual(expected, parsed); + } + + [Test] + public void DuplicateKeys_LastEntryWins() + { + var memoryStream = new MemoryStream(); + var output = CodedOutputStream.CreateInstance(memoryStream); + + var key = 10; + var value1 = 20; + var value2 = 30; + + // First entry + output.WriteTag(TestMap.MapInt32Int32FieldNumber, WireFormat.WireType.LengthDelimited); + output.WriteRawVarint32(4); + output.WriteTag(1, WireFormat.WireType.Varint); + output.WriteInt32(key); + output.WriteTag(2, WireFormat.WireType.Varint); + output.WriteInt32(value1); + + // Second entry - same key, different value + output.WriteTag(TestMap.MapInt32Int32FieldNumber, WireFormat.WireType.LengthDelimited); + output.WriteRawVarint32(4); + output.WriteTag(1, WireFormat.WireType.Varint); + output.WriteInt32(key); + output.WriteTag(2, WireFormat.WireType.Varint); + output.WriteInt32(value2); + output.Flush(); + + var parsed = TestMap.Parser.ParseFrom(memoryStream.ToArray()); + Assert.AreEqual(value2, parsed.MapInt32Int32[key]); } [Test]
diff --git a/csharp/src/ProtocolBuffers.Test/ProtocolBuffers.Test.csproj b/csharp/src/ProtocolBuffers.Test/ProtocolBuffers.Test.csproj index d11e259..c3fd8ba 100644 --- a/csharp/src/ProtocolBuffers.Test/ProtocolBuffers.Test.csproj +++ b/csharp/src/ProtocolBuffers.Test/ProtocolBuffers.Test.csproj
@@ -78,6 +78,7 @@ <Compile Include="GeneratedMessageTest.cs" /> <Compile Include="Collections\MapFieldTest.cs" /> <Compile Include="Collections\RepeatedFieldTest.cs" /> + <Compile Include="TestProtos\MapUnittestProto3.cs" /> <Compile Include="TestProtos\UnittestImportProto3.cs" /> <Compile Include="TestProtos\UnittestImportPublicProto3.cs" /> <Compile Include="TestProtos\UnittestIssues.cs" />
diff --git a/csharp/src/ProtocolBuffers/Collections/MapField.cs b/csharp/src/ProtocolBuffers/Collections/MapField.cs index 6834cfb..9eed833 100644 --- a/csharp/src/ProtocolBuffers/Collections/MapField.cs +++ b/csharp/src/ProtocolBuffers/Collections/MapField.cs
@@ -261,7 +261,7 @@ public override int GetHashCode() { var valueComparer = EqualityComparer<TValue>.Default; - int hash = 19; + int hash = 0; foreach (var pair in list) { hash ^= pair.Key.GetHashCode() * 31 + valueComparer.GetHashCode(pair.Value); @@ -380,7 +380,6 @@ private readonly Codec codec; internal TKey Key { get; set; } internal TValue Value { get; set; } - internal int Size { get; set; } internal MessageAdapter(Codec codec) {
diff --git a/csharp/src/ProtocolBuffers/Collections/RepeatedField.cs b/csharp/src/ProtocolBuffers/Collections/RepeatedField.cs index ebfc522..4d4212c 100644 --- a/csharp/src/ProtocolBuffers/Collections/RepeatedField.cs +++ b/csharp/src/ProtocolBuffers/Collections/RepeatedField.cs
@@ -193,7 +193,7 @@ public override int GetHashCode() { - int hash = 23; + int hash = 0; for (int i = 0; i < count; i++) { hash = hash * 31 + array[i].GetHashCode();
diff --git a/csharp/src/ProtocolBuffers/FieldCodec.cs b/csharp/src/ProtocolBuffers/FieldCodec.cs index 931b54d..617af2f 100644 --- a/csharp/src/ProtocolBuffers/FieldCodec.cs +++ b/csharp/src/ProtocolBuffers/FieldCodec.cs
@@ -33,11 +33,16 @@ return new FieldCodec<int>(input => input.ReadSInt32(), (output, value) => output.WriteSInt32(value), CodedOutputStream.ComputeSInt32Size, tag); } - public static FieldCodec<uint> ForFixedInt32(uint tag) + public static FieldCodec<uint> ForFixed32(uint tag) { return new FieldCodec<uint>(input => input.ReadFixed32(), (output, value) => output.WriteFixed32(value), CodedOutputStream.ComputeFixed32Size, tag); } + public static FieldCodec<int> ForSFixed32(uint tag) + { + return new FieldCodec<int>(input => input.ReadSFixed32(), (output, value) => output.WriteSFixed32(value), CodedOutputStream.ComputeSFixed32Size, tag); + } + public static FieldCodec<uint> ForUInt32(uint tag) { return new FieldCodec<uint>(input => input.ReadUInt32(), (output, value) => output.WriteUInt32(value), CodedOutputStream.ComputeUInt32Size, tag); @@ -53,11 +58,16 @@ return new FieldCodec<long>(input => input.ReadSInt64(), (output, value) => output.WriteSInt64(value), CodedOutputStream.ComputeSInt64Size, tag); } - public static FieldCodec<ulong> ForFixedInt64(uint tag) + public static FieldCodec<ulong> ForFixed64(uint tag) { return new FieldCodec<ulong>(input => input.ReadFixed64(), (output, value) => output.WriteFixed64(value), CodedOutputStream.ComputeFixed64Size, tag); } + public static FieldCodec<long> ForSFixed64(uint tag) + { + return new FieldCodec<long>(input => input.ReadSFixed64(), (output, value) => output.WriteSFixed64(value), CodedOutputStream.ComputeSFixed64Size, tag); + } + public static FieldCodec<ulong> ForUInt64(uint tag) { return new FieldCodec<ulong>(input => input.ReadUInt64(), (output, value) => output.WriteUInt64(value), CodedOutputStream.ComputeUInt64Size, tag);
diff --git a/src/google/protobuf/compiler/csharp/csharp_message.cc b/src/google/protobuf/compiler/csharp/csharp_message.cc index 9e055f6..9e2fe9b 100644 --- a/src/google/protobuf/compiler/csharp/csharp_message.cc +++ b/src/google/protobuf/compiler/csharp/csharp_message.cc
@@ -408,7 +408,7 @@ // Start with a non-zero value to easily distinguish between null and "empty" messages. printer->Print( "public override int GetHashCode() {\n" - " int hash = 17;\n"); + " int hash = 1;\n"); printer->Indent(); for (int i = 0; i < descriptor_->field_count(); i++) { scoped_ptr<FieldGeneratorBase> generator(
diff --git a/src/google/protobuf/map_unittest_proto3.proto b/src/google/protobuf/map_unittest_proto3.proto new file mode 100644 index 0000000..16be277 --- /dev/null +++ b/src/google/protobuf/map_unittest_proto3.proto
@@ -0,0 +1,120 @@ +// Protocol Buffers - Google's data interchange format +// Copyright 2008 Google Inc. All rights reserved. +// https://developers.google.com/protocol-buffers/ +// +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following disclaimer +// in the documentation and/or other materials provided with the +// distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived from +// this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +// This file is mostly equivalent to map_unittest.proto, but imports +// unittest_proto3.proto instead of unittest.proto, so that it only +// uses proto3 messages. This makes it suitable for testing +// implementations which only support proto3. +// The TestRequiredMessageMap message has been removed as there are no +// required fields in proto3. +syntax = "proto3"; + +option cc_enable_arenas = true; +option csharp_namespace = "Google.Protobuf.TestProtos"; + +import "google/protobuf/unittest_proto3.proto"; + +// We don't put this in a package within proto2 because we need to make sure +// that the generated code doesn't depend on being in the proto2 namespace. +// In map_test_util.h we do "using namespace unittest = protobuf_unittest". +package protobuf_unittest; + +// Tests maps. +message TestMap { + map<int32 , int32 > map_int32_int32 = 1; + map<int64 , int64 > map_int64_int64 = 2; + map<uint32 , uint32 > map_uint32_uint32 = 3; + map<uint64 , uint64 > map_uint64_uint64 = 4; + map<sint32 , sint32 > map_sint32_sint32 = 5; + map<sint64 , sint64 > map_sint64_sint64 = 6; + map<fixed32 , fixed32 > map_fixed32_fixed32 = 7; + map<fixed64 , fixed64 > map_fixed64_fixed64 = 8; + map<sfixed32, sfixed32> map_sfixed32_sfixed32 = 9; + map<sfixed64, sfixed64> map_sfixed64_sfixed64 = 10; + map<int32 , float > map_int32_float = 11; + map<int32 , double > map_int32_double = 12; + map<bool , bool > map_bool_bool = 13; + map<string , string > map_string_string = 14; + map<int32 , bytes > map_int32_bytes = 15; + map<int32 , MapEnum > map_int32_enum = 16; + map<int32 , ForeignMessage> map_int32_foreign_message = 17; +} + +message TestMapSubmessage { + TestMap test_map = 1; +} + +message TestMessageMap { + map<int32, TestAllTypes> map_int32_message = 1; +} + +// Two map fields share the same entry default instance. +message TestSameTypeMap { + map<int32, int32> map1 = 1; + map<int32, int32> map2 = 2; +} + +enum MapEnum { + MAP_ENUM_FOO = 0; + MAP_ENUM_BAR = 1; + MAP_ENUM_BAZ = 2; +} + +message TestArenaMap { + map<int32 , int32 > map_int32_int32 = 1; + map<int64 , int64 > map_int64_int64 = 2; + map<uint32 , uint32 > map_uint32_uint32 = 3; + map<uint64 , uint64 > map_uint64_uint64 = 4; + map<sint32 , sint32 > map_sint32_sint32 = 5; + map<sint64 , sint64 > map_sint64_sint64 = 6; + map<fixed32 , fixed32 > map_fixed32_fixed32 = 7; + map<fixed64 , fixed64 > map_fixed64_fixed64 = 8; + map<sfixed32, sfixed32> map_sfixed32_sfixed32 = 9; + map<sfixed64, sfixed64> map_sfixed64_sfixed64 = 10; + map<int32 , float > map_int32_float = 11; + map<int32 , double > map_int32_double = 12; + map<bool , bool > map_bool_bool = 13; + map<int32 , MapEnum > map_int32_enum = 14; + map<int32 , ForeignMessage> map_int32_foreign_message = 15; +} + +// Previously, message containing enum called Type cannot be used as value of +// map field. +message MessageContainingEnumCalledType { + enum Type { + TYPE_FOO = 0; + } + map<int32, MessageContainingEnumCalledType> type = 1; +} + +// Previously, message cannot contain map field called "entry". +message MessageContainingMapCalledEntry { + map<int32, int32> entry = 1; +}
diff --git a/src/google/protobuf/unittest_proto3.proto b/src/google/protobuf/unittest_proto3.proto index 41fa56d..f59d217 100644 --- a/src/google/protobuf/unittest_proto3.proto +++ b/src/google/protobuf/unittest_proto3.proto
@@ -140,11 +140,6 @@ string oneof_string = 113; bytes oneof_bytes = 114; } - - // Sample maps - map<string, NestedMessage> map_string_to_nested_message = 200; - map<int32, bytes> map_int32_to_bytes = 201; - map<bool, NestedEnum> map_bool_to_enum = 202; } // This proto includes a recusively nested message.