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.

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

Convert Request and ResponseWriter to interfaces #10

Merged
merged 1 commit into from
Mar 18, 2024
Merged

Conversation

bufdev
Copy link
Member

@bufdev bufdev commented Mar 17, 2024

The primary purpose of this PR is to convert Request and ResponseWriter into interfaces that each contain a private method, preventing construction outside of this module.

Our motivation for this started with wanting to not duplicate logic across buf and protoplugin. There's some work we have to do to support editions that effectively would be repeated in both modules, and protoplugin already largely copies the protoplugin package within buf - it's how this module started in the first place. By centralizing the logic, we get one place to keep it up to date, and a concentrated set of (soon to be added) testing within this module to make sure it all works properly. We already had better validation within protoplugin than we ever had in buf, and we want to take advantage of that while we work to support editions.

buf, however, does some special things that other plugin authors won't do. Specifically, it has the ability to take multiple CodeGeneratorRequests, and output a single CodeGeneratorResponse after executing the equivalent of a Handler in parallel. No one should do this outside of buf - it's a super-speciality use-case, and creating parallelizable CodeGeneratorRequests correctly is a challenge in itself - few need to proxy to other plugins, let alone in parallel. Part of what buf does is call what amounts to newRequest for each CodeGeneratorRequest, and create a single ResponseWriter that each parallel call calls to.

Before this PR, protoplugin had the Request and ResponseWriter types as structs, without any useful ability to create these types outside of this package and use them. This was fine, as we purposefully wanted to limit Handler invocation to the Main and Run functions. However, to take advantage of protoplugin within buf, and to allow the parallel execution use case, we had to do one of two things:

  1. Re-create the parallel logic within protoplugin itself. This seemed bad, and would dirty up the API for this single use case.
  2. Make it possible to create Requests and ResponseWriters in useful ways, and then support the use case of Handlers being invoked outside of Main or Run.

(2) seemed preferable, including outside of this specific use case - if we expose these Request and ResponseWriter types, and tell users to implement a Handler type, it seems appropriate to let users invoke these Handlers (including in testing) on their own. However, a central tenant of this package is that users can rely on Requests being validated, and ResponseWriters behaving appropriately. If we're telling users to construct their own Requests, and Requests were structs, Golang idiomatic behavior would say that request := &protoplugin.Request{} should be a valid way to construct a request. We need to force users through a constructor, however, for Request in its current form (which we like) to make any sense - validation has to be performed, and we want users to rely on the fact that Requests are always valid.

@mfridman and I discussed this approach offline and this seemed to make the most sense. @mfridman, this is the result of that - let me know what you think.

Of note, this also does the rename of HandlerEnv to PluginEnv, and removes the usage of pointers for Env and PluginEnv - the pointers don't really make sense in this case, and now that we're leaning into interfaces, there's less of an argument to use pointers to be consistent within this library. CompilerVersion remains a pointer when returned fromRequest, as presence is important.

The API will likely evolve further from here - it's not great that there are two options WithLenientValidation and ResponseWriterWithLenientValidation. But this gets the basics in.

@bufdev bufdev requested a review from mfridman March 17, 2024 23:48
// NewRequest returns a new Request for the CodeGeneratorRequest.
//
// The CodeGeneratorRequest will be validated as part of construction.
func NewRequest(codeGeneratorRequest *pluginpb.CodeGeneratorRequest) (Request, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Okay, this is making more sense now why it should be in the same package. Disregard my suggestion to move this out into a sub-package.

@bufdev bufdev merged commit 6e44eff into main Mar 18, 2024
5 checks passed
@bufdev bufdev deleted the interfaces branch March 18, 2024 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants