[dashboard] Fix JSONPath crash on Tenant details with resourceQuotas#2249
[dashboard] Fix JSONPath crash on Tenant details with resourceQuotas#2249sircthulhu wants to merge 2 commits intomaincozystack/cozystack:mainfrom fix/dashboard-flatmap-jsonpath-crashcozystack/cozystack:fix/dashboard-flatmap-jsonpath-crashCopy head branch name to clipboard
Conversation
…ders When viewing Tenant details with resourceQuotas, the "Used" column references $.status.used[_flatMapData_Key]. If the flatMap placeholder is not resolved (key missing from expanded row), the raw placeholder is passed to the JSONPath parser which crashes with a parse error. The original flatmap-dynamic-key.diff patch that handled this was removed during the 1.4.0 upgrade, assuming upstream included the fix. Upstream adopted the reordering (flatMap before customFields) but not the fallback protection. Add a patch that checks for unresolved _flatMap*_Key placeholders after regex substitution and returns null instead of calling jp.query() with an invalid expression. Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughA conditional guard was added to JSONPath querying in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the dashboard's Tenant details page, preventing a crash that occurred when resource quotas were present. The fix involves enhancing the JSONPath resolution logic to gracefully handle cases where dynamic placeholders remain unresolved, ensuring the application's stability and a smoother user experience. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request aims to fix a crash on the Tenant details page. While the approach is correct, the implemented patch contains a regular expression with a syntax error that will cause the application to crash. I've provided a critical review comment with a corrected and more robust version of the regular expression that fixes the syntax error and makes the check more specific to the issue at hand.
| } | ||
| - const jpQueryResult = jp.query(el, `$${resolvedJsonPath}`) | ||
| - fieldValue = Array.isArray(jpQueryResult) && jpQueryResult.length === 1 ? jpQueryResult[0] : jpQueryResult | ||
| + if (/_flatMap[^[\]]+_Key/.test(resolvedJsonPath)) { |
There was a problem hiding this comment.
This regular expression contains a syntax error and will cause the application to fail. The character class [^[[]] is invalid in JavaScript. To create a character class that matches any character except for [ or ], you must escape the brackets, like this: [^\[\]].
Additionally, to make this check more robust, it's better to match the specific pattern [<placeholder>] that causes the JSONPath parser to crash, rather than just the placeholder name itself. This avoids potential false positives.
The following suggestion corrects the syntax and makes the check more specific.
if (/\S*\_flatMap[^[\]]+_Key\S*/.test(resolvedJsonPath)) {
Use [^\]]+ (same as existing code in utils.ts) instead of [^[\]]+ for consistency. Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
What this PR does
Fixes a crash on the Tenant details page when
resourceQuotasare configured. The dashboard renders a ResourceQuota table with a "Used" column that references$.status.used[_flatMapData_Key]. When the flatMap placeholder is not resolved during initial render, the raw placeholder is passed to the JSONPath parser, causing:The original
flatmap-dynamic-key.diffpatch that handled this was removed during the 1.4.0 dashboard upgrade, assuming upstream included the fix. Upstream adopted the code reordering (flatMap expansion before customFields resolution) but not the fallback protection for unresolved placeholders.Adds a patch to
openapi-k8s-toolkitthat checks for unresolved_flatMap*_Keyplaceholders after regex substitution and returnsnullinstead of callingjp.query()with an invalid expression.Release note
Summary by CodeRabbit