Add null checks to GetOption #7130

ObsidianMinor · 2020-01-23T21:57:56Z

Null continues to erode away at my soul.

Should we add a TryGetOption API in another PR to better match up with the CustomOptions API @jtattermusch?

csharp/src/Google.Protobuf.Test/Reflection/CustomOptionsTest.cs

thesoftwarejedi

Using default(T) works fine for our use cases.

Falco20019 · 2020-03-05T15:47:17Z

I also just ran into that problem and would like the idea of having TryGetOption. I'm iterating over some fields and have to check if any has the option set. This is not possible with only using default(T).

thesoftwarejedi · 2020-03-05T19:26:24Z

@Falco20019 I suggest we merge this, but also add the Try versions of all methods in another PR. I can try to get to coding that this weekend using #7240 as the basis for it.

I'm not familiar with the release pipeline here or how to push this along. I think @ObsidianMinor is king.

ObsidianMinor · 2020-03-05T20:10:54Z

@thesoftwarejedi I'm working on a TryGet PR right now. Also I don't work at Google, I'm just a friendly OSS contributor. Ask @jtattermusch.

thesoftwarejedi · 2020-03-05T20:33:33Z

@thesoftwarejedi I'm working on a TryGet PR right now.

Perfect, thanks! Saved my weekend ;)

Also I don't work at Google, I'm just a friendly OSS contributor.

I figured as much. Thanks for your contributions!

ozevren · 2020-03-13T18:53:00Z

Is there any way I can help with getting this checked in? This and the Try overload our blocking some of my, so I am highly motivated to help out here. :)

jtattermusch · 2020-04-22T10:28:15Z

csharp/src/Google.Protobuf/Reflection/EnumDescriptor.cs

