Add buf.registry.descriptor.v1 package #102

bufdev · 2024-03-31T20:12:36Z

This adds the buf.registry.descriptor.v1 package, specifically containing the FileDescriptorProtoService. This service is meant to supplant the following constructs:

https://github.com/bufbuild/buf/blob/5cd39655aefedaedddf8743ed16944e722c598e1/proto/buf/alpha/registry/v1alpha1/image.proto#L22 - it entirely replaces this.
https://github.com/bufbuild/reflect-proto/blob/66bfea7a86e10b3cb421c4b90201b74becda519f/buf/reflect/v1beta1/file_descriptor_set.proto#L32 - this one is more debatable, but we never invested in this as an independent mechanism, so buf.registry.descriptor.v1 serves as the BSR-specific version of this (with additional features).
https://github.com/bufbuild/buf/blob/5cd39655aefedaedddf8743ed16944e722c598e1/proto/buf/alpha/image/v1/image.proto - in theory at least. This package contains all the same information, but not using the Image type, which we'd deprecate if we didn't already commit to it in v1 of bufbuild/buf.

Some design decisions made:

This continues to lean into the ResourceRef model, letting users easily reference modules, labels, or commits in a single RPC. This prevents some difficulty with caching, but no more than the DownloadService, which serves a similar mechanism at a source-file level. We decided earlier on that it's better to make this a one-RPC operation than worry about caching here, although we can revisit that (quickly) for both the FileDescriptorProtos service and DownloadService if we want (I think in practice, the current shape serves us best).
This uses a FieldMask for the first time, in what I think is a clean usage of it. We don't like FieldMask in general, but in lieu of a better option, this feels like what we should go with. We could discuss having our own tag-based idea of field mask, but we can also always just deprecate this field anyways.
This leans into the google.protobuf types as opposed to continuing down the path that buf.alpha.image.v1.Image started with quasi-extending FileDescriptorSet/FileDescriptorProto in an unsupported way. It returns the same information via a sideband type FileDescriptorProtoExtension that is included on GetFileDescriptorProtosResponse, which feels more in line with the rest of our APIs in registry-proto.
This returns repeated FileDescriptorProto instead of FileDescriptorSet. The primary motivator for this was the FieldMask field, however it likely just makes more sense - FileDescriptorProtos can be put into i.e. CodeGeneratorRequests, and I haven't seen a usage of FileDescriptorSet in the wild that i.e. adds fields to FileDescriptorSet (which of course, outside of what we do with buf.alpha.v1.Image, is impossible as it is, and we didn't even do it in buf.alpha.v1.Image). Making this repeated FileDescriptorProto` feels more appropriate.
The FileDescriptorProtoExtension type uses commit_id instead of Commit. This is in line with Graph, where only a commit_id is returned, as opposed to Download where a Commit is returned. With FileDescriptorProtos as well as Graphs, the Commit may be duplicated 1000s of times, so an ID feels more proper here to prevent duplicated information in the common case (on Download, Commits could in theory be duplicated if two or more ResourceRefs point to the same Commit, however this is an edge case).
This uses the symbols terminology from reflect-proto instead of types. This one is more debatable.
This uses exclude_source_code_info instead of exclude_source_info as the equivalent flag is named on protoc and buf. The actual FileDescriptorProto field is named source_code_info, so this feels more proper. Back when I was writing buf, I debated naming the flag --exclude-source-code-info to be honest, but didn't so as to have more parity with protoc.

Note that I did contemplate whether we wanted to split out DownloadService, UploadService into a new file package, and potentially GraphService into a graph package (the latter being less desirable, as we could have graphs of other things i.e. plugins). That's a conversation I still want to briefly have - I feel the current setup leans us toward the module package being a mega-package, and you could make an argument that descriptor should fit within it as well.

Alfus · 2024-03-31T22:31:25Z

(tag/value based field_mask for thrift: https://github.com/facebook/fbthrift/blob/main/thrift/lib/thrift/field_mask.thrift)

bufdev · 2024-04-01T14:37:19Z

buf/registry/descriptor/v1/file_descriptor_proto_extension.proto

+  // As opposed to syntax on a FileDescriptorProto, this field will be empty if no syntax was
+  // explicitly specified, and "proto2" if "proto2" was explicitly specified. Otherwise, this
+  // field matches the semantics of the syntax field on FileDescriptorProtos.
+  string specified_syntax = 3;


Additional design note - this was a bool syntax_specified in image.proto, but it seems clearer to make this the actual specified syntax, if it exists.

bufdev · 2024-04-01T14:37:46Z

buf/registry/descriptor/v1/file_descriptor_proto_service.proto

+  //
+  // SourceCodeInfo is optional additional information that is not required to describe a set
+  // of Protobuf APIs.
+  bool exclude_source_code_info = 3;


Open question whether this should be include or exclude - we decided for the HTTP GET endpoint to make this include.

In my mind, the decision on whether to call this include or exclude depends on whatever you want to be returned by default. If this should be returned by default, then exclude is right (opt-out). If it shouldn't be returned by default, then include is right (opt-in). To decide what the default would be, I would think about the anticipated use cases and whether a majority of those use cases would need or not need this info. (similarly for the other fields)

Writing tools against descriptors, I find myself constantly forgetting that the info is not returned by default and I need to set a flag somewhere in the chain of tools to ensure the info is available. I'd rather need to ask for it to be stripped as an optimization than have to remember to include it, personally.

This goes for the other opt-in/out questions below, as well.

Including by default is more convenient for use cases like the CLI. Excluding by default is better for constrained environments, where things suddenly stop working because a giant module with many comments shows up.

Giant modules have other downsides. Including by default is the least surprising behavior in my book.

bufdev · 2024-04-01T14:37:50Z

buf/registry/descriptor/v1/file_descriptor_proto_service.proto

+  //
+  // With the new Editions work, options are either runtime-retention or source-retention.
+  // Setting this option will exclude the latter.
+  bool exclude_source_retention_options = 4;


Open question whether this should be include or exclude - we decided for the HTTP GET endpoint to make this include.

bufdev · 2024-04-01T14:38:01Z

buf/registry/descriptor/v1/file_descriptor_proto_service.proto

+  // FileDescriptorProtoExtensions contain additional information about FileDescriptorProtos.
+  // We include this information in a separate message as FileDescriptorProtos are not
+  // extendable, and we do not want to modify the FileDescriptorProto shape.
+  bool exclude_file_descriptor_proto_extensions = 5;


Open question whether this should be include or exclude.

buf/registry/descriptor/v1/file_descriptor_proto_extension.proto

nicksnyder · 2024-04-01T17:26:17Z

buf/registry/descriptor/v1/file_descriptor_proto_extension.proto

+  // The explicitly-specified syntax within the file.
+  //
+  // If the syntax field is not set within a file, this is interpreted as the syntax "proto2".
+  // The protoc compiler does not set the syntax field if the syntax was "proto2", regardless of if
+  // the syntax was explicitly specified or not. This prevents programs (such as linters) from
+  // determining whether or not a syntax was explicitly specified within a file
+  //
+  // As opposed to syntax on a FileDescriptorProto, this field will be empty if no syntax was
+  // explicitly specified, and "proto2" if "proto2" was explicitly specified. Otherwise, this
+  // field matches the semantics of the syntax field on FileDescriptorProtos.


I found the middle paragraph here a bit of a tangent that was confusing on first read. Wondering if we can resequence this documentation so the behavior is documented first, and then the "why" is last (if necessary at all).

Suggested change

// The explicitly-specified syntax within the file.

//

// If the syntax field is not set within a file, this is interpreted as the syntax "proto2".

// The protoc compiler does not set the syntax field if the syntax was "proto2", regardless of if

// the syntax was explicitly specified or not. This prevents programs (such as linters) from

// determining whether or not a syntax was explicitly specified within a file

//

// As opposed to syntax on a FileDescriptorProto, this field will be empty if no syntax was

// explicitly specified, and "proto2" if "proto2" was explicitly specified. Otherwise, this

// field matches the semantics of the syntax field on FileDescriptorProtos.

// The explicitly-specified syntax within the file.

// If the syntax field is not set within a file, this is interpreted as the syntax "proto2".

//

// As opposed to syntax on a FileDescriptorProto, this field will be empty if no syntax was

// explicitly specified, and "proto2" if "proto2" was explicitly specified. This enables programs

// (such as linters) to determine whether or not a syntax was explicitly specified within a file.

// Otherwise, this field matches the semantics of the syntax field on FileDescriptorProtos.

buf/registry/descriptor/v1/file_descriptor_proto_service.proto

nicksnyder · 2024-04-01T18:19:08Z

buf/registry/descriptor/v1/file_descriptor_proto_service.proto

+  //
+  // SourceCodeInfo is optional additional information that is not required to describe a set
+  // of Protobuf APIs.
+  bool exclude_source_code_info = 3;


In my mind, the decision on whether to call this include or exclude depends on whatever you want to be returned by default. If this should be returned by default, then exclude is right (opt-out). If it shouldn't be returned by default, then include is right (opt-in). To decide what the default would be, I would think about the anticipated use cases and whether a majority of those use cases would need or not need this info. (similarly for the other fields)

nicksnyder · 2024-04-01T18:24:41Z

buf/registry/descriptor/v1/file_descriptor_proto_service.proto

+  bool exclude_source_code_info = 3;
+  // Exclude source-retention options from the returned FileDescriptorProtos.
+  //
+  // With the new Editions work, options are either runtime-retention or source-retention.


Suggested change

// With the new Editions work, options are either runtime-retention or source-retention.

// With Protobuf Editions, options are either runtime-retention or source-retention.

buf/registry/descriptor/v1/file_descriptor_proto_service.proto

jhump

https://github.com/bufbuild/reflect-proto/blob/66bfea7a86e10b3cb421c4b90201b74becda519f/buf/reflect/v1beta1/file_descriptor_set.proto#L32 - this one is more debatable, but we never invested in this as an independent mechanism, so buf.registry.descriptor.v1 serves as the BSR-specific version of this (with additional features).

By supplant this, you mean we plan to remove the existing reflection endpoint? We had a blog post about it, and it's used by the prototransform module, the stand-alone knit gateway, and the vanguard-envoy filter. I guess that last one isn't public so definitely not used. The others may not be used much, but it seems likely to be non-zero.

jhump · 2024-04-01T23:36:34Z

buf/registry/descriptor/v1/file_descriptor_proto_service.proto

+  // This will match the semantics used within CodeGeneratorRequests.
+  google.protobuf.compiler.Version compiler_version = 3 [(buf.validate.field).required = true];


Does this imply we're doing something new here? I thought we used to always set the version to nil/unset since version of the Buf CLI weren't really comparable to versions of protoc.

rodaine · 2024-04-02T18:08:33Z

buf/registry/descriptor/v1/file_descriptor_proto_extension.proto

+  // As opposed to syntax on a FileDescriptorProto, this field will be empty if no syntax was
+  // explicitly specified, and "proto2" if "proto2" was explicitly specified. Otherwise, this
+  // field matches the semantics of the syntax field on FileDescriptorProtos.
+  string specified_syntax = 3;


Do we want to validate this with an in constraint?

rodaine · 2024-04-02T18:12:29Z

buf/registry/descriptor/v1/file_descriptor_proto_service.proto

+  //
+  // SourceCodeInfo is optional additional information that is not required to describe a set
+  // of Protobuf APIs.
+  bool exclude_source_code_info = 3;


Writing tools against descriptors, I find myself constantly forgetting that the info is not returned by default and I need to set a flag somewhere in the chain of tools to ensure the info is available. I'd rather need to ask for it to be stripped as an optimization than have to remember to include it, personally.

This goes for the other opt-in/out questions below, as well.

timostamm · 2024-04-03T14:43:37Z

buf/registry/descriptor/v1/file_descriptor_proto_service.proto

+  // A symbol is a package, message, enum, service, method, or extension.
+  //
+  // Symbols are referenced by their fully-qualified name. For example, if "Foo" is a message within
+  // the package "foo.v1", the fully-qualified name is "foo.v1.Foo". Fully-qualfied names should


Suggested change

// the package "foo.v1", the fully-qualified name is "foo.v1.Foo". Fully-qualfied names should

// the package "foo.v1", the fully-qualified name is "foo.v1.Foo". Fully-qualified names should

timostamm · 2024-04-03T15:08:06Z

buf/registry/descriptor/v1/file_descriptor_proto_extension.proto

+  // As opposed to syntax on a FileDescriptorProto, this field will be empty if no syntax was
+  // explicitly specified, and "proto2" if "proto2" was explicitly specified. Otherwise, this
+  // field matches the semantics of the syntax field on FileDescriptorProtos.
+  string specified_syntax = 3;


So for a proto file with syntax="proto2", or without a syntax specified, FileDescriptorProto.syntax is blank, and this field is "proto2".

What happens for a file with edition="2023"? FileDescriptorProto.syntax will be "editions" in this case, but syntax wasn't really specified. Is this field "", or "editions"?

buf/registry/descriptor/v1/file_descriptor_proto_extension.proto

akshayjshah · 2024-05-20T16:37:52Z

buf/registry/descriptor/v1/file_descriptor_proto_service.proto

+  // Get compiled FileDescriptorProtos for the given Module, Label, or Commit.
+  rpc GetFileDescriptorProtos(GetFileDescriptorProtosRequest) returns (GetFileDescriptorProtosResponse) {
+    option idempotency_level = NO_SIDE_EFFECTS;
+  }


Late to the party, but this is the most important thing to me: the RPC should be side effect free, so we can expose it as an HTTP GET using Connect.

Co-authored-by: Stefan VanBuren <[email protected]>

…stry-proto into descriptor-package

github-actions · 2024-09-13T16:06:46Z

The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
`✅ passed`	`❌ failed (1)`	`✅ passed`	`✅ passed`	Sep 13, 2024, 4:06 PM

Add buf.registry.descriptor.v1 package

e2deecc

bufdev requested review from akshayjshah, rodaine, jhump, timostamm, Alfus and mfridman March 31, 2024 20:12

s/file_descriptor_proto_infos/file_descriptor_proto_extensions/

2b476f0

bufdev requested a review from nicksnyder March 31, 2024 20:29

Alfus approved these changes Mar 31, 2024

View reviewed changes

bufdev commented Apr 1, 2024

View reviewed changes

bufdev added 2 commits April 1, 2024 20:02

Merge branch 'main' into descriptor-package

514a6e5

Change to tuuid

a08a12b

nicksnyder reviewed Apr 1, 2024

View reviewed changes

Fix typos

2a81195

jhump reviewed Apr 1, 2024

View reviewed changes

rodaine approved these changes Apr 2, 2024

View reviewed changes

timostamm reviewed Apr 3, 2024

View reviewed changes

stefanvanburen reviewed Apr 19, 2024

View reviewed changes

buf/registry/descriptor/v1/file_descriptor_proto_extension.proto Outdated Show resolved Hide resolved

akshayjshah reviewed May 20, 2024

View reviewed changes

nicksnyder and others added 4 commits May 20, 2024 14:04

Update buf/registry/descriptor/v1/file_descriptor_proto_extension.proto

4cca335

Co-authored-by: Stefan VanBuren <[email protected]>

Merge branch 'main' into descriptor-package

52b59ac

commit

b68d822

gerge branch 'descriptor-package' of https://github.com/bufbuild/regi…

6aa1167

…stry-proto into descriptor-package

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add buf.registry.descriptor.v1 package #102

Add buf.registry.descriptor.v1 package #102

bufdev commented Mar 31, 2024

Alfus commented Mar 31, 2024

bufdev Apr 1, 2024

bufdev Apr 1, 2024

nicksnyder Apr 1, 2024

rodaine Apr 2, 2024

timostamm Apr 3, 2024

bufdev Apr 1, 2024

bufdev Apr 1, 2024

nicksnyder Apr 1, 2024

nicksnyder Apr 1, 2024

nicksnyder Apr 1, 2024

jhump left a comment

jhump Apr 1, 2024

rodaine Apr 2, 2024

rodaine Apr 2, 2024

timostamm Apr 3, 2024

timostamm Apr 3, 2024

akshayjshah May 20, 2024

github-actions bot commented Sep 13, 2024

	// With the new Editions work, options are either runtime-retention or source-retention.
	// With Protobuf Editions, options are either runtime-retention or source-retention.

		// This will match the semantics used within CodeGeneratorRequests.
		google.protobuf.compiler.Version compiler_version = 3 [(buf.validate.field).required = true];

	// the package "foo.v1", the fully-qualified name is "foo.v1.Foo". Fully-qualfied names should
	// the package "foo.v1", the fully-qualified name is "foo.v1.Foo". Fully-qualified names should

Add buf.registry.descriptor.v1 package #102

Are you sure you want to change the base?

Add buf.registry.descriptor.v1 package #102

Conversation

bufdev commented Mar 31, 2024

Alfus commented Mar 31, 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 13, 2024