[TT-7815] Migrate API with endpoints containing path parameter

[edsonmichaque]

User description

TT-7815

Summary	Cannot migrate API with endpoints containing path parameter
Type	Bug
Status	In Dev
Points	N/A
Labels	DoD_Fail, Re_open

---

Description

Related Issue

https://tyktech.atlassian.net/browse/TT-7815

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Refactoring or add test (improvements in base code or adds test coverage to functionality)

Checklist

• I ensured that the documentation is up to date
• I explained why this PR updates go.mod in detail with reasoning why it's required
• I would like a code coverage CI quality gate exception and have explained why

---

PR Type

Bug fix, Tests

---

Description

• Enhanced path parameter handling in API definitions.

• Added support for named and regex-based path parameters.

• Refactored splitPath logic for improved clarity and functionality.

• Introduced comprehensive unit tests for path parsing and validation.

---

Changes walkthrough 📝

	Relevant files
Enhancement	operation.go Refactor path parsing and parameter handling logic              apidef/oas/operation.go Added isParam field to pathPart struct. Enhanced splitPath to handle named and regex-based parameters. Refactored isRegex and introduced isParamName for validation. Improved buildPath to support new parameter types. +104/-20
Tests	operation_test.go Add unit tests for path parsing and validation                      apidef/oas/operation_test.go Added new test cases for splitPath and pathPart.String. Enhanced TestOAS_RegexPaths to validate parameter extraction. Introduced tests for complex path scenarios and edge cases. Improved test coverage for named and regex-based parameters. +295/-20

---

Need help?

Type /help how to ... in the comments thread for any questions about PR-Agent usage.

Check out the documentation for more information.

[buger]

Let's make that PR title a 💯 shall we? 💪

Your PR title and story title look slightly different. Just checking in to know if it was intentional!

Story Title	Cannot migrate API with endpoints containing path parameter
PR Title	[TT-7815] migrate path params

Check out this guide to learn more about PR best-practices.

[github-actions]

API Changes

