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

Improve comments & add procedure consts to generated code #480

Merged
merged 5 commits into from
Mar 25, 2023
Merged

Improve comments & add procedure consts to generated code #480

merged 5 commits into from
Mar 25, 2023

Conversation

akshayjshah
Copy link
Member

This PR improves protoc-gen-connect-go's generated code in two ways:

  • First, it propagates comments on the protobuf syntax or package statements to
    the generated code. This matches protoc-gen-go's behavior, with one small
    improvement: comments attached to the protobuf package statement are also
    attached to the Go package declaration (vs. leaving an intervening blank
    line). This lets package-level protobuf comments also serve as package-level
    GoDoc.
  • Second, it generates constants for each RPC's fully-qualified name. This lets
    users easily customize the behavior of interceptors or net/http middleware
    on a per-procedure basis. This has come up repeatedly, beginning shortly
    after release, so we ought to provide a satisfactory answer.
    protoc-gen-go-grpc began doing something similar in the most recent
    release.

Fixes #326, #470, and #471.

@akshayjshah
Propagate top-level comments from proto schemas
3f0fa00 
If the source schema has leading or leading detached comments on the
protobuf syntax or package declarations, propagate them to the generated
code. This matches the behavior of protoc-gen-go and protoc-gen-go-grpc,
with one small change: we keep protobuf package comments adjacent to the
Go package declaration, so that they appear as the package-level
documentation in GoDoc.

Fixes #470.
@akshayjshah
Generate constants for procedure names
5c87842 
Generate constants for each RPC's fully-qualified name.

This is particularly useful in Connect interceptors and net/http
middleware, where users often want their code to behave differently
depending on the RPC being called. (For example, they may have a subset
of procedures exempted from authentication.) Without this PR, the best
approach we can offer is either hard-coding a string literal (fragile if
the package or RPC is moved, renamed, or deleted) or reflection using
the descriptor embedded in protoc-gen-go's output.

protoc-gen-go-grpc recently began doing something very similar to this,
so it seems reasonable for us to do the same.

Of the open issues, this fixes #326.
@akshayjshah akshayjshah changed the title Improve comments & add procedure consts to generate code Improve comments & add procedure consts to generated code Mar 18, 2023
bufdev
bufdev reviewed Mar 20, 2023
View reviewed changes
cmd/protoc-gen-connect-go/main.go Show resolved Hide resolved
cmd/protoc-gen-connect-go/main.go Outdated Show resolved Hide resolved
cmd/protoc-gen-connect-go/main.go Show resolved Hide resolved
pkwarren
pkwarren reviewed Mar 21, 2023
View reviewed changes
Copy link
Contributor

@pkwarren pkwarren left a comment

Choose a reason for hiding this comment

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

Changes look great - I'm not great at naming so will let y'all figure out the best name for the constants.

