API improvements for dynamic usages

[jhump]

In light of recent experience report using this module with a dynamic schema, this PR proposes several changes to make such work easier. It also puts additional validation into initialization, so we can fail faster and (hopefully) with better error messages.

1. Don't make user provide a type resolver. If user does not provide a type resolver with a custom schema, use a smarter default that can use dynamic message types and then fallback to the global registry for anything unknown (including both extensions and message types).
2. Validate during initialization that the given type resolver can actually resolve all of the methods' request and response types.

There is also a small change in here that is not strictly related to dynamic usage but is instead related to proxying an RPC protocol to REST: if a request comes in via RPC protocol for a method that has no HTTP mapping and the target protocol is REST, this will now return a "404 not found" status code. It previously could result in a nil-dereference panic.

[emcfarlane]

I'm a bit confused on the choice to use the Option. I think this can be done within the register method to fetch the types and error if not available. Would this restrict previous behaviour of using vanguard-go to map rpc protocols (e.g. connectrpc to grpc) without transforming the body?

[emcfarlane]

I think we should instead support a default mapping of POST to <service>/<method>. See https://cloud.google.com/endpoints/docs/grpc/transcoding#where_to_configure_transcoding

[jhump]

Do you just mean to use the Connect protocol in this case? If not, that's not a thing that actually works today (and smells like an enhancement beyond the scope of this PR).

[emcfarlane]

Not Connect but effectively. Think it should be valid to have zero annotations and point towards a REST service, so don't want the handler to error here (not to implement this new behaviour in this PR).

[jhump]

Think it should be valid to have zero annotations and point towards a REST service, so don't want the handler to error here

But it's not actually valid yet. As a service to users, I think we should do as much as possible up-front to fail fast and give good error messages. That removes footguns from users and gives them clear information about why things aren't working.

You can always remove this check later, Once this repo actually supports what you're talking about. Though some sort of check seems useful to keep even then, like in case the service only has streaming endpoints (that don't use HttpBody).

[emcfarlane]

Could it instead of creating the fallback resolver instead create the type from the provided resolver or global registry and then fail if not able?

[emcfarlane]

I think you can use dynamicpb.NewMessageType if the resolver fails. e.g.:

func resolveType(messageDesc protoreflect.MessageDescriptor, resolver TypeResolver) (protoreflect.MessageType, error) {
	dynamicOk := resolver == nil
	if dynamicOk {
		resolver = protoregistry.GlobalTypes
	}
	messageType, err := resolver.FindMessageByName(messageDesc.FullName())
	if err != nil {
		if !errors.Is(err, protoregistry.NotFound) || !dynamicOk {
			return nil, fmt.Errorf("failed to resolve message type %s: %w", messageDesc.FullName(), err)
		}
		return dynamicpb.NewMessageType(messageDesc), nil
	}
	return messageType, nil
}

func (t *Transcoder) registerMethod(...) {
        // ...
	requestType, err := resolveType(methodDesc.Input(), opts.resolver)
	if err != nil {
		return fmt.Errorf("failed to resolve input type for method %s: %w", methodDesc.FullName(), err)
	}
	responseType, err := resolveType(methodDesc.Output(), opts.resolver)
	if err != nil {
		return fmt.Errorf("failed to resolve output type for method %s: %w", methodDesc.FullName(), err)
	}
	// ...
}

[emcfarlane]

lgtm!