Decompose CollectionDAO & EntityRepository; fix lineage/usage/isBot bugs

[harshach]

Describe your changes:

Fixes #28785

I decomposed the two backend "god-classes" — CollectionDAO (15,040 → 433 lines) and EntityRepository (12,797 → ~9,950 lines) — into cohesive, single-responsibility collaborators with no behavior change, and fixed several DAO-layer perf/correctness bugs found along the way. CollectionDAO is now a thin composite that extends 17 same-package domain aggregator interfaces (composite-interface-inheritance — zero call-site change), and EntityRepository delegates to 7 extracted collaborators while keeping its public/protected API intact for all 81 subclasses. The whole openmetadata-service test suite passes (5,800 tests, 0 failures), and a new CollectionDAOCompositionTest guards the JDBI wiring of the inherited @CreateSqlObject accessors. Each extraction was validated against the full suite, one collaborator at a time.

Type of change:

• Improvement
• Bug fix

High-level design:

CollectionDAO split (17 interfaces): RdfInfraDAOs, SearchReindexDAOs, AiGovernanceDAOs, FeedDAOs, ClassificationTagDAOs, TimeSeriesDAOs, ActivityAuditDAOs, GovernanceDAOs, EventSubscriptionDAOs, KnowledgeAssetDAOs, SystemTokenDAOs, DataAssetServiceDAOs, EntityDataDAOs, AccessControlDAOs, WorkflowDocStoreDAOs, OAuthDAOs, CoreRelationshipDAOs. JDBI's SqlObjectFactory discovers inherited @CreateSqlObject accessors via getMethods(), so daoCollection.xDAO() and CollectionDAO.X references resolve unchanged; only member-type import statements were repointed to the new declaring interface (compiler-enforced).

EntityRepository collaborators (7): EntityTaskWorkflows, EntityCacheLoaders, EntityCaches, EntityCacheInvalidator, InheritanceParentCache, BulkFieldFetcher, BulkImportService. Each holds a back-reference to the repository where needed and is reached via thin delegators, preserving overridable hooks and the protected API for the 62 EntityUpdater subclasses / 81 repositories. EntityUpdater itself was intentionally left in place (its 62-subclass inheritance coupling warrants a separate, dedicated refactor).

Bug/perf fixes: lineage findTo/FromPipeline operator-precedence bug (the relation filter was being dropped); entity_usage(entityType, usageDate) index for UsageDAO.computePercentile; Postgres isBot generated column was reading $.deleted instead of $.isBot (bots were counted as daily-active users); getMaxLastActivityTime/user-list isBot filters now use indexed columns; removed dead, malformed EntityExtensionDAO.update. Migrations are idempotent and live under bootstrap/sql/migrations/native/2.0.0/{mysql,postgres}/schemaChanges.sql.

Backward-compat: no public API or call-site changes; cache fields/inheritance behavior preserved via delegation; DB migrations are IF NOT EXISTS/guarded and validated on MySQL 8.0 + Postgres 16.

Tests:

Unit tests

• Added CollectionDAOCompositionTest (verifies every moved @CreateSqlObject accessor stays visible + annotated — the mechanism JDBI relies on at runtime).

Backend integration tests

• Not applicable — no API contract changes; existing openmetadata-service suite (5,800 tests) passes green and exercises the refactored DAO/repository layer end-to-end.

Ingestion / Playwright tests

• Not applicable (no ingestion or UI changes).

Manual testing performed

• Validated both DB migrations on ephemeral MySQL 8.0 and Postgres 16 containers: index creation is idempotent and EXPLAIN confirms the percentile query now does an index-only scan; the Postgres isBot fix flips a bot from counted→excluded in countDailyActiveUsers.

UI screen recording / screenshots:

Not applicable.

Checklist:

• I have read the CONTRIBUTING document.
• My PR is linked to a GitHub issue via Fixes #28785 above.
• For JSON Schema changes: not applicable; DB migrations added for the entity_usage index and Postgres isBot column fix.
• I have added tests and listed them above.

🤖 Generated with Claude Code

[github-actions]

✅ PR checks passed

The linked issue has a description and all required Shipping project fields set. Thanks!

[gitar-bot]

Code Review ⚠️ Changes requested 4 resolved / 5 findings