cmd/protoc-gen-connect-go/main.go Outdated Show resolved Hide resolved
cmd/protoc-gen-connect-go/main.go
wrapComments(g, "Note that these are different from the fully-qualified method names used by ",
"google.golang.org/protobuf/reflect/protoreflect. To convert from these constants to ",
"reflection-formatted method names, remove the leading slash and convert the ",
"remaining slash to a period.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of documenting this in the generated code, is it worth adding a utility function to convert a procedure to a protoreflect.FullName? That seems like a useful method and we could simplify the generated code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm hoping to minimize the number of places that the connect-go runtime couples closely to the protobuf runtime. I suspect that the most common reason people would make this translation is to get a method descriptor, so I'm planning to just expose that directly in a subsequent PR.

internal/gen/connect/ping/v1/pingv1connect/ping.connect.go
// Code generated by protoc-gen-connect-go. DO NOT EDIT.
//
// Source: connect/ping/v1/ping.proto

// The connect.ping.v1 package contains an echo service designed to test the
// connect-go implementation.
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the go code adds a trailing space here before the package statement.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. I'm not sure why - the commits over there don't clarify. It looks like this is perhaps an artifact of reusing the same function for the comments on the protobuf package and the comments on the protobuf syntax. For us, it seems nice to produce Go code with package docs.

internal/gen/connect/ping/v1/ping.pb.go Show resolved Hide resolved
internal/gen/connect/ping/v1/pingv1connect/ping.connect.go
@@ -12,10 +12,15 @@
// See the License for the specific language governing permissions and
// limitations under the License.

// The canonical location for this file is
Copy link
Contributor

Choose a reason for hiding this comment

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

Is one benefit now for people not using our license header tools is that the license in the .proto file will be included?

Copy link
Contributor

Choose a reason for hiding this comment

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

It does look like that is the result - here's a run from the plugins repo from two images exported w/ buf export: https://gist.github.com/pkwarren/0628b6e2412d7f6483e59c23344d9079

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep! This seemed like a good thing to me - do you think it's ok?

jchadwick-buf
jchadwick-buf approved these changes Mar 22, 2023
View reviewed changes
Copy link
Contributor

@jchadwick-buf jchadwick-buf left a comment

Choose a reason for hiding this comment

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

I didn't see anything of interest that hadn't already been pointed out, so LGTM.

@akshayjshah akshayjshah merged commit c1c69ea into main Mar 25, 2023
@akshayjshah akshayjshah deleted the ajs/gen branch March 25, 2023 02:07
@VoyTechnology
Copy link

Any estimates as to when this change will land in a release?

@akshayjshah
Copy link
Member Author

Likely Monday!

renovate bot referenced this pull request in open-feature/flagd Apr 14, 2023
@renovate
fix(deps): update module github.com/bufbuild/connect-go to v1.6.0 (#585)
8f2f467 
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
|
[github.com/bufbuild/connect-go](https://togithub.com/bufbuild/connect-go)
| require | minor | `v1.5.2` -> `v1.6.0` |

---

### Release Notes

<details>
<summary>bufbuild/connect-go</summary>

###
[`v1.6.0`](https://togithub.com/bufbuild/connect-go/releases/tag/v1.6.0)

[Compare
Source](https://togithub.com/bufbuild/connect-go/compare/v1.5.2...v1.6.0)

<!-- Release notes generated using configuration in .github/release.yml
at main -->

#### What's Changed

##### Enhancements

- Improve comments & add procedure consts to generated code by
[@&#8203;akshayjshah](https://togithub.com/akshayjshah) in
[https://github.com/bufbuild/connect-go/pull/480](https://togithub.com/bufbuild/connect-go/pull/480)
- Reduce per-call URL parsing cost by
[@&#8203;mattrobenolt](https://togithub.com/mattrobenolt) in
[https://github.com/bufbuild/connect-go/pull/467](https://togithub.com/bufbuild/connect-go/pull/467)
- Improve errors for outdated protobuf runtimes by
[@&#8203;akshayjshah](https://togithub.com/akshayjshah) in
[https://github.com/bufbuild/connect-go/pull/465](https://togithub.com/bufbuild/connect-go/pull/465)
- Switch README to use `buf curl` by
[@&#8203;akshayjshah](https://togithub.com/akshayjshah) in
[https://github.com/bufbuild/connect-go/pull/474](https://togithub.com/bufbuild/connect-go/pull/474)

##### Bugfixes

- Clarify purpose of handler_stream_test.go by
[@&#8203;Hirochon](https://togithub.com/Hirochon) in
[https://github.com/bufbuild/connect-go/pull/472](https://togithub.com/bufbuild/connect-go/pull/472)
- Make StreamType constants typed numerics by
[@&#8203;jhump](https://togithub.com/jhump) in
[https://github.com/bufbuild/connect-go/pull/486](https://togithub.com/bufbuild/connect-go/pull/486)
- Populate Spec and Peer in Client.CallServerStream by
[@&#8203;jhump](https://togithub.com/jhump) in
[https://github.com/bufbuild/connect-go/pull/487](https://togithub.com/bufbuild/connect-go/pull/487)

#### New Contributors

- [@&#8203;Hirochon](https://togithub.com/Hirochon) made their first
contribution in
[https://github.com/bufbuild/connect-go/pull/472](https://togithub.com/bufbuild/connect-go/pull/472)

**Full Changelog**:
bufbuild/connect-go@v1.5.2...v1.6.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/open-feature/flagd).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4zMS40IiwidXBkYXRlZEluVmVyIjoiMzUuMzEuNCJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
akshayjshah added a commit that referenced this pull request Jul 26, 2023
@akshayjshah
Improve comments & add procedure consts to generated code (#480)
d13c1a3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timostamm timostamm timostamm left review comments

@pkwarren pkwarren pkwarren approved these changes

@bufdev bufdev bufdev approved these changes

@jchadwick-buf jchadwick-buf jchadwick-buf approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

HTTP middleware can't identify RPC paths
6 participants
@akshayjshah @VoyTechnology @pkwarren @bufdev @timostamm @jchadwick-buf