Fixes #30117: keep soft-deleted owners in PATCH base to fix index crash - #30520
Fixes #30117: keep soft-deleted owners in PATCH base to fix index crash#30520mohityadav766 wants to merge 7 commits into
Conversation
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) <noreply@anthropic.com>
github-actions
Bot
added
backend
safe to test
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR addresses a backend PATCH failure triggered when an entity has soft-deleted user relationships (owners/experts/reviewers/followers) and the client computes a positional JSON Patch from an include=all payload.
Changes:
- Update
EntityRepository.patch()to load the PATCH “original” with relation-specific includes so soft-deleted user references are present when applying positional JSON Patch ops. - Add integration tests to reproduce and verify removal of soft-deleted owners (both “sorts last” crash case and “sorts middle” wrong-index removal case).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java | Changes PATCH base-load include behavior for specific user relation fields to prevent positional JSON Patch index errors when soft-deleted user refs exist. |
| openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestSuiteResourceIT.java | Adds integration coverage for PATCH removal of soft-deleted owners in two ordering scenarios. |
Comments suppressed due to low confidence (1)
openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:4338
- Same concern as the by-id PATCH path: loading the base with
PATCH_ORIGINAL_INCLUDESfor every PATCH can cause PATCHes that don't touch owners/experts/reviewers/followers to fail when one of those relations points at a soft-deleted user, because later validation resolves owners withInclude.NON_DELETEDand throws.
Gate PATCH_ORIGINAL_INCLUDES behind whether the patch actually modifies one of those fields; otherwise keep NON_DELETED behavior to avoid new failures.
// 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);
| T original; | ||
| try (var ignored = phase("patchLoadOriginal")) { | ||
| original = get(null, id, patchFields, NON_DELETED, false); | ||
| original = get(null, id, patchFields, PATCH_ORIGINAL_INCLUDES, false); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:445
- Changing PATCH to load the original with
owners/experts/reviewers/followers = Include.ALLfixes clients that diff against an include=all representation, but it can break clients that build a positional JSON Patch from a NON_DELETED owner array (they will now apply indices against a longer server-side base and can remove the wrong owner).
This is not just theoretical: the UI’s updateEntityField uses fast-json-patch positional diffs, and some entity GETs explicitly request includeRelations: 'owners:non-deleted,…' (e.g. openmetadata-ui/src/main/resources/ui/src/rest/tableAPI.ts:65-77). In a scenario where a soft-deleted owner sorts before an active owner, a client removing /owners/0 from its filtered array would cause the server to remove the soft-deleted owner instead, leaving the intended active owner unchanged.
Consider a contract-level fix: e.g. allow PATCH to accept an includeRelations/relationIncludes parameter (or header) so the client can declare which representation it diffed against, or switch the UI to a non-positional update for these arrays (whole-array replace / ID-based operations).
// 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));
| // =================================================================== | ||
| // 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. | ||
| // =================================================================== |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:443
- The new block comment for PATCH_ORIGINAL_INCLUDES has accidental line breaks (standalone "a", "entity", "latent") that make it hard to read and slightly misleading (experts/reviewers/followers validation doesn’t prevent the positional JSON Patch crash; it’s just out of scope). Consider rewriting the comment as a coherent paragraph while keeping the same intent.
// 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.
Code Review ✅ Approved 2 resolved / 2 findingsRetains soft-deleted owners in the PATCH base and updates validation logic to prevent index crashes and positional mismatches, addressing the PATCH base=ALL misaligns positional patches and Retained PATCH owners bypass validateOwners findings. ✅ 2 resolved✅ Edge Case: PATCH base=ALL misaligns positional patches from NON_DELETED clients
✅ Edge Case: Retained PATCH owners bypass validateOwners, risking sort NPE
OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar | Powered by Gitar — free for open source |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:443
- The new PATCH_ORIGINAL_INCLUDES block comment has broken wrapping (standalone "a", "entity", "latent" lines), which makes it harder to read and looks like an accidental formatter artifact. Please reflow it into complete sentences on each line.
// 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.
openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/BaseEntityIT.java:868
- This test uses Assumptions.assumeTrue(...) to verify conditions that should be deterministic (owners were just assigned and fetched with include=all). If this ever fails it indicates a real regression and should fail the test rather than silently skip it.
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");
Describe your changes:
Fixes #30117
Editing owners failed with
array item index is out of rangewhen an owner user was soft-deleted: detail pages fetch withinclude=all(so the soft-deleted owner is present) and compute a positional JSON Patch, but the server loaded the PATCH base withNON_DELETED, dropping that owner and shrinking the array so the positional op went out of range — and the dangling ownership could never be removed. I fixed this at the single shared chokepointEntityRepository.patch().Type of change:
High-level design:
Include.ALL(PATCH_ORIGINAL_INCLUDES), while the entity itself still loadsNON_DELETED— so the positional patch stays in range and the ID-basedupdateOwnersdiff removes the danglingOWNSrow.getValidatedOwnersForPatch: because the base now surfaces the soft-deleted owner,getValidatedOwners(which runs on every PATCH) would re-validate it againstNON_DELETEDand 404 on any edit (e.g. a description change). The new method retains owners already assigned in the base (a soft-deleted user is still a valid, existing assignment) and validates only newly added owners againstNON_DELETED, so a deleted user still can't be freshly assigned.NON_DELETEDon update and share the latent issue — left as the issue's explicit follow-up.Tests:
Use cases covered
Backend integration tests
openmetadata-integration-tests/.BaseEntityIT.patch_removeSoftDeletedOwner_removedWithoutCrash— generic; runs for every owner-supporting entity.TestSuiteResourceIT:patch_removeSoftDeletedOwnerSortingLast_removesOwnerWithoutCrash,patch_removeSoftDeletedOwnerSortingInMiddle_removesCorrectOwner,patch_nonOwnerField_withSoftDeletedOwnerPresent_succeeds.Ingestion integration tests
Playwright (UI) tests
Manual testing performed
Ran against the embedded app with real MySQL + Elasticsearch.
An array item index is out of range. Index: 2, Size: 2(the exact reported error); sort-middle → wrong owner removed; description edit →404 user instance ... not found.patch_entityUpdateOwner_200,patch_entityChangeOwner_200) still pass across 4 entities — no regression.UI screen recording / screenshots:
Not applicable.
Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.🤖 Generated with Claude Code
Greptile Summary
This PR changes shared entity PATCH processing to preserve soft-deleted owners in the patch base.
Confidence Score: 3/5
The PR is not yet safe to merge because clients using the default non-deleted representation can still submit positional owner patches against a different server-side array.
The server now always applies owner operations to an Include.ALL base, while ordinary entity reads default to NON_DELETED relations; when a soft-deleted owner changes array positions, a client-generated numeric owner operation can affect a different owner.
Files Needing Attention: openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java
Important Files Changed
Reviews (7): Last reviewed commit: "Merge branch 'main' into investigate-iss..." | Re-trigger Greptile
Context used: