-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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.
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
Add null checks to GetOption #7130
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -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) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 This actually has several downsides:
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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||||
{ | ||||
return extension.DefaultValue; | ||||
} | ||||
|
||||
var value = options.GetExtension(extension); | ||||
return value is IDeepCloneable<T> ? (value as IDeepCloneable<T>).Clone() : value; | ||||
} | ||||
|
||||
|
@@ -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> | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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);
andAssert.AreEqual(messageDescriptor.Fields[1].GetOption(Fieldopt), null);
would be related.