From 71c492da3cd51ba6f8030be06cd861af96dcf84b Mon Sep 17 00:00:00 2001 From: Sydney Acksman Date: Thu, 19 Sep 2019 22:51:31 -0500 Subject: [PATCH 1/6] Add some missing null-checks Remove IsInitialized checks accidentally left in MessageParser Simplify ExtensionCollection.CrossLink --- csharp/src/Google.Protobuf/CodedOutputStream.cs | 2 +- csharp/src/Google.Protobuf/JsonFormatter.cs | 2 +- csharp/src/Google.Protobuf/JsonParser.cs | 2 +- csharp/src/Google.Protobuf/MessageParser.cs | 12 ------------ .../Reflection/ExtensionCollection.cs | 10 ++++------ 5 files changed, 7 insertions(+), 21 deletions(-) diff --git a/csharp/src/Google.Protobuf/CodedOutputStream.cs b/csharp/src/Google.Protobuf/CodedOutputStream.cs index f9ad29086..1d76d2760 100644 --- a/csharp/src/Google.Protobuf/CodedOutputStream.cs +++ b/csharp/src/Google.Protobuf/CodedOutputStream.cs @@ -89,7 +89,7 @@ namespace Google.Protobuf private CodedOutputStream(byte[] buffer, int offset, int length) { this.output = null; - this.buffer = buffer; + this.buffer = ProtoPreconditions.CheckNotNull(buffer, nameof(buffer)); this.position = offset; this.limit = offset + length; leaveOpen = true; // Simple way of avoiding trying to dispose of a null reference diff --git a/csharp/src/Google.Protobuf/JsonFormatter.cs b/csharp/src/Google.Protobuf/JsonFormatter.cs index 2b4c5f0a8..5aaefe7a8 100644 --- a/csharp/src/Google.Protobuf/JsonFormatter.cs +++ b/csharp/src/Google.Protobuf/JsonFormatter.cs @@ -133,7 +133,7 @@ namespace Google.Protobuf /// The settings. public JsonFormatter(Settings settings) { - this.settings = settings; + this.settings = ProtoPreconditions.CheckNotNull(settings, nameof(settings)); } /// diff --git a/csharp/src/Google.Protobuf/JsonParser.cs b/csharp/src/Google.Protobuf/JsonParser.cs index fb594f2a4..3f88ea38f 100644 --- a/csharp/src/Google.Protobuf/JsonParser.cs +++ b/csharp/src/Google.Protobuf/JsonParser.cs @@ -110,7 +110,7 @@ namespace Google.Protobuf /// The settings. public JsonParser(Settings settings) { - this.settings = settings; + this.settings = ProtoPreconditions.CheckNotNull(settings, nameof(settings)); } /// diff --git a/csharp/src/Google.Protobuf/MessageParser.cs b/csharp/src/Google.Protobuf/MessageParser.cs index 76a350ce4..06d0f1059 100644 --- a/csharp/src/Google.Protobuf/MessageParser.cs +++ b/csharp/src/Google.Protobuf/MessageParser.cs @@ -72,7 +72,6 @@ namespace Google.Protobuf { IMessage message = factory(); message.MergeFrom(data, DiscardUnknownFields, Extensions); - CheckMergedRequiredFields(message); return message; } @@ -87,7 +86,6 @@ namespace Google.Protobuf { IMessage message = factory(); message.MergeFrom(data, offset, length, DiscardUnknownFields, Extensions); - CheckMergedRequiredFields(message); return message; } @@ -100,7 +98,6 @@ namespace Google.Protobuf { IMessage message = factory(); message.MergeFrom(data, DiscardUnknownFields, Extensions); - CheckMergedRequiredFields(message); return message; } @@ -113,7 +110,6 @@ namespace Google.Protobuf { IMessage message = factory(); message.MergeFrom(input, DiscardUnknownFields, Extensions); - CheckMergedRequiredFields(message); return message; } @@ -130,7 +126,6 @@ namespace Google.Protobuf { IMessage message = factory(); message.MergeDelimitedFrom(input, DiscardUnknownFields, Extensions); - CheckMergedRequiredFields(message); return message; } @@ -143,7 +138,6 @@ namespace Google.Protobuf { IMessage message = factory(); MergeFrom(message, input); - CheckMergedRequiredFields(message); return message; } @@ -176,12 +170,6 @@ namespace Google.Protobuf } } - internal static void CheckMergedRequiredFields(IMessage message) - { - if (!message.IsInitialized()) - throw new InvalidOperationException("Parsed message does not contain all required fields"); - } - /// /// Creates a new message parser which optionally discards unknown fields when parsing. /// diff --git a/csharp/src/Google.Protobuf/Reflection/ExtensionCollection.cs b/csharp/src/Google.Protobuf/Reflection/ExtensionCollection.cs index 38e33d7ff..9196b50dd 100644 --- a/csharp/src/Google.Protobuf/Reflection/ExtensionCollection.cs +++ b/csharp/src/Google.Protobuf/Reflection/ExtensionCollection.cs @@ -107,13 +107,11 @@ namespace Google.Protobuf.Reflection { descriptor.CrossLink(); - IList _; - if (!declarationOrder.TryGetValue(descriptor.ExtendeeType, out _)) - { - declarationOrder.Add(descriptor.ExtendeeType, new List()); - } + IList list; + if (!declarationOrder.TryGetValue(descriptor.ExtendeeType, out list)) + declarationOrder.Add(descriptor.ExtendeeType, list = new List()); - declarationOrder[descriptor.ExtendeeType].Add(descriptor); + list.Add(descriptor); } extensionsByTypeInDeclarationOrder = declarationOrder From 3457f8894074a3eb114250a3f2ce398628fc2cbd Mon Sep 17 00:00:00 2001 From: Sydney Acksman Date: Wed, 20 Nov 2019 10:55:48 -0600 Subject: [PATCH 2/6] Fix formatting --- csharp/src/Google.Protobuf/Reflection/ExtensionCollection.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/csharp/src/Google.Protobuf/Reflection/ExtensionCollection.cs b/csharp/src/Google.Protobuf/Reflection/ExtensionCollection.cs index 9196b50dd..a4f144aa8 100644 --- a/csharp/src/Google.Protobuf/Reflection/ExtensionCollection.cs +++ b/csharp/src/Google.Protobuf/Reflection/ExtensionCollection.cs @@ -109,7 +109,9 @@ namespace Google.Protobuf.Reflection IList list; if (!declarationOrder.TryGetValue(descriptor.ExtendeeType, out list)) + { declarationOrder.Add(descriptor.ExtendeeType, list = new List()); + } list.Add(descriptor); } From 6f09cc3a0c6acd77f3791e003f0e3aa72b257edc Mon Sep 17 00:00:00 2001 From: Sydney Acksman Date: Wed, 20 Nov 2019 10:56:34 -0600 Subject: [PATCH 3/6] Fix readability. --- csharp/src/Google.Protobuf/Reflection/ExtensionCollection.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/csharp/src/Google.Protobuf/Reflection/ExtensionCollection.cs b/csharp/src/Google.Protobuf/Reflection/ExtensionCollection.cs index a4f144aa8..9664559df 100644 --- a/csharp/src/Google.Protobuf/Reflection/ExtensionCollection.cs +++ b/csharp/src/Google.Protobuf/Reflection/ExtensionCollection.cs @@ -110,7 +110,8 @@ namespace Google.Protobuf.Reflection IList list; if (!declarationOrder.TryGetValue(descriptor.ExtendeeType, out list)) { - declarationOrder.Add(descriptor.ExtendeeType, list = new List()); + list = new List(); + declarationOrder.Add(descriptor.ExtendeeType, list); } list.Add(descriptor); From f084d622c282db684f59d60b95f45a76d5180e49 Mon Sep 17 00:00:00 2001 From: Sydney Acksman Date: Wed, 20 Nov 2019 11:54:20 -0600 Subject: [PATCH 4/6] Add test for not throwing on missing required --- .../Google.Protobuf.Test/GeneratedMessageTest.Proto2.cs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/csharp/src/Google.Protobuf.Test/GeneratedMessageTest.Proto2.cs b/csharp/src/Google.Protobuf.Test/GeneratedMessageTest.Proto2.cs index e1d4d7809..704f2e5af 100644 --- a/csharp/src/Google.Protobuf.Test/GeneratedMessageTest.Proto2.cs +++ b/csharp/src/Google.Protobuf.Test/GeneratedMessageTest.Proto2.cs @@ -261,6 +261,13 @@ namespace Google.Protobuf Assert.True(message.IsInitialized()); } + [Test] + public void RequiredFieldsNoThrow() + { + TestRequired.Parser.ParseFrom(new byte[0]); + (TestRequired.Parser as MessageParser).ParseFrom(new byte[0]); + } + [Test] public void RequiredFieldsInExtensions() { From 1a0ff9551a0720fef95a21946d9c8a1134ebda03 Mon Sep 17 00:00:00 2001 From: Sydney Acksman Date: Mon, 2 Dec 2019 13:50:51 -0600 Subject: [PATCH 5/6] Add comment and Assert.DoesNotThrow to RequiredFieldsNoThrow --- .../Google.Protobuf.Test/GeneratedMessageTest.Proto2.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/csharp/src/Google.Protobuf.Test/GeneratedMessageTest.Proto2.cs b/csharp/src/Google.Protobuf.Test/GeneratedMessageTest.Proto2.cs index 704f2e5af..1e22674c7 100644 --- a/csharp/src/Google.Protobuf.Test/GeneratedMessageTest.Proto2.cs +++ b/csharp/src/Google.Protobuf.Test/GeneratedMessageTest.Proto2.cs @@ -261,11 +261,14 @@ namespace Google.Protobuf Assert.True(message.IsInitialized()); } + // Code was accidentally left in message parser that threw exceptions when missing required fields after parsing. + // We've decided to not throw exceptions on missing fields, instead leaving it up to the consumer how they + // want to check and handle missing fields. [Test] public void RequiredFieldsNoThrow() { - TestRequired.Parser.ParseFrom(new byte[0]); - (TestRequired.Parser as MessageParser).ParseFrom(new byte[0]); + Assert.DoesNotThrow(() => TestRequired.Parser.ParseFrom(new byte[0])); + Assert.DoesNotThrow(() => (TestRequired.Parser as MessageParser).ParseFrom(new byte[0])); } [Test] From d5e964c8bb39a365b3d958c55cdc405049aacf29 Mon Sep 17 00:00:00 2001 From: Sydney Acksman Date: Tue, 3 Dec 2019 10:46:34 -0600 Subject: [PATCH 6/6] Make test comment a summary --- .../Google.Protobuf.Test/GeneratedMessageTest.Proto2.cs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/csharp/src/Google.Protobuf.Test/GeneratedMessageTest.Proto2.cs b/csharp/src/Google.Protobuf.Test/GeneratedMessageTest.Proto2.cs index 1e22674c7..718c3edcb 100644 --- a/csharp/src/Google.Protobuf.Test/GeneratedMessageTest.Proto2.cs +++ b/csharp/src/Google.Protobuf.Test/GeneratedMessageTest.Proto2.cs @@ -261,9 +261,11 @@ namespace Google.Protobuf Assert.True(message.IsInitialized()); } - // Code was accidentally left in message parser that threw exceptions when missing required fields after parsing. - // We've decided to not throw exceptions on missing fields, instead leaving it up to the consumer how they - // want to check and handle missing fields. + /// + /// Code was accidentally left in message parser that threw exceptions when missing required fields after parsing. + /// We've decided to not throw exceptions on missing fields, instead leaving it up to the consumer how they + /// want to check and handle missing fields. + /// [Test] public void RequiredFieldsNoThrow() {