api/router: avoid unneeded loops and fix path match (#1594)

* api/router: avoid unneeded loops and fix path match

* if match found in google api path syntax, not try pcre loop
* if path is not ending via $ sign, append it to pcre to avoid matching other strings like
  /api/account/register can be matched to /api/account
* api: add tests and validations

Signed-off-by: Vasiliy Tolstov <v.tolstov@unistack.org>

api/api.go

@@ -128,9 +128,18 @@ func Validate(e *Endpoint) error {
 	}
 
 	for _, p := range e.Path {
-		_, err := regexp.CompilePOSIX(p)
-		if err != nil {
-			return err
+		ps := p[0]
+		pe := p[len(p)-1]
+
+		if ps == '^' && pe == '$' {
+			_, err := regexp.CompilePOSIX(p)
+			if err != nil {
+				return err
+			}
+		} else if ps == '^' && pe != '$' {
+			return errors.New("invalid path")
+		} else if ps != '^' && pe == '$' {
+			return errors.New("invalid path")
 		}
 	}
 

api/api_test.go

@@ -111,3 +111,42 @@ func TestEncoding(t *testing.T) {
 		}
 	}
 }
+
+func TestValidate(t *testing.T) {
+	epPcre := &Endpoint{
+		Name:        "Foo.Bar",
+		Description: "A test endpoint",
+		Handler:     "meta",
+		Host:        []string{"foo.com"},
+		Method:      []string{"GET"},
+		Path:        []string{"^/test/?$"},
+	}
+	if err := Validate(epPcre); err != nil {
+		t.Fatal(err)
+	}
+
+	epGpath := &Endpoint{
+		Name:        "Foo.Bar",
+		Description: "A test endpoint",
+		Handler:     "meta",
+		Host:        []string{"foo.com"},
+		Method:      []string{"GET"},
+		Path:        []string{"/test/{id}"},
+	}
+	if err := Validate(epGpath); err != nil {
+		t.Fatal(err)
+	}
+
+	epPcreInvalid := &Endpoint{
+		Name:        "Foo.Bar",
+		Description: "A test endpoint",
+		Handler:     "meta",
+		Host:        []string{"foo.com"},
+		Method:      []string{"GET"},
+		Path:        []string{"/test/?$"},
+	}
+	if err := Validate(epPcreInvalid); err == nil {
+		t.Fatalf("invalid pcre %v", epPcreInvalid.Path[0])
+	}
+
+}

api/router/registry/registry.go

