[GitHub] dubee closed pull request #246: Fix Go regex for path parameter parsing

[GitBox <gi...@apache.org>]

dubee closed pull request #246: Fix Go regex for path parameter parsing
URL: https://github.com/apache/incubator-openwhisk-cli/pull/246
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/commands/api.go b/commands/api.go
index 468fb058..9f609190 100644
--- a/commands/api.go
+++ b/commands/api.go
@@ -43,7 +43,8 @@ const (
 	formatOptionYaml = "yaml"
 	formatOptionJson = "json"
 
-	pathParamRegex = `/\{([^/]+)}/|/\{([^/]+)}$`
+	pathParamRegex        = `\/\{([^\/]+)\}\/|\/\{([^\/]+)\}$|\{([^\/]+)}\/`
+	pathSegmentParamRegex = `^\/\{([^\/]+)\}\/$`
 )
 
 var apiCmd = &cobra.Command{
@@ -96,20 +97,34 @@ func isValidRelpath(relpath string) (error, bool) {
 	return nil, true
 }
 
-func hasPathParameters(path string) (bool, error) {
-	matched, err := regexp.MatchString(pathParamRegex, path)
-	hasBracket := strings.ContainsRune(path, '{') || strings.ContainsRune(path, '}')
+func getPathParameterNames(path string) ([]string, error) {
+	var pathParameters []string
+
+	regexObj, err := regexp.Compile(pathSegmentParamRegex)
 	if err != nil {
-		whisk.Debug(whisk.DbgInfo, "Unable to compile Regex '%s' to test against path '%s'\n", pathParamRegex, path)
-	}
-	if hasBracket && !matched {
-		errMsg := wski18n.T("Relative path '{{.path}}' does not include valid path parameters. Each parameter must be enclosed in curly braces '{}'.",
-			map[string]interface{}{"path": path})
-		whiskErr := whisk.MakeWskError(errors.New(errMsg), whisk.EXIT_CODE_ERR_GENERAL,
-			whisk.DISPLAY_MSG, whisk.DISPLAY_USAGE)
-		return false, whiskErr
+		whisk.Debug(whisk.DbgError, "Failed to match path '%s' to regular expressions `%s`\n", path, pathSegmentParamRegex)
+	} else {
+		segments := strings.Split(path, "/")
+		for _, segment := range segments {
+			segment = fmt.Sprintf("/%s/", segment)
+			matchedItems := regexObj.FindAllStringSubmatch(segment, -1)
+			for _, matchedParam := range matchedItems {
+				for idx, paramName := range matchedParam {
+					whisk.Debug(whisk.DbgInfo, "Path parameter submatch '%v'; idx %v\n", paramName, idx)
+					if idx > 0 && len(paramName) > 0 {
+						pathParameters = append(pathParameters, paramName)
+					}
+				}
+			}
+		}
 	}
-	return matched, nil
+
+	return pathParameters, err
+}
+
+func hasPathParameters(path string) (bool, error) {
+	pathParams, err := getPathParameterNames(path)
+	return len(pathParams) > 0, err
 }
 
 func isBasepathParameterized(basepath string) (error, bool) {
@@ -759,26 +774,27 @@ func getLargestApiNameSize(retApiArray *whisk.RetApiArray, apiPath string, apiVe
 	return maxNameSize
 }
 
-func generatePathParameters(relativePath string) []whisk.ApiParameter {
+func generatePathParameters(relativePath string) ([]whisk.ApiParameter, error) {
 	pathParams := []whisk.ApiParameter{}
-	regexObj, err := regexp.Compile(pathParamRegex)
-	if err != nil {
-		whisk.Debug(whisk.DbgError, "Failed to match path '%s' to regular expressions `%s`\n", relativePath, pathParamRegex)
-	}
-	matches := regexObj.FindAllString(relativePath, -1)
-	if matches != nil {
-		for _, paramName := range matches {
-			//The next 3 lines clean up the paramName, as the matches are something like `/{param}`
-			openIdx := strings.IndexRune(paramName, '{')
-			closeIdx := strings.IndexRune(paramName, '}')
-			paramName = string([]rune(paramName)[openIdx+1 : closeIdx])
-			param := whisk.ApiParameter{Name: paramName, In: "path", Required: true, Type: "string",
-				Description: wski18n.T("Default description for '{{.name}}'", map[string]interface{}{"name": paramName})}
+
+	pathParamNames, err := getPathParameterNames(relativePath)
+	if len(pathParamNames) > 0 && err == nil {
+		// Only create unique swagger entries
+		var uniqueParamNames []string
+		for _, name := range pathParamNames {
+			if !contains(uniqueParamNames, name) {
+				uniqueParamNames = append(uniqueParamNames, name)
+			}
+		}
+		for _, uniqueName := range uniqueParamNames {
+			whisk.Debug(whisk.DbgInfo, "Creating api parameter for '%s'\n", uniqueName)
+			param := whisk.ApiParameter{Name: uniqueName, In: "path", Required: true, Type: "string",
+				Description: wski18n.T("Default description for '{{.name}}'", map[string]interface{}{"name": uniqueName})}
 			pathParams = append(pathParams, param)
 		}
 	}
 
-	return pathParams
+	return pathParams, err
 }
 
 /*
@@ -896,10 +912,10 @@ func parseApi(cmd *cobra.Command, args []string) (*whisk.Api, *QualifiedName, er
 	if !basepathArgIsApiName {
 		api.Id = "API:" + api.Namespace + ":" + api.GatewayBasePath
 	}
-	api.PathParameters = generatePathParameters(api.GatewayRelPath)
+	api.PathParameters, err = generatePathParameters(api.GatewayRelPath)
 
 	whisk.Debug(whisk.DbgInfo, "Parsed api struct: %#v\n", api)
-	return api, qName, nil
+	return api, qName, err
 }
 
 func parseSwaggerApi() (*whisk.Api, error) {
diff --git a/commands/util.go b/commands/util.go
index 2fe6b5a1..570a93eb 100644
--- a/commands/util.go
+++ b/commands/util.go
@@ -1110,3 +1110,12 @@ func isApplicationError(err error) bool {
 
 	return applicationError
 }
+
+func contains(arr []string, element string) bool {
+	for _, e := range arr {
+		if e == element {
+			return true
+		}
+	}
+	return false
+}
diff --git a/tests/src/test/scala/whisk/core/cli/test/ApiGwCliTests.scala b/tests/src/test/scala/whisk/core/cli/test/ApiGwCliTests.scala
index 13ffc6ac..3465815c 100644
--- a/tests/src/test/scala/whisk/core/cli/test/ApiGwCliTests.scala
+++ b/tests/src/test/scala/whisk/core/cli/test/ApiGwCliTests.scala
@@ -37,60 +37,6 @@ class ApiGwCliTests extends ApiGwTests {
   override lazy val createCode = SUCCESS_EXIT
   behavior of "Cli Wsk api creation with path parameters no swagger"
 
-  it should "fail to create an API if the relative path contains invalid path parameters" in withAssetCleaner(wskprops) {(wp, assetHelper) =>
-    val actionName = "APIGWTEST_BAD_RELATIVE_PATH_ACTION"
-    val basePath = "/mybase/path"
-    val file = TestUtils.getTestActionFilename(s"echo-web-http.js")
-    assetHelper.withCleaner(wsk.action, actionName, confirmDelete = true) {
-      (action, _) =>
-        action.create(actionName, Some(file), web = Some("true"))
-    }
-    var relPath = "/bad/{path/value"
-    var rr = apiCreate(basepath = Some(basePath),
-      relpath = Some(relPath),
-      operation = Some("GET"),
-      action = Some(actionName),
-      expectedExitCode = ANY_ERROR_EXIT)
-    rr.stderr should include(
-      s"Relative path '${relPath}' does not include valid path parameters. Each parameter must be enclosed in curly braces '{}'.")
-
-    relPath = "/bad/path}/value"
-    rr = apiCreate(basepath = Some(basePath),
-      relpath = Some(relPath),
-      operation = Some("GET"),
-      action = Some(actionName),
-      expectedExitCode = ANY_ERROR_EXIT)
-    rr.stderr should include(
-      s"Relative path '${relPath}' does not include valid path parameters. Each parameter must be enclosed in curly braces '{}'.")
-
-    relPath = "/bad/{path/va}lue"
-    rr = apiCreate(basepath = Some(basePath),
-      relpath = Some(relPath),
-      operation = Some("GET"),
-      action = Some(actionName),
-      expectedExitCode = ANY_ERROR_EXIT)
-    rr.stderr should include(
-      s"Relative path '${relPath}' does not include valid path parameters. Each parameter must be enclosed in curly braces '{}'.")
-
-    relPath = "/ba}d/{path/value"
-    rr = apiCreate(basepath = Some(basePath),
-      relpath = Some(relPath),
-      operation = Some("GET"),
-      action = Some(actionName),
-      expectedExitCode = ANY_ERROR_EXIT)
-    rr.stderr should include(
-      s"Relative path '${relPath}' does not include valid path parameters. Each parameter must be enclosed in curly braces '{}'.")
-
-    relPath = "/ba}d/{p{at}h/value"
-    rr = apiCreate(basepath = Some(basePath),
-      relpath = Some(relPath),
-      operation = Some("GET"),
-      action = Some(actionName),
-      expectedExitCode = ANY_ERROR_EXIT)
-    rr.stderr should include(
-      s"Relative path '${relPath}' does not include valid path parameters. Each parameter must be enclosed in curly braces '{}'.")
-  }
-
   it should "fail to create an API if the base path contains path parameters" in withAssetCleaner(wskprops) {(wp, assetHelper) =>
     val actionName = "APIGWTEST_BAD_BASE_PATH_ACTION"
     val basePath = "/mybase/{path}"
@@ -193,10 +139,12 @@ class ApiGwCliTests extends ApiGwTests {
     wskprops) { (wp, assetHelper) =>
     val testName = "CLI_APIGWTEST_PATH_PARAMS2"
     val testBasePath = "/" + testName + "_bp"
-    val testRelPath = "/path/{with}/some/{path}"
+    val testRelPath = "/path/{with}/some/{double}/{extra}/{extra}/{path}"
     val testUrlName1 = "scooby"
     val testUrlName2 = "doo"
-    val testRelPathGet = s"/path/${testUrlName1}/some/$testUrlName2"
+    val testUrlName3 = "shaggy"
+    val testUrlName4 = "velma"
+    val testRelPathGet = s"/path/$testUrlName1/some/$testUrlName3/$testUrlName4/$testUrlName4/$testUrlName2"
     val testUrlOp = "get"
     val testApiName = testName + " API Name"
     val actionName = testName + "_action"
@@ -219,6 +167,7 @@ class ApiGwCliTests extends ApiGwTests {
       )
       verifyApiCreated(rr)
       val swaggerApiUrl = getSwaggerUrl(rr).replace("{with}", testUrlName1).replace("{path}", testUrlName2)
+          .replace("{double}", testUrlName3).replace("{extra}", testUrlName4)
 
       //Validate the api created contained parameters and they were correct
       rr = apiGet(basepathOrApiName = Some(testApiName))
@@ -227,9 +176,13 @@ class ApiGwCliTests extends ApiGwTests {
       rr.stdout should include regex (""""cors":\s*\{\s*\n\s*"enabled":\s*true""")
       rr.stdout should include regex (s"""target-url.*${actionName}.http${reqPath}""")
       val params = getParametersFromJson(rr, testRelPath)
-      params.size should be(2)
+
+      // should have 4, not 5 parameter definitions (i.e. don't define "extra" twice
+      params.size should be(4)
       validateParameter(params(0), "with", "path", true, "string", "Default description for 'with'")
-      validateParameter(params(1), "path", "path", true, "string", "Default description for 'path'")
+      validateParameter(params(1), "double", "path", true, "string", "Default description for 'double'")
+      validateParameter(params(2), "extra", "path", true, "string", "Default description for 'extra'")
+      validateParameter(params(3), "path", "path", true, "string", "Default description for 'path'")
 
       //Lets call the swagger url so we can make sure the response is valid and contains our path in the ow path
       val apiToInvoke = s"$swaggerApiUrl"


 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services