Decomposes CollectionDAO and EntityRepository into cohesive domain-specific collaborators while fixing several persistence bugs. Access control on lineage audit fields requires hardening, as the new implementation incorrectly trusts client-provided audit timestamps and user IDs.

⚠️ Security: Lineage audit fields createdBy/updatedBy now trust client payload

📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/LineageRepository.java:1752-1766 📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/LineageRepository.java:169

The refactor of addLineage replaced the previously authoritative audit-field assignment with applyTemporalFields, which now derives createdBy, updatedBy, createdAt and updatedAt from the incoming request body when no prior record exists.

Previously the code unconditionally did:

lineageDetails.setCreatedBy(updatedBy);
lineageDetails.setUpdatedBy(updatedBy);
lineageDetails.setCreatedAt(currentTime);
lineageDetails.setUpdatedAt(System.currentTimeMillis());

so these fields always reflected the authenticated principal (updatedBy) and server time.

Now:

• resolveCreatedBy returns incoming.getCreatedBy() when there is no prior record.
• applyTemporalFields sets updatedBy to incoming.getUpdatedBy() != null ? incoming.getUpdatedBy() : fallbackUser — i.e. a client-supplied updatedBy in the LineageDetails request body overrides the authenticated user.
• resolveUpdatedAt/resolveCreatedAt likewise honor client-supplied timestamps.

Because lineageDetails comes directly from the AddLineage request body (see addLineage), a caller can spoof the provenance/audit fields (who created/updated the lineage edge and when), and these spoofed values are persisted and propagated into the emitted ENTITY_LINEAGE_ADDED/UPDATED change events. This is an audit-integrity regression.

Suggested fix: keep the prior-record preservation behavior for consolidation, but always force updatedBy to the authenticated fallbackUser, and ignore client-supplied createdBy on initial creation (use fallbackUser). Timestamps should be server-generated.

Force updatedBy/updatedAt to the authenticated user and server time; only preserve the original createdBy from the prior record.

private static void applyTemporalFields(
    LineageDetails incoming, LineageDetails prior, String fallbackUser, long now) {
  incoming.setCreatedAt(resolveCreatedAt(incoming, prior, now));
  // Provenance must reflect the authenticated principal, never client-supplied values.
  incoming.setCreatedBy(prior != null && prior.getCreatedBy() != null ? prior.getCreatedBy() : fallbackUser);
  incoming.setUpdatedAt(now);
  incoming.setUpdatedBy(fallbackUser);
}

✅ 4 resolved

✅ Bug: isBot fix only on Postgres; confirm MySQL was already correct

📄 bootstrap/sql/migrations/native/2.0.0/postgres/schemaChanges.sql:341-352
The PR fixes the Postgres isBot generated column (migration 1.6.3 erroneously defined it as (json ->> 'deleted')::boolean). I verified that the corresponding MySQL migration (1.6.3) defined the column correctly as GENERATED ALWAYS AS (json -> '$.isBot'), so MySQL did NOT have the bug and correctly does not receive an equivalent fix in 2.0.0/mysql/schemaChanges.sql. This is consistent and correct — flagging only as a verification note so reviewers confirm no MySQL deployment was relying on the buggy behavior. No code change needed.

✅ Quality: Composition guard test only checks a hardcoded subset of moved accessors

📄 openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/CollectionDAOCompositionTest.java:35-49
CollectionDAOCompositionTest guards that @CreateSqlObject accessors moved into the new aggregator interfaces stay visible/annotated through CollectionDAO — the mechanism JDBI relies on at runtime. However it only validates the ~35 entries hardcoded in MOVED_ACCESSOR_TO_INTERFACE, not every moved accessor across all 17 interfaces. A regression where a non-listed accessor loses its annotation or visibility would not be caught. Consider deriving the expected accessor set by reflecting over the extended interfaces (iterate the extends list and collect their @CreateSqlObject methods) so the test stays exhaustive as the interfaces evolve.

✅ Performance: Postgres isBot migration rewrites user_entity table under exclusive lock