@@ -187,10 +187,13 @@ func (r *registryRouter) store(services []*registry.Service) {
 
 		for _, p := range ep.Endpoint.Path {
 			var pcreok bool
-			pcrereg, err := regexp.CompilePOSIX(p)
-			if err == nil {
-				cep.pcreregs = append(cep.pcreregs, pcrereg)
-				pcreok = true
+
+			if p[0] == '^' && p[len(p)-1] != '$' {
+				pcrereg, err := regexp.CompilePOSIX(p)
+				if err == nil {
+					cep.pcreregs = append(cep.pcreregs, pcrereg)
+					pcreok = true
+				}
 			}
 
 			rule, err := util.Parse(p)
@@ -359,6 +362,9 @@ func (r *registryRouter) Endpoint(req *http.Request) (*api.Service, error) {
 				}
 				continue
 			}
+			if logger.V(logger.DebugLevel, logger.DefaultLogger) {
+				logger.Debugf("api gpath match %s = %v", path, pathreg)
+			}
 			pMatch = true
 			ctx := req.Context()
 			md, ok := metadata.FromContext(ctx)
@@ -373,16 +379,21 @@ func (r *registryRouter) Endpoint(req *http.Request) (*api.Service, error) {
 			break
 		}
 
-		// 4. try path via pcre path matching
-		for _, pathreg := range cep.pcreregs {
-			if !pathreg.MatchString(req.URL.Path) {
-				if logger.V(logger.DebugLevel, logger.DefaultLogger) {
-					logger.Debugf("api pcre path not match %s != %v", path, pathreg)
+		if !pMatch {
+			// 4. try path via pcre path matching
+			for _, pathreg := range cep.pcreregs {
+				if !pathreg.MatchString(req.URL.Path) {
+					if logger.V(logger.DebugLevel, logger.DefaultLogger) {
+						logger.Debugf("api pcre path not match %s != %v", path, pathreg)
+					}
+					continue
 				}
-				continue
+				if logger.V(logger.DebugLevel, logger.DefaultLogger) {
+					logger.Debugf("api pcre path match %s != %v", path, pathreg)
+				}
+				pMatch = true
+				break
 			}
-			pMatch = true
-			break
 		}
 
 		if !pMatch {

api/router/router_test.go

@@ -34,6 +34,18 @@ func (s *testServer) Call(ctx context.Context, req *pb.Request, rsp *pb.Response
 	return nil
 }
 
+// TestHello implements helloworld.GreeterServer
+func (s *testServer) CallPcre(ctx context.Context, req *pb.Request, rsp *pb.Response) error {
+	rsp.Msg = "Hello " + req.Uuid
+	return nil
+}
+
+// TestHello implements helloworld.GreeterServer
+func (s *testServer) CallPcreInvalid(ctx context.Context, req *pb.Request, rsp *pb.Response) error {
+	rsp.Msg = "Hello " + req.Uuid
+	return nil
+}
+
 func initial(t *testing.T) (server.Server, client.Client) {
 	r := rmemory.NewRegistry()
 
@@ -81,7 +93,7 @@ func check(t *testing.T, addr string, path string, expected string) {
 	}
 }
 
-func TestRouterRegistry(t *testing.T) {
+func TestRouterRegistryPcre(t *testing.T) {
 	s, c := initial(t)
 	defer s.Stop()
 
@@ -152,7 +164,7 @@ func TestRouterStaticPcre(t *testing.T) {
 	check(t, hsrv.Addr, "http://%s/api/v0/test/call", `{"msg":"Hello "}`)
 }
 
-func TestRouterStaticG(t *testing.T) {
+func TestRouterStaticGpath(t *testing.T) {
 	s, c := initial(t)
 	defer s.Stop()
 
@@ -192,3 +204,42 @@ func TestRouterStaticG(t *testing.T) {
 	time.Sleep(1 * time.Second)
 	check(t, hsrv.Addr, "http://%s/api/v0/test/call/TEST", `{"msg":"Hello TEST"}`)
 }
+
+func TestRouterStaticPcreInvalid(t *testing.T) {
+	var ep *api.Endpoint
+	var err error
+
+	s, c := initial(t)
+	defer s.Stop()
+
+	router := rstatic.NewRouter(
+		router.WithHandler(rpc.Handler),
+		router.WithRegistry(s.Options().Registry),
+	)
+
+	ep = &api.Endpoint{
+		Name:    "foo.Test.Call",
+		Method:  []string{"POST"},
+		Path:    []string{"^/api/v0/test/call/?"},
+		Handler: "rpc",
+	}
+
+	err = router.Register(ep)
+	if err == nil {
+		t.Fatalf("invalid endpoint %v", ep)
+	}
+
+	ep = &api.Endpoint{
+		Name:    "foo.Test.Call",
+		Method:  []string{"POST"},
+		Path:    []string{"/api/v0/test/call/?$"},
+		Handler: "rpc",
+	}
+
+	err = router.Register(ep)
+	if err == nil {
+		t.Fatalf("invalid endpoint %v", ep)
+	}
+
+	_ = c
+}

api/router/static/static.go

@@ -110,10 +110,14 @@ func (r *staticRouter) Register(ep *api.Endpoint) error {
 
 	for _, p := range ep.Path {
 		var pcreok bool
-		pcrereg, err := regexp.CompilePOSIX(p)
-		if err == nil {
-			pcreregs = append(pcreregs, pcrereg)
-			pcreok = true
+
+		// pcre only when we have start and end markers
+		if p[0] == '^' && p[len(p)-1] == '$' {
+			pcrereg, err := regexp.CompilePOSIX(p)
+			if err == nil {
+				pcreregs = append(pcreregs, pcrereg)
+				pcreok = true
+			}
 		}
 
 		rule, err := util.Parse(p)
@@ -122,6 +126,7 @@ func (r *staticRouter) Register(ep *api.Endpoint) error {
 		} else if err != nil && pcreok {
 			continue
 		}
+
 		tpl := rule.Compile()
 		pathreg, err := util.NewPattern(tpl.Version, tpl.OpCodes, tpl.Pool, "")
 		if err != nil {
@@ -280,6 +285,9 @@ func (r *staticRouter) endpoint(req *http.Request) (*endpoint, error) {
 				}
 				continue
 			}
+			if logger.V(logger.DebugLevel, logger.DefaultLogger) {
+				logger.Debugf("api gpath match %s = %v", path, pathreg)
+			}
 			pMatch = true
 			ctx := req.Context()
 			md, ok := metadata.FromContext(ctx)
@@ -294,16 +302,18 @@ func (r *staticRouter) endpoint(req *http.Request) (*endpoint, error) {
 			break
 		}
 
-		// 4. try path via pcre path matching
-		for _, pathreg := range ep.pcreregs {
-			if !pathreg.MatchString(req.URL.Path) {
-				if logger.V(logger.DebugLevel, logger.DefaultLogger) {
-					logger.Debugf("api pcre path not match %s != %v", req.URL.Path, pathreg)
+		if !pMatch {
+			// 4. try path via pcre path matching
+			for _, pathreg := range ep.pcreregs {
+				if !pathreg.MatchString(req.URL.Path) {
+					if logger.V(logger.DebugLevel, logger.DefaultLogger) {
+						logger.Debugf("api pcre path not match %s != %v", req.URL.Path, pathreg)
+					}
+					continue
 				}
-				continue
+				pMatch = true
+				break
 			}
-			pMatch = true
-			break
 		}
 
 		if !pMatch {