From 755d9c00372dabc4339c4a46194d0b598e9879c0 Mon Sep 17 00:00:00 2001 From: Hiroto Funakoshi Date: Fri, 4 Sep 2020 13:29:51 +0900 Subject: [PATCH] :white_check_mark: Add test case for storage/blob/s3/writer/option (#656) * feat: add test and new validation logic Signed-off-by: hlts2 * fix: const value Signed-off-by: hlts2 * feat: add ignore options for goleak Signed-off-by: hlts2 * fix: apply suggestion Signed-off-by: hlts2 * feat: error for option Signed-off-by: hlts2 * feat: add option error Signed-off-by: hlts2 * fix: guideline Signed-off-by: hlts2 * fix: lint error Signed-off-by: hlts2 * Update docs/contributing/coding-style.md Co-authored-by: Kevin Diu * Update docs/contributing/coding-style.md * Update docs/contributing/coding-style.md * Update docs/contributing/coding-style.md * Update docs/contributing/coding-style.md * feat: add option error type Signed-off-by: hlts2 * Update docs/contributing/coding-style.md * Update docs/contributing/coding-style.md * feat: add test case Signed-off-by: hlts2 * Update internal/db/storage/blob/s3/writer/option_test.go * fix: apply suggestion Signed-off-by: hlts2 * :robot: Update license headers / Format go codes and yaml files Signed-off-by: vdaas-ci Co-authored-by: Kevin Diu Co-authored-by: vdaas-ci --- docs/contributing/coding-style.md | 31 +- internal/db/storage/blob/s3/writer/option.go | 53 +- .../db/storage/blob/s3/writer/option_test.go | 764 ++++++++---------- internal/db/storage/blob/s3/writer/writer.go | 5 +- internal/errors/option.go | 26 + 5 files changed, 441 insertions(+), 438 deletions(-) create mode 100644 internal/errors/option.go diff --git a/docs/contributing/coding-style.md b/docs/contributing/coding-style.md index 13c2895161..9c072dd33e 100644 --- a/docs/contributing/coding-style.md +++ b/docs/contributing/coding-style.md @@ -453,12 +453,20 @@ You can refer to [this section](#Struct-initialization) for more details of the We strongly recommend the following implementation to set the value using functional option. +If an invalid value is set to the functional option, the `ErrInvalidOption` error defined in the [internal/errors/option.go](https://github.com/vdaas/vald/blob/master/internal/errors/option.go) should be returned. + +The name argument (the first argument) of the `ErrInvalidOption` error should be the same as the functional option name without the `With` prefix. + +For example, the functional option name `WithVersion` should return the error with the argument `name` as `version`. + + ```go func WithVersion(version string) Option { return func(c *client) error { - if len(version) != 0 { - c.version = version + if len(version) == 0 { + return errors.ErrInvalidOption("version", version) } + c.version = version return nil } } @@ -470,7 +478,7 @@ We recommend the following implementation to parse the time string and set the t func WithTimeout(dur string) Option { func(c *client) error { if dur == "" { - return nil + return errors.ErrInvalidOption("timeout", dur) } d, err := timeutil.Parse(dur) if err != nil { @@ -488,7 +496,7 @@ We recommend the following implementation to append the value to the slice if th func WithHosts(hosts ...string) Option { return func(c *client) error { if len(hosts) == 0 { - return nil + return errors.ErrInvalidOption("hosts", hosts) } if c.hosts == nil { c.hosts = hosts @@ -500,6 +508,21 @@ func WithHosts(hosts ...string) Option { } ``` +We recommend the following implementation to apply the options. + +If the option failed to apply, an error wrapped with `ErrOptionFailed` defined in the [internal/errors/errors.go](https://github.com/vdaas/vald/blob/master/internal/errors/errors.go) should be returned. + +```go +func New(opts ...Option) (Server, error) { + srv := new(server) + for _, opt := range opts { + if err := opt(srv); err != nil { + return nil, errors.ErrOptionFailed(err, reflect.ValueOf(opt)) + } + } +} +``` + ## Program comments Program comments make easier to understand the source code. We suggest not to write many comments inside the source code unless the source code is very complicated and confusing; otherwise we should divide the source code into methods to keep the readability and usability of the source code. diff --git a/internal/db/storage/blob/s3/writer/option.go b/internal/db/storage/blob/s3/writer/option.go index 48f6f9c18c..a136d06af7 100644 --- a/internal/db/storage/blob/s3/writer/option.go +++ b/internal/db/storage/blob/s3/writer/option.go @@ -18,10 +18,13 @@ package writer import ( "github.com/aws/aws-sdk-go/service/s3" + "github.com/aws/aws-sdk-go/service/s3/s3manager" "github.com/vdaas/vald/internal/errgroup" + "github.com/vdaas/vald/internal/errors" ) -type Option func(w *writer) +// Option represents the functional option for writer. +type Option func(w *writer) error var ( defaultOpts = []Option{ @@ -31,44 +34,68 @@ var ( } ) +// WithErrGroup returns the option to set eg for writer. func WithErrGroup(eg errgroup.Group) Option { - return func(w *writer) { - if eg != nil { - w.eg = eg + return func(w *writer) error { + if eg == nil { + return errors.ErrInvalidOption("errgroup", eg) } + w.eg = eg + return nil } } +// WithService returns the option to set s for writer. func WithService(s *s3.S3) Option { - return func(w *writer) { - if s != nil { - w.service = s + return func(w *writer) error { + if s == nil { + return errors.ErrInvalidOption("service", s) } + w.service = s + return nil } } +// WithBucket returns the option to set bucket for writer. func WithBucket(bucket string) Option { - return func(w *writer) { + return func(w *writer) error { + if len(bucket) == 0 { + return errors.ErrInvalidOption("bucket", bucket) + } w.bucket = bucket + return nil } } +// WithKey returns the option to set key for writer. func WithKey(key string) Option { - return func(w *writer) { + return func(w *writer) error { + if len(key) == 0 { + return errors.ErrInvalidOption("key", key) + } w.key = key + return nil } } +// WithContentType returns the option to set ct for writer. func WithContentType(ct string) Option { - return func(w *writer) { - if ct != "" { - w.contentType = ct + return func(w *writer) error { + if len(ct) == 0 { + return errors.ErrInvalidOption("contentType", ct) } + w.contentType = ct + return nil } } +// WithMaxPartSize returns the option to set max for writer. func WithMaxPartSize(max int64) Option { - return func(w *writer) { + return func(w *writer) error { + if max < s3manager.MinUploadPartSize { + return errors.ErrInvalidOption("maxPartSize", max) + } w.maxPartSize = max + return nil } } diff --git a/internal/db/storage/blob/s3/writer/option_test.go b/internal/db/storage/blob/s3/writer/option_test.go index 5e6d8f9ebc..a8e071340e 100644 --- a/internal/db/storage/blob/s3/writer/option_test.go +++ b/internal/db/storage/blob/s3/writer/option_test.go @@ -17,90 +17,87 @@ package writer import ( + "reflect" "testing" "github.com/aws/aws-sdk-go/service/s3" + "github.com/aws/aws-sdk-go/service/s3/s3manager" "github.com/vdaas/vald/internal/errgroup" + "github.com/vdaas/vald/internal/errors" "go.uber.org/goleak" ) +var ( + goleakIgnoreOptions = []goleak.Option{ + goleak.IgnoreTopFunction("github.com/kpango/fastime.(*Fastime).StartTimerD.func1"), + } +) + func TestWithErrGroup(t *testing.T) { - type T = interface{} + type T = writer type args struct { eg errgroup.Group } + type fields struct { + eg errgroup.Group + } type want struct { obj *T - // Uncomment this line if the option returns an error, otherwise delete it - // err error + err error } type test struct { - name string - args args - want want - // Use the first line if the option returns an error. otherwise use the second line - // checkFunc func(want, *T, error) error - // checkFunc func(want, *T) error + name string + args args + fields fields + want want + checkFunc func(want, *T, error) error beforeFunc func(args) afterFunc func(args) } - // Uncomment this block if the option returns an error, otherwise delete it - /* - defaultCheckFunc := func(w want, obj *T, err error) error { - if !errors.Is(err, w.err) { - return errors.Errorf("got error = %v, want %v", err, w.err) - } - if !reflect.DeepEqual(obj, w.obj) { - return errors.Errorf("got = %v, want %v", obj, w.obj) - } - return nil - } - */ - - // Uncomment this block if the option do not returns an error, otherwise delete it - /* - defaultCheckFunc := func(w want, obj *T) error { - if !reflect.DeepEqual(obj, w.obj) { - return errors.Errorf("got = %v, want %v", obj, w.c) - } - return nil - } - */ + defaultCheckFunc := func(w want, obj *T, err error) error { + if !errors.Is(err, w.err) { + return errors.Errorf("got error = %v, want %v", err, w.err) + } + if !reflect.DeepEqual(obj, w.obj) { + return errors.Errorf("got = %v, want %v", obj, w.obj) + } + return nil + } tests := []test{ - // TODO test cases - /* - { - name: "test_case_1", - args: args { - eg: nil, - }, - want: want { - obj: new(T), - }, - }, - */ - - // TODO test cases - /* - func() test { - return test { - name: "test_case_2", - args: args { - eg: nil, - }, - want: want { - obj: new(T), - }, - } - }(), - */ + { + name: "set success when eg is not nil", + args: args{ + eg: errgroup.Get(), + }, + want: want{ + obj: &T{ + eg: errgroup.Get(), + }, + }, + }, + + { + name: "returns error when eg is nil", + args: args{ + eg: nil, + }, + fields: fields{ + eg: errgroup.Get(), + }, + want: want{ + obj: &T{ + eg: errgroup.Get(), + }, + err: errors.ErrInvalidOption("errgroup", nil), + }, + }, } for _, test := range tests { t.Run(test.name, func(tt *testing.T) { - defer goleak.VerifyNone(t) + defer goleak.VerifyNone(tt, goleakIgnoreOptions...) if test.beforeFunc != nil { test.beforeFunc(test.args) } @@ -108,112 +105,92 @@ func TestWithErrGroup(t *testing.T) { defer test.afterFunc(test.args) } - // Uncomment this block if the option returns an error, otherwise delete it - /* - if test.checkFunc == nil { - test.checkFunc = defaultCheckFunc - } - - got := WithErrGroup(test.args.eg) - obj := new(T) - if err := test.checkFunc(test.want, obj, got(obj)); err != nil { - tt.Errorf("error = %v", err) - } - */ - - // Uncomment this block if the option returns an error, otherwise delete it - /* - if test.checkFunc == nil { - test.checkFunc = defaultCheckFunc - } - got := WithErrGroup(test.args.eg) - obj := new(T) - got(obj) - if err := test.checkFunc(tt.want, obj); err != nil { - tt.Errorf("error = %v", err) - } - */ + if test.checkFunc == nil { + test.checkFunc = defaultCheckFunc + } + got := WithErrGroup(test.args.eg) + obj := &T{ + eg: test.fields.eg, + } + if err := test.checkFunc(test.want, obj, got(obj)); err != nil { + tt.Errorf("error = %v", err) + } }) } } func TestWithService(t *testing.T) { - type T = interface{} + type T = writer type args struct { s *s3.S3 } + type fields struct { + service *s3.S3 + } type want struct { obj *T - // Uncomment this line if the option returns an error, otherwise delete it - // err error + err error } type test struct { - name string - args args - want want - // Use the first line if the option returns an error. otherwise use the second line - // checkFunc func(want, *T, error) error - // checkFunc func(want, *T) error + name string + args args + fields fields + want want + checkFunc func(want, *T, error) error beforeFunc func(args) afterFunc func(args) } - // Uncomment this block if the option returns an error, otherwise delete it - /* - defaultCheckFunc := func(w want, obj *T, err error) error { - if !errors.Is(err, w.err) { - return errors.Errorf("got error = %v, want %v", err, w.err) - } - if !reflect.DeepEqual(obj, w.obj) { - return errors.Errorf("got = %v, want %v", obj, w.obj) - } - return nil - } - */ - - // Uncomment this block if the option do not returns an error, otherwise delete it - /* - defaultCheckFunc := func(w want, obj *T) error { - if !reflect.DeepEqual(obj, w.obj) { - return errors.Errorf("got = %v, want %v", obj, w.c) - } - return nil - } - */ + defaultCheckFunc := func(w want, obj *T, err error) error { + if !errors.Is(err, w.err) { + return errors.Errorf("got error = %v, want %v", err, w.err) + } + if !reflect.DeepEqual(obj, w.obj) { + return errors.Errorf("got = %v, want %v", obj, w.obj) + } + return nil + } tests := []test{ - // TODO test cases - /* - { - name: "test_case_1", - args: args { - s: nil, - }, - want: want { - obj: new(T), - }, - }, - */ - - // TODO test cases - /* - func() test { - return test { - name: "test_case_2", - args: args { - s: nil, - }, - want: want { - obj: new(T), - }, - } - }(), - */ + func() test { + s := new(s3.S3) + return test{ + name: "set success when s is not nil", + args: args{ + s: s, + }, + want: want{ + obj: &T{ + service: s, + }, + }, + } + }(), + + func() test { + s := new(s3.S3) + var ss *s3.S3 + return test{ + name: "returns error when s is nil", + args: args{ + s: ss, + }, + fields: fields{ + service: s, + }, + want: want{ + obj: &T{ + service: s, + }, + err: errors.ErrInvalidOption("service", ss), + }, + } + }(), } for _, test := range tests { t.Run(test.name, func(tt *testing.T) { - defer goleak.VerifyNone(t) + defer goleak.VerifyNone(tt, goleakIgnoreOptions...) if test.beforeFunc != nil { test.beforeFunc(test.args) } @@ -221,112 +198,76 @@ func TestWithService(t *testing.T) { defer test.afterFunc(test.args) } - // Uncomment this block if the option returns an error, otherwise delete it - /* - if test.checkFunc == nil { - test.checkFunc = defaultCheckFunc - } - - got := WithService(test.args.s) - obj := new(T) - if err := test.checkFunc(test.want, obj, got(obj)); err != nil { - tt.Errorf("error = %v", err) - } - */ - - // Uncomment this block if the option returns an error, otherwise delete it - /* - if test.checkFunc == nil { - test.checkFunc = defaultCheckFunc - } - got := WithService(test.args.s) - obj := new(T) - got(obj) - if err := test.checkFunc(tt.want, obj); err != nil { - tt.Errorf("error = %v", err) - } - */ + if test.checkFunc == nil { + test.checkFunc = defaultCheckFunc + } + got := WithService(test.args.s) + obj := &T{ + service: test.fields.service, + } + if err := test.checkFunc(test.want, obj, got(obj)); err != nil { + tt.Errorf("error = %v", err) + } }) } } func TestWithBucket(t *testing.T) { - type T = interface{} + type T = writer type args struct { bucket string } type want struct { obj *T - // Uncomment this line if the option returns an error, otherwise delete it - // err error + err error } type test struct { - name string - args args - want want - // Use the first line if the option returns an error. otherwise use the second line - // checkFunc func(want, *T, error) error - // checkFunc func(want, *T) error + name string + args args + want want + checkFunc func(want, *T, error) error beforeFunc func(args) afterFunc func(args) } - // Uncomment this block if the option returns an error, otherwise delete it - /* - defaultCheckFunc := func(w want, obj *T, err error) error { - if !errors.Is(err, w.err) { - return errors.Errorf("got error = %v, want %v", err, w.err) - } - if !reflect.DeepEqual(obj, w.obj) { - return errors.Errorf("got = %v, want %v", obj, w.obj) - } - return nil - } - */ - - // Uncomment this block if the option do not returns an error, otherwise delete it - /* - defaultCheckFunc := func(w want, obj *T) error { - if !reflect.DeepEqual(obj, w.obj) { - return errors.Errorf("got = %v, want %v", obj, w.c) - } - return nil - } - */ + defaultCheckFunc := func(w want, obj *T, err error) error { + if !errors.Is(err, w.err) { + return errors.Errorf("got error = %v, want %v", err, w.err) + } + if !reflect.DeepEqual(obj, w.obj) { + return errors.Errorf("got = %v, want %v", obj, w.obj) + } + return nil + } tests := []test{ - // TODO test cases - /* - { - name: "test_case_1", - args: args { - bucket: "", - }, - want: want { - obj: new(T), - }, - }, - */ - - // TODO test cases - /* - func() test { - return test { - name: "test_case_2", - args: args { - bucket: "", - }, - want: want { - obj: new(T), - }, - } - }(), - */ + { + name: "set success when bucket is not empty", + args: args{ + bucket: "bucket", + }, + want: want{ + obj: &T{ + bucket: "bucket", + }, + }, + }, + + { + name: "returns error when bucket is empty", + args: args{ + bucket: "", + }, + want: want{ + obj: new(T), + err: errors.ErrInvalidOption("bucket", ""), + }, + }, } for _, test := range tests { t.Run(test.name, func(tt *testing.T) { - defer goleak.VerifyNone(t) + defer goleak.VerifyNone(tt, goleakIgnoreOptions...) if test.beforeFunc != nil { test.beforeFunc(test.args) } @@ -334,112 +275,74 @@ func TestWithBucket(t *testing.T) { defer test.afterFunc(test.args) } - // Uncomment this block if the option returns an error, otherwise delete it - /* - if test.checkFunc == nil { - test.checkFunc = defaultCheckFunc - } - - got := WithBucket(test.args.bucket) - obj := new(T) - if err := test.checkFunc(test.want, obj, got(obj)); err != nil { - tt.Errorf("error = %v", err) - } - */ - - // Uncomment this block if the option returns an error, otherwise delete it - /* - if test.checkFunc == nil { - test.checkFunc = defaultCheckFunc - } - got := WithBucket(test.args.bucket) - obj := new(T) - got(obj) - if err := test.checkFunc(tt.want, obj); err != nil { - tt.Errorf("error = %v", err) - } - */ + if test.checkFunc == nil { + test.checkFunc = defaultCheckFunc + } + got := WithBucket(test.args.bucket) + obj := new(T) + if err := test.checkFunc(test.want, obj, got(obj)); err != nil { + tt.Errorf("error = %v", err) + } }) } } func TestWithKey(t *testing.T) { - type T = interface{} + type T = writer type args struct { key string } type want struct { obj *T - // Uncomment this line if the option returns an error, otherwise delete it - // err error + err error } type test struct { - name string - args args - want want - // Use the first line if the option returns an error. otherwise use the second line - // checkFunc func(want, *T, error) error - // checkFunc func(want, *T) error + name string + args args + want want + checkFunc func(want, *T, error) error beforeFunc func(args) afterFunc func(args) } - // Uncomment this block if the option returns an error, otherwise delete it - /* - defaultCheckFunc := func(w want, obj *T, err error) error { - if !errors.Is(err, w.err) { - return errors.Errorf("got error = %v, want %v", err, w.err) - } - if !reflect.DeepEqual(obj, w.obj) { - return errors.Errorf("got = %v, want %v", obj, w.obj) - } - return nil - } - */ - - // Uncomment this block if the option do not returns an error, otherwise delete it - /* - defaultCheckFunc := func(w want, obj *T) error { - if !reflect.DeepEqual(obj, w.obj) { - return errors.Errorf("got = %v, want %v", obj, w.c) - } - return nil - } - */ + defaultCheckFunc := func(w want, obj *T, err error) error { + if !errors.Is(err, w.err) { + return errors.Errorf("got error = %v, want %v", err, w.err) + } + if !reflect.DeepEqual(obj, w.obj) { + return errors.Errorf("got = %v, want %v", obj, w.obj) + } + return nil + } tests := []test{ - // TODO test cases - /* - { - name: "test_case_1", - args: args { - key: "", - }, - want: want { - obj: new(T), - }, - }, - */ - - // TODO test cases - /* - func() test { - return test { - name: "test_case_2", - args: args { - key: "", - }, - want: want { - obj: new(T), - }, - } - }(), - */ + { + name: "set success when key is not empty", + args: args{ + key: "key", + }, + want: want{ + obj: &T{ + key: "key", + }, + }, + }, + + { + name: "returns error when key is empty", + args: args{ + key: "", + }, + want: want{ + obj: new(T), + err: errors.ErrInvalidOption("key", ""), + }, + }, } for _, test := range tests { t.Run(test.name, func(tt *testing.T) { - defer goleak.VerifyNone(t) + defer goleak.VerifyNone(tt, goleakIgnoreOptions...) if test.beforeFunc != nil { test.beforeFunc(test.args) } @@ -447,112 +350,149 @@ func TestWithKey(t *testing.T) { defer test.afterFunc(test.args) } - // Uncomment this block if the option returns an error, otherwise delete it - /* - if test.checkFunc == nil { - test.checkFunc = defaultCheckFunc - } - - got := WithKey(test.args.key) - obj := new(T) - if err := test.checkFunc(test.want, obj, got(obj)); err != nil { - tt.Errorf("error = %v", err) - } - */ - - // Uncomment this block if the option returns an error, otherwise delete it - /* - if test.checkFunc == nil { - test.checkFunc = defaultCheckFunc - } - got := WithKey(test.args.key) - obj := new(T) - got(obj) - if err := test.checkFunc(tt.want, obj); err != nil { - tt.Errorf("error = %v", err) - } - */ + if test.checkFunc == nil { + test.checkFunc = defaultCheckFunc + } + got := WithKey(test.args.key) + obj := new(T) + if err := test.checkFunc(test.want, obj, got(obj)); err != nil { + tt.Errorf("error = %v", err) + } }) } } func TestWithMaxPartSize(t *testing.T) { - type T = interface{} + type T = writer type args struct { max int64 } type want struct { obj *T - // Uncomment this line if the option returns an error, otherwise delete it - // err error + err error + } + type test struct { + name string + args args + want want + checkFunc func(want, *T, error) error + beforeFunc func(args) + afterFunc func(args) + } + + defaultCheckFunc := func(w want, obj *T, err error) error { + if !errors.Is(err, w.err) { + return errors.Errorf("got error = %v, want %v", err, w.err) + } + if !reflect.DeepEqual(obj, w.obj) { + return errors.Errorf("got = %v, want %v", obj, w.obj) + } + return nil + } + + tests := []test{ + { + name: "set success when max is larger then MinUploadPartSize", + args: args{ + max: s3manager.MinUploadPartSize + 1000, + }, + want: want{ + obj: &T{ + maxPartSize: s3manager.MinUploadPartSize + 1000, + }, + }, + }, + + { + name: "returns error when max is smaller then MinUploadPartSize", + args: args{ + max: 10, + }, + want: want{ + obj: new(T), + err: errors.ErrInvalidOption("maxPartSize", 10), + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(tt *testing.T) { + defer goleak.VerifyNone(tt, goleakIgnoreOptions...) + if test.beforeFunc != nil { + test.beforeFunc(test.args) + } + if test.afterFunc != nil { + defer test.afterFunc(test.args) + } + + if test.checkFunc == nil { + test.checkFunc = defaultCheckFunc + } + got := WithMaxPartSize(test.args.max) + obj := new(T) + if err := test.checkFunc(test.want, obj, got(obj)); err != nil { + tt.Errorf("error = %v", err) + } + }) + } +} + +func TestWithContentType(t *testing.T) { + type T = writer + type args struct { + ct string + } + type want struct { + obj *T + err error } type test struct { - name string - args args - want want - // Use the first line if the option returns an error. otherwise use the second line - // checkFunc func(want, *T, error) error - // checkFunc func(want, *T) error + name string + args args + want want + checkFunc func(want, *T, error) error beforeFunc func(args) afterFunc func(args) } - // Uncomment this block if the option returns an error, otherwise delete it - /* - defaultCheckFunc := func(w want, obj *T, err error) error { - if !errors.Is(err, w.err) { - return errors.Errorf("got error = %v, want %v", err, w.err) - } - if !reflect.DeepEqual(obj, w.obj) { - return errors.Errorf("got = %v, want %v", obj, w.obj) - } - return nil - } - */ - - // Uncomment this block if the option do not returns an error, otherwise delete it - /* - defaultCheckFunc := func(w want, obj *T) error { - if !reflect.DeepEqual(obj, w.obj) { - return errors.Errorf("got = %v, want %v", obj, w.c) - } - return nil - } - */ + defaultCheckFunc := func(w want, obj *T, err error) error { + if !errors.Is(err, w.err) { + return errors.Errorf("got error = %v, want %v", err, w.err) + } + if !reflect.DeepEqual(obj, w.obj) { + return errors.Errorf("got = %v, want %v", obj, w.obj) + } + return nil + } tests := []test{ - // TODO test cases - /* - { - name: "test_case_1", - args: args { - max: 0, - }, - want: want { - obj: new(T), - }, - }, - */ - - // TODO test cases - /* - func() test { - return test { - name: "test_case_2", - args: args { - max: 0, - }, - want: want { - obj: new(T), - }, - } - }(), - */ + { + name: "set success when ct is not empty", + args: args{ + ct: "utf8", + }, + want: want{ + obj: &T{ + contentType: "utf8", + }, + }, + }, + + { + name: "returns error when ct is empty", + args: args{ + ct: "", + }, + want: want{ + obj: new(T), + err: errors.ErrInvalidOption("contentType", ""), + }, + }, } for _, test := range tests { t.Run(test.name, func(tt *testing.T) { - defer goleak.VerifyNone(t) + defer goleak.VerifyNone(tt, goleakIgnoreOptions...) if test.beforeFunc != nil { test.beforeFunc(test.args) } @@ -560,31 +500,15 @@ func TestWithMaxPartSize(t *testing.T) { defer test.afterFunc(test.args) } - // Uncomment this block if the option returns an error, otherwise delete it - /* - if test.checkFunc == nil { - test.checkFunc = defaultCheckFunc - } - - got := WithMaxPartSize(test.args.max) - obj := new(T) - if err := test.checkFunc(test.want, obj, got(obj)); err != nil { - tt.Errorf("error = %v", err) - } - */ - - // Uncomment this block if the option returns an error, otherwise delete it - /* - if test.checkFunc == nil { - test.checkFunc = defaultCheckFunc - } - got := WithMaxPartSize(test.args.max) - obj := new(T) - got(obj) - if err := test.checkFunc(tt.want, obj); err != nil { - tt.Errorf("error = %v", err) - } - */ + if test.checkFunc == nil { + test.checkFunc = defaultCheckFunc + } + + got := WithContentType(test.args.ct) + obj := new(T) + if err := test.checkFunc(test.want, obj, got(obj)); err != nil { + tt.Errorf("error = %v", err) + } }) } } diff --git a/internal/db/storage/blob/s3/writer/writer.go b/internal/db/storage/blob/s3/writer/writer.go index 7d83b99098..805814aeda 100644 --- a/internal/db/storage/blob/s3/writer/writer.go +++ b/internal/db/storage/blob/s3/writer/writer.go @@ -19,6 +19,7 @@ package writer import ( "context" "io" + "reflect" "sync" "github.com/aws/aws-sdk-go/aws" @@ -51,7 +52,9 @@ type Writer interface { func New(opts ...Option) Writer { w := new(writer) for _, opt := range append(defaultOpts, opts...) { - opt(w) + if err := opt(w); err != nil { + log.Warn(errors.ErrOptionFailed(err, reflect.ValueOf(opt))) + } } return w diff --git a/internal/errors/option.go b/internal/errors/option.go new file mode 100644 index 0000000000..bb56cdae16 --- /dev/null +++ b/internal/errors/option.go @@ -0,0 +1,26 @@ +// +// Copyright (C) 2019-2020 Vdaas.org Vald team ( kpango, rinx, kmrmt ) +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +package errors + +var ( + // ErrInvalidOption returns invalid option error. + ErrInvalidOption = func(name string, val interface{}) error { + if val == nil { + return Errorf("invalid option. name: %s, val: nil", name) + } + return Errorf("invalid option. name: %s, val: %#v", name, val) + } +)