📄 bootstrap/sql/migrations/native/2.0.0/postgres/schemaChanges.sql:347-352
The Postgres fix does ALTER TABLE user_entity DROP COLUMN IF EXISTS isBot followed by ADD COLUMN isBot BOOLEAN GENERATED ALWAYS AS ((json ->> 'isBot')::boolean) STORED NOT NULL. Adding a STORED generated column forces a full table rewrite and takes an ACCESS EXCLUSIVE lock on user_entity for the duration, in addition to the preceding full-table UPDATE ... jsonb_set(...). On deployments with very large user_entity tables this can mean a notable maintenance-window stall. The logic is correct and idempotent; this is purely an operational heads-up. Consider documenting expected runtime / running during a maintenance window, which the PR's migration notes partially do.

✅ Bug: deleteLineageBySource omits ENTITY_LINEAGE_DELETED change events

📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/LineageRepository.java:1093-1107
Both deleteLineage and deleteLineageByFQN were updated to emit an ENTITY_LINEAGE_DELETED change event after a successful delete. However, deleteLineageBySource (used by the DELETE .../lineage/{entityType}/{entityId}/{lineageSource} endpoint) deletes one or more lineage edges (deleteLineageBySourcePipeline / deleteLineageBySource) but does not emit any change event.

This is an inconsistency: consumers/alerts subscribed to lineage-deleted events will be notified for single-edge deletions but silently miss source-based bulk deletions. If lineage change events are meant to drive notifications/audit, the source-based path should emit ENTITY_LINEAGE_DELETED for each removed relation (it already loads relations for deleteLineageFromSearch).

🤖 Prompt for agents

Code Review: Decomposes CollectionDAO and EntityRepository into cohesive domain-specific collaborators while fixing several persistence bugs. Access control on lineage audit fields requires hardening, as the new implementation incorrectly trusts client-provided audit timestamps and user IDs.

1. ⚠️ Security: Lineage audit fields createdBy/updatedBy now trust client payload
   Files: openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/LineageRepository.java:1752-1766, openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/LineageRepository.java:169

   The refactor of `addLineage` replaced the previously authoritative audit-field assignment with `applyTemporalFields`, which now derives `createdBy`, `updatedBy`, `createdAt` and `updatedAt` from the incoming request body when no prior record exists.
   
   Previously the code unconditionally did:
   ```
   lineageDetails.setCreatedBy(updatedBy);
   lineageDetails.setUpdatedBy(updatedBy);
   lineageDetails.setCreatedAt(currentTime);
   lineageDetails.setUpdatedAt(System.currentTimeMillis());
   ```
   so these fields always reflected the authenticated principal (`updatedBy`) and server time.
   
   Now:
   - `resolveCreatedBy` returns `incoming.getCreatedBy()` when there is no prior record.
   - `applyTemporalFields` sets `updatedBy` to `incoming.getUpdatedBy() != null ? incoming.getUpdatedBy() : fallbackUser` — i.e. a client-supplied `updatedBy` in the `LineageDetails` request body overrides the authenticated user.
   - `resolveUpdatedAt`/`resolveCreatedAt` likewise honor client-supplied timestamps.
   
   Because `lineageDetails` comes directly from the `AddLineage` request body (see `addLineage`), a caller can spoof the provenance/audit fields (who created/updated the lineage edge and when), and these spoofed values are persisted and propagated into the emitted `ENTITY_LINEAGE_ADDED/UPDATED` change events. This is an audit-integrity regression.
   
   Suggested fix: keep the prior-record preservation behavior for consolidation, but always force `updatedBy` to the authenticated `fallbackUser`, and ignore client-supplied `createdBy` on initial creation (use `fallbackUser`). Timestamps should be server-generated.

   Fix (Force updatedBy/updatedAt to the authenticated user and server time; only preserve the original createdBy from the prior record.):
   private static void applyTemporalFields(
       LineageDetails incoming, LineageDetails prior, String fallbackUser, long now) {
     incoming.setCreatedAt(resolveCreatedAt(incoming, prior, now));
     // Provenance must reflect the authenticated principal, never client-supplied values.
     incoming.setCreatedBy(prior != null && prior.getCreatedBy() != null ? prior.getCreatedBy() : fallbackUser);
     incoming.setUpdatedAt(now);
     incoming.setUpdatedBy(fallbackUser);
   }

Options

Display: compact → Showing less information.

Comment with these commands to change:

Compact

gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}

[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