Add FieldLiteral proto message #2752

gilwong00 · 2024-02-05T21:53:01Z

Adds new FieldLiteral message in the doc.proto file.
These will allows will enable custom options(extensions) to be consumed from a proto file and return in the doc service used in core.

…roto

jhump

I think the protosource changes should probably be in their own PR. That way you can go ahead and merge that without impacting the BSR's API, and then pull those changes into core.

That would allow you to then open a PR in core that has the implementation of the new service, concurrently with the service definition PR. Maybe that's not how we usually do things, but it seems like a nice property to be able to merge the service API change and the implementation as close together as possible. Otherwise, you may want a comment on the new API options fields that they are new and may not yet be populated in any API responses. 🤷

private/pkg/protosource/field.go

private/pkg/protosource/protosource.go

proto/buf/alpha/registry/v1alpha1/doc.proto

jhump · 2024-02-05T22:20:02Z

proto/buf/alpha/registry/v1alpha1/doc.proto

@@ -281,6 +285,8 @@ message EnumValue {
  // Note that any leading and trailing `//` or spaces within a `/* */` block will be stripped.
  string description = 3;
  EnumValueOptions enum_value_options = 4;
+
+  repeated Option options = 5;


Also need a field like this above in Enum (around line 270).

Aside: I just realized that we don't have a place to show file options in the current UI (which is yet another place where options show up).

I know this PR's focus is for being able to show custom options, but this same arc of work will allow us to also show "features" of an element (part of upcoming "Protobuf Editions"). And with those features, file options are relevant, since features can be defined at the file level and then apply to all elements defined in the file.

The best way to address this (once we get to a point where we want to render features for editions) will probably be some extra code in the server handler, to merge in file-level features for rendering on each element.

No action need... just thinking out loud (or "through the keyboard" perhaps)...

jhump · 2024-02-05T22:21:35Z

proto/buf/alpha/registry/v1alpha1/doc.proto

@@ -320,6 +326,7 @@ message Message {
  MessageOptions message_options = 10;
  // implicitly_deprecated is true if its enclosing file or parent element is deprecated.
  bool implicitly_deprecated = 11;
+  repeated Option options = 12;


Curious that this contains no information about reserved ranges, reserved names, or extension ranges. Perhaps that was intentional? Or perhaps it was simply "out of scope" when the auto-generated docs were first implemented. (I wonder if it's appropriate to add a TODO about them?)

I mainly bring it up because an extension range is another thing that has options.

For now, no action needed.

proto/buf/alpha/registry/v1alpha1/doc.proto

private/pkg/protosource/option_extension_descriptor.go

jhump · 2024-02-05T22:39:53Z

proto/buf/alpha/registry/v1alpha1/doc.proto

  ServiceOptions service_options = 8;
  // implicitly_deprecated is true if its enclosing file is deprecated.
  bool implicitly_deprecated = 9;
+
+  repeated Option options = 10;


I see that many of the other fields also don't have doc comments, including the service_options field. But it might be nice to add something to each of these to clarify their purpose, and also to move them so they're adjacent. Something like so:

Suggested change

ServiceOptions service_options = 8;

// implicitly_deprecated is true if its enclosing file is deprecated.

bool implicitly_deprecated = 9;

repeated Option options = 10;

// implicitly_deprecated is true if its enclosing file is deprecated.

bool implicitly_deprecated = 9;

// A select sub-set of service options.

ServiceOptions service_options = 8;

// All options that are present on the service. This is a super-set of

// service_options and uses a dynamic representation so it can also

// accommodate custom options with arbitrary types.

repeated Option options = 10;

proto/buf/alpha/registry/v1alpha1/doc.proto

gilwong00 · 2024-02-05T23:51:22Z

I think the protosource changes should probably be in their own PR. That way you can go ahead and merge that without impacting the BSR's API, and then pull those changes into core.

That would allow you to then open a PR in core that has the implementation of the new service, concurrently with the service definition PR. Maybe that's not how we usually do things, but it seems like a nice property to be able to merge the service API change and the implementation as close together as possible. Otherwise, you may want a comment on the new API options fields that they are new and may not yet be populated in any API responses. 🤷

@jhump Good call out. I removed the usage of the FieldLiteral field in the messages I initially had them. This way we can still merge all the changes to the doc.proto without altering the service. I'll make a follow up PR to reimplement the usage of FieldLiteral.

jhump

Nice. Just a few more tiny things.

proto/buf/alpha/registry/v1alpha1/doc.proto

private/pkg/protosource/field.go

bufdev · 2024-02-06T22:29:34Z

proto/buf/alpha/registry/v1alpha1/doc.proto

@@ -377,6 +378,47 @@ message FieldOptions {
  int32 jstype = 4;
 }

+// FieldLiteral represents a field and its value. It can be used to model descriptor options.
+message FieldLiteral {


I don't actually see this used anywhere, where it is used?

Also how was the API created/designed?

The idea is to supplement the fields that look like FieldOptions field_options = 10 (across all of the descriptor types) with a field that instead looks like repeated FieldLiteral options = 11. This representation can model any option and is specifically intended for modeling custom options in a way that the frontend can render into the docs.

It's not used anywhere yet at my suggestion. As it is, this can be pulled into the other repo and the transformation from the protosource.OptionExtensionDescriptor model to the []*FieldLiteral model can be implemented and tested there w/out (yet) impacting the BSR API.

But I'm not super-familiar with the typical process of implementing BSR APIs. Is it common to update the API first and then leave this stuff in the API, but unimplemented, for however long it takes to actually implement? It seemed better to me to try to implement the server-side stuff alongside the API change and have them reviewed simultaneously (or concurrently?), to eliminate useless churn in the APIs (or introducing compatibility issues) if implementation reveals that changes are required.

I'm mostly interested how the specific messages were designed - are these net-new, or were they pulled from somewhere? If new-new, I need to review them more carefully. If not net-new, where did they derive from?

These are net new

@jhump is there not some pre-existing art that would suffice? Thoughts:

One of the well-known types.

Just using FieldOptions straight up.

@timostamm's work on extension and proto2 support with protobuf-es might change the answer as well.

Using FieldOptions (or other option types) makes consuming the value much more complicated since it will have extensions. So a consumer would need to have the schemata for all custom options in order to interpret the data.

Basically same goes for well-known types -- the closest would be google.protobuf.Value, but it represents JSON data (so does not distinguish between any of the numeric types, does not support enums except as string or int values, and supports null which isn't appropriate here). Aside from that, the only thing that can dynamically represent any kind of message is google.protobuf.Any and that has the same drawbacks as extensions: the consumer must have the schemata for everything in order to interpret this.

This is kind of like google.protobuf.Value, except it is for modeling arbitrary Protobuf values instead of JSON values. It's intended to provide a very easy way to work with the data without the schema.

While we could use the google.protobuf.*Options messages here, we would also have to ship the descriptors for the extensions and types they use. This only makes sense to me if we use descriptors for the entire package docs, replacing the current layer. This is feasible and has advantages, but it requires extra steps to split descriptor sets by package... and would obviously be a major pivot.

google.protobuf.Value cannot store the values NaN and Infinity for DOUBLE. It's unlikely that they appear in option values, but they can. It also cannot represent BYTES. This means that the data will contain strings with special meaning - base64 encoded BYTES, "NaN", "Infinity", and "-Infinity" - that the client cannot distinguish from other strings without knowing the schema, which leads to the same problem as with google.protobuf.*Options.

I agree with Josh here.

private/pkg/protosource/option_extension_descriptor.go

private/pkg/protosource/protosource.go

private/pkg/protosource/option_extension_descriptor.go

bufdev · 2024-02-07T21:15:59Z

To shortchange the back and forth, I pushed up what I think this should be in 89c0eec. I updated the function name to ForEachPresentOption, and updated the docs to more closely match the style of PresentExtensionNumbers.

However, one issue - I think there's a bit of a discrepancy here in how we are referencing options vs extensions. The docs for PresentExtensionNumbers:

	// PresentExtensionNumbers returns field numbers for all options that
	// have a set value on this descriptor.

The (now updated) docs for ForEachPresentOption (which match the substance of what was there pre-my-edits):

	// ForEachPresentOption iterates through all options that have a set value on this
	// descriptor, invoking fn for each present option.

The discrepency being in one case, we're saying "extension numbers", and in the other, we're saying "options", for what (doc-wise) is obstensibly the same item. What should we do here? I think the answer is that the docs for PresentExtensionNumbers are incorrect, and need to be scoped (which matches what is going on in the function, namely a fieldDescriptor.IsExtension() call), but I'm not entirely sure.

A note: when this is merged, main will have to be merged into bufmod, and protosource has moved to bufprotosource. @gilwong00 you'll be on hook for this. https://github.com/bufbuild/buf/tree/bufmod/private/bufpkg/bufprotosource

jhump · 2024-02-08T01:13:15Z

@bufdev, the discrepancy is intentional and kind of unavoidable because they do different things. PresentExtensionNumbers is just for extensions/custom options. This was used in combination with the other two methods I think, for getting source code info for custom options (like to do linting on protovalidate stuff I think?).

But the new method gets all options, not just extensions. This is because I think it will be best to treat all options uniformly in the generated docs, instead of having to special-case normal options vs custom options. (Especially with editions: special-casing the handling of the FeatureSet message doesn't make sense, and leaning on the same code that renders custom options makes more sense.)

bufdev · 2024-02-08T19:10:25Z

That makes sense - can we update the comments on PresentExtensionNumbers to reflect this?

bufdev · 2024-02-08T19:25:04Z

proto/buf/alpha/registry/v1alpha1/doc.proto

+message LiteralValue {
+  oneof value {
+    string string_value = 2;
+    int64 int_value = 3;


Why does this not contain all the primitive types? This API shouldn't be frontend-specific.

updated this to include all the primitive types

This is for representing values, not the schema.

Perhaps there might be a client that would want to distinguish int32 from int64 or double from float (though I can't think of any). But there's surely no need for fixed, sfixed, or sint in here.

@bufdev, is this API really not frontend specific? The comment on the service says otherwise:

buf/proto/buf/alpha/registry/v1alpha1/doc.proto

Line 19 in 17b4060

// DocService defines a set of APIs that are intended for use by bufwebd only.

Either way, the API is undoubtedly specific to docs, and we ultimately want to present the option value as text. I cannot think of a use case where I would want to distinguish even DOUBLE from FLOAT in the context of docs, and I'd prefer if we use the widest types only. I don't think it's an issue to include all types here, just that I don't see the need.

In general, APIs like this (and APIs in general) really shouldn't be designed with a specific caller in mind, but in this specific instance I don't know how much the field difference matters.

Pulling changes to `protosource` from #2752 into its own PR.

timostamm · 2024-02-09T13:33:04Z

proto/buf/alpha/registry/v1alpha1/doc.proto

+    ArrayLiteral array_value = 16;
+    MessageLiteral message_value = 17;


Repeated fields can be specified separately or joined. The following two options are identical:

option (google.api.http) = { get: "/v1/{name=messages/*}" additional_bindings { get: "xxx" } additional_bindings { get: "yyy" } };

option (google.api.http) = { get: "/v1/{name=messages/*}" additional_bindings: [ { get: "xxx" }, { get: "yyy" } ] };

I think it would be confusing to users if they use one form, but the docs show a different form. If I'm not mistaken, we can retain the form with this model. Just commenting here as a 👍.

(I'm assuming we expect clients to render options as proto source.)

I think it will be okay to render them uniformly instead of trying to re-create the source. And I'd suggest always doing the second form (array-literal style syntax, which will be more intuitive for most readers).

For one, I think it will look nicer if the APIs are presented consistently in documentation, instead of trying to preserve possibly idiosyncratic syntax in some sources. And two, re-creating the original source from just the descriptor (i.e. trying to re-construct formatting and such from just the source code info) is not really feasible and trying to merge actual sources with the descriptor into something that is more faithful to the original source would also be a real bear to implement.

proto/buf/alpha/registry/v1alpha1/doc.proto

timostamm · 2024-02-09T13:34:41Z

proto/buf/alpha/registry/v1alpha1/doc.proto

+  // Map of field names to value. Extension field names will be in the form:
+  // [fully.qualified.extension.Name].
+  repeated FieldLiteral fields = 1;


I suppose Any will have a "synthetic" field with the type URL here?

https://protobuf.com/docs/language-spec#any-messages

Great catch!

timostamm · 2024-02-09T13:36:00Z

proto/buf/alpha/registry/v1alpha1/doc.proto

+message LiteralValue {
+  oneof value {
+    string string_value = 2;
+    int64 int_value = 3;


@bufdev, is this API really not frontend specific? The comment on the service says otherwise:

buf/proto/buf/alpha/registry/v1alpha1/doc.proto

Line 19 in 17b4060

// DocService defines a set of APIs that are intended for use by bufwebd only.

Either way, the API is undoubtedly specific to docs, and we ultimately want to present the option value as text. I cannot think of a use case where I would want to distinguish even DOUBLE from FLOAT in the context of docs, and I'd prefer if we use the widest types only. I don't think it's an issue to include all types here, just that I don't see the need.

timostamm

IMO FieldLiteral is appropriate, although we should be aware that we cannot retain some details of the original form regarding formatting, numeric notation, and comments inside the option value (practical example).

I also want to point out that by modeling option values agnostically, clients that want to render them in the protobuf text format have to implement formatting for non-infinite float values and proper escaping of string and bytes. An opinionated alternative would be to provide all values as already formatted text.

timostamm · 2024-02-09T13:50:58Z

proto/buf/alpha/registry/v1alpha1/doc.proto

+  // When is_extension is true, name is the fully-qualified name of the
+  // extension. Otherwise, it is the simple name of the field.
+  string name = 1;
+  // The unique field number associated with the field.
+  int32 tag = 2;


I don't think that the fully qualified name of the extension is sufficient here.

For a practical example:

syntax = "proto3"; import "buf/validate/validate.proto"; message User { string username = 1 [(buf.validate.field).string = {min_len: 3}]; }

I assume we'd show the message in the docs as it appears in the source (although probably with additional line breaks), and link the option (at least the part buf.validate.field) to https://buf.build/bufbuild/protovalidate/docs/main:buf.validate#extensions.

For that to work across modules, we'll need a ImportModuleRef (assuming we want to follow the existing pattern).

Yes, good point. For hyper-linking to the docs for an extension (or message type in an Any), the module ref will be needed.

bufdev · 2024-02-12T16:37:00Z

proto/buf/alpha/registry/v1alpha1/doc.proto

+    uint64 uint_value = 4;
+    double double_value = 5;
+    bool bool_value = 6;
+    string enum_value = 7;


Why is this string?

We going to be showing this in docs so the name should be more useful than the number value to users.

@jhump should this semantically be ie enum_value_name_value, if that makes sense? It's not an enum, its an enum value, and its the name of the enum value...I don't know.

That seems like a lot of ceremony. It represents an enum value, so seems like a doc comment clarifying that this is the enum value name feels like it suffices. Another option is to extract out another type -- like maybe an EnumLiteral that has both a name and a number. But, honestly, that felts a little overkill 🤷

bufdev

Overall, for long-term maintainability here, I wish we could lean less into this as a frontend-specific API, and put more of the computation for docs into the FE going forward, i.e. I wish all that the generated documentation needed to do was query the reflection endpoint, get a FileDescriptorSet, and the frontend uses protobuf-es to do the rest. I recognize that's a major change and not in scope, so approving, but I do wonder if we're adding more pain for us down the road.

bufdev · 2024-02-12T21:28:39Z

proto/buf/alpha/registry/v1alpha1/doc.proto

+    uint64 uint_value = 4;
+    double double_value = 5;
+    bool bool_value = 6;
+    string enum_value = 7;


@jhump should this semantically be ie enum_value_name_value, if that makes sense? It's not an enum, its an enum value, and its the name of the enum value...I don't know.

Part 2 of #2752. Adding `FieldLiteral` to `Message`, `Enum`, `Service`, `Method` and `EnumValue` responses

gilwong00 added 6 commits January 26, 2024 12:37

Add custom option message and method to extract custom options from p…

61519bb

…roto

Add custom options to more proto messages

f7a73b2

Merge branch 'main' into gwong/custom-options

cd5a5e7

Add docs

2d08b8c

Rename custom options to options and update docs

7a67def

Clean up

b3561d0

gilwong00 requested a review from bufdev as a code owner February 5, 2024 21:53

gilwong00 requested review from jhump and bufdev and removed request for bufdev February 5, 2024 21:53

jhump reviewed Feb 5, 2024

View reviewed changes

proto/buf/alpha/registry/v1alpha1/doc.proto Outdated Show resolved Hide resolved

proto/buf/alpha/registry/v1alpha1/doc.proto Outdated Show resolved Hide resolved

proto/buf/alpha/registry/v1alpha1/doc.proto Outdated Show resolved Hide resolved

gilwong00 added 3 commits February 5, 2024 15:25

Feedback

eaeb872

Feedback

add9257

Add doc explaining map usage

6e34ba6

gilwong00 requested a review from jhump February 5, 2024 23:51

jhump reviewed Feb 6, 2024

View reviewed changes

proto/buf/alpha/registry/v1alpha1/doc.proto Outdated Show resolved Hide resolved

proto/buf/alpha/registry/v1alpha1/doc.proto Outdated Show resolved Hide resolved

proto/buf/alpha/registry/v1alpha1/doc.proto Outdated Show resolved Hide resolved

Feedback

208bd34

gilwong00 requested a review from jhump February 6, 2024 18:12

fyockm reviewed Feb 6, 2024

View reviewed changes

private/pkg/protosource/field.go Outdated Show resolved Hide resolved

Undo import update

3630870

bufdev reviewed Feb 6, 2024

View reviewed changes

private/pkg/protosource/option_extension_descriptor.go Outdated Show resolved Hide resolved

Feedback

dbfd00d

gilwong00 requested a review from bufdev February 7, 2024 19:44

jhump reviewed Feb 7, 2024

View reviewed changes

private/pkg/protosource/protosource.go Outdated Show resolved Hide resolved

bufdev reviewed Feb 7, 2024

View reviewed changes

private/pkg/protosource/protosource.go Outdated Show resolved Hide resolved

Feedback

738e213

gilwong00 requested review from bufdev and jhump February 7, 2024 20:30

jhump reviewed Feb 7, 2024

View reviewed changes

private/pkg/protosource/option_extension_descriptor.go Outdated Show resolved Hide resolved

bufdev added 2 commits February 7, 2024 16:02

Merge branch 'main' into gwong/custom-options

a207178

updates

89c0eec

gilwong00 requested a review from jhump February 7, 2024 21:21

Merge branch 'main' into gwong/custom-options

17b4060

bufdev reviewed Feb 8, 2024

View reviewed changes

gilwong00 added 2 commits February 8, 2024 12:26

Update PresentExtensionNumbers docs and add more primitives

a6b48de

Update comment

2da3f36

gilwong00 mentioned this pull request Feb 8, 2024

Add ForEachPresentOption method to optionExtensionDescriptor #2758

Merged

gilwong00 added a commit that referenced this pull request Feb 9, 2024

Add ForEachPresentOption method to optionExtensionDescriptor (#2758)

b585408

Pulling changes to `protosource` from #2752 into its own PR.

Merge branch 'main' into gwong/custom-options

148227e

timostamm reviewed Feb 9, 2024

View reviewed changes

Revert addition of primitive types

138554f

bufdev reviewed Feb 12, 2024

View reviewed changes

gilwong00 added 2 commits February 12, 2024 12:29

Feedback

321a091

Undo int change and add field type enum for any messages

508f129

bufdev approved these changes Feb 12, 2024

View reviewed changes

Clean up docs

d39c6ce

gilwong00 merged commit e3c2810 into main Feb 13, 2024
11 checks passed

gilwong00 deleted the gwong/custom-options branch February 13, 2024 00:01

gilwong00 changed the title ~~Add options to doc service~~ Add FieldLiteral proto message Feb 13, 2024

gilwong00 mentioned this pull request Feb 15, 2024

Add options to doc response #2770

Merged

gilwong00 added a commit that referenced this pull request Feb 16, 2024

Add options to doc response (#2770)

21acb56

Part 2 of #2752. Adding `FieldLiteral` to `Message`, `Enum`, `Service`, `Method` and `EnumValue` responses

		ArrayLiteral array_value = 16;
		MessageLiteral message_value = 17;

Add FieldLiteral proto message #2752

Add FieldLiteral proto message #2752

Conversation

gilwong00 commented Feb 5, 2024 • edited Loading

jhump left a 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

Choose a reason for hiding this comment

gilwong00 commented Feb 5, 2024

jhump left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhump Feb 7, 2024 • edited Loading

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

Choose a reason for hiding this comment

bufdev commented Feb 7, 2024

jhump commented Feb 8, 2024 • edited Loading

bufdev commented Feb 8, 2024

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

timostamm Feb 9, 2024 • edited Loading

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

timostamm left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bufdev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilwong00 commented Feb 5, 2024 •

edited

Loading

jhump Feb 7, 2024 •

edited

Loading

jhump commented Feb 8, 2024 •

edited

Loading

timostamm Feb 9, 2024 •

edited

Loading