Skip to content

Conversation

@harshach
Copy link
Collaborator

@harshach harshach commented Jan 8, 2026

Describe your changes:

Fixes

I worked on ... because ...

Type of change:

  • Bug fix
  • Improvement
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.

Summary by Gitar

  • Database migrations:
    • Added SQL schema changes in 1.10.1 and 1.12.0 migrations for MySQL and PostgreSQL to support audit log enhancements
  • Backend audit log improvements:
    • Enhanced AuditLogRepository and AuditLogResource with permission checks and filtering capabilities
    • Added async CSV export via CSVExportMessage and WebsocketNotificationHandler for large audit log exports
  • Frontend UI refactor:
    • New modular audit log components: AuditLogFilters and AuditLogList with dedicated interfaces and styles
    • Updated AuditLogsPage with improved UX including date range filters and export modal
  • Comprehensive test coverage:
    • Added 265+ lines of integration tests in AuditLogResourceIT.java covering authorization and security
    • Added 241+ lines of E2E tests in AuditLogs.spec.ts for export and filter workflows
    • Added 812+ lines of frontend unit tests in AuditLogsPage.test.tsx including XSS prevention
  • Internationalization:
    • Updated 14 language files with audit log-related translations

This will update automatically on new commits.


@github-actions
Copy link
Contributor

github-actions bot commented Jan 8, 2026

The Java checkstyle failed.

Please run mvn spotless:apply in the root of your repository and commit the changes to this PR.
You can also use pre-commit to automate the Java code formatting.

You can install the pre-commit hooks with make install_test precommit_install.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 8, 2026

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 65%
65.24% (52213/80033) 43.12% (25815/59869) 46.51% (8091/17396)

* Change UUID fields to type UUID from String

* Fix Row Mapper
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 8, 2026

const handleExport = (exportResponse: string) => {
if (exportResponse) {
const exportResponseData = JSON.parse(
exportResponse
Copy link

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
    }
  }
};

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 8, 2026

@gitar-bot
Copy link

gitar-bot bot commented Jan 8, 2026

🔍 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.

Issue

The TestCaseResourceTest.test_userAssignmentRemovedAfterUserDeletion test failed with an HTTP 500 error.

Root Cause

The test is encountering a relationship validation error:

Entity type testCaseResolutionStatus f1da995a-ef47-447e-a954-9b117293cd4f does not have expected relationship parentOf to/from entity type testCase

This error occurs in TestCaseResourceTest.java:5542 when calling getTestCaseFailureStatusByStateId.

Details

The failure is in:

  • Test: TestCaseResourceTest.lambda$test_userAssignmentRemovedAfterUserDeletion$117
  • Location: openmetadata-service/src/test/java/org/openmetadata/service/resources/dqtests/TestCaseResourceTest.java:5542
  • Error: The test expects to retrieve test case failure status, but the relationship between testCaseResolutionStatus and testCase entities appears to be missing or invalid

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 suggestions

Comprehensive audit log refactor with new filtering, search, and export capabilities. Previous JSON.parse issue in WebSocket handler remains unresolved.

Suggestions 💡 2 suggestions
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 uses JSON.parse(exportResponse) without a try-catch block. If the server sends a malformed JSON message (due to network issues, server bugs, or other edge cases), this will throw an unhandled exception and potentially break the export progress UI.

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 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
    }
  }
};
Resolved ✅ 2 resolved
Edge Case: WebSocket export cleanup not called when component unmounts

📄 openmetadata-ui/src/main/resources/ui/src/pages/AuditLogsPage/AuditLogsPage.tsx:230-260
In AuditLogsPage.tsx, when the component unmounts during an active export, the exportJobRef is not cleaned up and could lead to stale references. The useEffect cleanup function removes the socket listener, but exportJobRef.current still points to the old job. If the user navigates away and back, this could cause issues with overlapping exports.

Consider adding cleanup for the ref in the useEffect or in a separate cleanup effect:

useEffect(() => {
  return () => {
    exportJobRef.current = null;
  };
}, []);
Bug: WebSocket listener cleanup may leak if socket ref changes

📄 openmetadata-ui/src/main/resources/ui/src/pages/AuditLogsPage/AuditLogsPage.tsx
The WebSocket event listener for CSV export uses handleExport callback which is registered via socket?.on(). The cleanup function in useEffect calls socket?.off() to remove the listener. However, based on the test file showing mockSocketOn and mockSocketOff being called, the handleExport function reference is captured in closure.

If the handleExport function reference changes between renders (due to dependencies like exportJobRef, isExporting, etc.), the socket?.off(SOCKET_EVENTS.CSV_EXPORT_CHANNEL, handleExport) call in cleanup may not properly unregister the previously registered handler because it references a different function instance.

Suggested fix: Use a stable callback reference with useRef for the handler, or ensure handleExport is wrapped in useCallback with correct dependencies and that the cleanup captures the same function reference that was registered:

// Option 1: Use a ref for stable callback
const handleExportRef = useRef<(data: string) => void>();
handleExportRef.current = (exportResponse: string) => {
  // ... handler logic
};

useEffect(() => {
  const handler = (data: string) => handleExportRef.current?.(data);
  socket?.on(SOCKET_EVENTS.CSV_EXPORT_CHANNEL, handler);
  return () => {
    socket?.off(SOCKET_EVENTS.CSV_EXPORT_CHANNEL, handler);
  };
}, [socket]);

What Works Well

Clean 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.

Recommendations

Add 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 Gitar fix CI or enable auto-apply: gitar auto-apply:on

Options

Auto-apply is off Gitar will not commit updates to this branch.
Display: compact Hiding non-applicable rules.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | This comment will update automatically (Docs)

@harshach harshach merged commit 4c3f6dd into main Jan 8, 2026
32 of 38 checks passed
@harshach harshach deleted the fix_audit_logs branch January 8, 2026 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend safe to test Add this label to run secure Github workflows on PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants