Added filename length restriction for debug mode (-srd flag) Issue-5929 #5931
Conversation
WalkthroughThe changes modify the Changes
Possibly related issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Lercas
changed the title
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/output/output.go(1 hunks)
🔇 Additional comments (1)
pkg/output/output.go (1)
515-521: Verify the impact on existing debug files.
The changes in filename generation might affect existing debug files and tools that process them. Ensure this change is backward compatible or properly documented.
✅ Verification successful
Let me gather more context about how these fields are used in filename generation.
Let me check how these truncated values are used in the actual file generation.
Based on the gathered context, I can now provide the final response:
The truncation changes are safe and won't impact existing debug files
The truncation of host and templateID values occurs before the filename sanitization process and is only used for new debug file creation. This change:
- Happens at runtime for new file creation only
- Doesn't modify existing files
- Still maintains uniqueness with 57 and 97 characters respectively
- Occurs before the sanitization process that handles special characters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing debug files that might be affected by the new length restrictions
# Find all .txt files in the store response directory
echo "Checking for existing debug files with long names..."
fd -e txt -x sh -c '
filename=$(basename "{}")
if [ ${#filename} -gt 255 ]; then
echo "Found file exceeding 255 characters: {}"
echo "Length: ${#filename}"
fi
'
Length of output: 291
Script:
#!/bin/bash
# Search for code that uses host and templateID in filename generation
rg -U "func.*filename|func.*path" pkg/output/output.go -A 10
# Also check how these fields are used in debug file generation
ast-grep --pattern 'host := $_'
ast-grep --pattern 'templateID := $_'
Length of output: 4693
Script:
#!/bin/bash
# Search for the context around where these values are truncated
rg -U "host\[:57\]|templateID\[:97\]" pkg/output/output.go -B 10 -A 10
# Also check for any file writing or path generation logic
rg "WriteFile|SaveToFile|filepath.Join" pkg/output/output.go
Length of output: 1128
| if len(host) > 60 { | ||
| host = host[:57] + "..." | ||
| } | ||
| if len(templateID) > 100 { | ||
| templateID = templateID[:97] + "..." | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider total filename length and OS limitations.
The current implementation truncates host and templateID independently, but doesn't consider their combined length along with separators and the file extension. This could still result in exceeding OS filename limits.
+const (
+ // Common OS filename length limits
+ maxFilenameLength = 255 // typical limit for many filesystems
+ // Components
+ separatorLength = 1 // "_" separator
+ extensionLength = 4 // ".txt"
+)
func (w *StandardWriter) WriteStoreDebugData(host, templateID, eventType string, data string) {
if w.storeResponse {
+ // Calculate available space for host and templateID
+ availableLength := maxFilenameLength - separatorLength - extensionLength
+
+ // Distribute available length proportionally
+ hostRatio := 0.4 // 40% for host
+ templateIDRatio := 0.6 // 60% for templateID
+
+ maxHostLen := int(float64(availableLength) * hostRatio)
+ maxTemplateIDLen := int(float64(availableLength) * templateIDRatio)
+
if len(host) > maxHostLength {
host = host[:maxHostLength-ellipsisLength] + ellipsis
}
if len(templateID) > maxTemplateIDLength {
templateID = templateID[:maxTemplateIDLength-ellipsisLength] + ellipsis
}
+
+ // Ensure minimum length requirements
+ if len(host) < 1 || len(templateID) < 1 {
+ gologger.Warning().Msgf("Host or templateID too short after truncation: %s_%s", host, templateID)
+ }Committable suggestion skipped: line range outside the PR's diff.
| if len(host) > 60 { | ||
| host = host[:57] + "..." | ||
| } | ||
| if len(templateID) > 100 { | ||
| templateID = templateID[:97] + "..." | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract magic numbers into named constants.
The hardcoded length limits and truncation points should be defined as package-level constants for better maintainability and clarity.
+const (
+ maxHostLength = 60
+ maxTemplateIDLength = 100
+ ellipsis = "..."
+ ellipsisLength = len(ellipsis)
+)
func (w *StandardWriter) WriteStoreDebugData(host, templateID, eventType string, data string) {
if w.storeResponse {
- if len(host) > 60 {
- host = host[:57] + "..."
+ if len(host) > maxHostLength {
+ host = host[:maxHostLength-ellipsisLength] + ellipsis
}
- if len(templateID) > 100 {
- templateID = templateID[:97] + "..."
+ if len(templateID) > maxTemplateIDLength {
+ templateID = templateID[:maxTemplateIDLength-ellipsisLength] + ellipsis
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if len(host) > 60 { | |
| host = host[:57] + "..." | |
| } | |
| if len(templateID) > 100 { | |
| templateID = templateID[:97] + "..." | |
| } | |
| const ( | |
| maxHostLength = 60 | |
| maxTemplateIDLength = 100 | |
| ellipsis = "..." | |
| ellipsisLength = len(ellipsis) | |
| ) | |
| if len(host) > maxHostLength { | |
| host = host[:maxHostLength-ellipsisLength] + ellipsis | |
| } | |
| if len(templateID) > maxTemplateIDLength { | |
| templateID = templateID[:maxTemplateIDLength-ellipsisLength] + ellipsis | |
| } |
|
Thanks for your contribution @Lercas ! :) |
Proposed changes
Added a file name length restriction in the
WriteStoreDebugDatafunction for the debug mode triggered by the-srdflag. This change ensures that file names generated for debugging purposes are truncated. The update addresses the issue where excessively long file names caused by verbose requests could not be created due to operating system file name length limitations.Checklist
Summary by CodeRabbit
New Features
hostandtemplateIDparameters by truncating their lengths for better filename management.Bug Fixes
hostandtemplateIDvalues in debug data storage.Style