From 28d11b840f803fea16559634bfb8593ede2bcd8c Mon Sep 17 00:00:00 2001 From: xdrm-brackets Date: Thu, 21 Nov 2019 20:42:05 +0100 Subject: [PATCH 1/9] test internal/cerr package + add WrapString() to use a raw string --- internal/cerr/cerr.go | 8 ++++++ internal/cerr/cerr_test.go | 57 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) create mode 100644 internal/cerr/cerr_test.go diff --git a/internal/cerr/cerr.go b/internal/cerr/cerr.go index 57f4431..19a8e6d 100644 --- a/internal/cerr/cerr.go +++ b/internal/cerr/cerr.go @@ -16,6 +16,14 @@ func (err Error) Wrap(e error) *WrapError { } } +// WrapString returns a new error which wraps a new error created from a string. +func (err Error) WrapString(e string) *WrapError { + return &WrapError{ + base: err, + wrap: Error(e), + } +} + // WrapError is way to wrap errors recursively. type WrapError struct { base error diff --git a/internal/cerr/cerr_test.go b/internal/cerr/cerr_test.go new file mode 100644 index 0000000..476996c --- /dev/null +++ b/internal/cerr/cerr_test.go @@ -0,0 +1,57 @@ +package cerr + +import ( + "errors" + "fmt" + "testing" +) + +func TestConstError(t *testing.T) { + const cerr1 = Error("some-string") + const cerr2 = Error("some-other-string") + const cerr3 = Error("some-string") // same const value as @cerr1 + + if cerr1.Error() == cerr2.Error() { + t.Errorf("cerr1 should not be equal to cerr2 ('%s', '%s')", cerr1.Error(), cerr2.Error()) + } + if cerr2.Error() == cerr3.Error() { + t.Errorf("cerr2 should not be equal to cerr3 ('%s', '%s')", cerr2.Error(), cerr3.Error()) + } + if cerr1.Error() != cerr3.Error() { + t.Errorf("cerr1 should be equal to cerr3 ('%s', '%s')", cerr1.Error(), cerr3.Error()) + } +} + +func TestWrappedConstError(t *testing.T) { + const parent = Error("file error") + + const readErrorConst = Error("cannot read file") + var wrappedReadError = parent.Wrap(readErrorConst) + + expectedWrappedReadError := fmt.Sprintf("%s: %s", parent.Error(), readErrorConst.Error()) + if wrappedReadError.Error() != expectedWrappedReadError { + t.Errorf("expected '%s' (got '%s')", wrappedReadError.Error(), expectedWrappedReadError) + } +} +func TestWrappedStandardError(t *testing.T) { + const parent = Error("file error") + + var writeErrorStandard error = errors.New("cannot write file") + var wrappedWriteError = parent.Wrap(writeErrorStandard) + + expectedWrappedWriteError := fmt.Sprintf("%s: %s", parent.Error(), writeErrorStandard.Error()) + if wrappedWriteError.Error() != expectedWrappedWriteError { + t.Errorf("expected '%s' (got '%s')", wrappedWriteError.Error(), expectedWrappedWriteError) + } +} +func TestWrappedStringError(t *testing.T) { + const parent = Error("file error") + + var closeErrorString string = "cannot close file" + var wrappedCloseError = parent.WrapString(closeErrorString) + + expectedWrappedCloseError := fmt.Sprintf("%s: %s", parent.Error(), closeErrorString) + if wrappedCloseError.Error() != expectedWrappedCloseError { + t.Errorf("expected '%s' (got '%s')", wrappedCloseError.Error(), expectedWrappedCloseError) + } +} From 19e203c36407a53a819967d1883b3db541a88a3e Mon Sep 17 00:00:00 2001 From: xdrm-brackets Date: Thu, 21 Nov 2019 21:02:48 +0100 Subject: [PATCH 2/9] refactor internal/config - create cerr (constant errors) with wrapped context (service name, method, parameter name) - fix comments numbering - remove duplicate check --- internal/config/errors.go | 27 +++++++++++++++++++++++++++ internal/config/method.go | 36 +++++++++++++++--------------------- internal/config/service.go | 19 +++++-------------- 3 files changed, 47 insertions(+), 35 deletions(-) create mode 100644 internal/config/errors.go diff --git a/internal/config/errors.go b/internal/config/errors.go new file mode 100644 index 0000000..ef413e8 --- /dev/null +++ b/internal/config/errors.go @@ -0,0 +1,27 @@ +package config + +import "git.xdrm.io/go/aicra/internal/cerr" + +// ErrRead - a problem ocurred when trying to read the configuration file +const ErrRead = cerr.Error("cannot read config") + +// ErrFormat - a invalid format has been detected +const ErrFormat = cerr.Error("invalid config format") + +// ErrIllegalServiceName - an illegal character has been found in a service name +const ErrIllegalServiceName = cerr.Error("service must not contain any slash '/' nor '-' symbols") + +// ErrMissingMethodDesc - a method is missing its description +const ErrMissingMethodDesc = cerr.Error("missing method description") + +// ErrMissingParamDesc - a parameter is missing its description +const ErrMissingParamDesc = cerr.Error("missing parameter description") + +// ErrIllegalParamName - a parameter has an illegal name +const ErrIllegalParamName = cerr.Error("illegal parameter name (must not begin/end with '_')") + +// ErrMissingParamType - a parameter has an illegal type +const ErrMissingParamType = cerr.Error("missing parameter type") + +// ErrParamNameConflict - a parameter has a conflict with its name/rename field +const ErrParamNameConflict = cerr.Error("name conflict for parameter") diff --git a/internal/config/method.go b/internal/config/method.go index f570b84..1cb2a02 100644 --- a/internal/config/method.go +++ b/internal/config/method.go @@ -1,7 +1,6 @@ package config import ( - "fmt" "strings" ) @@ -10,7 +9,7 @@ func (methodDef *Method) checkAndFormat(servicePath string, httpMethod string) e // 1. fail on missing description if len(methodDef.Description) < 1 { - return fmt.Errorf("missing %s.%s description", servicePath, httpMethod) + return ErrMissingMethodDesc.WrapString(httpMethod + " " + servicePath) } // 2. stop if no parameter @@ -22,16 +21,16 @@ func (methodDef *Method) checkAndFormat(servicePath string, httpMethod string) e // 3. for each parameter for pName, pData := range methodDef.Parameters { - // check name + // 3.1. check name if strings.Trim(pName, "_") != pName { - return fmt.Errorf("invalid name '%s' must not begin/end with '_'", pName) + return ErrIllegalParamName.WrapString(httpMethod + " " + servicePath + " {" + pName + "}") } if len(pData.Rename) < 1 { pData.Rename = pName } - // 5. Check for name/rename conflict + // 3.2. Check for name/rename conflict for paramName, param := range methodDef.Parameters { // ignore self @@ -39,39 +38,34 @@ func (methodDef *Method) checkAndFormat(servicePath string, httpMethod string) e continue } - // 1. Same rename field + // 3.2.1. Same rename field if pData.Rename == param.Rename { - return fmt.Errorf("rename conflict for %s.%s parameter '%s'", servicePath, httpMethod, pData.Rename) + return ErrParamNameConflict.WrapString(httpMethod + " " + servicePath + " {" + pData.Rename + "}") } - // 2. Not-renamed field matches a renamed field + // 3.2.2. Not-renamed field matches a renamed field if pName == param.Rename { - return fmt.Errorf("name conflict for %s.%s parameter '%s'", servicePath, httpMethod, pName) + return ErrParamNameConflict.WrapString(httpMethod + " " + servicePath + " {" + pName + "}") } - // 3. Renamed field matches name + // 3.2.3. Renamed field matches name if pData.Rename == paramName { - return fmt.Errorf("name conflict for %s.%s parameter '%s'", servicePath, httpMethod, pName) + return ErrParamNameConflict.WrapString(httpMethod + " " + servicePath + " {" + pName + "}") } } - // 6. Manage invalid type - if len(pData.Type) < 1 { - return fmt.Errorf("invalid type for %s.%s parameter '%s'", servicePath, httpMethod, pName) - } - - // 7. Fail on missing description + // 3.3. Fail on missing description if len(pData.Description) < 1 { - return fmt.Errorf("missing description for %s.%s parameter '%s'", servicePath, httpMethod, pName) + return ErrMissingParamDesc.WrapString(httpMethod + " " + servicePath + " {" + pName + "}") } - // 8. Fail on missing type + // 3.4. Manage invalid type if len(pData.Type) < 1 { - return fmt.Errorf("missing type for %s.%s parameter '%s'", servicePath, httpMethod, pName) + return ErrMissingParamType.WrapString(httpMethod + " " + servicePath + " {" + pName + "}") } - // 9. Set optional + type + // 3.5. Set optional + type if pData.Type[0] == '?' { pData.Optional = true pData.Type = pData.Type[1:] diff --git a/internal/config/service.go b/internal/config/service.go index cf223fe..1c97701 100644 --- a/internal/config/service.go +++ b/internal/config/service.go @@ -2,20 +2,11 @@ package config import ( "encoding/json" - "fmt" "io" "net/http" "strings" - - "git.xdrm.io/go/aicra/internal/cerr" ) -// ErrRead - a problem ocurred when trying to read the configuration file -const ErrRead = cerr.Error("cannot read config") - -// ErrFormat - a invalid format has been detected -const ErrFormat = cerr.Error("invalid config format") - // Parse builds a service from a json reader and checks for most format errors. func Parse(r io.Reader) (*Service, error) { receiver := &Service{} @@ -87,20 +78,20 @@ func (svc *Service) checkAndFormat(servicePath string) error { } } - // 1. stop if no child */ + // 2. stop if no child */ if svc.Children == nil || len(svc.Children) < 1 { return nil } - // 2. for each service */ + // 3. for each service */ for childService, ctl := range svc.Children { - // 3. invalid name */ + // 3.1. invalid name */ if strings.ContainsAny(childService, "/-") { - return fmt.Errorf("service '%s' must not contain any slash '/' nor '-' symbols", childService) + return ErrIllegalServiceName.WrapString(childService) } - // 4. check recursively */ + // 3.2. check recursively */ err := ctl.checkAndFormat(childService) if err != nil { return err From 52b42e479ac96244c902dd31fad3c3d3eff8b250 Mon Sep 17 00:00:00 2001 From: xdrm-brackets Date: Thu, 21 Nov 2019 21:34:25 +0100 Subject: [PATCH 3/9] parameter name conflict: on rename conflicting rename, return original name --- internal/config/method.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/config/method.go b/internal/config/method.go index 1cb2a02..5131fdc 100644 --- a/internal/config/method.go +++ b/internal/config/method.go @@ -40,7 +40,7 @@ func (methodDef *Method) checkAndFormat(servicePath string, httpMethod string) e // 3.2.1. Same rename field if pData.Rename == param.Rename { - return ErrParamNameConflict.WrapString(httpMethod + " " + servicePath + " {" + pData.Rename + "}") + return ErrParamNameConflict.WrapString(httpMethod + " " + servicePath + " {" + pName + "}") } // 3.2.2. Not-renamed field matches a renamed field From aada9edff515739da23b4960c7b2026a96ed8c91 Mon Sep 17 00:00:00 2001 From: xdrm-brackets Date: Thu, 21 Nov 2019 21:48:24 +0100 Subject: [PATCH 4/9] test internal/config parsing and illegal/missing fields --- internal/config/config_test.go | 388 +++++++++++++++++++++++++++++++++ 1 file changed, 388 insertions(+) create mode 100644 internal/config/config_test.go diff --git a/internal/config/config_test.go b/internal/config/config_test.go new file mode 100644 index 0000000..fc0f209 --- /dev/null +++ b/internal/config/config_test.go @@ -0,0 +1,388 @@ +package config + +import ( + "fmt" + "net/http" + "strings" + "testing" +) + +func TestLegalServiceName(t *testing.T) { + tests := []struct { + Raw string + Error error + }{ + { + `{ + "/": { + "invalid/service-name": { + + } + } + }`, + ErrFormat.Wrap(ErrIllegalServiceName.WrapString("invalid/service-name")), + }, + { + `{ + "/": { + "invalid/service/name": { + + } + } + }`, + ErrFormat.Wrap(ErrIllegalServiceName.WrapString("invalid/service/name")), + }, + { + `{ + "/": { + "invalid-service-name": { + + } + } + }`, + ErrFormat.Wrap(ErrIllegalServiceName.WrapString("invalid-service-name")), + }, + + { + `{ + "/": { + "valid.service_name": { + } + } + }`, + nil, + }, + } + + for i, test := range tests { + + t.Run(fmt.Sprintf("service.%d", i), func(t *testing.T) { + _, err := Parse(strings.NewReader(test.Raw)) + + if err == nil && test.Error != nil { + t.Errorf("expected an error: '%s'", test.Error.Error()) + t.FailNow() + } + if err != nil && test.Error == nil { + t.Errorf("unexpected error: '%s'", err.Error()) + t.FailNow() + } + + if err != nil && test.Error != nil { + if err.Error() != test.Error.Error() { + t.Errorf("expected the error '%s' (got '%s')", test.Error.Error(), err.Error()) + t.FailNow() + } + } + }) + } +} +func TestAvailableMethods(t *testing.T) { + reader := strings.NewReader(`{ + "GET": { "info": "info" }, + "POST": { "info": "info" }, + "PUT": { "info": "info" }, + "DELETE": { "info": "info" } + }`) + srv, err := Parse(reader) + if err != nil { + t.Errorf("unexpected error (got '%s')", err) + t.FailNow() + } + + if srv.Method(http.MethodGet) == nil { + t.Errorf("expected method GET to be available") + t.Fail() + } + if srv.Method(http.MethodPost) == nil { + t.Errorf("expected method POST to be available") + t.Fail() + } + if srv.Method(http.MethodPut) == nil { + t.Errorf("expected method PUT to be available") + t.Fail() + } + if srv.Method(http.MethodDelete) == nil { + t.Errorf("expected method DELETE to be available") + t.Fail() + } + + if srv.Method(http.MethodPatch) != nil { + t.Errorf("expected method PATH to be UNavailable") + t.Fail() + } +} +func TestParseEmpty(t *testing.T) { + reader := strings.NewReader(`{}`) + _, err := Parse(reader) + if err != nil { + t.Errorf("unexpected error (got '%s')", err) + t.FailNow() + } +} +func TestParseJsonError(t *testing.T) { + reader := strings.NewReader(`{ + "GET": { + "info": "info + }, + }`) // trailing ',' is invalid JSON + _, err := Parse(reader) + if err == nil { + t.Errorf("expected error") + t.FailNow() + } +} + +func TestParseMissingMethodDescription(t *testing.T) { + tests := []struct { + Raw string + Error error + }{ + { // missing description + `{ + "GET": { + + } + }`, + ErrFormat.Wrap(ErrMissingMethodDesc.WrapString("GET /")), + }, + { // empty description + `{ + "GET": { + "info": "" + } + }`, + ErrFormat.Wrap(ErrMissingMethodDesc.WrapString("GET /")), + }, + { // valid description + `{ + "GET": { + "info": "a" + } + }`, + nil, + }, + { // valid description + `{ + "GET": { + "info": "some description" + } + }`, + nil, + }, + } + + for i, test := range tests { + + t.Run(fmt.Sprintf("method.%d", i), func(t *testing.T) { + _, err := Parse(strings.NewReader(test.Raw)) + + if err == nil && test.Error != nil { + t.Errorf("expected an error: '%s'", test.Error.Error()) + t.FailNow() + } + if err != nil && test.Error == nil { + t.Errorf("unexpected error: '%s'", err.Error()) + t.FailNow() + } + + if err != nil && test.Error != nil { + if err.Error() != test.Error.Error() { + t.Errorf("expected the error '%s' (got '%s')", test.Error.Error(), err.Error()) + t.FailNow() + } + } + }) + } + +} + +func TestParseParameters(t *testing.T) { + tests := []struct { + Raw string + Error error + ErrorAlternative error + }{ + { // invalid param name prefix + `{ + "GET": { + "info": "info", + "in": { + "_param1": {} + } + } + }`, + ErrFormat.Wrap(ErrIllegalParamName.WrapString("GET / {_param1}")), + nil, + }, + { // invalid param name suffix + `{ + "GET": { + "info": "info", + "in": { + "param1_": {} + } + } + }`, + ErrFormat.Wrap(ErrIllegalParamName.WrapString("GET / {param1_}")), + nil, + }, + + { // missing param description + `{ + "GET": { + "info": "info", + "in": { + "param1": {} + } + } + }`, + ErrFormat.Wrap(ErrMissingParamDesc.WrapString("GET / {param1}")), + nil, + }, + { // empty param description + `{ + "GET": { + "info": "info", + "in": { + "param1": { + "info": "" + } + } + } + }`, + ErrFormat.Wrap(ErrMissingParamDesc.WrapString("GET / {param1}")), + nil, + }, + + { // missing param type + `{ + "GET": { + "info": "info", + "in": { + "param1": { + "info": "valid" + } + } + } + }`, + ErrFormat.Wrap(ErrMissingParamType.WrapString("GET / {param1}")), + nil, + }, + { // empty param type + `{ + "GET": { + "info": "info", + "in": { + "param1": { + "info": "valid", + "type": "" + } + } + } + }`, + ErrFormat.Wrap(ErrMissingParamType.WrapString("GET / {param1}")), + nil, + }, + { // valid description + valid type + `{ + "GET": { + "info": "info", + "in": { + "param1": { + "info": "valid", + "type": "valid" + } + } + } + }`, + nil, + nil, + }, + + { // name conflict with rename + `{ + "GET": { + "info": "info", + "in": { + "param1": { "info": "valid", "type": "valid" }, + "param2": { "info": "valid", "type": "valid", "name": "param1" } + + } + } + }`, + // 2 possible errors as map order is not deterministic + ErrFormat.Wrap(ErrParamNameConflict.WrapString("GET / {param1}")), + ErrFormat.Wrap(ErrParamNameConflict.WrapString("GET / {param2}")), + }, + { // rename conflict with name + `{ + "GET": { + "info": "info", + "in": { + "param1": { "info": "valid", "type": "valid", "name": "param2" }, + "param2": { "info": "valid", "type": "valid" } + + } + } + }`, + // 2 possible errors as map order is not deterministic + ErrFormat.Wrap(ErrParamNameConflict.WrapString("GET / {param1}")), + ErrFormat.Wrap(ErrParamNameConflict.WrapString("GET / {param2}")), + }, + { // rename conflict with rename + `{ + "GET": { + "info": "info", + "in": { + "param1": { "info": "valid", "type": "valid", "name": "conflict" }, + "param2": { "info": "valid", "type": "valid", "name": "conflict" } + + } + } + }`, + // 2 possible errors as map order is not deterministic + ErrFormat.Wrap(ErrParamNameConflict.WrapString("GET / {param1}")), + ErrFormat.Wrap(ErrParamNameConflict.WrapString("GET / {param2}")), + }, + + { // both renamed with no conflict + `{ + "GET": { + "info": "info", + "in": { + "param1": { "info": "valid", "type": "valid", "name": "freename" }, + "param2": { "info": "valid", "type": "valid", "name": "freename2" } + + } + } + }`, + nil, + nil, + }, + } + + for i, test := range tests { + + t.Run(fmt.Sprintf("method.%d", i), func(t *testing.T) { + _, err := Parse(strings.NewReader(test.Raw)) + + if err == nil && test.Error != nil { + t.Errorf("expected an error: '%s'", test.Error.Error()) + t.FailNow() + } + if err != nil && test.Error == nil { + t.Errorf("unexpected error: '%s'", err.Error()) + t.FailNow() + } + + if err != nil && test.Error != nil { + if err.Error() != test.Error.Error() && err.Error() != test.ErrorAlternative.Error() { + t.Errorf("expected the error '%s' (got '%s')", test.Error.Error(), err.Error()) + t.FailNow() + } + } + }) + } + +} From 8cebf52405c7baf9524eafbd2238464aa080062c Mon Sep 17 00:00:00 2001 From: xdrm-brackets Date: Thu, 21 Nov 2019 22:00:48 +0100 Subject: [PATCH 5/9] add internal/config test for the Browse() method --- internal/config/config_test.go | 134 +++++++++++++++++++++++++++++++++ 1 file changed, 134 insertions(+) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index fc0f209..08e183c 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -386,3 +386,137 @@ func TestParseParameters(t *testing.T) { } } + +func TestBrowseSimple(t *testing.T) { + tests := []struct { + Raw string + Path []string + BrowseDepth int + ValidDepth bool + }{ + { // false positive -1 + `{ + "/" : { + "parent": { + "/": { + "subdir": {} + } + } + } + }`, + []string{"parent", "subdir"}, + 1, + false, + }, + { // false positive +1 + `{ + "/" : { + "parent": { + "/": { + "subdir": {} + } + } + } + }`, + []string{"parent", "subdir"}, + 3, + false, + }, + + { + `{ + "/" : { + "parent": { + "/": { + "subdir": {} + } + } + } + }`, + []string{"parent", "subdir"}, + 2, + true, + }, + { // unknown path + `{ + "/" : { + "parent": { + "/": { + "subdir": {} + } + } + } + }`, + []string{"x", "y"}, + 2, + false, + }, + { // unknown path + `{ + "/" : { + "parent": { + "/": { + "subdir": {} + } + } + } + }`, + []string{"parent", "y"}, + 1, + true, + }, + { // Warning: this case is important to understand the precedence of service paths over + // the value of some variables. Here if we send a string parameter in the GET method that + // unfortunately is equal to 'subdir', it will call the sub-service /parent/subdir' instead + // of the service /parent with its parameter set to the value 'subdir'. + `{ + "/" : { + "parent": { + "/": { + "subdir": {} + }, + "GET": { + "info": "valid-desc", + "in": { + "some-value": { + "info": "valid-desc", + "type": "valid-type" + } + } + } + } + } + }`, + []string{"parent", "subdir"}, + 2, + true, + }, + } + + for i, test := range tests { + + t.Run(fmt.Sprintf("method.%d", i), func(t *testing.T) { + srv, err := Parse(strings.NewReader(test.Raw)) + + if err != nil { + t.Errorf("unexpected error: '%s'", err) + t.FailNow() + } + + _, depth := srv.Browse(test.Path) + if test.ValidDepth { + if depth != test.BrowseDepth { + t.Errorf("expected a depth of %d (got %d)", test.BrowseDepth, depth) + t.FailNow() + } + } else { + if depth == test.BrowseDepth { + t.Errorf("expected a depth NOT %d (got %d)", test.BrowseDepth, depth) + t.FailNow() + } + + } + }) + } + +} From 6218327fd2b131f3ae8c71c6c32aee81ba4c06d6 Mon Sep 17 00:00:00 2001 From: xdrm-brackets Date: Thu, 21 Nov 2019 22:18:00 +0100 Subject: [PATCH 6/9] expand internal/config test coverage - make parameter type "?" invalid as it marks it as optional only - check optional vs. required parameters - test subservice in method description error check --- internal/config/config_test.go | 85 +++++++++++++++++++++++++++++++++- internal/config/method.go | 2 +- 2 files changed, 85 insertions(+), 2 deletions(-) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 08e183c..3d0c19c 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -146,6 +146,18 @@ func TestParseMissingMethodDescription(t *testing.T) { }`, ErrFormat.Wrap(ErrMissingMethodDesc.WrapString("GET /")), }, + { // missing description + `{ + "/": { + "subservice": { + "GET": { + + } + } + } + }`, + ErrFormat.Wrap(ErrMissingMethodDesc.WrapString("GET subservice")), + }, { // empty description `{ "GET": { @@ -197,6 +209,46 @@ func TestParseMissingMethodDescription(t *testing.T) { } +func TestOptionalParam(t *testing.T) { + reader := strings.NewReader(`{ + "GET": { + "info": "info", + "in": { + "optional": { "info": "valid-desc", "type": "?optional-type" }, + "required": { "info": "valid-desc", "type": "required-type" }, + "required2": { "info": "valid-desc", "type": "a" }, + "optional2": { "info": "valid-desc", "type": "?a" } + } + } + }`) + srv, err := Parse(reader) + if err != nil { + t.Errorf("unexpected error: '%s'", err) + t.FailNow() + } + + method := srv.Method(http.MethodGet) + if method == nil { + t.Errorf("expected GET method not to be nil") + t.FailNow() + } + for pName, param := range method.Parameters { + + if pName == "optional" || pName == "optional2" { + if !param.Optional { + t.Errorf("expected parameter '%s' to be optional", pName) + t.Failed() + } + } + if pName == "required" || pName == "required2" { + if param.Optional { + t.Errorf("expected parameter '%s' to be required", pName) + t.Failed() + } + } + } + +} func TestParseParameters(t *testing.T) { tests := []struct { Raw string @@ -284,6 +336,22 @@ func TestParseParameters(t *testing.T) { ErrFormat.Wrap(ErrMissingParamType.WrapString("GET / {param1}")), nil, }, + { // invalid type (optional mark only) + `{ + "GET": { + "info": "info", + "in": { + "param1": { + "info": "valid", + "type": "?" + } + } + } + }`, + + ErrFormat.Wrap(ErrMissingParamType.WrapString("GET / {param1}")), + ErrFormat.Wrap(ErrMissingParamType.WrapString("GET / {param1}")), + }, { // valid description + valid type `{ "GET": { @@ -291,7 +359,22 @@ func TestParseParameters(t *testing.T) { "in": { "param1": { "info": "valid", - "type": "valid" + "type": "a" + } + } + } + }`, + nil, + nil, + }, + { // valid description + valid OPTIONAL type + `{ + "GET": { + "info": "info", + "in": { + "param1": { + "info": "valid", + "type": "?valid" } } } diff --git a/internal/config/method.go b/internal/config/method.go index 5131fdc..a17df24 100644 --- a/internal/config/method.go +++ b/internal/config/method.go @@ -61,7 +61,7 @@ func (methodDef *Method) checkAndFormat(servicePath string, httpMethod string) e } // 3.4. Manage invalid type - if len(pData.Type) < 1 { + if len(pData.Type) < 1 || pData.Type == "?" { return ErrMissingParamType.WrapString(httpMethod + " " + servicePath + " {" + pName + "}") } From b18ea9849708b705dd69e20b207b3f046c818b99 Mon Sep 17 00:00:00 2001 From: xdrm-brackets Date: Thu, 21 Nov 2019 22:20:12 +0100 Subject: [PATCH 7/9] remove dead code --- internal/config/method.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/internal/config/method.go b/internal/config/method.go index a17df24..802766d 100644 --- a/internal/config/method.go +++ b/internal/config/method.go @@ -75,13 +75,3 @@ func (methodDef *Method) checkAndFormat(servicePath string, httpMethod string) e return nil } - -// scopeHasPermission returns whether the permission fulfills a given scope -func scopeHasPermission(permission string, scope []string) bool { - for _, s := range scope { - if permission == s { - return true - } - } - return false -} From 8ba58b4748a64ff78411ecb5f823de96117d7cb2 Mon Sep 17 00:00:00 2001 From: xdrm-brackets Date: Thu, 21 Nov 2019 22:27:57 +0100 Subject: [PATCH 8/9] test internal/config empty parameter rename should not rename --- internal/config/config_test.go | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 3d0c19c..466cec2 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -209,6 +209,35 @@ func TestParseMissingMethodDescription(t *testing.T) { } +func TestParamEmptyRenameNoRename(t *testing.T) { + reader := strings.NewReader(`{ + "GET": { + "info": "info", + "in": { + "original": { "info": "valid-desc", "type": "valid-type", "name": "" } + } + } + }`) + srv, err := Parse(reader) + if err != nil { + t.Errorf("unexpected error: '%s'", err) + t.FailNow() + } + + method := srv.Method(http.MethodGet) + if method == nil { + t.Errorf("expected GET method not to be nil") + t.FailNow() + } + for _, param := range method.Parameters { + + if param.Rename != "original" { + t.Errorf("expected the parameter 'original' not to be renamed to '%s'", param.Rename) + t.FailNow() + } + } + +} func TestOptionalParam(t *testing.T) { reader := strings.NewReader(`{ "GET": { From 4221f8cf2cee96d27b322885fb5f5f5dc6749883 Mon Sep 17 00:00:00 2001 From: xdrm-brackets Date: Thu, 21 Nov 2019 22:29:58 +0100 Subject: [PATCH 9/9] test internal/config trick to have a 100% coverage (as conflict check is undeterministic, merge all conditions) --- internal/config/config_test.go | 4 +++- internal/config/method.go | 10 +--------- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 466cec2..deb3170 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -490,7 +490,9 @@ func TestParseParameters(t *testing.T) { if err != nil && test.Error != nil { if err.Error() != test.Error.Error() && err.Error() != test.ErrorAlternative.Error() { - t.Errorf("expected the error '%s' (got '%s')", test.Error.Error(), err.Error()) + t.Errorf("got the error: '%s'", err.Error()) + t.Errorf("expected error (alternative 1): '%s'", test.Error.Error()) + t.Errorf("expected error (alternative 2): '%s'", test.ErrorAlternative.Error()) t.FailNow() } } diff --git a/internal/config/method.go b/internal/config/method.go index 802766d..14c3539 100644 --- a/internal/config/method.go +++ b/internal/config/method.go @@ -39,17 +39,9 @@ func (methodDef *Method) checkAndFormat(servicePath string, httpMethod string) e } // 3.2.1. Same rename field - if pData.Rename == param.Rename { - return ErrParamNameConflict.WrapString(httpMethod + " " + servicePath + " {" + pName + "}") - } - // 3.2.2. Not-renamed field matches a renamed field - if pName == param.Rename { - return ErrParamNameConflict.WrapString(httpMethod + " " + servicePath + " {" + pName + "}") - } - // 3.2.3. Renamed field matches name - if pData.Rename == paramName { + if pData.Rename == param.Rename || pName == param.Rename || pData.Rename == paramName { return ErrParamNameConflict.WrapString(httpMethod + " " + servicePath + " {" + pName + "}") }