Apply feedback requested in code review

Signed-off-by: Israel Blancas <iblancasa@gmail.com>

Author: iblancasa

experimental/experimental.go

@@ -63,10 +63,11 @@ func BufferPool(bufferPool mem.BufferPool) grpc.ServerOption {
 	return internal.BufferPool.(func(mem.BufferPool) grpc.ServerOption)(bufferPool)
 }
 
-// AcceptedCompressionNames returns a CallOption that limits the values
+// AcceptCompressors returns a CallOption that limits the values
 // advertised in the grpc-accept-encoding header for the provided RPC. The
 // supplied names must correspond to compressors registered via
-// encoding.RegisterCompressor. Passing no names advertises identity only.
-func AcceptedCompressionNames(names ...string) grpc.CallOption {
-	return internal.AcceptedCompressionNames.(func(...string) grpc.CallOption)(names...)
+// encoding.RegisterCompressor. Passing no names advertises "identity" (no
+// compression) only.
+func AcceptCompressors(names ...string) grpc.CallOption {
+	return internal.AcceptCompressors.(func(...string) grpc.CallOption)(names...)
 }

internal/experimental.go

@@ -26,7 +26,7 @@ var (
 	// option to configure a shared buffer pool for a grpc.Server.
 	BufferPool any // func (grpc.SharedBufferPool) grpc.ServerOption
 
-	// AcceptedCompressionNames is implemented by the grpc package and returns
+	// AcceptCompressors is implemented by the grpc package and returns
 	// a call option that restricts the grpc-accept-encoding header for a call.
-	AcceptedCompressionNames any // func(...string) grpc.CallOption
+	AcceptCompressors any // func(...string) grpc.CallOption
 )

rpc_util.go

@@ -44,7 +44,7 @@ import (
 )
 
 func init() {
-	internal.AcceptedCompressionNames = acceptedCompressionNames
+	internal.AcceptCompressors = AcceptCompressors
 }
 
 // Compressor defines the interface gRPC uses to compress a message.
@@ -167,23 +167,22 @@ type callInfo struct {
 	maxRetryRPCBufferSize       int
 	onFinish                    []func(err error)
 	authority                   string
-	acceptedResponseCompressors *acceptedCompressionConfig
+	acceptedResponseCompressors []string
 }
 
-type acceptedCompressionConfig struct {
-	headerValue string
-	allowed     map[string]struct{}
-}
-
-func (cfg *acceptedCompressionConfig) allows(name string) bool {
-	if cfg == nil {
+func acceptedCompressorAllows(allowed []string, name string) bool {
+	if allowed == nil {
 		return true
 	}
 	if name == "" || name == encoding.Identity {
 		return true
 	}
-	_, ok := cfg.allowed[name]
-	return ok
+	for _, a := range allowed {
+		if a == name {
+			return true
+		}
+	}
+	return false
 }
 
 func defaultCallInfo() *callInfo {
@@ -193,14 +192,12 @@ func defaultCallInfo() *callInfo {
 	}
 }
 
-func newAcceptedCompressionConfig(names []string) (*acceptedCompressionConfig, error) {
-	cfg := &acceptedCompressionConfig{
-		allowed: make(map[string]struct{}, len(names)),
-	}
+func newAcceptedCompressionConfig(names []string) ([]string, error) {
 	if len(names) == 0 {
-		return cfg, nil
+		return nil, nil
 	}
-	var ordered []string
+	var allowed []string
+	seen := make(map[string]struct{}, len(names))
 	for _, name := range names {
 		name = strings.TrimSpace(name)
 		if name == "" || name == encoding.Identity {
@@ -209,17 +206,13 @@ func newAcceptedCompressionConfig(names []string) (*acceptedCompressionConfig, e
 		if !grpcutil.IsCompressorNameRegistered(name) {
 			return nil, status.Errorf(codes.InvalidArgument, "grpc: compressor %q is not registered", name)
 		}
-		if _, dup := cfg.allowed[name]; dup {
+		if _, dup := seen[name]; dup {
 			continue
 		}
-		cfg.allowed[name] = struct{}{}
-		ordered = append(ordered, name)
+		seen[name] = struct{}{}
+		allowed = append(allowed, name)
 	}
-	if len(ordered) == 0 {
-		return nil, status.Error(codes.InvalidArgument, "grpc: no valid compressor names provided")
-	}
-	cfg.headerValue = strings.Join(ordered, ",")
-	return cfg, nil
+	return allowed, nil
 }
 
 // CallOption configures a Call before it starts or extracts information from
@@ -523,25 +516,25 @@ func (o CompressorCallOption) before(c *callInfo) error {
 }
 func (o CompressorCallOption) after(*callInfo, *csAttempt) {}
 
-func acceptedCompressionNames(names ...string) CallOption {
+func AcceptCompressors(names ...string) CallOption {
 	cp := append([]string(nil), names...)
-	return acceptedCompressionNamesCallOption{names: cp}
+	return AcceptCompressorsCallOption{names: cp}
 }
 
-type acceptedCompressionNamesCallOption struct {
+type AcceptCompressorsCallOption struct {
 	names []string
 }
 
-func (o acceptedCompressionNamesCallOption) before(c *callInfo) error {
-	cfg, err := newAcceptedCompressionConfig(o.names)
+func (o AcceptCompressorsCallOption) before(c *callInfo) error {
+	allowed, err := newAcceptedCompressionConfig(o.names)
 	if err != nil {
 		return err
 	}
-	c.acceptedResponseCompressors = cfg
+	c.acceptedResponseCompressors = allowed
 	return nil
 }
 
-func (acceptedCompressionNamesCallOption) after(*callInfo, *csAttempt) {}
+func (AcceptCompressorsCallOption) after(*callInfo, *csAttempt) {}
 
 // CallContentSubtype returns a CallOption that will set the content-subtype
 // for a call. For example, if content-subtype is "json", the Content-Type over
@@ -893,7 +886,7 @@ func outPayload(client bool, msg any, dataLength, payloadLength int, t time.Time
 	}
 }
 
-func checkRecvPayload(pf payloadFormat, recvCompress string, haveCompressor bool, isServer bool, acceptedCfg *acceptedCompressionConfig) *status.Status {
+func checkRecvPayload(pf payloadFormat, recvCompress string, haveCompressor bool, isServer bool) *status.Status {
 	switch pf {
 	case compressionNone:
 	case compressionMade:
@@ -906,9 +899,6 @@ func checkRecvPayload(pf payloadFormat, recvCompress string, haveCompressor bool
 			}
 			return status.Newf(codes.Internal, "grpc: Decompressor is not installed for grpc-encoding %q", recvCompress)
 		}
-		if !isServer && acceptedCfg != nil && !acceptedCfg.allows(recvCompress) {
-			return status.Newf(codes.FailedPrecondition, "grpc: peer compressed the response with %q which is not allowed by AcceptedCompressionNames", recvCompress)
-		}
 	default:
 		return status.Newf(codes.Internal, "grpc: received unexpected payload format %d", pf)
 	}
@@ -932,16 +922,15 @@ func (p *payloadInfo) free() {
 // the buffer is no longer needed.
 // TODO: Refactor this function to reduce the number of arguments.
 // See: https://google.github.io/styleguide/go/best-practices.html#function-argument-lists
-func recvAndDecompress(p *parser, s recvCompressor, dc Decompressor, maxReceiveMessageSize int, payInfo *payloadInfo, compressor encoding.Compressor, isServer bool, acceptedCfg *acceptedCompressionConfig,
-) (out mem.BufferSlice, err error) {
+func recvAndDecompress(p *parser, s recvCompressor, dc Decompressor, maxReceiveMessageSize int, payInfo *payloadInfo, compressor encoding.Compressor, isServer bool) (out mem.BufferSlice, err error) {
 	pf, compressed, err := p.recvMsg(maxReceiveMessageSize)
 	if err != nil {
 		return nil, err
 	}
 
 	compressedLength := compressed.Len()
 
-	if st := checkRecvPayload(pf, s.RecvCompress(), compressor != nil || dc != nil, isServer, acceptedCfg); st != nil {
+	if st := checkRecvPayload(pf, s.RecvCompress(), compressor != nil || dc != nil, isServer); st != nil {
 		compressed.Free()
 		return nil, st.Err()
 	}
@@ -1016,8 +1005,8 @@ type recvCompressor interface {
 // For the two compressor parameters, both should not be set, but if they are,
 // dc takes precedence over compressor.
 // TODO(dfawley): wrap the old compressor/decompressor using the new API?
-func recv(p *parser, c baseCodec, s recvCompressor, dc Decompressor, m any, maxReceiveMessageSize int, payInfo *payloadInfo, compressor encoding.Compressor, isServer bool, acceptedCfg *acceptedCompressionConfig) error {
-	data, err := recvAndDecompress(p, s, dc, maxReceiveMessageSize, payInfo, compressor, isServer, acceptedCfg)
+func recv(p *parser, c baseCodec, s recvCompressor, dc Decompressor, m any, maxReceiveMessageSize int, payInfo *payloadInfo, compressor encoding.Compressor, isServer bool) error {
+	data, err := recvAndDecompress(p, s, dc, maxReceiveMessageSize, payInfo, compressor, isServer)
 	if err != nil {
 		return err
 	}

rpc_util_test.go

@@ -48,85 +48,118 @@ const (
 	decompressionErrorMsg   = "invalid compression format"
 )
 
+type testCompressorForRegistry struct {
+	name string
+}
+
+func (c *testCompressorForRegistry) Compress(w io.Writer) (io.WriteCloser, error) {
+	return &testWriteCloser{w}, nil
+}
+
+func (c *testCompressorForRegistry) Decompress(r io.Reader) (io.Reader, error) {
+	return r, nil
+}
+
+func (c *testCompressorForRegistry) Name() string {
+	return c.name
+}
+
+type testWriteCloser struct {
+	io.Writer
+}
+
+func (w *testWriteCloser) Close() error {
+	return nil
+}
+
 func (s) TestNewAcceptedCompressionConfig(t *testing.T) {
+	// Register a test compressor for multi-compressor tests
+	testCompressor := &testCompressorForRegistry{name: "test-compressor"}
+	encoding.RegisterCompressor(testCompressor)
+	defer func() {
+		// Unregister the test compressor
+		encoding.RegisterCompressor(&testCompressorForRegistry{name: "test-compressor"})
+	}()
+
 	tests := []struct {
 		name        string
 		input       []string
-		wantHeader  string
-		wantAllowed map[string]struct{}
+		wantAllowed []string
 		wantErr     bool
 	}{
 		{
 			name:        "identity-only",
 			input:       nil,
-			wantHeader:  "",
-			wantAllowed: map[string]struct{}{},
+			wantAllowed: nil,
 		},
 		{
 			name:        "single valid",
 			input:       []string{"gzip"},
-			wantHeader:  "gzip",
-			wantAllowed: map[string]struct{}{"gzip": {}},
+			wantAllowed: []string{"gzip"},
 		},
 		{
 			name:        "dedupe and trim",
 			input:       []string{" gzip ", "gzip"},
-			wantHeader:  "gzip",
-			wantAllowed: map[string]struct{}{"gzip": {}},
+			wantAllowed: []string{"gzip"},
 		},
 		{
 			name:        "ignores identity",
 			input:       []string{"identity", "gzip"},
-			wantHeader:  "gzip",
-			wantAllowed: map[string]struct{}{"gzip": {}},
+			wantAllowed: []string{"gzip"},
+		},
+		{
+			name:        "explicit identity only",
+			input:       []string{"identity"},
+			wantAllowed: nil,
 		},
 		{
 			name:    "invalid compressor",
 			input:   []string{"does-not-exist"},
 			wantErr: true,
 		},
 		{
-			name:    "only whitespace",
-			input:   []string{"   ", "\t"},
+			name:        "only whitespace",
+			input:       []string{"   ", "\t"},
+			wantAllowed: nil,
+		},
+		{
+			name:        "multiple valid compressors",
+			input:       []string{"gzip", "test-compressor"},
+			wantAllowed: []string{"gzip", "test-compressor"},
+		},
+		{
+			name:        "multiple with identity and whitespace",
+			input:       []string{"gzip", "identity", " test-compressor ", "  "},
+			wantAllowed: []string{"gzip", "test-compressor"},
+		},
+		{
+			name:        "empty string in list",
+			input:       []string{"gzip", "", "test-compressor"},
+			wantAllowed: []string{"gzip", "test-compressor"},
+		},
+		{
+			name:    "mixed valid and invalid",
+			input:   []string{"gzip", "invalid-comp"},
 			wantErr: true,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			cfg, err := newAcceptedCompressionConfig(tt.input)
+			allowed, err := newAcceptedCompressionConfig(tt.input)
 			if (err != nil) != tt.wantErr {
 				t.Fatalf("newAcceptedCompressionConfig(%v) error = %v, wantErr %v", tt.input, err, tt.wantErr)
 			}
 			if tt.wantErr {
 				return
 			}
-			if cfg.headerValue != tt.wantHeader {
-				t.Fatalf("headerValue = %q, want %q", cfg.headerValue, tt.wantHeader)
-			}
-			if diff := cmp.Diff(tt.wantAllowed, cfg.allowed); diff != "" {
+			if diff := cmp.Diff(tt.wantAllowed, allowed); diff != "" {
 				t.Fatalf("allowed diff (-want +got): %v", diff)
 			}
 		})
 	}
 }
 
-func (s) TestCheckRecvPayloadHonorsAcceptedCompressors(t *testing.T) {
-	cfg, err := newAcceptedCompressionConfig([]string{"gzip"})
-	if err != nil {
-		t.Fatalf("newAcceptedCompressionConfig returned error: %v", err)
-	}
-
-	if st := checkRecvPayload(compressionMade, "gzip", true, false, cfg); st != nil {
-		t.Fatalf("checkRecvPayload returned error for allowed compressor: %v", st)
-	}
-
-	st := checkRecvPayload(compressionMade, "snappy", true, false, cfg)
-	if st == nil || st.Code() != codes.FailedPrecondition {
-		t.Fatalf("checkRecvPayload = %v, want code %v", st, codes.FailedPrecondition)
-	}
-}
-
 type fullReader struct {
 	data []byte
 }

server.go

@@ -1381,7 +1381,7 @@ func (s *Server) processUnaryRPC(ctx context.Context, stream *transport.ServerSt
 		defer payInfo.free()
 	}
 
-	d, err := recvAndDecompress(&parser{r: stream, bufferPool: s.opts.bufferPool}, stream, dc, s.opts.maxReceiveMessageSize, payInfo, decomp, true, nil)
+	d, err := recvAndDecompress(&parser{r: stream, bufferPool: s.opts.bufferPool}, stream, dc, s.opts.maxReceiveMessageSize, payInfo, decomp, true)
 	if err != nil {
 		if e := stream.WriteStatus(status.Convert(err)); e != nil {
 			channelz.Warningf(logger, s.channelz, "grpc: Server.processUnaryRPC failed to write status: %v", e)