Fixes #24833: Validate destination configuration in testDestination and event subscriptions

[manerow]

Fixes #24833

I worked on adding validation logic to the Event Subscription endpoints to ensure destination configurations are correct before processing. Previously, the testDestination API would return a 200 OK success status even if the configuration (e.g., email recipients) was empty, leading to silent failures.

Changes:

• Implemented validateDestinationConfig in EventSubscriptionResource to enforce checks for:
  • Email: Requires at least one receiver and valid email formats.
  • Webhook/Slack/Teams/GChat: Requires a valid, non-empty endpoint URL.
• Updated sendTestMessageAlert, create, and createOrUpdate to call this validation.
• Updated CatalogGenericExceptionMapper to correctly propagate WebApplicationException (ensuring 400 Bad Request is returned instead of 500).
• Added unit tests in EventSubscriptionResourceTest to cover scenarios like null configs, empty receiver lists, and invalid URL/email formats.

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.

• I have added a test that covers the exact scenario we are fixing. For complex issues, comment the issue number in the test for future reference.

[github-actions]

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.

[sonarqubecloud]

Quality Gate passed for 'open-metadata-ingestion'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[gitar-bot]

🔍 CI failure analysis for 0dba9cd: Maven-sonarcloud-ci job failed with 3 test failures: 2 are directly related to the PR's validation and exception handling improvements (IngestionPipelineLogStreamingResourceTest expecting 415 status code, EventSubscriptionResourceTest timeout due to new validation), and 1 unrelated glossary CSV import/export issue.

Issue

Maven SonarCloud CI job failed with 3 test failures

Root Cause

Two failures directly related to PR changes, one unrelated:

1. IngestionPipelineLogStreamingResourceTest (RELATED ✓)

Test: IngestionPipelineLogStreamingResourceTest.testWriteLogsWithDefaultStorage:175

Error:

Expected OK, NOT_IMPLEMENTED, or INTERNAL_SERVER_ERROR but got: 415

Cause: This is the same failure appearing in 4 CI jobs now (integration tests + maven builds). The PR's CatalogGenericExceptionMapper fix now correctly exposes HTTP 415 (Unsupported Media Type) instead of masking it as a 500 error.

The Fix: Update test assertion to accept 415 as valid:

true(response.getStatus() == Response.Status.UNSUPPORTED_MEDIA_TYPE.getStatusCode())

2. EventSubscriptionResourceTest (RELATED ✓)

Test: EventSubscriptionResourceTest.put_updateEndpointURL:208

Error:

ConditionTimeout Lambda expression expected (<true> or <true>) but was <false> within 10 seconds

Cause: The PR added validateDestinationConfig to enforce URL validation in EventSubscriptionResource.createOrUpdate. The test is now timing out because it's waiting for a condition that fails due to the new validation logic.

The Fix: Review the test to ensure it provides valid configuration data (non-empty URLs, proper formats) that passes the new validation requirements in validateDestinationConfig.

3. GlossaryResourceTest (UNRELATED ✗)

Test: GlossaryResourceTest.testGlossaryImportExport:1070

Error:

expected CSV fields: glossaryTermDateCp, glossaryTermDateTimeCp, glossaryTermDurationCp, glossaryTermEmailCp
but was: missing these custom property fields in export

Analysis: This is a glossary CSV import/export test failure with no connection to event subscription validation or exception handling. The PR changes event subscription endpoints and exception mapping - completely separate from glossary import/export functionality.

Details

Jobs Affected by This PR's Changes:

• 4 jobs failing with IngestionPipelineLogStreamingResourceTest (all need same fix)
• 1 job failing with EventSubscriptionResourceTest (needs validation review)

Total CI Landscape:

• 2 test types RELATED to PR (expected from validation/exception improvements)
• 6 failures UNRELATED to PR (1 glossary test, 3 Python deps, 2 flaky UI tests)

Impact Assessment:
The two PR-related test failures are expected consequences of the improvements:

1. Better error propagation (415 instead of 500)
2. Stricter validation enforcement

Both can be resolved by updating the tests to align with the new, more correct behavior.

Code Review 👍 Approved with suggestions

Good validation addition that addresses the original issue #24833, but two previous findings remain unresolved: missing switch default case and silently swallowed test message errors.

⚠️ Edge Case: Test message errors are silently swallowed, returning 200 OK

📄 openmetadata-service/src/main/java/org/openmetadata/service/resources/events/subscription/EventSubscriptionResource.java

In sendTestMessageAlert, when alert.sendTestMessage() throws an EventPublisherException, the exception is caught and only logged. The method then returns 200 OK with whatever destinations succeeded. If all destinations fail, the response will be an empty list with 200 OK, which doesn't indicate failure to the caller.

This partially defeats the purpose of the validation fix - while invalid configs are now caught upfront, actual delivery failures still result in silent success.

Suggestion: Consider either:

1. Returning an error response if no destinations succeeded
2. Including failure details in the response body (e.g., a map of destination -> success/failure status)

Example fix for option 1:

if (resultDestinations.isEmpty()) {
    throw new WebApplicationException(
        "All test messages failed to send",
        Response.Status.INTERNAL_SERVER_ERROR);
}

More details 💡 1 suggestion

💡 Edge Case: validateDestinationConfig missing default case in switch

📄 openmetadata-service/src/main/java/org/openmetadata/service/resources/events/subscription/EventSubscriptionResource.java

The switch statement in validateDestinationConfig handles specific destination types but has no default case. If a new SubscriptionType is added to the enum in the future, it will silently pass validation without any checks.

Suggestion: Add a default case that either logs a warning or throws an exception for unknown types:

switch (destination.getType()) {
    case EMAIL -> validateEmailConfig(config);
    case WEBHOOK, SLACK, MS_TEAMS, G_CHAT -> validateWebhookConfig(config);
    case ACTIVITY_FEED, GOVERNANCE_WORKFLOW_CHANGE_EVENT -> {}
    default -> LOG.warn("No validation implemented for destination type: {}", destination.getType());
}

What Works Well

Clear validation logic for email and webhook configurations with specific error messages. Proper null check added to EventSubscriptionMapper. Good test coverage for validation scenarios.

Recommendations

Consider adding a default case to the switch statement in validateDestinationConfig that throws an exception for unknown destination types, to prevent silent failures if new types are added. For the test message endpoint, consider returning a structured response that includes both successful and failed destinations with error details, rather than silently ignoring failures.

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