From fd0e1aaf3d86522292864b4a45d925a9f77bda5f Mon Sep 17 00:00:00 2001 From: mohitdeuex Date: Mon, 27 Jul 2026 17:15:19 +0530 Subject: [PATCH 1/5] Fixes #30117: keep soft-deleted owners in PATCH base to fix index crash Editing owners failed with "array item index out of range" when an owner user was soft-deleted. Detail pages fetch with include=all (soft-deleted owner present) and compute a positional JSON Patch, but the server loaded the PATCH base with NON_DELETED, which dropped that owner and shrank the array so the positional op referenced an index past the end. The dangling ownership also could not be removed. Both patch() entry points now load the original with a RelationIncludes that keeps the entity at NON_DELETED but resolves owners/experts/reviewers/ followers with ALL, so the positional patch stays in range and the ID-based updateOwners diff removes the dangling OWNS row. Adds two integration tests reproducing the reported crash (deleted owner sorting last) and the silent wrong-owner removal (deleted owner in the middle). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../it/tests/TestSuiteResourceIT.java | 100 ++++++++++++++++++ .../service/jdbi3/EntityRepository.java | 20 +++- 2 files changed, 118 insertions(+), 2 deletions(-) diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestSuiteResourceIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestSuiteResourceIT.java index 64ee4ff76ed8..1b80f8f050b7 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestSuiteResourceIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestSuiteResourceIT.java @@ -8,6 +8,7 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import com.fasterxml.jackson.databind.JsonNode; import es.co.elastic.clients.transport.rest5_client.low_level.Request; import es.co.elastic.clients.transport.rest5_client.low_level.Response; import es.co.elastic.clients.transport.rest5_client.low_level.Rest5Client; @@ -25,6 +26,7 @@ import org.junit.jupiter.api.parallel.Execution; import org.junit.jupiter.api.parallel.ExecutionMode; import org.openmetadata.it.bootstrap.TestSuiteBootstrap; +import org.openmetadata.it.factories.UserTestFactory; import org.openmetadata.it.util.SdkClients; import org.openmetadata.it.util.TestNamespace; import org.openmetadata.schema.api.data.CreateTable; @@ -39,6 +41,7 @@ import org.openmetadata.schema.entity.services.ingestionPipelines.PipelineStatus; import org.openmetadata.schema.entity.services.ingestionPipelines.PipelineStatusType; import org.openmetadata.schema.entity.services.ingestionPipelines.PipelineType; +import org.openmetadata.schema.entity.teams.User; import org.openmetadata.schema.metadataIngestion.SourceConfig; import org.openmetadata.schema.metadataIngestion.TestSuitePipeline; import org.openmetadata.schema.tests.DataQualityReport; @@ -386,6 +389,103 @@ void test_testSuiteWithOwner(TestNamespace ns) { assertFalse(fetched.getOwners().isEmpty()); } + // =================================================================== + // SOFT-DELETED OWNER PATCH TESTS (issue #30117) + // + // A detail page loads the entity with include=all, so a soft-deleted owner is present in the + // client's owner array and in the positional JSON Patch it computes. The server used to load the + // PATCH base with NON_DELETED, which dropped that owner and shrank the array — a positional op + // then referenced an index past the end ("array item index out of range") and the dangling + // ownership could never be removed. These tests reproduce that exact client behavior. + // =================================================================== + + @Test + void patch_removeSoftDeletedOwnerSortingLast_removesOwnerWithoutCrash(TestNamespace ns) { + OpenMetadataClient client = SdkClients.adminClient(); + + // Owners are ordered by name; the doomed owner is named to sort LAST so its positional index + // is the highest — the index that goes out of range once the server filters it from the base. + User ownerA = UserTestFactory.createUser(ns, "aaa_owner"); + User ownerB = UserTestFactory.createUser(ns, "mmm_owner"); + User doomed = UserTestFactory.createUser(ns, "zzz_owner"); + + CreateTestSuite request = new CreateTestSuite(); + request.setName(ns.prefix("testsuite_softdeleted_owner")); + request.setOwners( + List.of( + ownerA.getEntityReference(), ownerB.getEntityReference(), doomed.getEntityReference())); + String id = createEntity(request).getId().toString(); + + // The OWNS relationship survives a soft delete, so the user is genuinely still an owner. + client.users().delete(doomed.getId().toString()); + + TestSuite loaded = client.testSuites().get(id, "owners", "all"); + assertEquals( + 3, + loaded.getOwners().size(), + "include=all should still surface the soft-deleted owner the UI diffs against"); + int doomedIndex = ownerIndex(loaded, doomed.getId()); + assertEquals(2, doomedIndex, "soft-deleted owner should sort last"); + + JsonNode patch = + JsonUtils.readTree("[{\"op\":\"remove\",\"path\":\"/owners/" + doomedIndex + "\"}]"); + TestSuite patched = client.testSuites().patch(id, patch); + assertNotNull(patched, "PATCH must not fail with 'array item index out of range'"); + + List ownerIds = ownerIds(client.testSuites().get(id, "owners", "all")); + assertEquals(2, ownerIds.size(), "the dangling soft-deleted ownership should be removed"); + assertFalse(ownerIds.contains(doomed.getId()), "soft-deleted owner should be cleaned up"); + assertTrue( + ownerIds.contains(ownerA.getId()) && ownerIds.contains(ownerB.getId()), + "active owners must be preserved"); + } + + @Test + void patch_removeSoftDeletedOwnerSortingInMiddle_removesCorrectOwner(TestNamespace ns) { + OpenMetadataClient client = SdkClients.adminClient(); + + // The soft-deleted owner sorts in the MIDDLE. Under the old NON_DELETED base the server saw a + // shorter, re-indexed array, so the positional remove silently deleted the wrong (active) owner + // and left the soft-deleted one dangling. With the fix, client and server indexes stay aligned + // and the intended owner is removed. + User first = UserTestFactory.createUser(ns, "aaa_active"); + User doomed = UserTestFactory.createUser(ns, "mmm_doomed"); + User last = UserTestFactory.createUser(ns, "zzz_active"); + + CreateTestSuite request = new CreateTestSuite(); + request.setName(ns.prefix("testsuite_remove_middle_softdeleted")); + request.setOwners( + List.of( + first.getEntityReference(), doomed.getEntityReference(), last.getEntityReference())); + String id = createEntity(request).getId().toString(); + + client.users().delete(doomed.getId().toString()); + + TestSuite loaded = client.testSuites().get(id, "owners", "all"); + int doomedIndex = ownerIndex(loaded, doomed.getId()); + assertEquals(1, doomedIndex, "soft-deleted owner should sort in the middle"); + + JsonNode patch = + JsonUtils.readTree("[{\"op\":\"remove\",\"path\":\"/owners/" + doomedIndex + "\"}]"); + TestSuite patched = client.testSuites().patch(id, patch); + assertNotNull(patched, "PATCH must not fail with 'array item index out of range'"); + + List ownerIds = ownerIds(client.testSuites().get(id, "owners", "all")); + assertFalse( + ownerIds.contains(doomed.getId()), "the soft-deleted owner should be the one removed"); + assertTrue( + ownerIds.contains(first.getId()) && ownerIds.contains(last.getId()), + "both active owners must survive - the positional remove must not hit the wrong index"); + } + + private int ownerIndex(TestSuite suite, UUID ownerId) { + return ownerIds(suite).indexOf(ownerId); + } + + private List ownerIds(TestSuite suite) { + return suite.getOwners().stream().map(EntityReference::getId).collect(Collectors.toList()); + } + @Test void test_testSuiteFQNFormat(TestNamespace ns) { OpenMetadataClient client = SdkClients.adminClient(); diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java index 2294a01a37ca..f51ff3e99470 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java @@ -428,6 +428,22 @@ static final class LoaderRaceException extends RuntimeException { // Fields whose change rewrites a glossary term's FQN during move operations. private static final Set GLOSSARY_TERM_MOVE_FIELDS = Set.of("parent", "glossary"); + // A PATCH's base ("original") must resolve user references the same way the client did when it + // computed the diff. Detail pages fetch with include=all, so a soft-deleted owner (or expert, + // reviewer, follower) is in the client's base. Loading the server base with NON_DELETED drops it + // and shrinks the array, so a positional JSON-Patch op goes out of range ("array item index out + // of range") and the dangling relationship can never be removed. Resolve these relations with ALL + // for the patch base; the entity itself still loads NON_DELETED so a soft-deleted entity is not + // patched through this path. See issue #30117. + private static final RelationIncludes PATCH_ORIGINAL_INCLUDES = + new RelationIncludes( + NON_DELETED, + Map.of( + FIELD_OWNERS, ALL, + FIELD_EXPERTS, ALL, + FIELD_REVIEWERS, ALL, + FIELD_FOLLOWERS, ALL)); + /** * Canonical {@link #CACHE_WITH_NAME} key. User FQNs are lowercased at the DB layer * ({@code UserDAO.findEntityByName}), so the Guava cache must use the same normalization — @@ -4257,7 +4273,7 @@ public final PatchResponse patch( // Get only the fields relevant to this patch operation T original; try (var ignored = phase("patchLoadOriginal")) { - original = get(null, id, patchFields, NON_DELETED, false); + original = get(null, id, patchFields, PATCH_ORIGINAL_INCLUDES, false); } // Validate ETag if If-Match header is provided @@ -4319,7 +4335,7 @@ public final PatchResponse patch( // Get only the fields relevant to this patch operation T original; try (var ignored = phase("patchLoadOriginalByName")) { - original = getByName(null, fqn, patchFields, NON_DELETED, false); + original = getByName(null, fqn, patchFields, PATCH_ORIGINAL_INCLUDES, false); } // Validate ETag if If-Match header is provided From d811cc98a7e772b6889a9aec2876d6703aab552b Mon Sep 17 00:00:00 2001 From: mohitdeuex Date: Tue, 28 Jul 2026 12:55:08 +0530 Subject: [PATCH 2/5] Fixes #30117: tolerate existing soft-deleted owners on PATCH; generic test The first pass loaded the PATCH base with owner/expert/reviewer/follower refs resolved as ALL. That fixed the positional "array item index out of range" crash on owner removal, but getValidatedOwners (which runs on every PATCH) re-validates the whole owner list against NON_DELETED, so ANY edit (e.g. a description change) on an entity carrying a dangling soft-deleted owner then failed with 404 "user instance ... not found". - Narrow the base override to owners only. Experts/reviewers/followers are validated on update against NON_DELETED and would hit the same 404; they are left unchanged as a separate follow-up, per the issue. - Add getValidatedOwnersForPatch: retain owners already assigned in the base (a soft-deleted user is still a valid, existing assignment) and validate only newly added owners against NON_DELETED, so a deleted user still cannot be freshly assigned. - Add a generic BaseEntityIT test so every owner-supporting entity is covered, plus a TestSuite regression test asserting a non-owner edit succeeds and preserves the soft-deleted owner. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../openmetadata/it/tests/BaseEntityIT.java | 85 +++++++++++++++++++ .../it/tests/TestSuiteResourceIT.java | 27 ++++++ .../service/jdbi3/EntityRepository.java | 59 +++++++++---- 3 files changed, 156 insertions(+), 15 deletions(-) diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/BaseEntityIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/BaseEntityIT.java index 6adc2002edff..58eaededf1fd 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/BaseEntityIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/BaseEntityIT.java @@ -26,6 +26,7 @@ import org.junit.jupiter.api.extension.ExtendWith; import org.openmetadata.it.auth.JwtAuthProvider; import org.openmetadata.it.bootstrap.SharedEntities; +import org.openmetadata.it.factories.UserTestFactory; import org.openmetadata.it.util.BulkApi; import org.openmetadata.it.util.EntityValidation; import org.openmetadata.it.util.SdkClients; @@ -820,6 +821,90 @@ void patch_entityChangeOwner_200(TestNamespace ns) { "Owner should be removed"); } + /** + * Test: Editing owners must not crash when one owner is soft-deleted (issue #30117). + * + *

