C#: Mark GetOption API as obsolete and expose the "GetOptions()" method on descriptors instead #7434

jtattermusch · 2020-04-28T12:16:42Z

The currently existing new API for getting custom options (introduced in #5350 as part of proto2 extensions support) has some serious issues

it is incomplete: it doesn't allow distinguishing between the custom option not being present at all VS being present and having the default value.
the GetOption API basically just mimics the existing GetExtension API (with repeated / non-repeated overloads), creating unnecessary duplication (which then leads to problems such as C#: MethodDescriptor.GetOption throws NullReferenceException #7127)
the new API offers no way of accessing the "standard" (non-custom) options at all.
the GetOption API is not consistent with what other protobuf implementations (C++, java, python) do to access custom options.

This PR removes the unnecessary (and partially broken) GetOption API and replaces it by a more straightforward approach:

expose Options property for each descriptor that allows one to access the corresponding "options" message (but use a defensive copy because C# messages are inherently mutable).
the custom options are retrieved just as any other extensions, using the standard extension API (HasExtension, GetExtension). This is the same approach that is used in C++, Java and Python

(the GetOption API has been marked as "experimental and subject to change" when releasing the C# proto2 support, so removing it if fine to do).

jtattermusch · 2020-04-28T12:17:26Z

CC @ObsidianMinor @JamesNK @jskeet

jtattermusch · 2020-04-28T12:20:09Z

Alternative approaches are in PRs https://github.com/protocolbuffers/protobuf/pull/7130/files
https://github.com/protocolbuffers/protobuf/pull/7282/files (this PR tries to find a simple solution to the problems encountered when reviewing those PRs).

jskeet · 2020-04-28T12:59:23Z

I'm fine with deprecating the existing API, but we shouldn't make breaking changes by removing the existing one unless you're planning on creating a major version bump (which I'd strongly discourage).

I'm uncomfortable with the idea of APIs being labelled as "experimental" in general - particularly when that designation is only visible if you happen to read the right documentation.

I'm similarly nervous about my own #7429 PR, but at least that's in the generated code rather than the support library. (You only get broken when you regenerate, and then it's at compile time.) This change could easily mean that a minor version bump in which support library is consumed ends up breaking a deployed application.

jtattermusch · 2020-04-28T13:27:39Z

I'm fine with deprecating the existing API, but we shouldn't make breaking changes by removing the existing one unless you're planning on creating a major version bump (which I'd strongly discourage).

I'm uncomfortable with the idea of APIs being labelled as "experimental" in general - particularly when that designation is only visible if you happen to read the right documentation.

I'm similarly nervous about my own #7429 PR, but at least that's in the generated code rather than the support library. (You only get broken when you regenerate, and then it's at compile time.) This change could easily mean that a minor version bump in which support library is consumed ends up breaking a deployed application.

I'm fine with both approaches (removing the GetOption method or only marking it as deprecated).
Some arguments why just removing GetOption API might be fine:

The API only existed in 3.11.0 release and higher (and the whole proto2 support was marked as experimental in the release notes: https://github.com/protocolbuffers/protobuf/releases/tag/v3.11.0).
Proto2 support is still pretty new so likely it doesn't have many users and those who tried the new GetOption API quickly found that it's broken can only be used with workarounds (so changing/fixing the API doesn't seem very surprising). See e.g. C#: CustomOptions broken and deprecated without working alternatives #7043 FTR

jskeet · 2020-04-28T13:33:33Z

If the option had only been available for Proto2-based code, I'd be at least more comfortable - but we've been using it (in API generation) for proto3-based options. Again, I don't think marking something as experimental just in release notes is a good way of excusing breaking changes. If we want to be able to make breaking changes in the support library itself, I really think we need a better way of doing so. (Marking it as deprecated would, somewhat oddly, be a reasonable starting point - something that brings it to the attention of users.)

And yes, it needed fixing to start with - but it was fixed. It's not great in terms of API surface, but it works for at least some scenarios (e.g. how we're using it in https://github.com/googleapis/gapic-generator-csharp)

I tend to think that the benefits of violating semver have to be really, really huge to counter the downsides, which can include lack of trust in future releases. Sometimes it's the best-of-a-bad-bunch option, but in this case I think using deprecation is better.

jtattermusch · 2020-04-28T17:01:10Z

If the option had only been available for Proto2-based code, I'd be at least more comfortable - but we've been using it (in API generation) for proto3-based options. Again, I don't think marking something as experimental just in release notes is a good way of excusing breaking changes. If we want to be able to make breaking changes in the support library itself, I really think we need a better way of doing so. (Marking it as deprecated would, somewhat oddly, be a reasonable starting point - something that brings it to the attention of users.)

And yes, it needed fixing to start with - but it was fixed. It's not great in terms of API surface, but it works for at least some scenarios (e.g. how we're using it in https://github.com/googleapis/gapic-generator-csharp)

I tend to think that the benefits of violating semver have to be really, really huge to counter the downsides, which can include lack of trust in future releases. Sometimes it's the best-of-a-bad-bunch option, but in this case I think using deprecation is better.

Fair enough, I'll add back the GetOption methods and mark them as obsolete.

jskeet · 2020-04-28T17:06:42Z

Fab, thanks. I can then have a look at the "new" API without being distracted ;)

jtattermusch · 2020-04-28T17:26:28Z

If the option had only been available for Proto2-based code, I'd be at least more comfortable - but we've been using it (in API generation) for proto3-based options. Again, I don't think marking something as experimental just in release notes is a good way of excusing breaking changes. If we want to be able to make breaking changes in the support library itself, I really think we need a better way of doing so. (Marking it as deprecated would, somewhat oddly, be a reasonable starting point - something that brings it to the attention of users.)
And yes, it needed fixing to start with - but it was fixed. It's not great in terms of API surface, but it works for at least some scenarios (e.g. how we're using it in https://github.com/googleapis/gapic-generator-csharp)
I tend to think that the benefits of violating semver have to be really, really huge to counter the downsides, which can include lack of trust in future releases. Sometimes it's the best-of-a-bad-bunch option, but in this case I think using deprecation is better.

Fair enough, I'll add back the GetOption methods and mark them as obsolete.

Done.

haberman · 2020-04-28T18:09:58Z

Note that custom options can be self-referential. Here is valid .proto file that we should ensure works properly in C#:

syntax = "proto2";

import "google/protobuf/descriptor.proto";

message FooOptions {
  // Custom field option used in definition of the custom option's message.
  optional int32 foo = 1 [(foo_options) = {foo: 1234}];
}

extend google.protobuf.FieldOptions {
  // Custom field option used on the definition of that field option.
  optional int32 bar_options = 1000 [(bar_options) = 1234];

  optional FooOptions foo_options = 1001;
}

ObsidianMinor · 2020-04-28T18:37:43Z

I'm still unsure how I feel about doing defensive copies but that may just be the Rust developer in me that's worried and not the C# developer. It will make the API simpler in the long run and will be easier to maintain so it should be ok.

jtattermusch · 2020-04-28T19:05:36Z

I discussed the defensive copy with @haberman and we agreed that at least at the first glance it seemed to be a reasonable solution. I think @haberman is the ultimate authority here so I'm gonna go with his opinion.

jskeet · 2020-04-28T19:27:19Z

Yes, I'm pretty sure we need the defensive copy. It would be awful if reflection could modify the descriptors globally.

JamesNK · 2020-04-28T19:45:30Z

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

+        /// Custom options can be retrieved as extensions of the returned message.
+        /// NOTE: A defensive copy is created each time this property is retrieved.
+        /// </summary>
+        public EnumOptions Options => (Proto.Options as IDeepCloneable<EnumOptions>)?.Clone();


Can Proto.Options on an EnumDescriptor ever not be IDeepCloneable<EnumOptions>? Same question for all the other as casts. I prefer to do a regular cast unless there is a reason.

Good point, removed the cast.

JamesNK · 2020-04-28T19:47:12Z

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

        public CustomOptions CustomOptions => new CustomOptions(Proto.Options?._extensions?.ValuesByNumber);

+        /// <summary>


Can null ever be returned?

Improved the docs.

haberman · 2020-04-28T19:58:12Z

Yes I agree that it would be better to avoid the copy if possible. But without const message instances like we have in C++ I don't see a way to do it.

haberman · 2020-04-28T19:59:19Z

By the way, here's an even more diabolical example of custom options.

syntax = "proto2";

import "google/protobuf/descriptor.proto";

message FooOptions {
  // Custom field option used in definition of the extension message.
  optional int32 int_opt = 1 [(foo_options) = {
             int_opt: 1
             [foo_int_opt]: 2
             [foo_foo_opt]: {
               int_opt: 3
             }
  }];
  extensions 1000 to max;
}

extend google.protobuf.FieldOptions {
  // Custom field option used on the definition of that field option.
  optional int32 bar_options = 1000 [(bar_options) = 1234];

  optional FooOptions foo_options = 1001;
}

extend FooOptions {
  optional int32 foo_int_opt = 1000;
  optional FooOptions foo_foo_opt = 1001;
}

jtattermusch · 2020-04-29T07:11:45Z

because Options creates a defensive copy each time you request it, it might be better to do var options = methodDescriptor.Options; first and then use that instance repeatedly (but it doesn't affect correctness, only efficiency).

You should consider a method instead of a property. Properties are assumed to be free/cheap to access. Methods indicate there is a cost and the value should be assigned to a local variable. I don't know what name you would choose. GetOptions is already taken.

looks like GetOption() is taken, GetOptions() is not.

Not sure if Options property or GetOptions method is better (the clone is actually going to be pretty cheap for most normal cases).

jtattermusch · 2020-04-29T10:52:47Z

By the way, here's an even more diabolical example of custom options.

syntax = "proto2";

import "google/protobuf/descriptor.proto";

message FooOptions {
  // Custom field option used in definition of the extension message.
  optional int32 int_opt = 1 [(foo_options) = {
             int_opt: 1
             [foo_int_opt]: 2
             [foo_foo_opt]: {
               int_opt: 3
             }
  }];
  extensions 1000 to max;
}

extend google.protobuf.FieldOptions {
  // Custom field option used on the definition of that field option.
  optional int32 bar_options = 1000 [(bar_options) = 1234];

  optional FooOptions foo_options = 1001;
}

extend FooOptions {
  optional int32 foo_int_opt = 1000;
  optional FooOptions foo_foo_opt = 1001;
}

Do these proto definitions already exist somewhere in the protobuf codebase? I'd expect so because presumably other languages already have tests for these cases?

jtattermusch · 2020-05-04T16:59:46Z

Friendly ping, can we get this reviewed and merged?

jskeet · 2020-05-04T17:03:02Z

It wasn't clear to me that it was ready for another review given the nature of the last few comments. I'll have another look tomorrow.

jtattermusch · 2020-05-04T17:12:32Z

It wasn't clear to me that it was ready for another review given the nature of the last few comments. I'll have another look tomorrow.

I think the only undecided topic is whether to expose Options property (even though it actually created a copy of the options) or whether to have GetOptions() method instead (as @JamesNK has suggested)
I think I'm fine with the current approach (a property) because the copy will be pretty lightweight in most normal cases (options are handwritten in the .proto file so usually there will only be a few of them present) and IMHO using a property feels more natural. That said, I don't feel strongly about this and I'd be happy to be convinced otherwise.

jskeet · 2020-05-05T06:46:08Z

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

        public CustomOptions CustomOptions => new CustomOptions(Proto.Options?._extensions?.ValuesByNumber);

+        /// <summary>
+        /// The <c>EnumOptions</c>, defined in <c>descriptor.proto</c>.
+        /// If the options message is not present (=there are no options), <c>null</c> is returned.


Suggested change

/// If the options message is not present (=there are no options), <c>null</c> is returned.

/// If the options message is not present (i.e. there are no options), <c>null</c> is returned.

(Ditto in all options, of course.)

haberman · 2020-05-05T23:29:37Z

If it is making a copy, putting it behind a method like GetOptions() seems less surprising to me.

jtattermusch · 2020-05-06T15:48:25Z

If it is making a copy, putting it behind a method like GetOptions() seems less surprising to me.

@jskeet do you have a preference? I still think using a property is fine.

jskeet · 2020-05-06T15:50:14Z

I'd just about prefer a method here - it really does make it clearer - but I'm not massively bothered either way.

jtattermusch · 2020-05-06T15:51:19Z

I'd just about prefer a method here - it really does make it clearer - but I'm not massively bothered either way.

Ok, looks like I've been outvoted then. I'll turn this into a GetOptions() method.

jtattermusch · 2020-05-06T16:54:37Z

@haberman all feedback addressed.

jtattermusch · 2020-05-11T07:40:46Z

Test failures seem unrelated, merging.

jtattermusch · 2020-05-12T06:46:17Z

@haberman I think we should cherry-pick this for 3.12.x branch (It's a fix).

…se_options C#: Get rid of broken GetOption API and expose the "GetOptions()" method on descriptors instead

jtattermusch added 3 commits April 28, 2020 10:49

expose Options property on descriptors

944055a

adjust CustomOptionsTest.cs

9720ae4

add a few more tests

f4ff220

googlebot added the cla: yes label Apr 28, 2020

jtattermusch requested review from haberman and anandolee April 28, 2020 12:16

jtattermusch added the c# label Apr 28, 2020

jtattermusch added release notes: yes labels Apr 28, 2020

protobuf-kokoro removed the kokoro:run label Apr 28, 2020

jtattermusch mentioned this pull request Apr 28, 2020

Add TryGetExtension API (with TryGetOption) #7282

Closed

jtattermusch added 2 commits April 28, 2020 19:14

add back GetOption methods, but mark them as obsolete

062c2dc

add back GetOption tests

402ce8b

jtattermusch added the kokoro:run label Apr 28, 2020

JamesNK reviewed Apr 28, 2020

View reviewed changes

clarify docs

ff999c9

protobuf-kokoro removed the kokoro:run label Apr 29, 2020

jtattermusch mentioned this pull request Apr 30, 2020

C# Add tests for self-referential options #7448

Merged

haberman mentioned this pull request May 1, 2020

Remove Has/Clear members for C# message fields in proto2 #7429

Merged

jskeet approved these changes May 5, 2020

View reviewed changes

address review comments

0688883

jtattermusch added the kokoro:run label May 6, 2020

protobuf-kokoro removed the kokoro:run label May 6, 2020

jskeet approved these changes May 6, 2020

View reviewed changes

jtattermusch changed the title ~~C#: Get rid of broken GetOption API and expose the "Options" property on descriptors instead~~ C#: Get rid of broken GetOption API and expose the "GetOptions()" property on descriptors instead May 7, 2020

jtattermusch changed the title ~~C#: Get rid of broken GetOption API and expose the "GetOptions()" property on descriptors instead~~ C#: Get rid of broken GetOption API and expose the "GetOptions()" method on descriptors instead May 7, 2020

haberman approved these changes May 8, 2020

View reviewed changes

jtattermusch merged commit bf3eef9 into protocolbuffers:master May 11, 2020

This was referenced May 11, 2020

Add null checks to GetOption #7130

Closed

C#: MethodDescriptor.GetOption throws NullReferenceException #7127

Closed

Add backwards compatibility for custom options #7005

Closed

jtattermusch changed the title ~~C#: Get rid of broken GetOption API and expose the "GetOptions()" method on descriptors instead~~ C#: Mark GetOption API as obsolete and expose the "GetOptions()" method on descriptors instead May 12, 2020

haberman pushed a commit to haberman/protobuf that referenced this pull request May 12, 2020

Merge pull request protocolbuffers#7434 from jtattermusch/csharp_expo…

fc5ded3

…se_options C#: Get rid of broken GetOption API and expose the "GetOptions()" method on descriptors instead

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C#: Mark GetOption API as obsolete and expose the "GetOptions()" method on descriptors instead #7434

C#: Mark GetOption API as obsolete and expose the "GetOptions()" method on descriptors instead #7434

jtattermusch commented Apr 28, 2020

jtattermusch commented Apr 28, 2020 •

edited

jtattermusch commented Apr 28, 2020

jskeet commented Apr 28, 2020

jtattermusch commented Apr 28, 2020

jskeet commented Apr 28, 2020

jtattermusch commented Apr 28, 2020

jskeet commented Apr 28, 2020

jtattermusch commented Apr 28, 2020

haberman commented Apr 28, 2020

ObsidianMinor commented Apr 28, 2020

jtattermusch commented Apr 28, 2020

jskeet commented Apr 28, 2020

JamesNK Apr 28, 2020 •

edited

jtattermusch Apr 29, 2020

JamesNK Apr 28, 2020 •

edited

jtattermusch Apr 29, 2020

haberman commented Apr 28, 2020

haberman commented Apr 28, 2020

jtattermusch commented Apr 29, 2020 •

edited

jtattermusch commented Apr 29, 2020

jtattermusch commented May 4, 2020

jskeet commented May 4, 2020

jtattermusch commented May 4, 2020

jskeet May 5, 2020

jskeet May 5, 2020

haberman commented May 5, 2020

jtattermusch commented May 6, 2020

jskeet commented May 6, 2020

jtattermusch commented May 6, 2020

jtattermusch commented May 6, 2020

jtattermusch commented May 11, 2020

jtattermusch commented May 12, 2020

		public CustomOptions CustomOptions => new CustomOptions(Proto.Options?._extensions?.ValuesByNumber);

		/// <summary>

	/// If the options message is not present (=there are no options), <c>null</c> is returned.
	/// If the options message is not present (i.e. there are no options), <c>null</c> is returned.

C#: Mark GetOption API as obsolete and expose the "GetOptions()" method on descriptors instead #7434

C#: Mark GetOption API as obsolete and expose the "GetOptions()" method on descriptors instead #7434

Conversation

jtattermusch commented Apr 28, 2020

jtattermusch commented Apr 28, 2020 • edited

jtattermusch commented Apr 28, 2020

jskeet commented Apr 28, 2020

jtattermusch commented Apr 28, 2020

jskeet commented Apr 28, 2020

jtattermusch commented Apr 28, 2020

jskeet commented Apr 28, 2020

jtattermusch commented Apr 28, 2020

haberman commented Apr 28, 2020

ObsidianMinor commented Apr 28, 2020

jtattermusch commented Apr 28, 2020

jskeet commented Apr 28, 2020

JamesNK Apr 28, 2020 • edited

Choose a reason for hiding this comment

jtattermusch Apr 29, 2020

Choose a reason for hiding this comment

JamesNK Apr 28, 2020 • edited

Choose a reason for hiding this comment

jtattermusch Apr 29, 2020

Choose a reason for hiding this comment

haberman commented Apr 28, 2020

haberman commented Apr 28, 2020

jtattermusch commented Apr 29, 2020 • edited

jtattermusch commented Apr 29, 2020

jtattermusch commented May 4, 2020

jskeet commented May 4, 2020

jtattermusch commented May 4, 2020

jskeet May 5, 2020

Choose a reason for hiding this comment

jskeet May 5, 2020

Choose a reason for hiding this comment

haberman commented May 5, 2020

jtattermusch commented May 6, 2020

jskeet commented May 6, 2020

jtattermusch commented May 6, 2020

jtattermusch commented May 6, 2020

jtattermusch commented May 11, 2020

jtattermusch commented May 12, 2020

jtattermusch commented Apr 28, 2020 •

edited

JamesNK Apr 28, 2020 •

edited

JamesNK Apr 28, 2020 •

edited

jtattermusch commented Apr 29, 2020 •

edited