feat(util): implement type name conversion utility #85
Conversation
• Adds ConvertToTypeName and capGroupSingularLength functions • Supports formatting group and singular names for consistent type naming Signed-off-by: Bastian Echterhölter <bastian.echterhoelter@sap.com> On-behalf-of: @SAP <bastian.echterhoelter@sap.com>
WalkthroughAdds a .gitignore rule for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
• Renames variables for better readability and understanding Signed-off-by: Bastian Echterhölter <bastian.echterhoelter@sap.com> On-behalf-of: @SAP <bastian.echterhoelter@sap.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
fga/util/util.go (2)
8-8: Document the rationale for the magic number.Consider adding a comment explaining why 50 is chosen as the maximum relation length. This would help future maintainers understand the constraint.
10-24: Add documentation for the exported function.The exported function
ConvertToTypeNamelacks a doc comment. Go best practices recommend documenting all exported functions.Consider adding documentation like:
// ConvertToTypeName converts a group and singular name into a normalized type name. // If group is empty, it defaults to "core". The result is lowercase with dots // replaced by underscores and is capped to ensure "create_<result>" fits within // maxRelationLength characters. func ConvertToTypeName(group, singular string) string {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignore(1 hunks)fga/util/util.go(1 hunks)fga/util/util_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
fga/util/util_test.go (1)
fga/util/util.go (1)
ConvertToTypeName(10-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
.gitignore (1)
6-6: LGTM!The addition of
.claudeto the gitignore is appropriate for excluding IDE/tool-specific files from version control.fga/util/util_test.go (2)
7-90: Excellent test coverage for ConvertToTypeName.The test cases comprehensively cover the main scenarios including edge cases like empty inputs, special characters, dots, and case sensitivity. Well done!
92-166: Add test case for small maxLength values to catch the panic bug.The test suite doesn't cover the edge case where
maxLength < 7, which would expose the panic bug incapGroupSingularLength(see comment in util.go). Once the implementation is fixed, add a test case to prevent regression.Example test case to add after the fix:
{ name: "maxLength too small to fit create_ prefix", group: "a", singular: "b", maxLength: 5, expected: "b", // or some minimal valid identifier },
• Add detailed documentation for ConvertToTypeName and capGroupSingularLength functions • Ensure consistent naming conventions for authorization systems Signed-off-by: Bastian Echterhölter <bastian.echterhoelter@sap.com> On-behalf-of: @SAP <bastian.echterhoelter@sap.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
fga/util/util.go (1)
71-80: Verify test coverage for the panic scenario identified in previous review.The past review correctly identified that this code can panic when
maxLengthis too small (specifically whentruncateLen >= len(groupSingular)). While a fix was suggested, I'd like to confirm that your test suite inutil_test.goincludes test cases for this edge case to prevent regressions once the fix is applied.Run the following script to check if tests cover small maxLength values:
#!/bin/bash # Description: Check if tests cover the panic scenario with small maxLength values # Search for test cases that use maxLength values less than 15 rg -n -A 5 -B 2 'capGroupSingularLength.*[0-9]+' fga/util/util_test.go # Also search for any test cases with very short group/singular combinations rg -n -A 3 'maxLength.*[0-9]|group.*"[a-z]".*singular.*"[a-z]"' fga/util/util_test.go
🧹 Nitpick comments (1)
fga/util/util.go (1)
44-62: Consider adding input validation to trim whitespace.The function doesn't trim leading or trailing spaces from inputs. If callers pass strings like
" apps "or"deployment ", these spaces will be preserved in the type name, potentially causing issues with authorization systems.Consider adding input trimming at the start of the function:
func ConvertToTypeName(group, singular string) string { + group = strings.TrimSpace(group) + singular = strings.TrimSpace(singular) + if group == "" { group = "core" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fga/util/util.go(1 hunks)fga/util/util_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- fga/util/util_test.go
🔇 Additional comments (3)
fga/util/util.go (3)
1-11: LGTM: Clear package documentation and appropriate imports.The package documentation effectively describes the purpose and context for FGA systems.
13-19: LGTM: Well-documented constant with clear rationale.The maximum length is appropriately documented, including the relation format that drives the constraint.
64-70: Fix documentation to match implementation.The comment at line 69 describes the format as
"create_<group>_<singular>", but the actual implementation at line 73 creates"create_<group>_<singular>s"with a trailing "s" to pluralize the resource name.Apply this diff to fix the documentation:
// capGroupSingularLength creates a group_singular string and truncates it if necessary // to ensure the resulting relation names don't exceed the specified maximum length. // // This function is used internally by ConvertToTypeName to handle length constraints // imposed by authorization systems. It calculates the potential length of the longest -// relation that would be created ("create_<group>_<singular>") and truncates the +// relation that would be created ("create_<group>_<singular>s") and truncates the // group_singular combination if needed.Likely an incorrect or invalid review comment.
2 participants
Summary by CodeRabbit