-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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(dashboard): Add safety checks to areas that throw runtime errors in dashboard filters #22648
fix(dashboard): Add safety checks to areas that throw runtime errors in dashboard filters #22648
Conversation
…nentId is present in dashboard data structure and add unit tests for this case
Codecov Report
@@ Coverage Diff @@
## master #22648 +/- ##
=======================================
Coverage 67.03% 67.03%
=======================================
Files 1859 1859
Lines 71043 71054 +11
Branches 7776 7783 +7
=======================================
+ Hits 47622 47631 +9
+ Misses 21397 21396 -1
- Partials 2024 2027 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
title: 'All panels', | ||
}; | ||
|
||
const layout: Layout = { |
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.
Could/should this be moved to a separate fixtures type of file to make this test file more readable?
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.
@lyndsiWilliams that is a great point. I am waiting to hear back on timing as this is desired for a patch asap. Depending on time I will either make change in this PR, or I will do a small refinement follow on with the change so we are not blocked waiting on the git checks to re-run.
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.
Good idea, @lyndsiWilliams. I'm going to approve and merge so that we can get this bug fix in, and @eric-briscoe will follow up with a new PR to clean up the test.
title: 'All panels', | ||
}; | ||
|
||
const layout: Layout = { |
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.
Good idea, @lyndsiWilliams. I'm going to approve and merge so that we can get this bug fix in, and @eric-briscoe will follow up with a new PR to clean up the test.
…in dashboard filters (apache#22648) (cherry picked from commit 02e8511)
🏷️ preset:2023.01 |
SUMMARY
There are cases where invalid componentIds have been present in dashboard data structures passed through this utility function. This PR adds runtime safety checks in the code to avoid runtime errors in cases where we can safely ignore and id that does not return a component and other cases that could throw an uncaught error disrupting recursion and putting application in failure state.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
This is a fix that does not have easily reproducible steps, but new added unit tests can create the error case seen in production system and verify these changes address the issue.
General testing should be done around creating, editing, and deleting dashboard filters and behavior should work as it did before these changes, no new errors should be present
ADDITIONAL INFORMATION