-
Notifications
You must be signed in to change notification settings - Fork 75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Disallow trailing commas when uploading options lists #14438
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a modification to the Changes
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 (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/src/Designer/Services/Implementation/OptionsService.cs (1)
119-119
: Consider enhancing error messaging for trailing comma scenarios.The change correctly disallows trailing commas, but when users upload JSON with trailing commas, they'll receive a generic
JsonException
. Consider wrapping the deserialization in a try-catch block to provide a more specific error message about trailing commas not being supported.- new JsonSerializerOptions { WriteIndented = true }); + try + { + new JsonSerializerOptions { WriteIndented = true }); + } + catch (JsonException ex) when (ex.Message.Contains("trailing comma")) + { + throw new InvalidOptionsFormatException("Trailing commas are not supported in options lists. Please remove any trailing commas from your JSON."); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/src/Designer/Services/Implementation/OptionsService.cs
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Analyze
- GitHub Check: Run integration tests against actual gitea and db
🔇 Additional comments (1)
backend/src/Designer/Services/Implementation/OptionsService.cs (1)
119-119
: Verify the change through unit tests.The change to disallow trailing commas is a behavioral change that should be verified through unit tests.
Let's check if there are any existing tests for this scenario:
Would you like me to help create a unit test that verifies the rejection of trailing commas?
Description
We should not allow trailing commas in option lists, because Apps do not support option lists that contain them.
Related Issue(s)
Verification
Summary by CodeRabbit