Repository navigation
Conversation
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
|
@mohityadav766, could you review the per-service decryption fallback? It preserves service identities and pagination when one connection cannot decrypt, while leaving stored configuration unchanged. The description includes unit and API results for both database/search profiles. |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
| protected ResultList<T> decryptOrNullify( | ||
| SecurityContext securityContext, ResultList<T> services) { | ||
| listOrEmpty(services.getData()).forEach(service -> decryptOrNullify(securityContext, service)); | ||
| if (services.getData() != null) { |
There was a problem hiding this comment.
Thanks, updated this to use nullOrEmpty. The null and empty list tests and the affected backend checks pass.
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Code Review ✅ Approved 1 closed / 1 findings🟡 Medium risk · List endpoints now return broken services with null connections; clients could mistake undecryptable configuration for an intentionally absent connection. Keeps service lists available when a service connection cannot be decrypt by returning the affected service with its connection cleared, while healthy services and single-service reads stay unchanged. The previously missing record of the list-versus-single-read connection fallback rule is now added to the tree. No open issues remain. ✅ 1 closed✅ Quality: List vs. single-read connection fallback rule is not recorded in the tree
VerificationRules No rules evaluated Functional validation Not enabledAuto-approval Not enabledFor more verification details, comment OptionsDisplay: compact → Showing compact verification results. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
This branch has not been deployed
2 participants
Describe your changes:
Fixes #31887
One service with an undecryptable connection currently fails the entire list request. Handle
InvalidServiceConnectionExceptionper list entry and return that service with its connection cleared. Healthy services remain usable, and the broken service remains identifiable so its configuration can be repaired.Suggested reviewers
@mohityadav766.
Type of change:
High-level design:
ADR:2026-10-17-service-list-connection-fallback records the deliberate list-versus-single-service error contract, the response-only fallback and its repair implications.
Use each service resource's existing
nullifyConnectionhook. Keep the originalResultListenvelope, entry order, pagination and permissions, including replacement objects returned by the hook. Log the service category and UUID without the exception or secret-bearing details. Single-service reads, writes and unrelated errors retain their existing behavior; the fallback does not persist changes.The tests include all service categories. The MCP placeholder becomes a real HTTP/API-key masking test, and the Overview fixture now creates the consumer principal its test client already uses. Metadata resilience cases run in isolation because that endpoint ignores domain filtering and reports a global total.
Tests:
Use cases covered
Unit tests
ServiceEntityResourceTest: 6 passed, zero failures/skips. The original implementation produced two intended errors from the unhandled list exception.JaCoCo measured 56/58 lines, 96.55% in the changed production class. The changed list method and helper have 100% line coverage.
Backend integration tests
Added
ServiceConnectionResilienceIT; updatedMcpServiceResourceITandServicesOverviewResourceIT.Both engine pairs ran the same 25 selected API classes:
All 19 new resilience invocations, the new MCP smoke test and 27 Overview cases pass on both profiles. These owned-case counts were checked by testcase classname across all XML reports because concurrent Failsafe reports can credit tests to another suite.
The MySQL run used the repository affected-test runner:
make java_affected_run ARGS='--add-area services --reason "shared service list fallback affects all service categories" --keep-going'The PostgreSQL run used the same selection and parallel lane with
-Ppostgres-opensearch. Exact Maven commands and per-class results are included below. The nightly-onlyServiceDeleteSearchCleanupScaleITwas not run locally.Local Java test run: PASSED
4e57bca9ba27(baseorigin/main) — uncommitted changes were presentThe tested files were copied byte for byte into this PR's isolated branch. The generated block records the original validation checkout rather than claiming a later commit was the test-run HEAD.
The review follow-up at
a4f47a6117a209b343bb90bb9a189a1937c4cbd8adds only the decision record and one Java comment. All four regression test files remain byte-identical to the validated commit, and removing that exact comment reproduces the validated production file byte for byte. Executable behavior is unchanged. Fresh Java 21 Spotless apply/check, decision-record validation, impact-map ownership and enabled commit hooks passed; the planner still selects the same 25 API classes. The completed unit/API results above remain applicable, so those suites were not repeated for this documentation-only follow-up.Final
spotless:apply spotless:check, impact-map ownership andgit diff --checkpassed. No workflow, schema or impact-map change is needed.Selected classes, report counts and commands
Local Java test run: PASSED — details
4e57bca9ba27(baseorigin/main) — uncommitted changes were presentTests run locally
ServiceEntityResourceTest(6 passed)APIServiceResourceIT(78 passed, 15 skipped),BaseEntityIT$ListEntityHistoryByTimestampPaginationTest(2 passed, 3 skipped),ConfigResourceIT(5 passed),DashboardServiceResourceIT(58 passed, 11 skipped),DatabaseServiceResourceIT(1 passed, 1 skipped),DatabaseSmokeIT(6 passed),DriveServiceResourceIT(52 passed, 8 skipped),GlossaryTermResourceIT(154 passed, 3 skipped),LLMServiceResourceIT(27 passed, 5 skipped),McpServiceResourceIT(13 passed),MessagingServiceResourceIT(365 passed, 55 skipped),MetadataServiceResourceIT(43 passed, 7 skipped),MlModelServiceResourceIT(60 passed, 9 skipped),MlModelServiceSearchAliasIT(4 passed),PipelineServiceResourceIT(421 passed, 70 skipped),SearchServiceResourceIT(70 passed, 14 skipped),SecurityServiceResourceIT(62 passed, 9 skipped),SeedDataPresenceIT(2 passed),ServiceAttributePolicyIT(26 passed, 4 skipped),ServiceConnectionResilienceIT(19 passed),ServiceLineagePipelineRoutingMigrationIT(8 passed),ServiceLineageRefcountIT(56 passed, 6 skipped),ServicesOverviewResourceIT(70 passed, 7 skipped),StorageServiceResourceIT(294 passed, 48 skipped),TableResourceIT(166 passed, 6 skipped),UserResourceIT(335 passed, 38 skipped)Per-class counts come from the test reports, which can credit a test to the wrong class when classes run concurrently, and keep only the last run of a nested class that several test classes inherit. The summary's step totals are Maven's own counts.
Why these tests
added by author: shared service list fallback affects all service categories←area servicesarea services←openmetadata-service/src/main/java/org/openmetadata/service/resources/services/ServiceEntityResource.javacalled from entity APIService←openmetadata-service/src/main/java/org/openmetadata/service/resources/services/ServiceEntityResource.javacalled from entity DashboardService←openmetadata-service/src/main/java/org/openmetadata/service/resources/services/ServiceEntityResource.javacalled from entity DatabaseService←openmetadata-service/src/main/java/org/openmetadata/service/resources/services/ServiceEntityResource.javacalled from entity DriveService←openmetadata-service/src/main/java/org/openmetadata/service/resources/services/ServiceEntityResource.javacalled from entity LLMService←openmetadata-service/src/main/java/org/openmetadata/service/resources/services/ServiceEntityResource.javacalled from entity McpService←openmetadata-service/src/main/java/org/openmetadata/service/resources/services/ServiceEntityResource.javacalled from entity MessagingService←openmetadata-service/src/main/java/org/openmetadata/service/resources/services/ServiceEntityResource.javacalled from entity MetadataService←openmetadata-service/src/main/java/org/openmetadata/service/resources/services/ServiceEntityResource.javacalled from entity MlModelService←openmetadata-service/src/main/java/org/openmetadata/service/resources/services/ServiceEntityResource.javacalled from entity PipelineService←openmetadata-service/src/main/java/org/openmetadata/service/resources/services/ServiceEntityResource.javacalled from entity SearchService←openmetadata-service/src/main/java/org/openmetadata/service/resources/services/ServiceEntityResource.javacalled from entity SecurityService←openmetadata-service/src/main/java/org/openmetadata/service/resources/services/ServiceEntityResource.javacalled from entity StorageService←openmetadata-service/src/main/java/org/openmetadata/service/resources/services/ServiceEntityResource.javachanged test←openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/McpServiceResourceIT.java,openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/ServiceConnectionResilienceIT.java,openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/ServicesOverviewResourceIT.java,openmetadata-service/src/test/java/org/openmetadata/service/resources/services/ServiceEntityResourceTest.javasmoke←code other than a test class changedImpacted, not run locally (1)
ServiceDeleteSearchCleanupScaleIT→ nightly: openmetadata-nightly k8s-java-it (scale-it)Commands
PostgreSQL/OpenSearch rerun of the same selection, 3,396 tests including 409 skips, zero failures/errors:
Ingestion integration tests
Not applicable, no ingestion changes.
Playwright (UI) tests
Not applicable, no UI changes.
NOT NEEDED for this backend-only PR.
Manual testing performed
Scripted a standalone HTTP/storage/recovery probe against the refreshed source-built server. Created a unique domain and three password-bearing services, corrupted one test-owned stored password and confirmed the list returned HTTP 200 with all three identities. The healthy connections remained available and the broken connection was absent. Its single-service GET still returned HTTP 400. The probe verified unchanged stored JSON, restored the exact snapshot and deleted its test entities. Before the fix, the same corrupted list returned HTTP 400.
UI screen recording:
Not applicable, backend-only bug fix.
Checklist:
Fixes <issue-number>: <short explanation>.Fixes #31887.docs/decisions/2026-10-17-service-list-connection-fallback.mdand cited it fromdecryptServiceForList; lists retain broken service identities with a cleared connection, while single-service reads and writes remain strict.
The PR appears safe to merge.Summary
Service lists keep services whose connections cannot decrypt, with their connections cleared. Single-service operations remain strict.
Reviews (2) · Last reviewed commit: "Fixes 31887: Use nullOrEmpty for service..." · Reviewed by Greptile