Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add null checks to GetOption #7130

Closed
wants to merge 3 commits into from
Closed

Add null checks to GetOption #7130

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6143428
Add null checks to GetOption
ObsidianMinor Jan 23, 2020
5609c03
Change assertions to assert default value
ObsidianMinor Jan 25, 2020
30cd2d4
Return default extension value if options don't exist
ObsidianMinor Apr 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions csharp/src/Google.Protobuf.Test/Reflection/CustomOptionsTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,13 +184,21 @@ public void NoOptions()
var fileDescriptor = UnittestProto3Reflection.Descriptor;
var messageDescriptor = TestAllTypes.Descriptor;
Assert.NotNull(fileDescriptor.CustomOptions);
Assert.DoesNotThrow(() => { fileDescriptor.GetOption(Fileopt); });
ObsidianMinor marked this conversation as resolved.
Show resolved Hide resolved
Assert.NotNull(messageDescriptor.CustomOptions);
Assert.DoesNotThrow(() => { messageDescriptor.GetOption(Msgopt); });
Assert.NotNull(messageDescriptor.Fields[1].CustomOptions);
Assert.DoesNotThrow(() => { messageDescriptor.Fields[1].GetOption(Fieldopt); });
Assert.NotNull(fileDescriptor.Services[0].CustomOptions);
Assert.DoesNotThrow(() => { fileDescriptor.Services[0].GetOption(Serviceopt); });
Assert.NotNull(fileDescriptor.Services[0].Methods[0].CustomOptions);
Assert.DoesNotThrow(() => { fileDescriptor.Services[0].Methods[0].GetOption(Methodopt); });
Assert.NotNull(fileDescriptor.EnumTypes[0].CustomOptions);
Assert.DoesNotThrow(() => { fileDescriptor.EnumTypes[0].GetOption(Enumopt); });
Assert.NotNull(fileDescriptor.EnumTypes[0].Values[0].CustomOptions);
Assert.DoesNotThrow(() => { fileDescriptor.EnumTypes[0].Values[0].GetOption(EnumValueOpt1); });
Assert.NotNull(TestAllTypes.Descriptor.Oneofs[0].CustomOptions);
Assert.DoesNotThrow(() => { TestAllTypes.Descriptor.Oneofs[0].GetOption(OneofOpt1); });
}

[Test]
Expand Down
10 changes: 8 additions & 2 deletions csharp/src/Google.Protobuf/Reflection/EnumDescriptor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,13 @@ public EnumValueDescriptor FindValueByName(string name)
/// </summary>
public T GetOption<T>(Extension<EnumOptions, T> extension)
{
var value = Proto.Options.GetExtension(extension);
var options = Proto.Options;
if (options == null)
{
return default(T);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

var value = options.GetExtension(extension);
return value is IDeepCloneable<T> ? (value as IDeepCloneable<T>).Clone() : value;
}

Expand All @@ -145,7 +151,7 @@ public T GetOption<T>(Extension<EnumOptions, T> extension)
/// </summary>
public RepeatedField<T> GetOption<T>(RepeatedExtension<EnumOptions, T> extension)
{
return Proto.Options.GetExtension(extension).Clone();
return Proto.Options?.GetExtension(extension)?.Clone();
}
}
}
10 changes: 8 additions & 2 deletions csharp/src/Google.Protobuf/Reflection/EnumValueDescriptor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,13 @@ public sealed class EnumValueDescriptor : DescriptorBase
/// </summary>
public T GetOption<T>(Extension<EnumValueOptions, T> extension)
{
var value = Proto.Options.GetExtension(extension);
var options = Proto.Options;
if (options == null)
{
return default(T);
}

var value = options.GetExtension(extension);
return value is IDeepCloneable<T> ? (value as IDeepCloneable<T>).Clone() : value;
}

Expand All @@ -90,7 +96,7 @@ public T GetOption<T>(Extension<EnumValueOptions, T> extension)
/// </summary>
public RepeatedField<T> GetOption<T>(RepeatedExtension<EnumValueOptions, T> extension)
{
return Proto.Options.GetExtension(extension).Clone();
return Proto.Options?.GetExtension(extension)?.Clone();
}
}
}
Expand Down
10 changes: 8 additions & 2 deletions csharp/src/Google.Protobuf/Reflection/FieldDescriptor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,13 @@ public MessageDescriptor ExtendeeType
/// </summary>
public T GetOption<T>(Extension<FieldOptions, T> extension)
{
var value = Proto.Options.GetExtension(extension);
var options = Proto.Options;
if (options == null)
{
return default(T);
}

var value = options.GetExtension(extension);
return value is IDeepCloneable<T> ? (value as IDeepCloneable<T>).Clone() : value;
}

