From 2b691c5dbd727080082b9e1545e8e9947d8f3ac5 Mon Sep 17 00:00:00 2001 From: Yousong Zhou Date: Tue, 11 Sep 2018 03:52:23 +0000 Subject: [PATCH 1/4] validators: StringMultiChoicesValidator: add test case --- pkg/cloudcommon/validators/validators_test.go | 85 +++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/pkg/cloudcommon/validators/validators_test.go b/pkg/cloudcommon/validators/validators_test.go index 29f110a2bc..5e9f82849b 100644 --- a/pkg/cloudcommon/validators/validators_test.go +++ b/pkg/cloudcommon/validators/validators_test.go @@ -143,6 +143,91 @@ func TestStringChoicesValidator(t *testing.T) { } } +func TestStringMultiChoicesValidator(t *testing.T) { + type MultiChoicesC struct { + *C + KeepDup bool + } + choices := NewChoices("choice0", "choice1") + cases := []*MultiChoicesC{ + { + C: &C{ + Name: "missing non-optional", + In: `{}`, + Out: `{}`, + Optional: false, + Err: ERR_MISSING_KEY, + ValueWant: "", + }, + }, + { + C: &C{ + Name: "missing optional", + In: `{}`, + Out: `{}`, + Optional: true, + ValueWant: "", + }, + }, + { + C: &C{ + Name: "missing with default", + In: `{}`, + Out: `{s: "choice0,choice1"}`, + Default: "choice0,choice1", + ValueWant: "choice0,choice1", + }, + }, + { + C: &C{ + Name: "good choices", + In: `{"s": "choice0,choice1"}`, + Out: `{"s": "choice0,choice1"}`, + ValueWant: "choice0,choice1", + }, + }, + { + C: &C{ + Name: "keep dup", + In: `{"s": "choice0,choice0,choice1,choice0"}`, + Out: `{"s": "choice0,choice0,choice1,choice0"}`, + ValueWant: "choice0,choice0,choice1,choice0", + }, + KeepDup: true, + }, + { + C: &C{ + Name: "strip dup", + In: `{"s": "choice0,choice0,choice1,choice0"}`, + Out: `{"s": "choice0,choice1"}`, + ValueWant: "choice0,choice1", + }, + }, + { + C: &C{ + Name: "invalid choice", + In: `{"s": "choice0,choicex"}`, + Out: `{"s": "choice0,choicex"}`, + Err: ERR_INVALID_CHOICE, + ValueWant: "", + }, + }, + } + for _, c := range cases { + t.Run(c.Name, func(t *testing.T) { + v := NewStringMultiChoicesValidator("s", choices).Sep(",").KeepDup(c.KeepDup) + if c.Default != nil { + s := c.Default.(string) + v.Default(s) + } + if c.Optional { + v.Optional(true) + } + testS(t, v, c.C) + }) + } +} + func TestBoolValidator(t *testing.T) { cases := []*C{ { From cc70be165f06217863fc190ad714c1ec4d3873a8 Mon Sep 17 00:00:00 2001 From: Yousong Zhou Date: Tue, 11 Sep 2018 06:32:57 +0000 Subject: [PATCH 2/4] validators: rework error handling - Return httperrors by default (return ValidateError when testing) - Special casing sql.ErrNoRows for model not found --- pkg/cloudcommon/validators/errors.go | 27 ++++++++++++++++--- pkg/cloudcommon/validators/validators_test.go | 2 ++ 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/pkg/cloudcommon/validators/errors.go b/pkg/cloudcommon/validators/errors.go index 57c7271ece..9045fc93ed 100644 --- a/pkg/cloudcommon/validators/errors.go +++ b/pkg/cloudcommon/validators/errors.go @@ -1,9 +1,14 @@ package validators import ( + "database/sql" "fmt" + + "yunion.io/x/onecloud/pkg/httperrors" ) +var returnHttpError = true + type ErrType uintptr const ( @@ -21,7 +26,7 @@ const ( var errTypeToString = map[ErrType]string{ ERR_SUCCESS: "No error", ERR_GENERAL: "General error", - ERR_MISSING_KEY: "Missing_key error", + ERR_MISSING_KEY: "Missing key error", ERR_INVALID_TYPE: "Invalid type error", ERR_INVALID_CHOICE: "Invalid choice error", ERR_NOT_IN_RANGE: "Not in range error", @@ -85,16 +90,30 @@ func newModelManagerError(modelKeyword string) error { } func newModelNotFoundError(modelKeyword, idOrName string, err error) error { - msg := fmt.Sprintf("cannot find %q with id/name %q: %s", - modelKeyword, idOrName, err) + msg := fmt.Sprintf("cannot find %q with id/name %q", + modelKeyword, idOrName) + if err != sql.ErrNoRows { + msg += ": " + err.Error() + } return newError(ERR_MODEL_NOT_FOUND, msg) } func newError(typ ErrType, msg string) error { - return &ValidateError{ + err := &ValidateError{ ErrType: typ, Msg: msg, } + if returnHttpError { + switch typ { + case ERR_SUCCESS: + return nil + case ERR_GENERAL, ERR_MODEL_MANAGER: + return httperrors.NewInternalServerError(msg) + default: + return httperrors.NewInputParameterError(msg) + } + } + return err } func IsModelNotFoundError(err error) bool { diff --git a/pkg/cloudcommon/validators/validators_test.go b/pkg/cloudcommon/validators/validators_test.go index 5e9f82849b..508e4ea2e4 100644 --- a/pkg/cloudcommon/validators/validators_test.go +++ b/pkg/cloudcommon/validators/validators_test.go @@ -47,6 +47,8 @@ type C struct { } func testS(t *testing.T, v IValidator, c *C) { + returnHttpError = false + j, _ := jsonutils.ParseString(c.In) jd := j.(*jsonutils.JSONDict) err := v.Validate(jd) From d98e3b325f9e8c3f262acd0f0109d89ea331767d Mon Sep 17 00:00:00 2001 From: Yousong Zhou Date: Tue, 11 Sep 2018 08:43:59 +0000 Subject: [PATCH 3/4] mcclient: options: allow listing all resources including pending_deleted --- pkg/mcclient/options/base.go | 45 +++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/pkg/mcclient/options/base.go b/pkg/mcclient/options/base.go index 1062e83346..5480be1395 100644 --- a/pkg/mcclient/options/base.go +++ b/pkg/mcclient/options/base.go @@ -142,23 +142,24 @@ func ListStructToParams(v interface{}) (*jsonutils.JSONDict, error) { } type BaseListOptions struct { - Limit *int `default:"20" help:"Page limit"` - Offset *int `default:"0" help:"Page offset"` - OrderBy []string `help:"Name of the field to be ordered by"` - Order string `help:"List order" choices:"desc|asc"` - Details *bool `help:"Show more details" default:"false"` - Search string `help:"Filter results by a simple keyword search"` - Meta *bool `help:"Piggyback metadata information"` - Filter []string `help:"Filters"` - JointFilter []string `help:"Filters with joint table col; joint_tbl.related_key(origin_key).filter_col.filter_cond(filters)"` - FilterAny *bool `help:"If true, match if any of the filters matches; otherwise, match if all of the filters match"` - Admin *bool `help:"Is an admin call?"` - Tenant string `help:"Tenant ID or Name"` - User string `help:"User ID or Name"` - System *bool `help:"Show system resource"` - PendingDelete *bool `help:"Show pending deleted resource"` - Field []string `help:"Show only specified fields"` - ShowEmulated *bool `help:"Show all resources including the emulated resources"` + Limit *int `default:"20" help:"Page limit"` + Offset *int `default:"0" help:"Page offset"` + OrderBy []string `help:"Name of the field to be ordered by"` + Order string `help:"List order" choices:"desc|asc"` + Details *bool `help:"Show more details" default:"false"` + Search string `help:"Filter results by a simple keyword search"` + Meta *bool `help:"Piggyback metadata information"` + Filter []string `help:"Filters"` + JointFilter []string `help:"Filters with joint table col; joint_tbl.related_key(origin_key).filter_col.filter_cond(filters)"` + FilterAny *bool `help:"If true, match if any of the filters matches; otherwise, match if all of the filters match"` + Admin *bool `help:"Is an admin call?"` + Tenant string `help:"Tenant ID or Name"` + User string `help:"User ID or Name"` + System *bool `help:"Show system resource"` + PendingDelete *bool `help:"Show only pending deleted resource"` + PendingDeleteAll *bool `help:"Show all resources including pending deleted" json:"-"` + Field []string `help:"Show only specified fields"` + ShowEmulated *bool `help:"Show all resources including the emulated resources"` } func (opts *BaseListOptions) Params() (*jsonutils.JSONDict, error) { @@ -169,10 +170,16 @@ func (opts *BaseListOptions) Params() (*jsonutils.JSONDict, error) { if len(opts.Filter) == 0 { params.Remove("filter_any") } + if BoolV(opts.PendingDeleteAll) { + params.Set("pending_delete", jsonutils.NewString("all")) + } if opts.Admin == nil { - requiresSystem := len(opts.Tenant) > 0 || BoolV(opts.System) || BoolV(opts.PendingDelete) + requiresSystem := len(opts.Tenant) > 0 || + BoolV(opts.System) || + BoolV(opts.PendingDelete) || + BoolV(opts.PendingDeleteAll) if requiresSystem { - params.Set("admin", jsonutils.NewBool(true)) + params.Set("admin", jsonutils.JSONTrue) } } return params, nil From af0603d6db82f884552644b3b78f02c8e6ffae56 Mon Sep 17 00:00:00 2001 From: Yousong Zhou Date: Tue, 11 Sep 2018 12:55:49 +0000 Subject: [PATCH 4/4] mcclient: options: accomodate time.Time type --- pkg/mcclient/options/base.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pkg/mcclient/options/base.go b/pkg/mcclient/options/base.go index 5480be1395..eafefcceec 100644 --- a/pkg/mcclient/options/base.go +++ b/pkg/mcclient/options/base.go @@ -3,6 +3,7 @@ package options import ( "fmt" "reflect" + "time" "yunion.io/x/jsonutils" "yunion.io/x/pkg/gotypes" @@ -88,6 +89,11 @@ func optionsStructRvToParams(rv reflect.Value) (*jsonutils.JSONDict, error) { if ft.Anonymous { continue } + if f.Type() == gotypes.TimeType { + t := f.Interface().(time.Time) + p.Set(name, jsonutils.NewTimeString(t)) + continue + } // TODO msg := fmt.Sprintf("do not know what to do with non-anonymous struct field: %s", ft.Name) panic(msg)