Proposal: write public constants for method names on code generation about twirp HOT 4 CLOSED

Hi @rafaelrubbioli . It sounds like you're concerned about spelling mistakes when using the non-type-safe hooks API that Twirp provides (such as if method == "MyMethodd"), or possibly when renaming (if method == "MyOldMethod"). Is that right?

I see a few options for that, none of which is perfect.

First, adding constants as you suggested. It's simple and easy to use, but means a much larger exported interface in each Twirp package. Each new kind of exported identifier we add to Twirp brings a new chance of colliding with the other set of exported identifiers that are already in the package. When a package includes definitions for several services, the constants for the methods for each service need to be kept separate, which seems like it would bring new chances for the sort of bug that prompted this suggestion in the first place (right method name for a different service, wrong for this service).

As a second option, a new code generator (maybe called protoc-gen-twirp-method-constants) could write out an exported constant string for each service+method. It has a similar set of problems as the first option, but wouldn't apply to all Twirp users.

A third option is to use the ServiceDescriptor method to get the original protobuf type data for the service. One way to use that would be to hand-write constants for the method name, and then check them against the authoritative list of methods in a test. Or, option 3b, to write a code generator that uses that ServiceDescriptor to write out a const for each method. (Which would basically be option 2, but without needing to implement the protobuf generator API or to modify the protoc call.)

I think option 3a would be the easiest to get going. Does that work for you?

Or, can you say more about the need for hooks on specific methods? Is it something that could be addressed well with more direct Go code?

type guardHats struct {
  Haberdasher
}

func (g *guardHats) MakeHat(ctx context.Context, in *Size) (*Hat, error) {
  if err := checkAuthorization(ctx); err != nil {
    return nil, err
  }
  out, err := g.Haberdasher.MakeHat(ctx, in)
  return out, err
}

from twirp.

@rhysh Thanks for addressing my issue!
Yes, I was not thinking about the exported interface size, but I think collisions could easily be avoided by using the service name as a prefix for the constant name, for example. I don't think is worth the effort of writing a new code generator, though, it was just a thought I had to improve code resilience.

I'm not quite sure I understand the third option, could you elaborate or write an example of usage?

from twirp.

Here's what I have in mind for option 3a, to check a list of hand-written constants against the Twirp's current code generation: https://go.dev/play/p/FnEo9NYIxED

That runs in an init function, but it could also be in a test or elsewhere.

package main

import (
	"bytes"
	"compress/gzip"
	"fmt"
	"io/ioutil"

	"github.com/twitchtv/twirp/example"
	"google.golang.org/protobuf/proto"
	"google.golang.org/protobuf/types/descriptorpb"
)

const (
	hatCreationMethod = "/twitch.twirp.example.Haberdasher/MakeHat"
	hatSalesMethod    = "/twitch.twirp.example.Haberdasher/SellHat"
)

func init() {
	// extract service definition
	buf, idx := example.NewHaberdasherServer(nil).ServiceDescriptor()
	gz, err := gzip.NewReader(bytes.NewReader(buf))
	if err != nil {
		panic(err)
	}
	buf, err = ioutil.ReadAll(gz)
	if err != nil {
		panic(err)
	}
	var fd descriptorpb.FileDescriptorProto
	err = proto.Unmarshal(buf, &fd)
	if err != nil {
		panic(err)
	}

	// build a list of hand-written const values to confirm
	methodConsts := map[string]struct{}{
		hatCreationMethod: {},
		hatSalesMethod:    {},
	}

	// verify that every hand-written const appears in the service definition
	srv := fd.Service[idx]
	for _, m := range srv.Method {
		name := fmt.Sprintf("/%s.%s/%s", fd.GetPackage(), srv.GetName(), m.GetName())
		delete(methodConsts, name)
		fmt.Printf("DEBUG: found method %q\n", name)
	}

	// complain about leftovers
	for m := range methodConsts {
		fmt.Printf("PROBLEM: did not find method for hand-written const %q\n", m)
	}
	if len(methodConsts) > 0 {
		panic("need to update hand-written constants")
	}
}

func main() {}

DEBUG: found method "/twitch.twirp.example.Haberdasher/MakeHat"
PROBLEM: did not find method for hand-written const "/twitch.twirp.example.Haberdasher/SellHat"
panic: need to update hand-written constants

goroutine 1 [running]:
main.init.0()
	/tmp/sandbox694368234/prog.go:55 +0x505

Program exited.

from twirp.

@rhysh got it now!
This would fit well in a test, that way it would prevent invalid constants to be deployed on CI/CD!
Thanks

from twirp.