no api changes detected

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Possible Issue The splitPath function introduces new logic for handling path segments, including regex and parameter detection. Ensure that edge cases, such as invalid parameter names or complex regex patterns, are handled correctly without breaking existing functionality. func splitPath(inPath string) ([]pathPart, bool) { trimmedPath := strings.Trim(inPath, "/") if trimmedPath == "" { return []pathPart{}, false } parts := strings.Split(trimmedPath, "/") result := make([]pathPart, len(parts)) found := 0 nCustomRegex := 0 for k, value := range parts { // Handle non-bracketed path segments if !strings.HasPrefix(value, "{") && !strings.HasSuffix(value, "}") { name := value result[k] = pathPart{ name: name, value: value, isRegex: isRegex(value), } if isRegex(value) { nCustomRegex++ found++ result[k].name = fmt.Sprintf("customRegex%d", nCustomRegex) result[k].isRegex = true } continue } // Handle bracketed path segments segment := trimPart(value) // Simple parameter case: {name} if isParamName(segment) { result[k] = pathPart{ name: segment, isParam: true, } found++ continue } // Parameter with pattern case: {name:pattern} if name, pattern, ok := strings.Cut(segment, ":"); ok && isParamName(name) { result[k] = pathPart{ name: name, value: pattern, isRegex: true, isParam: true, } found++ continue } // Direct regex pattern case: {pattern} nCustomRegex++ result[k] = pathPart{ name: fmt.Sprintf("customRegex%d", nCustomRegex), value: segment, isRegex: true, } found++ } return result, found > 0 } Code Clarity The isParamName function checks for valid parameter names but has multiple conditions that could benefit from additional comments or refactoring for clarity. Ensure the logic is clear and maintainable. // isParamName checks if a string is a valid variable name containing only alphanumeric and underscore characters func isParamName(s string) bool { if len(s) == 0 { return false } // Check if string ends with hyphen if s[len(s)-1] == '-' { return false } for i, c := range s { if i == 0 && !((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || c == '_') { return false } if !((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '_') { return false } } return true Test Coverage The new test cases for splitPath and pathPart.String cover a wide range of scenarios. Verify that all edge cases are adequately tested, especially for invalid inputs and complex regex patterns. func TestOAS_RegexPaths(t *testing.T) { t.Parallel() type test struct { input string want string params []string } tests := []test{ {"/v1.Service", "/v1.Service", []string{}}, {"/v1.Service/stats.Service", "/v1.Service/stats.Service", []string{}}, {"/users/documents/list", "/users/documents/list", []string{}}, {"/group/.+", "/group/{customRegex1}", []string{"customRegex1"}}, {"/group/.*", "/group/{customRegex1}", []string{"customRegex1"}}, {"/group/[^a]*", "/group/{customRegex1}", []string{"customRegex1"}}, {"/group/foo$", "/group/{customRegex1}", []string{"customRegex1"}}, {"/group/[^a]*/.*", "/group/{customRegex1}/{customRegex2}", []string{"customRegex1", "customRegex2"}}, {"/users/documents/list", "/users/documents/list", []string{}}, {"/users/{id}/profile", "/users/{id}/profile", []string{"id"}}, {"/users/{id:[0-9]+}/profile", "/users/{id}/profile", []string{"id"}}, {"/files/{.*}/download", "/files/{customRegex1}/download", []string{"customRegex1"}}, {"/{id:[0-9]+}/{name:[a-zA-Z]+}/{.*}", "/{id}/{name}/{customRegex1}", []string{"id", "name", "customRegex1"}}, {"", "", []string{}}, {"/", "/", []string{}}, {"/users/profile/", "/users/profile/", []string{}}, {"/dates/{date:[0-9]{4}-[0-9]{2}-[0-9]{2}}/events", "/dates/{date}/events", []string{"date"}}, {"/.+", "/{customRegex1}", []string{"customRegex1"}}, {"/.+/", "/{customRegex1}/", []string{"customRegex1"}}, {"/users/my-profile/", "/users/my-profile/", []string{}}, {"/[0-9]+/[a-zA-Z]+/.*", "/{customRegex1}/{customRegex2}/{customRegex3}", []string{"customRegex1", "customRegex2", "customRegex3"}}, } for i, tc := range tests { var oas OAS oas.Paths = openapi3.Paths{} _ = oas.getOperationID(tc.input, "GET") pathKeys := make([]string, 0, len(oas.Paths)) for k := range oas.Paths { pathKeys = append(pathKeys, k) } assert.Lenf(t, oas.Paths, 1, "Expected one path key being created, got %#v", pathKeys) _, ok := oas.Paths[tc.want] assert.True(t, ok) p, ok := oas.Paths[tc.want] assert.Truef(t, ok, "test %d: path doesn't exist in OAS: %v", i, tc.want) assert.Lenf(t, p.Parameters, len(tc.params), "test %d: expected %d parameters, got %d", i, len(tc.params), len(p.Parameters)) extractedParams := make([]string, 0, len(p.Parameters)) for _, param := range p.Parameters { if param.Value == nil { continue } if param.Value.Schema == nil { continue } if param.Value.Schema.Value == nil { continue } extractedParams = append(extractedParams, param.Value.Name) } assert.ElementsMatch(t, tc.params, extractedParams) // rebuild original link got := tc.want for _, param := range p.Parameters { require.NotNilf(t, param.Value, "test %d: missing value", i) require.NotNilf(t, param.Value.Schema, "test %d: missing schema", i) require.NotNilf(t, param.Value.Schema.Value, "test %d: missing schema value", i) paramName := "{" + param.Value.Name + "}" if param.Value.Schema.Value.Pattern == "" { continue } pattern := param.Value.Schema.Value.Pattern isNamedParam := isParamName(param.Value.Name) && !strings.HasPrefix(param.Value.Name, "customRegex") hasBraces := false for _, part := range strings.Split(tc.input, "/") { if strings.HasPrefix(part, "{") && strings.HasSuffix(part, "}") { hasBraces = true break } } switch { case isNamedParam: // For named parameters like "id", use {id:pattern} format got = strings.ReplaceAll(got, paramName, fmt.Sprintf("{%s:%s}", param.Value.Name, pattern)) case hasBraces: // If original input used braces, maintain them got = strings.ReplaceAll(got, paramName, "{"+pattern+"}") default: // Otherwise use the pattern directly got = strings.ReplaceAll(got, paramName, pattern) } } assert.Equalf(t, tc.input, got, "test %d: rebuilt link, expected %v, got %v", i, tc.input, got) } } func TestPathPartString(t *testing.T) { tests := []struct { name string pathPart pathPart want string }{ { name: "regular path part", pathPart: pathPart{ name: "users", value: "users", isRegex: false, }, want: "users", }, { name: "regex path part", pathPart: pathPart{ name: "customRegex1", value: ".*", isRegex: true, }, want: "{customRegex1}", }, { name: "named pattern format", pathPart: pathPart{ name: "id", value: "{id:[0-9]+}", isRegex: false, }, want: "{id}", }, { name: "named pattern with multiple colons", pathPart: pathPart{ name: "path", value: "{path:.*:more:stuff}", isRegex: false, }, want: "{path}", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got := tt.pathPart.String() if got != tt.want { t.Errorf("pathPart.String() = %v, want %v", got, tt.want) } }) } } func TestSplitPath(t *testing.T) { tests := []struct { name string path string expectedParts []pathPart expectedRegex bool }{ { name: "standard path without regex", path: "/users/documents/list", expectedParts: []pathPart{ {name: "users", value: "users", isRegex: false, isParam: false}, {name: "documents", value: "documents", isRegex: false, isParam: false}, {name: "list", value: "list", isRegex: false, isParam: false}, }, expectedRegex: false, }, { name: "path with named parameter", path: "/users/{id}/profile", expectedParts: []pathPart{ {name: "users", value: "users", isRegex: false, isParam: false}, {name: "id", value: "", isRegex: false, isParam: true}, {name: "profile", value: "profile", isRegex: false, isParam: false}, }, expectedRegex: true, }, { name: "path with regex pattern", path: "/users/{id:[0-9]+}/profile", expectedParts: []pathPart{ {name: "users", value: "users", isRegex: false, isParam: false}, {name: "id", value: "[0-9]+", isRegex: true, isParam: true}, {name: "profile", value: "profile", isRegex: false, isParam: false}, }, expectedRegex: true, }, { name: "path with direct regex", path: "/files/{.*}/download", expectedParts: []pathPart{ {name: "files", value: "files", isRegex: false, isParam: false}, {name: "customRegex1", value: ".*", isRegex: true, isParam: false}, {name: "download", value: "download", isRegex: false, isParam: false}, }, expectedRegex: true, }, { name: "path with multiple regex patterns", path: "/{id:[0-9]+}/{name:[a-zA-Z]+}/{.*}", expectedParts: []pathPart{ {name: "id", value: "[0-9]+", isRegex: true, isParam: true}, {name: "name", value: "[a-zA-Z]+", isRegex: true, isParam: true}, {name: "customRegex1", value: ".*", isRegex: true, isParam: false}, }, expectedRegex: true, }, { name: "path with invalid parameter names", path: "/users/{123}/{user-name}", expectedParts: []pathPart{ {name: "users", value: "users", isRegex: false, isParam: false}, {name: "customRegex1", value: "123", isRegex: true, isParam: false}, {name: "customRegex2", value: "user-name", isRegex: true, isParam: false}, }, expectedRegex: true, }, { name: "empty path", path: "", expectedParts: []pathPart{}, expectedRegex: false, }, { name: "root path", path: "/", expectedParts: []pathPart{}, expectedRegex: false, }, { name: "path with trailing slash", path: "/users/profile/", expectedParts: []pathPart{ {name: "users", value: "users", isRegex: false, isParam: false}, {name: "profile", value: "profile", isRegex: false, isParam: false}, }, expectedRegex: false, }, { name: "path with complex regex pattern", path: "/dates/{date:[0-9]{4}-[0-9]{2}-[0-9]{2}}/events", expectedParts: []pathPart{ {name: "dates", value: "dates", isRegex: false, isParam: false}, {name: "date", value: "[0-9]{4}-[0-9]{2}-[0-9]{2}", isRegex: true, isParam: true}, {name: "events", value: "events", isRegex: false, isParam: false}, }, expectedRegex: true, }, { name: "root regex pattern", path: "/.+", expectedParts: []pathPart{ {name: "customRegex1", value: ".+", isRegex: true, isParam: false}, }, expectedRegex: true, }, { name: "root regex pattern with trailing slash", path: "/.+", expectedParts: []pathPart{ {name: "customRegex1", value: ".+", isRegex: true, isParam: false}, }, expectedRegex: true, }, { name: "path with trailing slash", path: "/users/my-profile/", expectedParts: []pathPart{ {name: "users", value: "users", isRegex: false, isParam: false}, {name: "my-profile", value: "my-profile", isRegex: false, isParam: false}, }, expectedRegex: false, }, { name: "path with multiple regex patterns", path: "/[0-9]+/[a-zA-Z]+/.*", expectedParts: []pathPart{ {name: "customRegex1", value: "[0-9]+", isRegex: true, isParam: false}, {name: "customRegex2", value: "[a-zA-Z]+", isRegex: true, isParam: false}, {name: "customRegex3", value: ".*", isRegex: true, isParam: false}, }, expectedRegex: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { parts, hasRegex := splitPath(tt.path) if hasRegex != tt.expectedRegex { t.Errorf("splitPath() hasRegex = %v, want %v", hasRegex, tt.expectedRegex) } if len(parts) != len(tt.expectedParts) { t.Errorf("splitPath() returned %d parts, want %d", len(parts), len(tt.expectedParts)) return } for i, part := range parts { expected := tt.expectedParts[i] if part.name != expected.name { t.Errorf("Part %d name = %v, want %v", i, part.name, expected.name) } if part.value != expected.value { t.Errorf("Part %d value = %v, want %v", i, part.value, expected.value) } if part.isRegex != expected.isRegex { t.Errorf("Part %d isRegex = %v, want %v", i, part.isRegex, expected.isRegex) } if part.isParam != expected.isParam { t.Errorf("Part %d isParam = %v, want %v", i, part.isParam, expected.isParam) } } }) }

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Add tests for invalid parameter names Add test cases to TestSplitPath to explicitly validate behavior when encountering invalid parameter names, such as those starting with numbers or containing hyphens. apidef/oas/operation_test.go [447-453] { name: "path with invalid parameter names", path: "/users/{123}/{user-name}", expectedParts: []pathPart{ {name: "users", value: "users", isRegex: false, isParam: false}, {name: "customRegex1", value: "123", isRegex: true, isParam: false}, {name: "customRegex2", value: "user-name", isRegex: true, isParam: false}, }, expectedRegex: true, }, +{ + name: "path with invalid starting character", + path: "/users/{1invalid}", + expectedParts: []pathPart{ + {name: "users", value: "users", isRegex: false, isParam: false}, + {name: "customRegex1", value: "1invalid", isRegex: true, isParam: false}, + }, + expectedRegex: true, +}, Suggestion importance[1-10]: 9 __ Why: Adding test cases for invalid parameter names enhances the test coverage and ensures that the implemented validation logic works as expected. This is a significant improvement for verifying the correctness of the changes introduced in the PR.	High
Possible issue	Validate parameter names in splitPath Add a check in splitPath to ensure that invalid parameter names (e.g., starting with numbers or containing invalid characters) are not processed as valid parameters. apidef/oas/operation.go [548-554] if name, pattern, ok := strings.Cut(segment, ":"); ok && isParamName(name) { + if !isValidParamName(name) { + continue + } result[k] = pathPart{ name: name, value: pattern, isRegex: true, isParam: true, } found++ continue } Suggestion importance[1-10]: 8 __ Why: Adding a validation check for parameter names in splitPath ensures that invalid names are not processed as valid parameters, which is crucial for maintaining the integrity of the path parsing logic. This directly addresses a potential issue in the PR.	Medium
	Fix regex detection in splitPath In splitPath, ensure that nCustomRegex is incremented only when a valid regex pattern is detected to avoid mislabeling invalid segments as regex. apidef/oas/operation.go [560-565] -nCustomRegex++ -result[k] = pathPart{ - name: fmt.Sprintf("customRegex%d", nCustomRegex), - value: segment, - isRegex: true, +if isRegex(segment) { + nCustomRegex++ + result[k] = pathPart{ + name: fmt.Sprintf("customRegex%d", nCustomRegex), + value: segment, + isRegex: true, + } } Suggestion importance[1-10]: 8 __ Why: The suggestion ensures that nCustomRegex is incremented only when a valid regex pattern is detected, which prevents mislabeling invalid segments as regex. This is a meaningful improvement to the logic in splitPath and addresses a potential bug.	Medium
	Improve validation for parameter names Ensure that isParamName correctly validates parameter names by explicitly checking for invalid characters like hyphens, which are currently allowed in the middle of the name but not at the end. apidef/oas/operation.go [597-599] -if !((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '_') { +if !((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '_') || strings.Contains(s, "-") { return false } Suggestion importance[1-10]: 7 __ Why: The suggestion improves the validation logic for parameter names by explicitly disallowing hyphens within the name, which aligns with the intent of the function. This enhancement ensures stricter validation and prevents potential issues with invalid parameter names.	Medium

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
98.8% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[titpetric]

this is invalid by mux; read through httputil/mux* ; i feel a significant part is already implemented in httputil, maybe needs extending (mux.go, we already have logic for /* and /*/ which equals this patch matching case.

With {.*} - this mux tag is not a valid input, while {_} or {_:.*} would be. Is this a case we need to cover? Technically it would "work" in mux because .* would be treated as a name, using the default parameter regex. customRegex in this case should ignore {} and be isRegex=false

[jeffy-mathew]

@titpetric qq: IsMuxTemplate is checking only for existence of curly braces which are opened and closed. And if it is returning true, whatever is inside {} is replaced with ([^/]+).
This means {id:[a-z]} is also replaced with ([^/]+), is it expected?

[titpetric]

Rather you extend some existing tests for httputil.mux with these; that behavior is correct, and can extend the internal apis and existing tests for an overview.