Fix Illegal Characters #132
Conversation
WalkthroughThe PR enhances path handling in RevitIniReader by supporting multiple render appearance paths with quote trimming, pipe-to-semicolon conversion, per-path validation, and conditional resolution based on path rooting (absolute vs. relative). It also refines the error window UI layout with center alignment and bottom margin spacing. Changes
Sequence DiagramsequenceDiagram
participant Code as Calling Code
participant Reader as RevitIniReader
participant Validator as Path Validator
participant Resolver as Path Resolver
Code->>Reader: GetAdditionalRenderAppearancePaths()
activate Reader
Reader->>Reader: Trim quotes, replace pipes with semicolons
Reader->>Reader: Split on semicolons into sub-paths
loop For each sub-path
Reader->>Validator: Validate against invalid path chars
alt Invalid path detected
Validator-->>Reader: Skip + log warning
else Valid path
Validator-->>Reader: Proceed
Reader->>Resolver: Check if rooted absolute path
alt Rooted path
Resolver-->>Reader: Use path as-is
else Relative path
Resolver-->>Reader: Combine with user profile
end
Reader->>Reader: Normalize path, add to results
end
end
Reader-->>Code: Return all valid absolute paths
deactivate Reader
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Poem
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 |
![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 (2)
Common_glTF_Exporter/Utils/RevitIniReader.cs(1 hunks)Common_glTF_Exporter/Windows/ErrorWindow.xaml(1 hunks)
⏰ 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: build
🔇 Additional comments (2)
Common_glTF_Exporter/Windows/ErrorWindow.xaml (1)
66-68: LGTM! Improved button layout.The addition of horizontal centering and bottom margin improves the visual layout of the error window buttons.
Common_glTF_Exporter/Utils/RevitIniReader.cs (1)
42-49: Path cleaning logic is sound.The double trimming and quote removal is intentional and correct:
- Line 42 removes outer quotes from the entire segment
- Line 49 removes quotes from individual sub-paths after splitting
The pipe-to-semicolon conversion effectively handles illegal path characters.
| if (Path.GetInvalidPathChars().Any(c => trimmedPath.Contains(c))) | ||
| { | ||
| // Skip or log invalid path | ||
| Console.WriteLine($"Skipping invalid path: {trimmedPath}"); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Invalid character validation may be insufficient, and logging is inappropriate.
Two concerns:
-
Path.GetInvalidPathChars()only returns null and control characters (0-31), but does NOT include characters like<,>,*,?that are invalid in Windows paths. While the code handles|and", other invalid characters might slip through and causePath.GetFullPath()to throw on lines 59-60. -
Console.WriteLineis inappropriate for Revit plugins, as there's no console and the message will be lost. Consider using a proper logging framework or the Revit API's logging mechanisms.
Verify the sufficiency of the current validation:
🏁 Script executed:
#!/bin/bash
# Check if there's error handling around Path.GetFullPath calls
rg -nP --type=cs -C3 'Path\.GetFullPath\(' -g '!**/test/**'Length of output: 1455
---
🌐 Web query:
```web
Path.GetInvalidPathChars C# .NET what characters does it return
💡 Result:
Path.GetInvalidPathChars returns a char[] clone of the internal RealInvalidPathChars array — on Windows .NET that list is:
- '"' (double quote)
- '<' (less-than)
- '>' (greater-than)
- '|' (vertical bar)
- '\0' (null)
- control characters U+0001 through U+001F
(So: '"', '<', '>', '|', '\0', and chars 1..31). The exact set can vary by platform and file system and the method's docs say it is not guaranteed to be complete. [1][2][3]
Sources:
[1] .NET Path.cs source (RealInvalidPathChars and InvalidFileNameChars lists).
[2] Microsoft docs: Path.GetInvalidPathChars (remarks about not guaranteed complete).
[3] Discussion / examples (Stack Overflow).
Invalid character validation is incomplete, and logging is inappropriate for Revit plugins.
Two concerns:
-
Path.GetInvalidPathChars()returns",<,>,|, null, and control characters, but does NOT include*or?, which are also invalid in Windows paths. Characters that slip through validation could causePath.GetFullPath()on lines 59–60 to throw an exception. Currently there is no try-catch around these calls to handle failures gracefully. -
Console.WriteLine()is inappropriate for Revit plugins, as there's no console and messages are lost. Consider using a proper logging framework or the Revit API's logging mechanisms.
| string fullPath = Path.IsPathRooted(trimmedPath) | ||
| ? Path.GetFullPath(trimmedPath) | ||
| : Path.GetFullPath(Path.Combine(userProfile, trimmedPath)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add exception handling for Path.GetFullPath().
Path.GetFullPath() can throw ArgumentException, PathTooLongException, NotSupportedException, and ArgumentNullException even after validation. If an invalid path slips through the validation at line 51, this will cause unhandled exceptions.
Wrap the path resolution in a try-catch block:
- string fullPath = Path.IsPathRooted(trimmedPath)
- ? Path.GetFullPath(trimmedPath)
- : Path.GetFullPath(Path.Combine(userProfile, trimmedPath));
-
- absolutePaths.Add(fullPath);
+ try
+ {
+ string fullPath = Path.IsPathRooted(trimmedPath)
+ ? Path.GetFullPath(trimmedPath)
+ : Path.GetFullPath(Path.Combine(userProfile, trimmedPath));
+
+ absolutePaths.Add(fullPath);
+ }
+ catch (Exception ex) when (ex is ArgumentException || ex is PathTooLongException || ex is NotSupportedException)
+ {
+ // Log using appropriate Revit logging mechanism
+ // For now, skip the invalid path
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In Common_glTF_Exporter/Utils/RevitIniReader.cs around lines 58 to 60, the
Path.GetFullPath calls can still throw ArgumentException, PathTooLongException,
NotSupportedException or ArgumentNullException; wrap the path resolution in a
try-catch that catches those exceptions (or Exception if you prefer broader
safety), log or record the failure with the offending trimmedPath and exception
message, and skip or continue processing that entry (e.g., treat fullPath as
invalid and do not add it to results) so the parser does not crash when an
invalid path slips through validation.
Summary by CodeRabbit
Style
Bug Fixes