Expand All @@ -315,7 +321,7 @@ public T GetOption<T>(Extension<FieldOptions, T> extension)
/// </summary>
public RepeatedField<T> GetOption<T>(RepeatedExtension<FieldOptions, T> extension)
{
return Proto.Options.GetExtension(extension).Clone();
return Proto.Options?.GetExtension(extension)?.Clone();
}

/// <summary>
Expand Down
10 changes: 8 additions & 2 deletions csharp/src/Google.Protobuf/Reflection/FileDescriptor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

{
return default(T);
}

var value = options.GetExtension(extension);
return value is IDeepCloneable<T> ? (value as IDeepCloneable<T>).Clone() : value;
}

Expand All @@ -564,7 +570,7 @@ public T GetOption<T>(Extension<FileOptions, T> extension)
/// </summary>
public RepeatedField<T> GetOption<T>(RepeatedExtension<FileOptions, T> extension)
{
return Proto.Options.GetExtension(extension).Clone();
return Proto.Options?.GetExtension(extension)?.Clone();
}

/// <summary>
Expand Down
10 changes: 8 additions & 2 deletions csharp/src/Google.Protobuf/Reflection/MessageDescriptor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,13 @@ internal bool IsExtensionsInitialized(IMessage message)
/// </summary>
public T GetOption<T>(Extension<MessageOptions, T> extension)
{
var value = Proto.Options.GetExtension(extension);
var options = Proto.Options;
if (options == null)
{
return default(T);
}

var value = options.GetExtension(extension);
return value is IDeepCloneable<T> ? (value as IDeepCloneable<T>).Clone() : value;
}

Expand All @@ -280,7 +286,7 @@ public T GetOption<T>(Extension<MessageOptions, T> extension)
/// </summary>
public Collections.RepeatedField<T> GetOption<T>(RepeatedExtension<MessageOptions, T> extension)
{
return Proto.Options.GetExtension(extension).Clone();
return Proto.Options?.GetExtension(extension)?.Clone();
}

/// <summary>
Expand Down
10 changes: 8 additions & 2 deletions csharp/src/Google.Protobuf/Reflection/MethodDescriptor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,13 @@ public sealed class MethodDescriptor : DescriptorBase
/// </summary>
public T GetOption<T>(Extension<MethodOptions, T> extension)
{
var value = Proto.Options.GetExtension(extension);
var options = Proto.Options;
if (options == null)
{
return default(T);
}

var value = options.GetExtension(extension);
return value is IDeepCloneable<T> ? (value as IDeepCloneable<T>).Clone() : value;
}

Expand All @@ -90,7 +96,7 @@ public T GetOption<T>(Extension<MethodOptions, T> extension)
/// </summary>
public RepeatedField<T> GetOption<T>(RepeatedExtension<MethodOptions, T> extension)
{
return Proto.Options.GetExtension(extension).Clone();
return Proto.Options?.GetExtension(extension)?.Clone();
}

internal MethodDescriptor(MethodDescriptorProto proto, FileDescriptor file,
Expand Down
10 changes: 8 additions & 2 deletions csharp/src/Google.Protobuf/Reflection/OneofDescriptor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,13 @@ public MessageDescriptor ContainingType
/// </summary>
public T GetOption<T>(Extension<OneofOptions, T> extension)
{
var value = proto.Options.GetExtension(extension);
var options = proto.Options;
if (options == null)
{
return default(T);
}

var value = options.GetExtension(extension);
return value is IDeepCloneable<T> ? (value as IDeepCloneable<T>).Clone() : value;
}

Expand All @@ -122,7 +128,7 @@ public T GetOption<T>(Extension<OneofOptions, T> extension)
/// </summary>
public RepeatedField<T> GetOption<T>(RepeatedExtension<OneofOptions, T> extension)
{
return proto.Options.GetExtension(extension).Clone();
return proto.Options?.GetExtension(extension)?.Clone();
}

internal void CrossLink()
Expand Down
10 changes: 8 additions & 2 deletions csharp/src/Google.Protobuf/Reflection/ServiceDescriptor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,13 @@ public MethodDescriptor FindMethodByName(String name)
/// </summary>
public T GetOption<T>(Extension<ServiceOptions, T> extension)
{
var value = Proto.Options.GetExtension(extension);
var options = Proto.Options;
if (options == null)
{
return default(T);
}

var value = options.GetExtension(extension);
return value is IDeepCloneable<T> ? (value as IDeepCloneable<T>).Clone() : value;
}

Expand All @@ -111,7 +117,7 @@ public T GetOption<T>(Extension<ServiceOptions, T> extension)
/// </summary>
public RepeatedField<T> GetOption<T>(RepeatedExtension<ServiceOptions, T> extension)
{
return Proto.Options.GetExtension(extension).Clone();
return Proto.Options?.GetExtension(extension)?.Clone();
}

internal void CrossLink()
Expand Down