From 30862195a10bde7c35867f2686b4fec7039c4221 Mon Sep 17 00:00:00 2001 From: xdrm-brackets Date: Sat, 4 Apr 2020 15:39:00 +0200 Subject: [PATCH] config: refactor, simplify, test, remove redundant comments --- internal/config/config.go | 184 +++++++++++++++++---------------- internal/config/config_test.go | 107 +++++++++++++++++++ internal/config/errors.go | 48 ++++----- internal/config/parameter.go | 15 +-- internal/config/service.go | 17 ++- 5 files changed, 236 insertions(+), 135 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 259d70e..bfda9df 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -16,17 +16,18 @@ type Server struct { Services []*Service } -// Parse a reader into a server. Server.Types must be set beforehand to +// Parse a configuration into a server. Server.Types must be set beforehand to // make datatypes available when checking and formatting the read configuration. func (srv *Server) Parse(r io.Reader) error { - if err := json.NewDecoder(r).Decode(&srv.Services); err != nil { + err := json.NewDecoder(r).Decode(&srv.Services) + if err != nil { return fmt.Errorf("%s: %w", errRead, err) } - if err := srv.validate(); err != nil { + err = srv.validate() + if err != nil { return fmt.Errorf("%s: %w", errFormat, err) } - return nil } @@ -39,11 +40,9 @@ func (server Server) validate(datatypes ...datatype.T) error { } } - // check for collisions if err := server.collide(); err != nil { return fmt.Errorf("%s: %w", errFormat, err) } - return nil } @@ -58,7 +57,11 @@ func (server Server) Find(r *http.Request) *Service { return nil } -// collide returns if there is collision between services +// collide returns if there is collision between any service for the same method and colliding paths. +// Note that service path collision detection relies on datatypes: +// - example 1: `/user/{id}` and `/user/articles` will not collide as {id} is an int and "articles" is not +// - example 2: `/user/{name}` and `/user/articles` will collide as {name} is a string so as "articles" +// - example 3: `/user/{name}` and `/user/{id}` will collide as {name} and {id} cannot be checked against their potential values func (server *Server) collide() error { length := len(server.Services) @@ -68,103 +71,104 @@ func (server *Server) collide() error { aService := server.Services[a] bService := server.Services[b] - // ignore different method if aService.Method != bService.Method { continue } - aParts := SplitURL(aService.Pattern) - bParts := SplitURL(bService.Pattern) - - // not same size - if len(aParts) != len(bParts) { + aURIParts := SplitURL(aService.Pattern) + bURIParts := SplitURL(bService.Pattern) + if len(aURIParts) != len(bURIParts) { continue } - partErrors := make([]error, 0) - - // for each part - for pi, aPart := range aParts { - bPart := bParts[pi] - - aIsCapture := len(aPart) > 1 && aPart[0] == '{' - bIsCapture := len(bPart) > 1 && bPart[0] == '{' - - // both captures -> as we cannot check, consider a collision - if aIsCapture && bIsCapture { - partErrors = append(partErrors, fmt.Errorf("(%s '%s') vs (%s '%s'): %w (path %s and %s)", aService.Method, aService.Pattern, bService.Method, bService.Pattern, errPatternCollision, aPart, bPart)) - continue - } - - // no capture -> check equal - if !aIsCapture && !bIsCapture { - if aPart == bPart { - partErrors = append(partErrors, fmt.Errorf("(%s '%s') vs (%s '%s'): %w (same path '%s')", aService.Method, aService.Pattern, bService.Method, bService.Pattern, errPatternCollision, aPart)) - continue - } - } - - // A captures B -> check type (B is A ?) - if aIsCapture { - input, exists := aService.Input[aPart] - - // fail if no type or no validator - if !exists || input.Validator == nil { - partErrors = append(partErrors, fmt.Errorf("(%s '%s') vs (%s '%s'): %w (invalid type for %s)", aService.Method, aService.Pattern, bService.Method, bService.Pattern, errPatternCollision, aPart)) - continue - } - - // fail if not valid - if _, valid := input.Validator(bPart); valid { - partErrors = append(partErrors, fmt.Errorf("(%s '%s') vs (%s '%s'): %w (%s captures '%s')", aService.Method, aService.Pattern, bService.Method, bService.Pattern, errPatternCollision, aPart, bPart)) - continue - } - - // B captures A -> check type (A is B ?) - } else if bIsCapture { - input, exists := bService.Input[bPart] - - // fail if no type or no validator - if !exists || input.Validator == nil { - partErrors = append(partErrors, fmt.Errorf("(%s '%s') vs (%s '%s'): %w (invalid type for %s)", aService.Method, aService.Pattern, bService.Method, bService.Pattern, errPatternCollision, bPart)) - continue - } - - // fail if not valid - if _, valid := input.Validator(aPart); valid { - partErrors = append(partErrors, fmt.Errorf("(%s '%s') vs (%s '%s'): %w (%s captures '%s')", aService.Method, aService.Pattern, bService.Method, bService.Pattern, errPatternCollision, bPart, aPart)) - continue - } - } - - partErrors = append(partErrors, nil) - + err := checkURICollision(aURIParts, bURIParts, aService.Input, bService.Input) + if err != nil { + return fmt.Errorf("(%s '%s') vs (%s '%s'): %w", aService.Method, aService.Pattern, bService.Method, bService.Pattern, err) } - - // if at least 1 url part does not match -> ok - var firstError error - oneMismatch := false - for _, err := range partErrors { - if err != nil && firstError == nil { - firstError = err - } - - if err == nil { - oneMismatch = true - continue - } - } - - if !oneMismatch { - return firstError - } - } } return nil } +// check if uri of services A and B collide +func checkURICollision(uriA, uriB []string, inputA, inputB map[string]*Parameter) error { + var errors = []error{} + + // for each part + for pi, aPart := range uriA { + bPart := uriB[pi] + + // no need for further check as it has been done earlier in the validation process + aIsCapture := len(aPart) > 1 && aPart[0] == '{' + bIsCapture := len(bPart) > 1 && bPart[0] == '{' + + // both captures -> as we cannot check, consider a collision + if aIsCapture && bIsCapture { + errors = append(errors, fmt.Errorf("%w (path %s and %s)", errPatternCollision, aPart, bPart)) + continue + } + + // no capture -> check strict equality + if !aIsCapture && !bIsCapture { + if aPart == bPart { + errors = append(errors, fmt.Errorf("%w (same path '%s')", errPatternCollision, aPart)) + continue + } + } + + // A captures B -> check type (B is A ?) + if aIsCapture { + input, exists := inputA[aPart] + + // fail if no type or no validator + if !exists || input.Validator == nil { + errors = append(errors, fmt.Errorf("%w (invalid type for %s)", errPatternCollision, aPart)) + continue + } + + // fail if not valid + if _, valid := input.Validator(bPart); valid { + errors = append(errors, fmt.Errorf("%w (%s captures '%s')", errPatternCollision, aPart, bPart)) + continue + } + + // B captures A -> check type (A is B ?) + } else if bIsCapture { + input, exists := inputB[bPart] + + // fail if no type or no validator + if !exists || input.Validator == nil { + errors = append(errors, fmt.Errorf("%w (invalid type for %s)", errPatternCollision, bPart)) + continue + } + + // fail if not valid + if _, valid := input.Validator(aPart); valid { + errors = append(errors, fmt.Errorf("%w (%s captures '%s')", errPatternCollision, bPart, aPart)) + continue + } + } + + errors = append(errors, nil) + + } + + // at least 1 URI part not matching -> no collision + var firstError error + for _, err := range errors { + if err != nil && firstError == nil { + firstError = err + } + + if err == nil { + return nil + } + } + + return firstError +} + // SplitURL without empty sets func SplitURL(url string) []string { trimmed := strings.Trim(url, " /\t\r\n") diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 11e9ad7..831691e 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -877,6 +877,36 @@ func TestMatchSimple(t *testing.T) { "/a", false, }, + { // root url + `[ { + "method": "GET", + "path": "/a", + "info": "info", + "in": {} + } ]`, + "/", + false, + }, + { + `[ { + "method": "GET", + "path": "/a", + "info": "info", + "in": {} + } ]`, + "/", + false, + }, + { + `[ { + "method": "GET", + "path": "/", + "info": "info", + "in": {} + } ]`, + "/", + true, + }, { `[ { "method": "GET", @@ -997,3 +1027,80 @@ func TestMatchSimple(t *testing.T) { } } + +func TestFindPriority(t *testing.T) { + t.Parallel() + tests := []struct { + Config string + URL string + MatchingDesc string + }{ + { + `[ + { "method": "GET", "path": "/a", "info": "s1" }, + { "method": "GET", "path": "/", "info": "s2" } + ]`, + "/", + "s2", + }, + { + `[ + { "method": "GET", "path": "/", "info": "s2" }, + { "method": "GET", "path": "/a", "info": "s1" } + ]`, + "/", + "s2", + }, + { + `[ + { "method": "GET", "path": "/a", "info": "s1" }, + { "method": "GET", "path": "/", "info": "s2" } + ]`, + "/a", + "s1", + }, + { + `[ + { "method": "GET", "path": "/a/b/c", "info": "s1" }, + { "method": "GET", "path": "/a/b", "info": "s2" } + ]`, + "/a/b/c", + "s1", + }, + { + `[ + { "method": "GET", "path": "/a/b/c", "info": "s1" }, + { "method": "GET", "path": "/a/b", "info": "s2" } + ]`, + "/a/b/", + "s2", + }, + } + + for i, test := range tests { + + t.Run(fmt.Sprintf("method.%d", i), func(t *testing.T) { + srv := &Server{} + srv.Types = append(srv.Types, builtin.AnyDataType{}) + srv.Types = append(srv.Types, builtin.IntDataType{}) + srv.Types = append(srv.Types, builtin.BoolDataType{}) + err := srv.Parse(strings.NewReader(test.Config)) + + if err != nil { + t.Errorf("unexpected error: '%s'", err) + t.FailNow() + } + + req := httptest.NewRequest(http.MethodGet, test.URL, nil) + service := srv.Find(req) + if service == nil { + t.Errorf("expected to find a service") + t.FailNow() + } + if service.Description != test.MatchingDesc { + t.Errorf("expected description '%s', got '%s'", test.MatchingDesc, service.Description) + t.FailNow() + } + }) + } +} diff --git a/internal/config/errors.go b/internal/config/errors.go index a11596d..91f4997 100644 --- a/internal/config/errors.go +++ b/internal/config/errors.go @@ -7,53 +7,53 @@ func (err cerr) Error() string { return string(err) } -// errRead - a problem ocurred when trying to read the configuration file +// errRead - read error const errRead = cerr("cannot read config") -// errUnknownMethod - invalid http method +// errUnknownMethod - unknown http method const errUnknownMethod = cerr("unknown HTTP method") -// errFormat - a invalid format has been detected +// errFormat - invalid format const errFormat = cerr("invalid config format") -// errPatternCollision - there is a collision between 2 services' patterns (same method) +// errPatternCollision - collision between 2 services' patterns const errPatternCollision = cerr("pattern collision") -// errInvalidPattern - a service pattern is malformed -const errInvalidPattern = cerr("must begin with a '/' and not end with") +// errInvalidPattern - malformed service pattern +const errInvalidPattern = cerr("malformed service path: must begin with a '/' and not end with") -// errInvalidPatternBraceCapture - a service pattern brace capture is invalid -const errInvalidPatternBraceCapture = cerr("invalid uri capturing braces") +// errInvalidPatternBraceCapture - invalid brace capture +const errInvalidPatternBraceCapture = cerr("invalid uri parameter") -// errUnspecifiedBraceCapture - a parameter brace capture is not specified in the pattern -const errUnspecifiedBraceCapture = cerr("capturing brace missing in the path") +// errUnspecifiedBraceCapture - missing path brace capture +const errUnspecifiedBraceCapture = cerr("missing uri parameter") -// errMandatoryRename - capture/query parameters must have a rename -const errMandatoryRename = cerr("capture and query parameters must have a 'name'") +// errUndefinedBraceCapture - missing capturing brace definition +const errUndefinedBraceCapture = cerr("missing uri parameter definition") -// errUndefinedBraceCapture - a parameter brace capture in the pattern is not defined in parameters -const errUndefinedBraceCapture = cerr("capturing brace missing input definition") +// errMandatoryRename - capture/query parameters must be renamed +const errMandatoryRename = cerr("uri and query parameters must be renamed") // errMissingDescription - a service is missing its description const errMissingDescription = cerr("missing description") -// errIllegalOptionalURIParam - an URI parameter cannot be optional -const errIllegalOptionalURIParam = cerr("URI parameter cannot be optional") +// errIllegalOptionalURIParam - uri parameter cannot optional +const errIllegalOptionalURIParam = cerr("uri parameter cannot be optional") -// errOptionalOption - an output is optional +// errOptionalOption - cannot have optional output const errOptionalOption = cerr("output cannot be optional") -// errMissingParamDesc - a parameter is missing its description +// errMissingParamDesc - missing parameter description const errMissingParamDesc = cerr("missing parameter description") -// errUnknownDataType - a parameter has an unknown datatype name -const errUnknownDataType = cerr("unknown data type") +// errUnknownDataType - unknown parameter datatype +const errUnknownDataType = cerr("unknown parameter datatype") -// errIllegalParamName - a parameter has an illegal name +// errIllegalParamName - illegal parameter name const errIllegalParamName = cerr("illegal parameter name") -// errMissingParamType - a parameter has an illegal type +// errMissingParamType - missing parameter type const errMissingParamType = cerr("missing parameter type") -// errParamNameConflict - a parameter has a conflict with its name/rename field -const errParamNameConflict = cerr("name conflict for parameter") +// errParamNameConflict - name/rename conflict +const errParamNameConflict = cerr("parameter name conflict") diff --git a/internal/config/parameter.go b/internal/config/parameter.go index 2a0a0cd..23bfd3d 100644 --- a/internal/config/parameter.go +++ b/internal/config/parameter.go @@ -11,33 +11,29 @@ type Parameter struct { Description string `json:"info"` Type string `json:"type"` Rename string `json:"name,omitempty"` - // ExtractType is the type of data the datatype returns + Optional bool + // ExtractType is the type the Validator will cast into ExtractType reflect.Type - // Optional is set to true when the type is prefixed with '?' - Optional bool - - // Validator is inferred from @Type + // Validator is inferred from the "type" property Validator datatype.Validator } func (param *Parameter) validate(datatypes ...datatype.T) error { - // missing description if len(param.Description) < 1 { return errMissingParamDesc } - // invalid type if len(param.Type) < 1 || param.Type == "?" { return errMissingParamType } - // optional type transform + // optional type if param.Type[0] == '?' { param.Optional = true param.Type = param.Type[1:] } - // assign the datatype + // find validator for _, dtype := range datatypes { param.Validator = dtype.Build(param.Type, datatypes...) param.ExtractType = dtype.Type() @@ -48,6 +44,5 @@ func (param *Parameter) validate(datatypes ...datatype.T) error { if param.Validator == nil { return errUnknownDataType } - return nil } diff --git a/internal/config/service.go b/internal/config/service.go index 1d67456..c713db2 100644 --- a/internal/config/service.go +++ b/internal/config/service.go @@ -22,15 +22,15 @@ type Service struct { Input map[string]*Parameter `json:"in"` Output map[string]*Parameter `json:"out"` - // references to url parameters - // format: '/uri/{param}' + // Captures contains references to URI parameters from the `Input` map. The format + // of these parameter names is "{paramName}" Captures []*BraceCapture - // references to Query parameters - // format: 'GET@paranName' + // Query contains references to HTTP Query parameters from the `Input` map. + // Query parameters names are "GET@paramName", this map contains escaped names (e.g. "paramName") Query map[string]*Parameter - // references for form parameters (all but Captures and Query) + // Form references form parameters from the `Input` map (all but Captures and Query). Form map[string]*Parameter } @@ -43,16 +43,12 @@ type BraceCapture struct { // Match returns if this service would handle this HTTP request func (svc *Service) Match(req *http.Request) bool { - // method if req.Method != svc.Method { return false } - - // check path if !svc.matchPattern(req.RequestURI) { return false } - return true } @@ -61,13 +57,12 @@ func (svc *Service) matchPattern(uri string) bool { uriparts := SplitURL(uri) parts := SplitURL(svc.Pattern) - // fail if size differ if len(uriparts) != len(parts) { return false } // root url '/' - if len(parts) == 0 { + if len(parts) == 0 && len(uriparts) == 0 { return true }