-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Fix audit logs #25127
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 audit logs #25127
Conversation
|
The Java checkstyle failed. Please run You can install the pre-commit hooks with |
* Change UUID fields to type UUID from String * Fix Row Mapper
openmetadata-ui/src/main/resources/ui/src/pages/AuditLogsPage/AuditLogsPage.tsx
Show resolved
Hide resolved
|
| const handleExport = (exportResponse: string) => { | ||
| if (exportResponse) { | ||
| const exportResponseData = JSON.parse( | ||
| exportResponse |
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.
💡 Edge Case: Missing try-catch around JSON.parse in WebSocket handler
Details
The WebSocket message handler in handleExport parses the incoming message using JSON.parse without error handling:
const exportResponseData = JSON.parse(
exportResponse
) as CSVExportWebsocketResponse;If the WebSocket receives malformed JSON (network issues, server bugs, or unexpected message types on the channel), this will throw an exception that could crash the component or leave the export UI in an inconsistent state.
Suggested fix: Wrap the JSON.parse in a try-catch:
const handleExport = (exportResponse: string) => {
if (exportResponse) {
try {
const exportResponseData = JSON.parse(
exportResponse
) as CSVExportWebsocketResponse;
handleExportWebSocketMessage(exportResponseData);
} catch (e) {
console.error('Failed to parse WebSocket export message:', e);
// Optionally show error to user or reset export state
}
}
};This reverts commit af71a45.
|
🔍 CI failure analysis for 4e6a21b: SonarCloud CI failed due to a test error in TestCaseResourceTest related to entity relationship validation after the PR's changes to audit logs.IssueThe Root CauseThe test is encountering a relationship validation error: This error occurs in DetailsThe failure is in:
The build ran for over 3 hours before failing this test. The logs show extensive entity relationship cleanup testing was passing, but this specific test about user deletion and its impact on test case assignments is failing. This could be related to the PR's audit log changes if they affect how entity relationships are tracked or cleaned up during user deletion operations. Code Review 👍 Approved with suggestionsComprehensive audit log refactor with new filtering, search, and export capabilities. Previous JSON.parse issue in WebSocket handler remains unresolved. Suggestions 💡 2 suggestionsEdge Case: Missing try-catch around JSON.parse in WebSocket handler📄 openmetadata-ui/src/main/resources/ui/src/pages/AuditLogsPage/AuditLogsPage.tsx:252-256 The WebSocket message handler uses Impact: Export progress would fail silently with a console error if malformed data is received. Suggested fix: const handleExport = (exportResponse: string) => {
if (exportResponse) {
try {
const exportResponseData = JSON.parse(exportResponse) as CSVExportWebsocketResponse;
handleExportWebSocketMessage(exportResponseData);
} catch (error) {
LOG.error('Failed to parse export WebSocket message:', error);
// Optionally show an error toast or update export status to failed
}
}
};Edge Case: Missing try-catch around JSON.parse in WebSocket handler📄 openmetadata-ui/src/main/resources/ui/src/pages/AuditLogsPage/AuditLogsPage.tsx:252-256 The WebSocket message handler in const exportResponseData = JSON.parse(
exportResponse
) as CSVExportWebsocketResponse;If the WebSocket receives malformed JSON (network issues, server bugs, or unexpected message types on the channel), this will throw an exception that could crash the component or leave the export UI in an inconsistent state. Suggested fix: Wrap the JSON.parse in a try-catch: const handleExport = (exportResponse: string) => {
if (exportResponse) {
try {
const exportResponseData = JSON.parse(
exportResponse
) as CSVExportWebsocketResponse;
handleExportWebSocketMessage(exportResponseData);
} catch (e) {
console.error('Failed to parse WebSocket export message:', e);
// Optionally show error to user or reset export state
}
}
};Resolved ✅ 2 resolvedEdge Case: WebSocket export cleanup not called when component unmounts
Bug: WebSocket listener cleanup may leak if socket ref changes
What Works WellClean modular architecture with separate AuditLogFilters and AuditLogList components. Comprehensive test coverage including unit tests, integration tests, and E2E tests. Well-designed batched export approach to avoid resource contention with concurrent writes. RecommendationsAdd try-catch around the JSON.parse call in the WebSocket handler (handleExport) in AuditLogsPage.tsx to gracefully handle malformed WebSocket messages without crashing the export flow. Tip Comment OptionsAuto-apply is off Gitar will not commit updates to this branch. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | This comment will update automatically (Docs) |



Describe your changes:
Fixes
I worked on ... because ...
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>Summary by Gitar
1.10.1and1.12.0migrations for MySQL and PostgreSQL to support audit log enhancementsAuditLogRepositoryandAuditLogResourcewith permission checks and filtering capabilitiesCSVExportMessageandWebsocketNotificationHandlerfor large audit log exportsAuditLogFiltersandAuditLogListwith dedicated interfaces and stylesAuditLogsPagewith improved UX including date range filters and export modalAuditLogResourceIT.javacovering authorization and securityAuditLogs.spec.tsfor export and filter workflowsAuditLogsPage.test.tsxincluding XSS preventionThis will update automatically on new commits.