Reproduces the exact client behavior for every owner-supporting entity: a detail page loads + * the entity with include=all (so a soft-deleted owner is still present) and computes a positional + * JSON Patch. The server used to load the PATCH base with NON_DELETED, dropping that owner and + * shrinking the array, so the positional remove referenced an index past the end ("array item + * index out of range") and the dangling ownership could never be removed. + */ + @Test + void patch_removeSoftDeletedOwner_removedWithoutCrash(TestNamespace ns) { + if (!supportsOwners || !supportsPatch) return; + + OpenMetadataClient client = SdkClients.adminClient(); + + // Owners are ordered by name, so the "zzz" owner sorts last and takes the highest positional + // index - the index that goes out of range once the server filters it from a NON_DELETED base. + User activeOwner = UserTestFactory.createUser(ns, "aaa_active_owner"); + User doomedOwner = UserTestFactory.createUser(ns, "zzz_doomed_owner"); + + T created = createEntity(createMinimalRequest(ns)); + String entityPath = entityPath(created.getId().toString()); + + // Assign both owners via an explicit whole-array add patch. This is robust across entities - it + // avoids the SDK update path's per-entity snapshot/diff quirks. + String ownersValue = + JsonUtils.pojoToJson( + List.of(activeOwner.getEntityReference(), doomedOwner.getEntityReference())); + client + .getHttpClient() + .executeForString( + HttpMethod.PATCH, + entityPath, + JsonUtils.readTree( + "[{\"op\":\"add\",\"path\":\"/owners\",\"value\":" + ownersValue + "}]")); + + // The OWNS relationship survives a soft delete, so the user is genuinely still an owner. + client.users().delete(doomedOwner.getId().toString()); + + JsonNode ownersBefore = ownersWithIncludeAll(entityPath); + int doomedIndex = indexOfOwner(ownersBefore, doomedOwner.getId()); + Assumptions.assumeTrue( + indexOfOwner(ownersBefore, activeOwner.getId()) >= 0 && doomedIndex >= 0, + "both explicit owners must be assigned and the soft-deleted one surfaced by include=all"); + + // The exact positional JSON Patch the owner widget computes from that include=all base. Before + // the fix this fails with "array item index is out of range"; executeForString throws on + // non-2xx. + JsonNode patch = + JsonUtils.readTree("[{\"op\":\"remove\",\"path\":\"/owners/" + doomedIndex + "\"}]"); + client.getHttpClient().executeForString(HttpMethod.PATCH, entityPath, patch); + + JsonNode ownersAfter = ownersWithIncludeAll(entityPath); + assertEquals( + -1, + indexOfOwner(ownersAfter, doomedOwner.getId()), + "the dangling soft-deleted owner should be removed"); + assertTrue( + indexOfOwner(ownersAfter, activeOwner.getId()) >= 0, "the active owner must be preserved"); + } + + private String entityPath(String id) { + String base = getResourcePath(); + return base.endsWith("/") ? base + id : base + "/" + id; + } + + private JsonNode ownersWithIncludeAll(String entityPath) { + String body = + SdkClients.adminClient() + .getHttpClient() + .executeForString(HttpMethod.GET, entityPath + "?fields=owners&include=all", null); + return JsonUtils.readTree(body).path("owners"); + } + + private int indexOfOwner(JsonNode owners, UUID ownerId) { + int index = -1; + for (int i = 0; i < owners.size(); i++) { + if (ownerId.toString().equals(owners.get(i).path("id").asText())) { + index = i; + } + } + return index; + } + // =================================================================== // VERSION-BASED CONCURRENCY TESTS (Using entity version for optimistic locking) // =================================================================== diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestSuiteResourceIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestSuiteResourceIT.java index 1b80f8f050b7..62d0e497b529 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestSuiteResourceIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestSuiteResourceIT.java @@ -486,6 +486,33 @@ private List ownerIds(TestSuite suite) { return suite.getOwners().stream().map(EntityReference::getId).collect(Collectors.toList()); } + @Test + void patch_nonOwnerField_withSoftDeletedOwnerPresent_succeeds(TestNamespace ns) { + // Editing an unrelated field must not fail owner re-validation just because a still-assigned + // owner was soft-deleted. The PATCH base now surfaces that owner, so validating the whole owner + // list against NON_DELETED would 404 - existing owners must be tolerated. See issue #30117. + OpenMetadataClient client = SdkClients.adminClient(); + User active = UserTestFactory.createUser(ns, "aaa_active"); + User doomed = UserTestFactory.createUser(ns, "zzz_doomed"); + CreateTestSuite request = new CreateTestSuite(); + request.setName(ns.prefix("testsuite_edit_with_dangling_owner")); + request.setOwners(List.of(active.getEntityReference(), doomed.getEntityReference())); + String id = createEntity(request).getId().toString(); + client.users().delete(doomed.getId().toString()); + + JsonNode patch = + JsonUtils.readTree( + "[{\"op\":\"replace\",\"path\":\"/description\",\"value\":\"edited with dangling owner\"}]"); + TestSuite patched = client.testSuites().patch(id, patch); + assertEquals("edited with dangling owner", patched.getDescription()); + + // The still-assigned soft-deleted owner is preserved (not silently dropped by the edit). + List ownerIds = ownerIds(client.testSuites().get(id, "owners", "all")); + assertTrue( + ownerIds.contains(active.getId()) && ownerIds.contains(doomed.getId()), + "an unrelated edit must preserve existing owners, including the soft-deleted one"); + } + @Test void test_testSuiteFQNFormat(TestNamespace ns) { OpenMetadataClient client = SdkClients.adminClient(); diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java index 574991429dbb..6106507f1555 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java @@ -428,21 +428,16 @@ static final class LoaderRaceException extends RuntimeException { // Fields whose change rewrites a glossary term's FQN during move operations. private static final Set GLOSSARY_TERM_MOVE_FIELDS = Set.of("parent", "glossary"); - // A PATCH's base ("original") must resolve user references the same way the client did when it - // computed the diff. Detail pages fetch with include=all, so a soft-deleted owner (or expert, - // reviewer, follower) is in the client's base. Loading the server base with NON_DELETED drops it - // and shrinks the array, so a positional JSON-Patch op goes out of range ("array item index out - // of range") and the dangling relationship can never be removed. Resolve these relations with ALL - // for the patch base; the entity itself still loads NON_DELETED so a soft-deleted entity is not - // patched through this path. See issue #30117. + // A PATCH's base ("original") must resolve OWNER references the same way the client did when it + // computed the diff. Detail pages fetch with include=all, so a soft-deleted owner is in the + // client's base. A NON_DELETED server base drops it and shrinks the array, so a positional + // JSON-Patch op goes out of range ("array item index out of range") and the dangling ownership + // can never be removed. Resolve owners with ALL for the patch base; the entity itself still loads + // NON_DELETED so a soft-deleted entity is not patched through this path. (Experts / reviewers / + // followers share the latent issue but are validated on update against NON_DELETED, so they are + // left unchanged here - a separate follow-up. See issue #30117.) private static final RelationIncludes PATCH_ORIGINAL_INCLUDES = - new RelationIncludes( - NON_DELETED, - Map.of( - FIELD_OWNERS, ALL, - FIELD_EXPERTS, ALL, - FIELD_REVIEWERS, ALL, - FIELD_FOLLOWERS, ALL)); + new RelationIncludes(NON_DELETED, Map.of(FIELD_OWNERS, ALL)); /** * Canonical {@link #CACHE_WITH_NAME} key. User FQNs are lowercased at the DB layer @@ -4409,7 +4404,7 @@ private PatchResponse patchCommonWithOptimisticLocking( // Validate and populate owners List validatedOwners; try (var ignored = phase("patchValidateOwners")) { - validatedOwners = getValidatedOwners(updated.getOwners()); + validatedOwners = getValidatedOwnersForPatch(original.getOwners(), updated.getOwners()); } updated.setOwners(validatedOwners); @@ -7863,6 +7858,40 @@ protected List getValidatedOwners(List owners) return refs; } + /** + * Owner validation for PATCH. Owners already assigned to the entity (present in {@code + * originalOwners}) are retained even when the user has since been soft-deleted - a deactivated user + * is still a valid, existing assignment. Only newly added owners are validated against {@code + * NON_DELETED}, so a deleted user still cannot be freshly assigned. This keeps unrelated edits + * (description, tags, ...) working on an entity that carries a dangling soft-deleted owner, which + * the PATCH base now surfaces (see {@link #PATCH_ORIGINAL_INCLUDES}). See issue #30117. + */ + private List getValidatedOwnersForPatch( + List originalOwners, List updatedOwners) { + List result = updatedOwners; + boolean skipValidation = + nullOrEmpty(updatedOwners) + || updatedOwners.stream().allMatch(owner -> Boolean.TRUE.equals(owner.getInherited())); + if (!skipValidation) { + Set existingOwnerIds = + listOrEmpty(originalOwners).stream() + .map(EntityReference::getId) + .collect(Collectors.toSet()); + List retainedOwners = + updatedOwners.stream() + .filter(owner -> existingOwnerIds.contains(owner.getId())) + .collect(Collectors.toList()); + List newOwners = + updatedOwners.stream() + .filter(owner -> !existingOwnerIds.contains(owner.getId())) + .collect(Collectors.toList()); + result = new ArrayList<>(retainedOwners); + result.addAll(listOrEmpty(validateOwners(newOwners))); + result.sort(Comparator.comparing(EntityReference::getName)); + } + return result; + } + protected List getValidatedDomains(List domains) { if (nullOrEmpty(domains)) { return domains; From 9d263731c242adc7655fb743290070ae7c9aa389 Mon Sep 17 00:00:00 2001 From: mohitdeuex Date: Tue, 28 Jul 2026 17:57:37 +0530 Subject: [PATCH 3/5] Fixes #30117: address review - retain authoritative owner refs; document tradeoff - getValidatedOwnersForPatch: retained (already-assigned) owners now use the authoritative ref from the base (resolved with ALL, so names are populated) instead of the client-supplied ref, and the sort uses a null-safe comparator. Prevents a possible NPE/500 when a client sends a minimal (id+type, no name) ref for an already-assigned owner, and keeps response metadata authoritative. - Document the known trade-off on PATCH_ORIGINAL_INCLUDES: callers that diff owners against a NON_DELETED view (a GET sending owners:non-deleted) can misalign against the ALL base; the representation-aware fix is the follow-up the issue flags as a design call. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../service/jdbi3/EntityRepository.java | 42 ++++++++++++------- 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java index 6106507f1555..bf1af0ba4e40 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java @@ -428,14 +428,19 @@ static final class LoaderRaceException extends RuntimeException { // Fields whose change rewrites a glossary term's FQN during move operations. private static final Set GLOSSARY_TERM_MOVE_FIELDS = Set.of("parent", "glossary"); - // A PATCH's base ("original") must resolve OWNER references the same way the client did when it - // computed the diff. Detail pages fetch with include=all, so a soft-deleted owner is in the - // client's base. A NON_DELETED server base drops it and shrinks the array, so a positional - // JSON-Patch op goes out of range ("array item index out of range") and the dangling ownership - // can never be removed. Resolve owners with ALL for the patch base; the entity itself still loads - // NON_DELETED so a soft-deleted entity is not patched through this path. (Experts / reviewers / - // followers share the latent issue but are validated on update against NON_DELETED, so they are - // left unchanged here - a separate follow-up. See issue #30117.) + // A PATCH's base ("original") must resolve OWNER references the same way the client diffed + // against. Detail pages fetch with include=all, so a soft-deleted owner is in the client's base; + // a + // NON_DELETED server base drops it and shrinks the array, so a positional JSON-Patch op goes out + // of range ("array item index out of range") and the dangling ownership can never be removed. + // Resolve owners with ALL here; the entity itself still loads NON_DELETED so a soft-deleted + // entity + // is not patched through this path. Trade-off: a client that instead diffs owners against a + // NON_DELETED view (a GET sending includeRelations=owners:non-deleted) can misalign against this + // longer base - the representation-aware fix (the PATCH declaring the base it diffed against) is + // the follow-up the issue flags as a design call. Experts / reviewers / followers share the + // latent + // crash but are validated on update against NON_DELETED, so they are left unchanged. See #30117. private static final RelationIncludes PATCH_ORIGINAL_INCLUDES = new RelationIncludes(NON_DELETED, Map.of(FIELD_OWNERS, ALL)); @@ -7873,21 +7878,28 @@ private List getValidatedOwnersForPatch( nullOrEmpty(updatedOwners) || updatedOwners.stream().allMatch(owner -> Boolean.TRUE.equals(owner.getInherited())); if (!skipValidation) { - Set existingOwnerIds = - listOrEmpty(originalOwners).stream() - .map(EntityReference::getId) - .collect(Collectors.toSet()); + Map existingOwnersById = new HashMap<>(); + for (EntityReference owner : listOrEmpty(originalOwners)) { + existingOwnersById.put(owner.getId(), owner); + } + // Retain owners already assigned, using the authoritative ref from the base (resolved with + // ALL, so a since-soft-deleted owner keeps its populated name/type and is not re-validated). + // Validate only newly added owners against NON_DELETED so a deleted user cannot be freshly + // assigned. List retainedOwners = updatedOwners.stream() - .filter(owner -> existingOwnerIds.contains(owner.getId())) + .filter(owner -> existingOwnersById.containsKey(owner.getId())) + .map(owner -> existingOwnersById.get(owner.getId())) .collect(Collectors.toList()); List newOwners = updatedOwners.stream() - .filter(owner -> !existingOwnerIds.contains(owner.getId())) + .filter(owner -> !existingOwnersById.containsKey(owner.getId())) .collect(Collectors.toList()); result = new ArrayList<>(retainedOwners); result.addAll(listOrEmpty(validateOwners(newOwners))); - result.sort(Comparator.comparing(EntityReference::getName)); + result.sort( + Comparator.comparing( + EntityReference::getName, Comparator.nullsLast(Comparator.naturalOrder()))); } return result; } From d92a21ec0373d763bb2619b38947ec26128f84d4 Mon Sep 17 00:00:00 2001 From: mohitdeuex Date: Wed, 29 Jul 2026 00:23:12 +0530 Subject: [PATCH 4/5] Fixes #30117: hide soft-deleted owners on Test Suite page (align with data assets) Revert the server-side PATCH-base change. Loading the base with owners=ALL fixed the Test Suite crash but regressed the 17+ data-asset detail pages that fetch owners with includeRelations=owners:non-deleted: a positional owner remove computed against the filtered array removed the wrong owner when applied to the longer ALL base. A single server base cannot align with both client owner representations, so the patch itself must carry it - a separate design call the issue flags. Fix the reported crash where the divergence actually is: the Test Suite detail page was the only owner-editing page fetching owners with include=all and no includeRelations override. getTestSuiteByName now defaults owners/experts to non-deleted like every data-asset page, so a soft-deleted owner is not surfaced and the owner-edit diff stays aligned with the server's NON_DELETED PATCH base - no "array item index out of range". Showing and removing soft-deleted owners remains the issue's separate design call. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../openmetadata/it/tests/BaseEntityIT.java | 85 ------------ .../it/tests/TestSuiteResourceIT.java | 127 ------------------ .../service/jdbi3/EntityRepository.java | 63 +-------- .../resources/ui/src/rest/testAPI.test.ts | 35 ++++- .../src/main/resources/ui/src/rest/testAPI.ts | 12 +- 5 files changed, 44 insertions(+), 278 deletions(-) diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/BaseEntityIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/BaseEntityIT.java index 58eaededf1fd..6adc2002edff 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/BaseEntityIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/BaseEntityIT.java @@ -26,7 +26,6 @@ import org.junit.jupiter.api.extension.ExtendWith; import org.openmetadata.it.auth.JwtAuthProvider; import org.openmetadata.it.bootstrap.SharedEntities; -import org.openmetadata.it.factories.UserTestFactory; import org.openmetadata.it.util.BulkApi; import org.openmetadata.it.util.EntityValidation; import org.openmetadata.it.util.SdkClients; @@ -821,90 +820,6 @@ void patch_entityChangeOwner_200(TestNamespace ns) { "Owner should be removed"); } - /** - * Test: Editing owners must not crash when one owner is soft-deleted (issue #30117). - * - *

Reproduces the exact client behavior for every owner-supporting entity: a detail page loads - * the entity with include=all (so a soft-deleted owner is still present) and computes a positional - * JSON Patch. The server used to load the PATCH base with NON_DELETED, dropping that owner and - * shrinking the array, so the positional remove referenced an index past the end ("array item - * index out of range") and the dangling ownership could never be removed. - */ - @Test - void patch_removeSoftDeletedOwner_removedWithoutCrash(TestNamespace ns) { - if (!supportsOwners || !supportsPatch) return; - - OpenMetadataClient client = SdkClients.adminClient(); - - // Owners are ordered by name, so the "zzz" owner sorts last and takes the highest positional - // index - the index that goes out of range once the server filters it from a NON_DELETED base. - User activeOwner = UserTestFactory.createUser(ns, "aaa_active_owner"); - User doomedOwner = UserTestFactory.createUser(ns, "zzz_doomed_owner"); - - T created = createEntity(createMinimalRequest(ns)); - String entityPath = entityPath(created.getId().toString()); - - // Assign both owners via an explicit whole-array add patch. This is robust across entities - it - // avoids the SDK update path's per-entity snapshot/diff quirks. - String ownersValue = - JsonUtils.pojoToJson( - List.of(activeOwner.getEntityReference(), doomedOwner.getEntityReference())); - client - .getHttpClient() - .executeForString( - HttpMethod.PATCH, - entityPath, - JsonUtils.readTree( - "[{\"op\":\"add\",\"path\":\"/owners\",\"value\":" + ownersValue + "}]")); - - // The OWNS relationship survives a soft delete, so the user is genuinely still an owner. - client.users().delete(doomedOwner.getId().toString()); - - JsonNode ownersBefore = ownersWithIncludeAll(entityPath); - int doomedIndex = indexOfOwner(ownersBefore, doomedOwner.getId()); - Assumptions.assumeTrue( - indexOfOwner(ownersBefore, activeOwner.getId()) >= 0 && doomedIndex >= 0, - "both explicit owners must be assigned and the soft-deleted one surfaced by include=all"); - - // The exact positional JSON Patch the owner widget computes from that include=all base. Before - // the fix this fails with "array item index is out of range"; executeForString throws on - // non-2xx. - JsonNode patch = - JsonUtils.readTree("[{\"op\":\"remove\",\"path\":\"/owners/" + doomedIndex + "\"}]"); - client.getHttpClient().executeForString(HttpMethod.PATCH, entityPath, patch); - - JsonNode ownersAfter = ownersWithIncludeAll(entityPath); - assertEquals( - -1, - indexOfOwner(ownersAfter, doomedOwner.getId()), - "the dangling soft-deleted owner should be removed"); - assertTrue( - indexOfOwner(ownersAfter, activeOwner.getId()) >= 0, "the active owner must be preserved"); - } - - private String entityPath(String id) { - String base = getResourcePath(); - return base.endsWith("/") ? base + id : base + "/" + id; - } - - private JsonNode ownersWithIncludeAll(String entityPath) { - String body = - SdkClients.adminClient() - .getHttpClient() - .executeForString(HttpMethod.GET, entityPath + "?fields=owners&include=all", null); - return JsonUtils.readTree(body).path("owners"); - } - - private int indexOfOwner(JsonNode owners, UUID ownerId) { - int index = -1; - for (int i = 0; i < owners.size(); i++) { - if (ownerId.toString().equals(owners.get(i).path("id").asText())) { - index = i; - } - } - return index; - } - // =================================================================== // VERSION-BASED CONCURRENCY TESTS (Using entity version for optimistic locking) // =================================================================== diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestSuiteResourceIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestSuiteResourceIT.java index 62d0e497b529..64ee4ff76ed8 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestSuiteResourceIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestSuiteResourceIT.java @@ -8,7 +8,6 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; -import com.fasterxml.jackson.databind.JsonNode; import es.co.elastic.clients.transport.rest5_client.low_level.Request; import es.co.elastic.clients.transport.rest5_client.low_level.Response; import es.co.elastic.clients.transport.rest5_client.low_level.Rest5Client; @@ -26,7 +25,6 @@ import org.junit.jupiter.api.parallel.Execution; import org.junit.jupiter.api.parallel.ExecutionMode; import org.openmetadata.it.bootstrap.TestSuiteBootstrap; -import org.openmetadata.it.factories.UserTestFactory; import org.openmetadata.it.util.SdkClients; import org.openmetadata.it.util.TestNamespace; import org.openmetadata.schema.api.data.CreateTable; @@ -41,7 +39,6 @@ import org.openmetadata.schema.entity.services.ingestionPipelines.PipelineStatus; import org.openmetadata.schema.entity.services.ingestionPipelines.PipelineStatusType; import org.openmetadata.schema.entity.services.ingestionPipelines.PipelineType; -import org.openmetadata.schema.entity.teams.User; import org.openmetadata.schema.metadataIngestion.SourceConfig; import org.openmetadata.schema.metadataIngestion.TestSuitePipeline; import org.openmetadata.schema.tests.DataQualityReport; @@ -389,130 +386,6 @@ void test_testSuiteWithOwner(TestNamespace ns) { assertFalse(fetched.getOwners().isEmpty()); } - // =================================================================== - // SOFT-DELETED OWNER PATCH TESTS (issue #30117) - // - // A detail page loads the entity with include=all, so a soft-deleted owner is present in the - // client's owner array and in the positional JSON Patch it computes. The server used to load the - // PATCH base with NON_DELETED, which dropped that owner and shrank the array — a positional op - // then referenced an index past the end ("array item index out of range") and the dangling - // ownership could never be removed. These tests reproduce that exact client behavior. - // =================================================================== - - @Test - void patch_removeSoftDeletedOwnerSortingLast_removesOwnerWithoutCrash(TestNamespace ns) { - OpenMetadataClient client = SdkClients.adminClient(); - - // Owners are ordered by name; the doomed owner is named to sort LAST so its positional index - // is the highest — the index that goes out of range once the server filters it from the base. - User ownerA = UserTestFactory.createUser(ns, "aaa_owner"); - User ownerB = UserTestFactory.createUser(ns, "mmm_owner"); - User doomed = UserTestFactory.createUser(ns, "zzz_owner"); - - CreateTestSuite request = new CreateTestSuite(); - request.setName(ns.prefix("testsuite_softdeleted_owner")); - request.setOwners( - List.of( - ownerA.getEntityReference(), ownerB.getEntityReference(), doomed.getEntityReference())); - String id = createEntity(request).getId().toString(); - - // The OWNS relationship survives a soft delete, so the user is genuinely still an owner. - client.users().delete(doomed.getId().toString()); - - TestSuite loaded = client.testSuites().get(id, "owners", "all"); - assertEquals( - 3, - loaded.getOwners().size(), - "include=all should still surface the soft-deleted owner the UI diffs against"); - int doomedIndex = ownerIndex(loaded, doomed.getId()); - assertEquals(2, doomedIndex, "soft-deleted owner should sort last"); - - JsonNode patch = - JsonUtils.readTree("[{\"op\":\"remove\",\"path\":\"/owners/" + doomedIndex + "\"}]"); - TestSuite patched = client.testSuites().patch(id, patch); - assertNotNull(patched, "PATCH must not fail with 'array item index out of range'"); - - List ownerIds = ownerIds(client.testSuites().get(id, "owners", "all")); - assertEquals(2, ownerIds.size(), "the dangling soft-deleted ownership should be removed"); - assertFalse(ownerIds.contains(doomed.getId()), "soft-deleted owner should be cleaned up"); - assertTrue( - ownerIds.contains(ownerA.getId()) && ownerIds.contains(ownerB.getId()), - "active owners must be preserved"); - } - - @Test - void patch_removeSoftDeletedOwnerSortingInMiddle_removesCorrectOwner(TestNamespace ns) { - OpenMetadataClient client = SdkClients.adminClient(); - - // The soft-deleted owner sorts in the MIDDLE. Under the old NON_DELETED base the server saw a - // shorter, re-indexed array, so the positional remove silently deleted the wrong (active) owner - // and left the soft-deleted one dangling. With the fix, client and server indexes stay aligned - // and the intended owner is removed. - User first = UserTestFactory.createUser(ns, "aaa_active"); - User doomed = UserTestFactory.createUser(ns, "mmm_doomed"); - User last = UserTestFactory.createUser(ns, "zzz_active"); - - CreateTestSuite request = new CreateTestSuite(); - request.setName(ns.prefix("testsuite_remove_middle_softdeleted")); - request.setOwners( - List.of( - first.getEntityReference(), doomed.getEntityReference(), last.getEntityReference())); - String id = createEntity(request).getId().toString(); - - client.users().delete(doomed.getId().toString()); - - TestSuite loaded = client.testSuites().get(id, "owners", "all"); - int doomedIndex = ownerIndex(loaded, doomed.getId()); - assertEquals(1, doomedIndex, "soft-deleted owner should sort in the middle"); - - JsonNode patch = - JsonUtils.readTree("[{\"op\":\"remove\",\"path\":\"/owners/" + doomedIndex + "\"}]"); - TestSuite patched = client.testSuites().patch(id, patch); - assertNotNull(patched, "PATCH must not fail with 'array item index out of range'"); - - List ownerIds = ownerIds(client.testSuites().get(id, "owners", "all")); - assertFalse( - ownerIds.contains(doomed.getId()), "the soft-deleted owner should be the one removed"); - assertTrue( - ownerIds.contains(first.getId()) && ownerIds.contains(last.getId()), - "both active owners must survive - the positional remove must not hit the wrong index"); - } - - private int ownerIndex(TestSuite suite, UUID ownerId) { - return ownerIds(suite).indexOf(ownerId); - } - - private List ownerIds(TestSuite suite) { - return suite.getOwners().stream().map(EntityReference::getId).collect(Collectors.toList()); - } - - @Test - void patch_nonOwnerField_withSoftDeletedOwnerPresent_succeeds(TestNamespace ns) { - // Editing an unrelated field must not fail owner re-validation just because a still-assigned - // owner was soft-deleted. The PATCH base now surfaces that owner, so validating the whole owner - // list against NON_DELETED would 404 - existing owners must be tolerated. See issue #30117. - OpenMetadataClient client = SdkClients.adminClient(); - User active = UserTestFactory.createUser(ns, "aaa_active"); - User doomed = UserTestFactory.createUser(ns, "zzz_doomed"); - CreateTestSuite request = new CreateTestSuite(); - request.setName(ns.prefix("testsuite_edit_with_dangling_owner")); - request.setOwners(List.of(active.getEntityReference(), doomed.getEntityReference())); - String id = createEntity(request).getId().toString(); - client.users().delete(doomed.getId().toString()); - - JsonNode patch = - JsonUtils.readTree( - "[{\"op\":\"replace\",\"path\":\"/description\",\"value\":\"edited with dangling owner\"}]"); - TestSuite patched = client.testSuites().patch(id, patch); - assertEquals("edited with dangling owner", patched.getDescription()); - - // The still-assigned soft-deleted owner is preserved (not silently dropped by the edit). - List ownerIds = ownerIds(client.testSuites().get(id, "owners", "all")); - assertTrue( - ownerIds.contains(active.getId()) && ownerIds.contains(doomed.getId()), - "an unrelated edit must preserve existing owners, including the soft-deleted one"); - } - @Test void test_testSuiteFQNFormat(TestNamespace ns) { OpenMetadataClient client = SdkClients.adminClient(); diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java index bf1af0ba4e40..ce5774d1e80a 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java @@ -428,22 +428,6 @@ static final class LoaderRaceException extends RuntimeException { // Fields whose change rewrites a glossary term's FQN during move operations. private static final Set GLOSSARY_TERM_MOVE_FIELDS = Set.of("parent", "glossary"); - // A PATCH's base ("original") must resolve OWNER references the same way the client diffed - // against. Detail pages fetch with include=all, so a soft-deleted owner is in the client's base; - // a - // NON_DELETED server base drops it and shrinks the array, so a positional JSON-Patch op goes out - // of range ("array item index out of range") and the dangling ownership can never be removed. - // Resolve owners with ALL here; the entity itself still loads NON_DELETED so a soft-deleted - // entity - // is not patched through this path. Trade-off: a client that instead diffs owners against a - // NON_DELETED view (a GET sending includeRelations=owners:non-deleted) can misalign against this - // longer base - the representation-aware fix (the PATCH declaring the base it diffed against) is - // the follow-up the issue flags as a design call. Experts / reviewers / followers share the - // latent - // crash but are validated on update against NON_DELETED, so they are left unchanged. See #30117. - private static final RelationIncludes PATCH_ORIGINAL_INCLUDES = - new RelationIncludes(NON_DELETED, Map.of(FIELD_OWNERS, ALL)); - /** * Canonical {@link #CACHE_WITH_NAME} key. User FQNs are lowercased at the DB layer * ({@code UserDAO.findEntityByName}), so the Guava cache must use the same normalization — @@ -4293,7 +4277,7 @@ public final PatchResponse patch( // Get only the fields relevant to this patch operation T original; try (var ignored = phase("patchLoadOriginal")) { - original = get(null, id, patchFields, PATCH_ORIGINAL_INCLUDES, false); + original = get(null, id, patchFields, NON_DELETED, false); } // Validate ETag if If-Match header is provided @@ -4355,7 +4339,7 @@ public final PatchResponse patch( // Get only the fields relevant to this patch operation T original; try (var ignored = phase("patchLoadOriginalByName")) { - original = getByName(null, fqn, patchFields, PATCH_ORIGINAL_INCLUDES, false); + original = getByName(null, fqn, patchFields, NON_DELETED, false); } // Validate ETag if If-Match header is provided @@ -4409,7 +4393,7 @@ private PatchResponse patchCommonWithOptimisticLocking( // Validate and populate owners List validatedOwners; try (var ignored = phase("patchValidateOwners")) { - validatedOwners = getValidatedOwnersForPatch(original.getOwners(), updated.getOwners()); + validatedOwners = getValidatedOwners(updated.getOwners()); } updated.setOwners(validatedOwners); @@ -7863,47 +7847,6 @@ protected List getValidatedOwners(List owners) return refs; } - /** - * Owner validation for PATCH. Owners already assigned to the entity (present in {@code - * originalOwners}) are retained even when the user has since been soft-deleted - a deactivated user - * is still a valid, existing assignment. Only newly added owners are validated against {@code - * NON_DELETED}, so a deleted user still cannot be freshly assigned. This keeps unrelated edits - * (description, tags, ...) working on an entity that carries a dangling soft-deleted owner, which - * the PATCH base now surfaces (see {@link #PATCH_ORIGINAL_INCLUDES}). See issue #30117. - */ - private List getValidatedOwnersForPatch( - List originalOwners, List updatedOwners) { - List result = updatedOwners; - boolean skipValidation = - nullOrEmpty(updatedOwners) - || updatedOwners.stream().allMatch(owner -> Boolean.TRUE.equals(owner.getInherited())); - if (!skipValidation) { - Map existingOwnersById = new HashMap<>(); - for (EntityReference owner : listOrEmpty(originalOwners)) { - existingOwnersById.put(owner.getId(), owner); - } - // Retain owners already assigned, using the authoritative ref from the base (resolved with - // ALL, so a since-soft-deleted owner keeps its populated name/type and is not re-validated). - // Validate only newly added owners against NON_DELETED so a deleted user cannot be freshly - // assigned. - List retainedOwners = - updatedOwners.stream() - .filter(owner -> existingOwnersById.containsKey(owner.getId())) - .map(owner -> existingOwnersById.get(owner.getId())) - .collect(Collectors.toList()); - List newOwners = - updatedOwners.stream() - .filter(owner -> !existingOwnersById.containsKey(owner.getId())) - .collect(Collectors.toList()); - result = new ArrayList<>(retainedOwners); - result.addAll(listOrEmpty(validateOwners(newOwners))); - result.sort( - Comparator.comparing( - EntityReference::getName, Comparator.nullsLast(Comparator.naturalOrder()))); - } - return result; - } - protected List getValidatedDomains(List domains) { if (nullOrEmpty(domains)) { return domains; diff --git a/openmetadata-ui/src/main/resources/ui/src/rest/testAPI.test.ts b/openmetadata-ui/src/main/resources/ui/src/rest/testAPI.test.ts index 53de6da5e453..d24118da7206 100644 --- a/openmetadata-ui/src/main/resources/ui/src/rest/testAPI.test.ts +++ b/openmetadata-ui/src/main/resources/ui/src/rest/testAPI.test.ts @@ -883,7 +883,7 @@ describe('testAPI tests', () => { }); describe('getTestSuiteByName', () => { - it('should fetch test suite by name with params', async () => { + it('should default owners/experts to non-deleted so soft-deleted owners stay hidden (issue #30117)', async () => { const mockGet = jest.fn().mockResolvedValue({ data: mockTestSuite }); jest.mock('./index', () => ({ __esModule: true, @@ -894,17 +894,42 @@ describe('testAPI tests', () => { const { getTestSuiteByName } = require('./testAPI'); - const params = { fields: ['owners', 'tests'] }; - - const result = await getTestSuiteByName('test.suite.name', params); + const result = await getTestSuiteByName('test.suite.name', { + fields: ['owners', 'tests'], + }); expect(mockGet).toHaveBeenCalledWith( expect.stringContaining('/dataQuality/testSuites/name/'), - { params } + { + params: { + fields: ['owners', 'tests'], + includeRelations: 'owners:non-deleted,experts:non-deleted', + }, + } ); expect(result).toEqual(mockTestSuite); }); + it('should respect a caller-provided includeRelations', async () => { + const mockGet = jest.fn().mockResolvedValue({ data: mockTestSuite }); + jest.mock('./index', () => ({ + __esModule: true, + default: { + get: mockGet, + }, + })); + + const { getTestSuiteByName } = require('./testAPI'); + + await getTestSuiteByName('test.suite.name', { + includeRelations: 'owners:all', + }); + + expect(mockGet).toHaveBeenCalledWith(expect.any(String), { + params: { includeRelations: 'owners:all' }, + }); + }); + it('should encode FQN with special characters', async () => { const mockGet = jest.fn().mockResolvedValue({ data: mockTestSuite }); jest.mock('./index', () => ({ diff --git a/openmetadata-ui/src/main/resources/ui/src/rest/testAPI.ts b/openmetadata-ui/src/main/resources/ui/src/rest/testAPI.ts index fdee07369529..408915c0485a 100644 --- a/openmetadata-ui/src/main/resources/ui/src/rest/testAPI.ts +++ b/openmetadata-ui/src/main/resources/ui/src/rest/testAPI.ts @@ -413,7 +413,17 @@ export const getTestSuiteByName = async ( ) => { const response = await APIClient.get( `${testSuiteUrl}/name/${getEncodedFqn(name)}`, - { params } + { + // Resolve owners/experts as non-deleted (consistent with data-asset detail pages), so a + // soft-deleted owner is not surfaced and the owner-edit diff stays aligned with the server's + // NON_DELETED PATCH base - otherwise a positional patch throws "array item index out of + // range". See issue #30117. + params: { + ...params, + includeRelations: + params?.includeRelations ?? 'owners:non-deleted,experts:non-deleted', + }, + } ); return response.data; From 4111c8888641170a450135b6318ea29006887fda Mon Sep 17 00:00:00 2001 From: mohitdeuex Date: Wed, 29 Jul 2026 14:45:34 +0530 Subject: [PATCH 5/5] Fixes #30117: honor includeRelations on Test Suite getByName (frontend fix was a no-op) getTestSuiteByName sends includeRelations=owners:non-deleted, but the Test Suite getByName endpoint - unlike getById and every other entity's getByName (e.g. TableResource) - did not accept the param, so the server still resolved owners with include=all and surfaced the soft-deleted owner. The owner-edit crash therefore remained. Wire includeRelations through getByName (same pattern as getById / TableResource.getByName), and add an IT that proves it: an include=all fetch still surfaces the soft-deleted owner, but includeRelations=owners:non-deleted hides it. Verified RED (without the wiring the assertion fails - owner still surfaced) and GREEN. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../it/tests/TestSuiteResourceIT.java | 50 +++++++++++++++++++ .../resources/dqtests/TestSuiteResource.java | 14 +++++- 2 files changed, 62 insertions(+), 2 deletions(-) diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestSuiteResourceIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestSuiteResourceIT.java index 64ee4ff76ed8..d548a03e4618 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestSuiteResourceIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestSuiteResourceIT.java @@ -8,6 +8,7 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import com.fasterxml.jackson.databind.JsonNode; import es.co.elastic.clients.transport.rest5_client.low_level.Request; import es.co.elastic.clients.transport.rest5_client.low_level.Response; import es.co.elastic.clients.transport.rest5_client.low_level.Rest5Client; @@ -25,6 +26,7 @@ import org.junit.jupiter.api.parallel.Execution; import org.junit.jupiter.api.parallel.ExecutionMode; import org.openmetadata.it.bootstrap.TestSuiteBootstrap; +import org.openmetadata.it.factories.UserTestFactory; import org.openmetadata.it.util.SdkClients; import org.openmetadata.it.util.TestNamespace; import org.openmetadata.schema.api.data.CreateTable; @@ -39,6 +41,7 @@ import org.openmetadata.schema.entity.services.ingestionPipelines.PipelineStatus; import org.openmetadata.schema.entity.services.ingestionPipelines.PipelineStatusType; import org.openmetadata.schema.entity.services.ingestionPipelines.PipelineType; +import org.openmetadata.schema.entity.teams.User; import org.openmetadata.schema.metadataIngestion.SourceConfig; import org.openmetadata.schema.metadataIngestion.TestSuitePipeline; import org.openmetadata.schema.tests.DataQualityReport; @@ -386,6 +389,53 @@ void test_testSuiteWithOwner(TestNamespace ns) { assertFalse(fetched.getOwners().isEmpty()); } + @Test + void get_byName_honorsIncludeRelations_hidesSoftDeletedOwner(TestNamespace ns) { + // The Test Suite detail page fetches getByName with includeRelations=owners:non-deleted so a + // soft-deleted owner is hidden and the owner-edit diff stays aligned with the server's + // NON_DELETED PATCH base - otherwise a positional patch throws "array item index out of range". + // getByName previously ignored includeRelations (unlike getById); this guards that wiring. + // See issue #30117. + OpenMetadataClient client = SdkClients.adminClient(); + User active = UserTestFactory.createUser(ns, "aaa_active"); + User doomed = UserTestFactory.createUser(ns, "zzz_doomed"); + CreateTestSuite request = new CreateTestSuite(); + request.setName(ns.prefix("testsuite_getbyname_includerel")); + request.setOwners(List.of(active.getEntityReference(), doomed.getEntityReference())); + String fqn = createEntity(request).getFullyQualifiedName(); + client.users().delete(doomed.getId().toString()); + + // include=all with no includeRelations still surfaces the soft-deleted owner... + assertTrue( + ownerIdsByName(fqn, "").contains(doomed.getId()), + "include=all should surface the soft-deleted owner"); + + // ...but getByName must honor includeRelations=owners:non-deleted and filter it out. + List filtered = ownerIdsByName(fqn, "&includeRelations=owners:non-deleted"); + assertFalse( + filtered.contains(doomed.getId()), + "getByName must hide the soft-deleted owner when includeRelations=owners:non-deleted"); + assertTrue(filtered.contains(active.getId()), "the active owner must remain"); + } + + private List ownerIdsByName(String fqn, String extraQuery) { + String body = + SdkClients.adminClient() + .getHttpClient() + .executeForString( + HttpMethod.GET, + "/v1/dataQuality/testSuites/name/" + + fqn + + "?fields=owners&include=all" + + extraQuery, + null); + List ids = new ArrayList<>(); + for (JsonNode owner : JsonUtils.readTree(body).path("owners")) { + ids.add(UUID.fromString(owner.path("id").asText())); + } + return ids; + } + @Test void test_testSuiteFQNFormat(TestNamespace ns) { OpenMetadataClient client = SdkClients.adminClient(); diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/resources/dqtests/TestSuiteResource.java b/openmetadata-service/src/main/java/org/openmetadata/service/resources/dqtests/TestSuiteResource.java index 3c8964515743..8418d0fbfa5d 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/resources/dqtests/TestSuiteResource.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/resources/dqtests/TestSuiteResource.java @@ -426,8 +426,18 @@ public TestSuite getByName( schema = @Schema(implementation = Include.class)) @QueryParam("include") @DefaultValue("non-deleted") - Include include) { - return getByNameInternal(uriInfo, securityContext, name, fieldsParam, include); + Include include, + @Parameter( + description = + "Per-relation include control. Format: field:value,field2:value2. " + + "Example: owners:non-deleted,followers:all. " + + "Valid values: all, deleted, non-deleted. " + + "If not specified for a field, uses the entity's include value.", + schema = @Schema(type = "string", example = "owners:non-deleted,followers:all")) + @QueryParam("includeRelations") + String includeRelations) { + return getByNameInternal( + uriInfo, securityContext, name, fieldsParam, include, includeRelations); } @GET