Refactor/acl decoder #435
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,71 @@ import ( | |
| "github.com/stoewer/go-strcase" | ||
| ) | ||
|
|
||
| func ParsePath(parts []string, idx int, t reflect.Type) []string { | ||
| if idx >= len(parts) { | ||
| return []string{} | ||
| } | ||
|
|
||
| if t.Kind() == reflect.Map { | ||
|
|
||
| if idx >= len(parts) { | ||
| return []string{} | ||
| } | ||
|
|
||
| elemType := t.Elem() | ||
| keyEndIdx := idx + 1 | ||
|
|
||
| if elemType.Kind() == reflect.Struct { | ||
| for i := idx + 1; i < len(parts); i++ { | ||
| found := false | ||
|
|
||
| for j := 0; j < elemType.NumField(); j++ { | ||
|
Comment on lines
+25
to
+28
Member
There was a problem hiding this comment. These loops can probably be replaced with: for j := range elemType.NumField() {
...
}Also applies to lines 52 and 62. |
||
| field := elemType.Field(j) | ||
| if strings.EqualFold(parts[i], field.Name) { | ||
| keyEndIdx = i | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if found { | ||
| break | ||
| } | ||
| } | ||
| } | ||
|
|
||
| keyParts := parts[idx:keyEndIdx] | ||
| keyName := strings.ToLower(strings.Join(keyParts, "_")) | ||
|
Comment on lines
+43
to
+44
Contributor
There was a problem hiding this comment. 🧩 Analysis chainValidate input parts to prevent empty string segments. If While the past review suggested filtering in Consider one of these approaches: Option 1: Filter empty strings from keyParts := parts[idx:keyEndIdx]
+ filtered := make([]string, 0, len(keyParts))
+ for _, part := range keyParts {
+ if part != "" {
+ filtered = append(filtered, part)
+ }
+ }
+ if len(filtered) == 0 {
+ return []string{}
+ }
- keyName := strings.ToLower(strings.Join(keyParts, "_"))
+ keyName := strings.ToLower(strings.Join(filtered, "_"))Option 2: Validate at entry that if idx >= len(parts) {
return []string{}
}
+ // Skip empty parts
+ for idx < len(parts) && parts[idx] == "" {
+ idx++
+ }
+ if idx >= len(parts) {
+ return []string{}
+ }Run this script to check if there are existing test cases or usage patterns that might produce empty string segments: 🏁 Script executed: #!/bin/bash
# Search for Split calls that might produce empty strings
rg -nP --type=go 'strings\.Split.*["\047]-["\047]'
# Search for how parts are constructed before calling ParsePath
rg -nP --type=go -B5 'ParsePath\(' | grep -E 'Split|parts.*='Length of output: 79 🏁 Script executed: #!/bin/bash
# Examine the decoders.go file around lines 43-44
echo "=== Content of decoders.go lines 35-55 ==="
sed -n '35,55p' internal/utils/decoders/decoders.go
# Show the full ParsePath function signature and initial logic
echo -e "\n=== ParsePath function signature and initial lines ==="
sed -n '1,100p' internal/utils/decoders/decoders.go | tail -50
# Find how ParsePath is called
echo -e "\n=== All ParsePath call sites ==="
rg -nP --type=go 'ParsePath\(' -B3 -A1Length of output: 3425 🏁 Script executed: #!/bin/bash
# Get the full normalizeKeys function to see how parts array is built
echo "=== normalizeKeys function (lines 88-175) ==="
sed -n '88,175p' internal/utils/decoders/decoders.go
# Also check for any test cases that might show expected behavior
echo -e "\n=== Test files for decoders ==="
find . -name '*decoder*test*.go' -o -name '*test*decoder*.go' | head -10
# Look for test cases that call ParsePath or normalizeKeys
echo -e "\n=== Test content ==="
fd -e 'go' -path '*/test*' | xargs grep -l 'ParsePath\|normalizeKeys' 2>/dev/null | head -5Length of output: 2080 🏁 Script executed: #!/bin/bash
# Look at test files to see if edge cases with empty strings or double delimiters are tested
echo "=== Test file list ==="
ls -la internal/utils/decoders/*test.go
echo -e "\n=== Content of env_decoder_test.go ==="
cat internal/utils/decoders/env_decoder_test.go | head -100
echo -e "\n=== Search for test cases with double delimiters or consecutive separators ==="
grep -rn '\-\-\|__' internal/utils/decoders/*test.go
echo -e "\n=== Search for any validation of parts array in ParsePath ==="
sed -n '10,50p' internal/utils/decoders/decoders.goLength of output: 2657 Filter empty string segments from keyParts before joining at line 44. The concern is valid. Go's Implement Option 1 from the original suggestion—filter empty strings before joining: keyParts := parts[idx:keyEndIdx]
+ filtered := make([]string, 0, len(keyParts))
+ for _, part := range keyParts {
+ if part != "" {
+ filtered = append(filtered, part)
+ }
+ }
+ if len(filtered) == 0 {
+ return []string{}
+ }
- keyName := strings.ToLower(strings.Join(keyParts, "_"))
+ keyName := strings.ToLower(strings.Join(filtered, "_"))This handles the issue at the source where keys are constructed, preventing malformed output downstream.
🤖 Prompt for AI Agents
Contributor
There was a problem hiding this comment.
|
||
|
|
||
| rest := ParsePath(parts, keyEndIdx, elemType) | ||
| result := append([]string{keyName}, rest...) | ||
| return result | ||
| } | ||
|
|
||
| if t.Kind() == reflect.Struct { | ||
| for i := 0; i < t.NumField(); i++ { | ||
| field := t.Field(i) | ||
| if field.Type.Kind() == reflect.Map { | ||
| rest := ParsePath(parts, idx, field.Type) | ||
| if len(rest) > 0 { | ||
| return rest | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for i := 0; i < t.NumField(); i++ { | ||
| field := t.Field(i) | ||
| if strings.EqualFold(parts[idx], field.Name) { | ||
| rest := ParsePath(parts, idx+1, field.Type) | ||
| result := append([]string{strings.ToLower(field.Name)}, rest...) | ||
| return result | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return []string{} | ||
| } | ||
|
|
||
| func normalizeKeys[T any](input map[string]string, root string, sep string) map[string]string { | ||
| knownKeys := getKnownKeys[T]() | ||
| normalized := make(map[string]string) | ||
|
|
@@ -74,3 +139,57 @@ func getKnownKeys[T any]() []string { | |
|
|
||
| return keys | ||
| } | ||
|
|
||
| func normalizeACLKeys[T any](input map[string]string, root string, sep string) map[string]string { | ||
| normalized := make(map[string]string) | ||
| var t T | ||
| rootType := reflect.TypeOf(t) | ||
|
|
||
| for k, v := range input { | ||
| parts := strings.Split(strings.ToLower(k), sep) | ||
|
|
||
| if len(parts) < 2 { | ||
| continue | ||
| } | ||
|
|
||
| // Two cases: | ||
| // 1. Keys starting with "tinyauth" (env vars): tinyauth_apps_... | ||
| // 2. Keys starting with root directly (flags): apps-... | ||
| startIdx := 0 | ||
| if parts[0] == "tinyauth" { | ||
| if len(parts) < 3 { | ||
| continue | ||
| } | ||
| if parts[1] != root { | ||
| continue | ||
| } | ||
| startIdx = 2 // Skip "tinyauth" and root | ||
| } else if parts[0] == root { | ||
| startIdx = 1 // Skip root only | ||
| } else { | ||
| continue | ||
| } | ||
|
|
||
| if startIdx < len(parts) { | ||
| parsedParts := ParsePath(parts[startIdx:], 0, rootType) | ||
|
|
||
| if len(parsedParts) == 0 { | ||
| continue | ||
| } | ||
|
|
||
| final := "tinyauth." + root | ||
|
|
||
| for _, part := range parsedParts { | ||
| if strings.Contains(part, "_") { | ||
| final += "." + part | ||
| } else { | ||
| final += "." + strcase.LowerCamelCase(part) | ||
| } | ||
| } | ||
|
|
||
| normalized[final] = v | ||
| } | ||
| } | ||
|
|
||
| return normalized | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The flags can be parsed directly here avoiding the need to store them in the
rootCmdstruct. Since we are fetching directly fromos.Args, cobra will not touch them so nothing to worry about.