Skip to content

Fixes 32946: Apply current ownership and relationship changes once (5/7) - #33348

Draft
harshach wants to merge 1 commit into
harshach/entity-stack-04-decisionsfrom
harshach/entity-stack-05-ownership
Draft

harshach wants to merge 1 commit into
harshach/entity-stack-04-decisionsfrom
harshach/entity-stack-05-ownership

Conversation

@harshach

Copy link
Copy Markdown
Collaborator

Describe your changes:

Part of #329465/7 in the compatible repository stack, preserving the existing public/protected facade.

Reconciles owner/domain references without database access and applies their current-state delta once, gates shared relationship writes during historical comparisons and avoids rewriting unchanged chart dashboard memberships.

Type of change:

  • Improvement

High-level design:

Base: harshach/entity-stack-04-decisions · Next: harshach/entity-stack-06-shared-updates · Merge in order, retarget/restack children after their parent merges; this head requires no successor code.

  • Production delta for this slice: +121/-129 lines, counting helpers; EntityRepository budget: 13456 lines
  • Existing single-transaction, nested retry/rollback, Redis and synchronous post-commit contracts retained
  • Full session diff, complete capabilities, remaining dependency migration, explicit request state and outbox consolidation remain follow-up work

Tests:

Use cases covered

PATCH/import replacement, unchanged references, removal, session cancellation and persisted owner/domain reads by ID/name; six predecessor cases fail their write budget, all eight pass with the change

Unit tests

  • 107 passed; 0 skipped; no failures/errors — focused selection
  • Whole-class gate: NOT MET, 8/18 executable classes below 90%; EntityReferenceChange 100.0%, EntityRepository 54.94%, ChartRepository 72.3%; no class identity warnings

Backend integration tests

  • PostgreSQL/OpenSearch/Redis: 29 passed, 1 assumption-aborted, no failures
  • Added/updated tests: EntityConsolidationWritesIT, EntityReferenceChangeTest, EntityRepositorySizeBudgetTest
  • Predecessor control: six of eight ownership write budgets fail, including four relationship deletes where one is required

Ingestion integration tests

Not applicable — no ingestion code changes

Playwright / UI recording

Not applicable — no UI changes

Manual testing performed

Automated local container executions above; no manual UI testing

Reproduce after building the backend and compiling ITs:

mvn -pl openmetadata-integration-tests -Ppostgres-opensearch -DcacheProvider=redis \
  -Dit.test=EntityConsolidationWritesIT,EntityRepositoryGoldenIT,OneTransactionFlushAtomicityIT \
  failsafe:integration-test@parallel-tests failsafe:verify

Checklist:

  • Issue linked as a partial step; the umbrella issue remains open
  • Java 21 service and integration-test compilation; Spotless apply/check
  • Focused behavior checks for this exact head
  • Existing public/protected bytecode declarations and descriptors retained; only the four dependency constructors added from slice 3 onward
  • Full required regression/CI and coverage gates
  • Calibrated API latency/load/allocation acceptance

Draft: coverage and performance acceptance are incomplete, and the earlier combined artifact's broad RDF failure remains unresolved; focused passing results do not replace those gates.

Tested head: b91c6413feb061e0cedf28f351f112b9632aabf8 · Native artifact SHA-256: 6e562484417305c2410ba686e831f1374b08559ff4f0212bd464edae168b54d4

@github-actions github-actions Bot added backend safe to test Add this label to run secure Github workflows on PRs labels Sep 15, 2026
Comment on lines +9579 to +9582
final var mode =
importMode
? IMPORT_OWNERS
: preserveUserOwners ? RETAIN : operation.isPatch() ? REPLACE : REPLACE_IF_NONEMPTY;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Quality: Nested ternary for Mode selection hurts readability

The Mode selection uses a 4-way nested ternary (importMode ? IMPORT_OWNERS : preserveUserOwners ? RETAIN : operation.isPatch() ? REPLACE : REPLACE_IF_NONEMPTY). This is functionally correct but hard to scan and audit; a small extracted helper or switch/if-chain per the project's preference for switch expressions would make the owner-vs-domain mode differences clearer. Consider factoring the mode decision into a named private method.

Was this helpful? React with 👍 / 👎

@gitar-bot

gitar-bot Bot commented Sep 15, 2026

Copy link
Copy Markdown
Code Review 👍 Approved with suggestions 0 resolved / 1 findings

Reconciles owner/domain references without database access and applies their current-state delta once, gating shared relationship writes during historical comparisons. Consider factoring the nested ternary for Mode selection in EntityRepository (lines 9579–9582, 9840–9843) into a named private method for improved readability.

💡 Quality: Nested ternary for Mode selection hurts readability

📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:9579-9582 📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:9840-9843

The Mode selection uses a 4-way nested ternary (importMode ? IMPORT_OWNERS : preserveUserOwners ? RETAIN : operation.isPatch() ? REPLACE : REPLACE_IF_NONEMPTY). This is functionally correct but hard to scan and audit; a small extracted helper or switch/if-chain per the project's preference for switch expressions would make the owner-vs-domain mode differences clearer. Consider factoring the mode decision into a named private method.

🤖 Prompt for agents
Code Review: Reconciles owner/domain references without database access and applies their current-state delta once, gating shared relationship writes during historical comparisons. Consider factoring the nested ternary for `Mode` selection in `EntityRepository` (lines 9579–9582, 9840–9843) into a named private method for improved readability.

1. 💡 Quality: Nested ternary for Mode selection hurts readability
   Files: openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:9579-9582, openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:9840-9843

   The `Mode` selection uses a 4-way nested ternary (`importMode ? IMPORT_OWNERS : preserveUserOwners ? RETAIN : operation.isPatch() ? REPLACE : REPLACE_IF_NONEMPTY`). This is functionally correct but hard to scan and audit; a small extracted helper or `switch`/if-chain per the project's preference for switch expressions would make the owner-vs-domain mode differences clearer. Consider factoring the mode decision into a named private method.

Review coverage

Rules No rules evaluated

Functional validation Not enabled · Set up

Auto-approval Not enabled · Set up

Options

Display: compact → Counting what did not apply, without listing it.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source

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.

1 participant