+            var options = Proto.Options;
+            if (options == null)
+            {
+                return default(T);


I don't think returning default(T) is a good idea. It seems that for e.g. extension of type int, you won't be able to differentiate between the option not being present at all (which should return null) and the option having value of 0.

Given that the GetOption API is relatively new (and looks like it has been kind of broken for the very beginning), should we just introduce TryGetOption methods instead, to keep the semantics clean?

the GetOption can either be removed (that should be fine because it was only introduced as part of the proto2 work and we've marked proto2 support as "experimental") or it can just throw an exception if the value is not present (which renders it not very useful, so we might wanna mark it as obsolete?).

Returning default(T) is a mistake. It should be the default value for the extension.

Still, even if you return extension.DefaultValue, it is unclear to me how I would distinguish between 1. the option not being present at all and 2. the option being present and being set to the default value (AFAIK options/extensions don't need to have default values).

You can't, which is why #7282 was made. Should we also add a HasOption API to go with it?

jtattermusch · 2020-04-22T10:30:37Z

csharp/src/Google.Protobuf.Test/Reflection/CustomOptionsTest.cs

@@ -184,13 +184,21 @@ public void NoOptions()
            var fileDescriptor = UnittestProto3Reflection.Descriptor;
            var messageDescriptor = TestAllTypes.Descriptor;
            Assert.NotNull(fileDescriptor.CustomOptions);
+            Assert.AreEqual(fileDescriptor.GetOption(Fileopt), null);


why are we intermixing the GetOption tests with a CustomOptions-related test (and all that in CustomOptionsTest.cs).
I think it would be better if GetOption methods was tested in a separate test case.

I thought that even though the GetOption APIs weren't in a "CustomOptions" type they still worked with "custom options" so it'd be easier to test everything relating to custom options together.

I'm probably fine keeping the test in the same file (CustomOptionsTest.cs), but we should at least put them in a separate test case (method) as I proposed. I think like this the test will be pretty hard to maintain and refactor
(and frankly it's not clear to me why Assert.NotNull(messageDescriptor.Fields[1].CustomOptions); and Assert.AreEqual(messageDescriptor.Fields[1].GetOption(Fieldopt), null); would be related.

Falco20019 · 2020-04-24T04:57:21Z

Wouldn’t it be nicer if Proto.Options would not be null but always defined empty and that GetExtension would then just return the DefaultValue of the extension requested? This would avoid a lot of code duplication.

jtattermusch · 2020-04-27T16:02:14Z

csharp/src/Google.Protobuf/Reflection/FileDescriptor.cs

@@ -555,7 +555,13 @@ public override string ToString()
        /// </summary>
        public T GetOption<T>(Extension<FileOptions, T> extension)
        {
-            var value = Proto.Options.GetExtension(extension);
+            var options = Proto.Options;
+            if (options == null)


actually, looking at the whole options API in protobuf C# from a more high level perspective, we only need to check whether options == null because e.g. the FileDescriptor doesn't provide access to the file options (the property FileDescriptor.Proto is internal, so one cannot access to FileDescriptor.Proto.Options).

This actually has several downsides:

we need extra logic such as a GetOption methods, instead of just exposing e.g. the FileOptions msg (or other options) on a descriptor and letting the user use GetExtension methods the same way they would do it for any other message with extensions (which is the way reading custom options is done for e.g. C++, Java and python, as documented here: https://developers.google.com/protocol-buffers/docs/proto#customoptions)

because the "options" messages are not exposed, one cannot access the standard (=the non-extension ones) options at all.

This makes me think that the whole GetOptions() and TryGetOption() API is a bad idea and we should rather focus on how to expose the descriptor.Options property safely, because then we wouldn't need to worry about the GetOptions/TryGetOptions etc. API's at all (they could be removed and everyone could just rely on standard extension APIs).

The only way we could expose descriptor.Options safely is by deep cloning it (since all the protos are supposed to be immutable). With these APIs the only thing we need to clone is the value, if we need to clone it at all.

C++ has const references, Java has readers, and Python parses a local options value (but allows mutation).

Maybe we could do it like Python but, again, we'd still clone the whole options message once for every unique descriptor on get.

Maybe the deep clone isn't really all that problematic. Most projects would only have a few options in place and it seems fair to require users to cache the results they got from e.g. messageDescriptor.Options if they wanted to process a large number of options.
It seems to me that just exposing description.Options (as a defensive copy) would lead to a more logical and straightforward API (with smaller footprint and fewer duplications, and also more similar to what other languages do) - WDYT?

Could we make a internal wrapper class that exposes the IExtendableMessage API but only implements the pure functions? Then GetOptions would just return an IExtendableMessage implementation.

But you actually want to be able to access e.g. the FileOptions' properties (the "standard" options, such as "java_package" etc.)

see

protobuf/csharp/src/Google.Protobuf/Reflection/Descriptor.cs

Line 3955 in 4059c61

public string JavaPackage {

jtattermusch · 2020-05-11T07:42:53Z

Superseded by #7434 (GetOption API is now obsolete).

Add null checks to GetOption

6143428

googlebot added the cla: yes label Jan 23, 2020

ObsidianMinor requested a review from jtattermusch January 23, 2020 21:58

JamesNK reviewed Jan 23, 2020

View reviewed changes

csharp/src/Google.Protobuf.Test/Reflection/CustomOptionsTest.cs Outdated Show resolved Hide resolved

Change assertions to assert default value

5609c03

jskeet mentioned this pull request Feb 5, 2020

C# Custom Options: TypeInitializationException: InnerException: An item with the same key has already been added #7174

Closed

ObsidianMinor mentioned this pull request Feb 21, 2020

C# .NET GetOption doesn't have a way to test a descriptor for options #7240

Closed

thesoftwarejedi reviewed Feb 21, 2020

View reviewed changes

jtattermusch self-assigned this Feb 28, 2020

jtattermusch added the kokoro:run label Mar 17, 2020

protobuf-kokoro removed the kokoro:run label Mar 17, 2020

xtroncode mentioned this pull request Mar 20, 2020

C# GetOption: throws NullReferenceException #7222

Closed

jtattermusch reviewed Apr 22, 2020

View reviewed changes

Return default extension value if options don't exist

30cd2d4

jtattermusch reviewed Apr 27, 2020

View reviewed changes

jtattermusch closed this May 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add null checks to GetOption #7130

Add null checks to GetOption #7130

ObsidianMinor commented Jan 23, 2020

thesoftwarejedi left a comment

Falco20019 commented Mar 5, 2020

thesoftwarejedi commented Mar 5, 2020

ObsidianMinor commented Mar 5, 2020

thesoftwarejedi commented Mar 5, 2020

ozevren commented Mar 13, 2020

jtattermusch Apr 22, 2020

ObsidianMinor Apr 24, 2020

jtattermusch Apr 24, 2020 •

edited

ObsidianMinor Apr 25, 2020

jtattermusch Apr 22, 2020

ObsidianMinor Apr 22, 2020

jtattermusch Apr 24, 2020

Falco20019 commented Apr 24, 2020

jtattermusch Apr 27, 2020

ObsidianMinor Apr 27, 2020 •

edited

jtattermusch Apr 27, 2020

ObsidianMinor Apr 27, 2020

jtattermusch Apr 27, 2020

jtattermusch commented May 11, 2020

Add null checks to GetOption #7130

Add null checks to GetOption #7130

Conversation

ObsidianMinor commented Jan 23, 2020

thesoftwarejedi left a comment

Choose a reason for hiding this comment

Falco20019 commented Mar 5, 2020

thesoftwarejedi commented Mar 5, 2020

ObsidianMinor commented Mar 5, 2020

thesoftwarejedi commented Mar 5, 2020

ozevren commented Mar 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtattermusch Apr 24, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Falco20019 commented Apr 24, 2020

Choose a reason for hiding this comment

ObsidianMinor Apr 27, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtattermusch commented May 11, 2020

jtattermusch Apr 24, 2020 •

edited

ObsidianMinor Apr 27, 2020 •

edited