From 5f7141f25c64cc9654e75a1b2cd601ea3c5a2e42 Mon Sep 17 00:00:00 2001 From: Josh Humphries <2035234+jhump@users.noreply.github.com> Date: Thu, 6 Apr 2023 10:57:33 -0400 Subject: [PATCH 1/2] fix WithCompression to match docs; add tests --- compression.go | 3 ++ compression_test.go | 107 ++++++++++++++++++++++++++++++++++++++++++++ option.go | 5 +-- 3 files changed, 111 insertions(+), 4 deletions(-) diff --git a/compression.go b/compression.go index 56241f4e..3f3a8812 100644 --- a/compression.go +++ b/compression.go @@ -65,6 +65,9 @@ func newCompressionPool( newDecompressor func() Decompressor, newCompressor func() Compressor, ) *compressionPool { + if newDecompressor == nil && newCompressor == nil { + return nil + } return &compressionPool{ decompressors: sync.Pool{ New: func() any { return newDecompressor() }, diff --git a/compression_test.go b/compression_test.go index d815d165..3db142a4 100644 --- a/compression_test.go +++ b/compression_test.go @@ -54,3 +54,110 @@ func TestAcceptEncodingOrdering(t *testing.T) { _, _ = client.CallUnary(context.Background(), NewRequest(&emptypb.Empty{})) assert.True(t, called) } + +func TestClientCompressionOptionTest(t *testing.T) { + t.Parallel() + const testURL = "http://foo.bar.com/service/method" + + checkPools := func(t *testing.T, config *clientConfig) { + t.Helper() + assert.Equal(t, len(config.CompressionNames), len(config.CompressionPools)) + for _, name := range config.CompressionNames { + pool := config.CompressionPools[name] + assert.NotNil(t, pool) + } + } + dummyDecompressCtor := func() Decompressor { return nil } + dummyCompressCtor := func() Compressor { return nil } + + t.Run("defaults", func(t *testing.T) { + t.Parallel() + config, err := newClientConfig(testURL, nil) + assert.Nil(t, err) + assert.Equal(t, config.CompressionNames, []string{compressionGzip}) + checkPools(t, config) + }) + t.Run("WithAcceptCompression", func(t *testing.T) { + t.Parallel() + opts := []ClientOption{WithAcceptCompression("foo", dummyDecompressCtor, dummyCompressCtor)} + config, err := newClientConfig(testURL, opts) + assert.Nil(t, err) + assert.Equal(t, config.CompressionNames, []string{compressionGzip, "foo"}) + checkPools(t, config) + }) + t.Run("WithAcceptCompression-empty-name-noop", func(t *testing.T) { + t.Parallel() + opts := []ClientOption{WithAcceptCompression("", dummyDecompressCtor, dummyCompressCtor)} + config, err := newClientConfig(testURL, opts) + assert.Nil(t, err) + assert.Equal(t, config.CompressionNames, []string{compressionGzip}) + checkPools(t, config) + }) + t.Run("WithAcceptCompression-nil-ctors-noop", func(t *testing.T) { + t.Parallel() + opts := []ClientOption{WithAcceptCompression("foo", nil, nil)} + config, err := newClientConfig(testURL, opts) + assert.Nil(t, err) + assert.Equal(t, config.CompressionNames, []string{compressionGzip}) + checkPools(t, config) + }) + t.Run("WithAcceptCompression-nil-ctors-unregisters", func(t *testing.T) { + t.Parallel() + opts := []ClientOption{WithAcceptCompression("gzip", nil, nil)} + config, err := newClientConfig(testURL, opts) + assert.Nil(t, err) + assert.Equal(t, config.CompressionNames, nil) + checkPools(t, config) + }) +} + +func TestHandlerCompressionOptionTest(t *testing.T) { + t.Parallel() + const testProc = "/service/method" + + checkPools := func(t *testing.T, config *handlerConfig) { + t.Helper() + assert.Equal(t, len(config.CompressionNames), len(config.CompressionPools)) + for _, name := range config.CompressionNames { + pool := config.CompressionPools[name] + assert.NotNil(t, pool) + } + } + dummyDecompressCtor := func() Decompressor { return nil } + dummyCompressCtor := func() Compressor { return nil } + + t.Run("defaults", func(t *testing.T) { + t.Parallel() + config := newHandlerConfig(testProc, nil) + assert.Equal(t, config.CompressionNames, []string{compressionGzip}) + checkPools(t, config) + }) + t.Run("WithCompression", func(t *testing.T) { + t.Parallel() + opts := []HandlerOption{WithCompression("foo", dummyDecompressCtor, dummyCompressCtor)} + config := newHandlerConfig(testProc, opts) + assert.Equal(t, config.CompressionNames, []string{compressionGzip, "foo"}) + checkPools(t, config) + }) + t.Run("WithCompression-empty-name-noop", func(t *testing.T) { + t.Parallel() + opts := []HandlerOption{WithCompression("", dummyDecompressCtor, dummyCompressCtor)} + config := newHandlerConfig(testProc, opts) + assert.Equal(t, config.CompressionNames, []string{compressionGzip}) + checkPools(t, config) + }) + t.Run("WithCompression-nil-ctors-noop", func(t *testing.T) { + t.Parallel() + opts := []HandlerOption{WithCompression("foo", nil, nil)} + config := newHandlerConfig(testProc, opts) + assert.Equal(t, config.CompressionNames, []string{compressionGzip}) + checkPools(t, config) + }) + t.Run("WithCompression-nil-ctors-does-not-unregister", func(t *testing.T) { + t.Parallel() + opts := []HandlerOption{WithCompression("gzip", nil, nil)} + config := newHandlerConfig(testProc, opts) + assert.Equal(t, config.CompressionNames, []string{compressionGzip}) + checkPools(t, config) + }) +} diff --git a/option.go b/option.go index a997b71a..ed17209e 100644 --- a/option.go +++ b/option.go @@ -48,9 +48,6 @@ func WithAcceptCompression( newDecompressor func() Decompressor, newCompressor func() Compressor, ) ClientOption { - if newDecompressor == nil && newCompressor == nil { - return &compressionOption{Name: name} - } return &compressionOption{ Name: name, CompressionPool: newCompressionPool(newDecompressor, newCompressor), @@ -93,7 +90,7 @@ func WithSendCompression(name string) ClientOption { // WithSendGzip configures the client to gzip requests. Since clients have // access to a gzip compressor by default, WithSendGzip doesn't require -// [WithSendCompresion]. +// [WithSendCompression]. // // Some servers don't support gzip, so clients default to sending uncompressed // requests. From d5ecb751878dc193584208858a648cca4bff259f Mon Sep 17 00:00:00 2001 From: Josh Humphries <2035234+jhump@users.noreply.github.com> Date: Tue, 11 Apr 2023 09:40:44 -0400 Subject: [PATCH 2/2] make WithCompression and WithAcceptCompression behave the same way with respect to nil constructors --- compression_test.go | 4 ++-- option.go | 34 +++++++++++++++++++--------------- 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/compression_test.go b/compression_test.go index 3db142a4..e6dcfa5c 100644 --- a/compression_test.go +++ b/compression_test.go @@ -153,11 +153,11 @@ func TestHandlerCompressionOptionTest(t *testing.T) { assert.Equal(t, config.CompressionNames, []string{compressionGzip}) checkPools(t, config) }) - t.Run("WithCompression-nil-ctors-does-not-unregister", func(t *testing.T) { + t.Run("WithCompression-nil-ctors-unregisters", func(t *testing.T) { t.Parallel() opts := []HandlerOption{WithCompression("gzip", nil, nil)} config := newHandlerConfig(testProc, opts) - assert.Equal(t, config.CompressionNames, []string{compressionGzip}) + assert.Equal(t, config.CompressionNames, nil) checkPools(t, config) }) } diff --git a/option.go b/option.go index ed17209e..181a7303 100644 --- a/option.go +++ b/option.go @@ -43,6 +43,8 @@ type ClientOption interface { // Clients accept gzipped responses by default, using a compressor backed by the // standard library's [gzip] package with the default compression level. Use // [WithSendGzip] to compress requests with gzip. +// +// Calling WithAcceptCompression with an empty name is a no-op. func WithAcceptCompression( name string, newDecompressor func() Decompressor, @@ -113,9 +115,11 @@ type HandlerOption interface { // compressors and decompressors. // // By default, handlers support gzip using the standard library's -// [compress/gzip] package at the default compression level. +// [compress/gzip] package at the default compression level. To remove support for +// a previously-registered compression algorithm, use WithCompression with nil +// decompressor and compressor constructors. // -// Calling WithCompression with an empty name or nil constructors is a no-op. +// Calling WithCompression with an empty name is a no-op. func WithCompression( name string, newDecompressor func() Decompressor, @@ -317,31 +321,31 @@ type compressionOption struct { } func (o *compressionOption) applyToClient(config *clientConfig) { + o.apply(&config.CompressionNames, config.CompressionPools) +} + +func (o *compressionOption) applyToHandler(config *handlerConfig) { + o.apply(&config.CompressionNames, config.CompressionPools) +} + +func (o *compressionOption) apply(configuredNames *[]string, configuredPools map[string]*compressionPool) { if o.Name == "" { return } if o.CompressionPool == nil { - delete(config.CompressionPools, o.Name) + delete(configuredPools, o.Name) var names []string - for _, name := range config.CompressionNames { + for _, name := range *configuredNames { if name == o.Name { continue } names = append(names, name) } - config.CompressionNames = names - return - } - config.CompressionPools[o.Name] = o.CompressionPool - config.CompressionNames = append(config.CompressionNames, o.Name) -} - -func (o *compressionOption) applyToHandler(config *handlerConfig) { - if o.Name == "" || o.CompressionPool == nil { + *configuredNames = names return } - config.CompressionPools[o.Name] = o.CompressionPool - config.CompressionNames = append(config.CompressionNames, o.Name) + configuredPools[o.Name] = o.CompressionPool + *configuredNames = append(*configuredNames, o.Name) } type compressMinBytesOption struct {