ErrorDetail issue with grpcurl

ErrorDetail issue with connect-grpcreflect-go v1.0.0 and grpcurl

May I submit a pull request?

Problem

When using connect-grpcreflect-go v1.0.0, ErrorDetail is not displayed when an error occurs in response to a request from the client.
This problem does not occur when using google.golang.org/grpc v1.51.0.

Difference in behavior

With 'google.golang.org/grpc v1.51.0'

https://github.com/2yanpath/grpc-error-detail-test

% grpcurl -plaintext -d '{"name": ""}' localhost:8081 greet.v1.GreetService.Greet
ERROR:
  Code: InvalidArgument
  Message: name is required
  Details:
  1)	{"@type":"type.googleapis.com/google.rpc.BadRequest","fieldViolations":[{"field":"username","description":"should not empty"}]}

Expected respone

With 'connect-go v1.7.0' and 'connect-grpcreflect-go v1.0.0'

https://github.com/2yanpath/connect-error-detail-test
branch: main

% grpcurl -plaintext -d '{"name": ""}' localhost:8082 greet.v1.GreetService.Greet
ERROR:
  Code: InvalidArgument
  Message: name is required
  Details:
  1)	{"@error":"google.rpc.BadRequest is not recognized; see @value for raw binary message data","@type":"type.googleapis.com/google.rpc.BadRequest","@value":"ChwKCHVzZXJuYW1lEhBzaG91bGQgbm90IGVtcHR5"}

problem: ErrorDetail cannot be displayed.

[After fix] With 'connect-go v1.7.0' and 'connect-grpcreflect-go v1.0.0 based fix branch'

https://github.com/2yanpath/connect-error-detail-test
branch: fix/error-detail

% grpcurl -plaintext -d '{"name": ""}' localhost:8082 greet.v1.GreetService.Greet
ERROR:
  Code: InvalidArgument
  Message: name is required
  Details:
  1)	{"@type":"type.googleapis.com/google.rpc.BadRequest","fieldViolations":[{"field":"username","description":"should not empty"}]}

Code difference

google.golang.org/grpc v1.51.0

serverreflection.go

// allExtensionNumbersForTypeName returns all extension numbers for the given type.
func (s *serverReflectionServer) allExtensionNumbersForTypeName(name string) ([]int32, error) {
	var numbers []int32
	s.extResolver.RangeExtensionsByMessage(protoreflect.FullName(name), func(xt protoreflect.ExtensionType) bool {
		numbers = append(numbers, int32(xt.TypeDescriptor().Number()))
		return true
	})
	sort.Slice(numbers, func(i, j int) bool {
		return numbers[i] < numbers[j]
	})
	if len(numbers) == 0 {
                 //-- NOTICE no error is thrown, so ([]int32{}, nil) is returned
		// maybe return an error if given type name is not known
		if _, err := s.descResolver.FindDescriptorByName(protoreflect.FullName(name)); err != nil {
			return nil, err
		}
	}
	return numbers, nil
}

connect-grpcreflect-go v1.0.0

grpcreflect.go

func (r *Reflector) getAllExtensionNumbersOfType(fqn string) ([]int32, error) {
	nums := []int32{}
	name := protoreflect.FullName(fqn)
	r.extensionResolver.RangeExtensionsByMessage(name, func(ext protoreflect.ExtensionType) bool {
		num := int32(ext.TypeDescriptor().Number())
		nums = append(nums, num)
		return true
	})
	if len(nums) == 0 {
                 //-- NOTICE an error occurs and (nil, [the error below]) is returned
		return nil, fmt.Errorf("no extensions for type %q", fqn)
	}
	sort.Slice(nums, func(i, j int) bool {
		return nums[i] < nums[j]
	})
	return nums, nil
}

[After fix] grpcreflect.go

func (r *Reflector) getAllExtensionNumbersOfType(fqn string) ([]int32, error) {
	nums := []int32{}
	name := protoreflect.FullName(fqn)
	r.extensionResolver.RangeExtensionsByMessage(name, func(ext protoreflect.ExtensionType) bool {
		num := int32(ext.TypeDescriptor().Number())
		nums = append(nums, num)
		return true
	})
	if len(nums) == 0 {
		if _, err := r.descriptorResolver.FindDescriptorByName(name); err != nil {
			return nil, err
		}
	}
	sort.Slice(nums, func(i, j int) bool {
		return nums[i] < nums[j]
	})
	return nums, nil
}

Expose Connect client for the gRPC reflection protocol

Trying to create a client that uses connect-go to communicate with a server that supports gRPC reflection is rather a hassle. Each such usage must use custom code generation (like this repo does) to avoid adding a dependency on google.golang.org/grpc/reflection (which would transitively bring in all of gRPC 😭). Given the name of this repo, it seems within bounds for this repo to provide that.

I know the API would want to avoid exposing the generated base types, for aesthetic reasons, but I don't think that's a very big lift. The only real question IMO is whether to drop it in the main grpcreflect package or to add a grpcreflectclient sub-package.

WDYT?

Update Makefile to be more connect-go-like

Re: ripping out makego. Not a high priority.

connectrpc / grpcreflect-go Goto Github PK

grpcreflect-go's People

Contributors

Stargazers

Watchers

Forkers

grpcreflect-go's Issues