Fixing sanitizefilename.Sanitize only on file name

[svaroqui]

Contributor Agreement

By submitting this pull request, I agree to the terms outlined in the Contributor Agreement.

[claude]

Code Review: PR #1340 - Fixing sanitizefilename.Sanitize only on file name

Summary

This PR fixes a bug where the sanitizefilename.Sanitize() function was being applied to the full file path instead of just the filename. The changes ensure sanitization is only applied to the database table name component, allowing the output directory path to remain unchanged.

✅ Strengths

1. Correct Bug Fix: The original code applied sanitization to the entire path (filepath.Join() result), which could remove or modify legitimate path separators and directory structure. This fix correctly applies sanitization only to the filename portion.

2. Comprehensive Changes: All 6 locations where this pattern occurred were consistently updated, ensuring the fix is complete across the codebase.

3. Code Simplification: The refactored code is more concise and easier to understand - inline sanitization within the filepath.Join() call reduces unnecessary intermediate variables.

4. Functional Correctness: By sanitizing only the tableName before concatenation with .sql.gz, the function prevents invalid characters in database/table names while preserving the output directory path structure.

🔍 Observations

1. Artifact Issue: The last change (line 258) appears incomplete or incorrect:

   tablePath = tablePath  // This is a no-op

   This line should likely be removed entirely since it just assigns the variable to itself. The outputDir should already be a valid path, so no sanitization is needed on just the filename "metadata".

2. Error Handling: The code uses bare error ignoring (f, _ = os.Create(tablePath)). While not introduced by this PR, consider adding error handling for file creation operations to prevent silent failures.

3. Input Validation: The tableName construction involves multiple string operations with hardcoded patterns. While sanitization helps, additional validation of database/schema names could improve robustness.

🎯 Recommendations

1. Fix the No-Op Assignment: Remove or correct line 258 where tablePath = tablePath serves no purpose.

2. Consider Error Handling: In a follow-up PR, add error handling for file I/O operations:

   f, err := os.Create(tablePath)
   if err != nil {
       // Handle error appropriately
   }

3. Path Validation: Ensure outputDir is validated before use to prevent path traversal issues.

📊 Code Quality

• Quality: Good - Addresses the root cause correctly
• Test Coverage: Unable to verify from PR - ensure regression tests exist
• Security: ✅ Improved - Prevents path sanitization from corrupting directory structure
• Performance: No impact

Verdict

✅ Approved (with minor note about line 258)

The fix is fundamentally sound and addresses the bug correctly. The no-op assignment at line 258 should be reviewed, but the overall change improves code correctness and security by preventing path corruption during filename sanitization.