Skip to content

fix(composite-key): use composite key on java side#6523

Draft
RomuDeuxfois wants to merge 6 commits into
mainfrom
bugfix/composite-key
Draft

fix(composite-key): use composite key on java side#6523
RomuDeuxfois wants to merge 6 commits into
mainfrom
bugfix/composite-key

Conversation

@RomuDeuxfois

@RomuDeuxfois RomuDeuxfois commented Jul 1, 2026

Copy link
Copy Markdown
Member

What

Completes the composite-key migration for connector entities (Injector, Collector, Executor) and the injector-contract relationship, so the Hibernate model matches the composite primary key (id, tenant_id) that already ships in the schema.

Why

Built-in connector ids are shared across tenants, so id alone cannot be the primary key. The database already uses a composite (id, tenant_id) key, but the JPA mapping was still single @Id, with native-query workarounds for the resulting multi-tenant bugs (a cross-tenant contract leak, and an UPDATE that matched every tenant). This PR moves the model to composite keys and removes those workarounds, fixing the bugs at the mapping level.

Key changes

  • Injector, Collector, Executor use a composite @IdClass(ConnectorCompositeId) (id, tenant_id); InjectorContract uses @EmbeddedId.
  • The injector/contract link is remodelled from @ManyToMany to a dedicated join entity InjectorInjectorContract. A @ManyToMany cannot express the shared tenant_id column in both join groups; the join entity can, and it makes cross-tenant links structurally impossible at the database level.
  • Inject.injector uses @NotFound(IGNORE) so an uninstalled connector degrades to null instead of failing audit serialization on every inject update.
  • Tenant provisioning copies the default contracts without their links; the links are recreated in a foreign-key-safe way when the built-in injectors register (they do not exist yet at copy time).
  • The cross-tenant link invariant is enforced in the InjectorInjectorContract constructor, the single point where every link is created.

Testing

  • Full openaev-api suite: 3688 tests, no regressions attributable to these changes.
  • New regression coverage: tenant provisioning correctly links its built-in connectors (real Postgres and tenant filter), plus a unit test for the cross-tenant link guard.

Follow-ups (documented separately)

Three latent, design-level items were deliberately deferred rather than patched here, to avoid regression risk on working code:

  • equals / hashCode alignment with the composite key,
  • observability on silently-null @NotFound associations,
  • the asymmetric @NotFound on the join entity.

@RomuDeuxfois RomuDeuxfois requested a review from Copilot July 1, 2026 16:31
@github-actions github-actions Bot added the filigran team Item from the Filigran team. label Jul 1, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates connector entities (Collector/Injector/Executor) and their repositories to use the database’s composite primary key (id, tenant_id) on the Java side, reducing cross-tenant ambiguity and removing the need for tenant @ManyToOne relations on those connector entities.

Changes:

  • Switch connector entities to composite identifiers via @IdClass(ConnectorCompositeId.class) and introduce TenantIdBase + TenantIdBaseListener.
  • Update Spring Data repositories to use ConnectorCompositeId as the ID type and adjust service/API/test code to set/use tenantId directly.
  • Simplify builtin injector update logic to use standard save() now that updates are correctly tenant-scoped by the composite key.

Reviewed changes

Copilot reviewed 34 out of 34 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
openaev-model/src/main/java/io/openaev/database/repository/InjectorRepository.java Switch repository ID type to ConnectorCompositeId; remove workaround native update.
openaev-model/src/main/java/io/openaev/database/repository/ExecutorRepository.java Switch repository ID type to ConnectorCompositeId.
openaev-model/src/main/java/io/openaev/database/repository/CollectorRepository.java Switch repository ID type to ConnectorCompositeId; remove unused finder.
openaev-model/src/main/java/io/openaev/database/model/TenantIdBase.java New interface for entities storing raw tenantId (no Tenant relation).
openaev-model/src/main/java/io/openaev/database/model/Injector.java Convert to composite key; replace Tenant relation with tenantId; update join table columns.
openaev-model/src/main/java/io/openaev/database/model/Executor.java Convert to composite key; replace Tenant relation with tenantId.
openaev-model/src/main/java/io/openaev/database/model/ConnectorCompositeId.java New composite ID type (id, tenantId) for connector PKs.
openaev-model/src/main/java/io/openaev/database/model/Collector.java Convert to composite key; replace Tenant relation with tenantId.
openaev-model/src/main/java/io/openaev/database/model/BaseConnectorEntity.java Remove Persistable/new-entity tracking (but needs cleanup per comments).
openaev-model/src/main/java/io/openaev/database/audit/TenantIdBaseListener.java New listener to set tenantId from TenantContext on persist.
openaev-api/src/test/java/io/openaev/utils/fixtures/composers/CollectorComposer.java Remove “force merge” logic now that PK is composite.
openaev-api/src/test/java/io/openaev/service/EndpointServiceTest.java Update test setup to set executor.tenantId.
openaev-api/src/test/java/io/openaev/rest/InjectorApiTest.java Assert against injector.tenantId instead of injector.tenant.id.
openaev-api/src/test/java/io/openaev/rest/inject_expectation/InjectExpectationServiceTest.java Remove connector new-entity flag usage.
openaev-api/src/test/java/io/openaev/executors/tanium/service/TaniumExecutorServiceTest.java Update executor setup to set tenantId.
openaev-api/src/test/java/io/openaev/executors/sentinelone/service/SentinelOneExecutorServiceTest.java Update executor setup to set tenantId.
openaev-api/src/test/java/io/openaev/executors/paloaltocortex/service/PaloAltoCortexExecutorServiceTest.java Update executor setup to set tenantId.
openaev-api/src/test/java/io/openaev/executors/ExecutorServiceTest.java Update assertions to validate tenantId directly.
openaev-api/src/test/java/io/openaev/executors/crowdstrike/service/CrowdstrikeExecutorServiceTest.java Update executor setup to set tenantId.
openaev-api/src/test/java/io/openaev/executors/caldera/service/CalderaExecutorServiceTest.java Update executor setup to set tenantId.
openaev-api/src/main/java/io/openaev/service/InjectorService.java Update injector creation/update paths to set/use tenantId; update ID-based lookup to composite IDs.
openaev-api/src/main/java/io/openaev/service/EndpointService.java Set endpoint/agent tenants from executor tenantId via id-only Tenant references.
openaev-api/src/main/java/io/openaev/rest/injector/InjectorApi.java Pass current tenant to composite-ID lookup.
openaev-api/src/main/java/io/openaev/rest/injector_contract/InjectorContractService.java Replace injector.getTenant() usage with new Tenant(injector.getTenantId()) and tenantId-based queries.
openaev-api/src/main/java/io/openaev/rest/collector/service/CollectorService.java Set collector.tenantId on registration.
openaev-api/src/main/java/io/openaev/rest/collector/CollectorApi.java Fix lookup by using findByIdAndTenantId instead of findById.
openaev-api/src/main/java/io/openaev/importer/V1_DataImporter.java Set contract tenant from dummy injector tenantId.
openaev-api/src/main/java/io/openaev/executors/tanium/service/TaniumExecutorService.java Use executor.tenantId for tenant-scoped operations.
openaev-api/src/main/java/io/openaev/executors/sentinelone/service/SentinelOneExecutorService.java Use executor.tenantId for tenant-scoped operations and entity creation.
openaev-api/src/main/java/io/openaev/executors/paloaltocortex/service/PaloAltoCortexExecutorService.java Use executor.tenantId for tenant-scoped operations and entity creation.
openaev-api/src/main/java/io/openaev/executors/ExecutorService.java Simplify registration to set only tenantId (no Tenant relation on executor).
openaev-api/src/main/java/io/openaev/executors/crowdstrike/service/CrowdStrikeExecutorService.java Use executor.tenantId for tenant-scoped operations and entity creation.
openaev-api/src/main/java/io/openaev/executors/caldera/service/CalderaExecutorService.java Use executor.tenantId for tenant-scoped operations and entity creation.
openaev-api/src/main/java/io/openaev/api/chaining/InjectExecutionStep.java Set injector tenantId when creating injects from chaining steps.

Comment thread openaev-model/src/main/java/io/openaev/database/model/BaseConnectorEntity.java Outdated
Comment on lines 11 to 15
@@ -19,29 +15,9 @@
* correctly uses {@code persist()} for new entities and {@code merge()} for existing ones.
Comment on lines +99 to +103
@JoinColumn(name = "injector_id", referencedColumnName = "injector_id"),
@JoinColumn(name = "tenant_id", referencedColumnName = "tenant_id")
})
},
inverseJoinColumns =
@JoinColumn(name = "injector_contract_id", referencedColumnName = "injector_contract_id"))
@RomuDeuxfois RomuDeuxfois force-pushed the bugfix/composite-key branch from 94fd7ea to 2631f9a Compare July 2, 2026 06:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 53 out of 53 changed files in this pull request and generated 2 comments.

private String tenantId;

@NotFound(action = NotFoundAction.IGNORE)
@ManyToOne(fetch = FetchType.EAGER)
Comment on lines +3 to +13
import java.io.Serializable;
import java.util.Objects;

/** Composite identifier for the {@link InjectorInjectorContract} join entity. */
public class InjectorInjectorContractId implements Serializable {

private String injectorId;
private String injectorContractId;
private String tenantId;

public InjectorInjectorContractId() {}
@codecov

codecov Bot commented Jul 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.64045% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 44.42%. Comparing base (4829b8d) to head (96a7e55).

Files with missing lines Patch % Lines
.../main/java/io/openaev/service/InjectorService.java 75.00% 7 Missing and 2 partials ⚠️
...ain/java/io/openaev/rest/injector/InjectorApi.java 0.00% 1 Missing ⚠️
...est/injector_contract/InjectorContractService.java 94.44% 1 Missing ⚠️

❌ Your project check has failed because the head coverage (2.89%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6523      +/-   ##
============================================
- Coverage     44.47%   44.42%   -0.05%     
+ Complexity     7538     7536       -2     
============================================
  Files          2316     2316              
  Lines         64296    64311      +15     
  Branches       8552     8555       +3     
============================================
- Hits          28595    28573      -22     
- Misses        33874    33912      +38     
+ Partials       1827     1826       -1     
Flag Coverage Δ
backend 66.80% <87.64%> (-0.08%) ⬇️
e2e 18.37% <ø> (ø)
frontend 2.89% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@laugiov

laugiov commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Known follow-ups (deliberately deferred)

These items were found while finishing this PR and intentionally NOT fixed here, to keep the PR regression-free. They are latent (no live path today) and better decided at the design level than patched under pressure.


1. equals / hashCode are not aligned with the new composite key (deferred on purpose)

What

The connector primary key became composite (id, tenant_id). But identity in Java code was left keyed on id only:

  • Injector.equals / Injector.hashCode compare id only.
  • Collector and Executor inherit Lombok @Data equality from BaseConnectorEntity, which covers id/name/type/external but NOT tenant_id.
  • The in-memory dedup inside InjectorContract.addInjector (current.contains(injector)) is therefore also id-only.

So two connectors that share a business id across two tenants (for example the built-in email injector in tenant A and in tenant B) are considered equal by Java, even though they are two distinct rows.

Why it does not break anything today

Every connector read is scoped to a single tenant by the Hibernate tenantFilter. Java collections of connectors are therefore single-tenant, where business ids are unique, so id-only equality is correct in practice. This is a latent trap, not a live bug. No current code path iterates connectors across tenants in one collection.

Why it was deferred (not fixed in the PR)

  1. It is pre-existing: the id-only equality was already there before this PR, it is not a regression introduced by the composite-key completion work.
  2. Fixing it is genuinely risky. equals / hashCode are used everywhere (every Set, Map, .contains, Hibernate's own entity dedup, the addInjector dedup). Making them composite-aware can change subtle behavior in code that currently relies on id-only identity, so it carries real regression risk. That conflicts with the "no regression" bar this PR was held to.
  3. It is a design-level call about the composite-key model as a whole.

Recommendation

Fix it in a dedicated follow-up, not in this PR, and validate with the full multi-tenant suite. Concretely:

  • Injector.equals / hashCode, and the BaseConnectorEntity equality for Collector / Executor, should compare (id, tenant_id).
  • The addInjector dedup should then also be composite-aware.
  • Alternatively, if the team decides the tenantFilter guarantee is a sufficient guard forever, document that decision explicitly next to the entities so the id-only equality is a conscious choice rather than an oversight.

An independent code audit rated this MAJOR (it is the invariant a multi-tenancy model should hold), while also confirming it is latent with no confirmed live path today. Both statements are true: important to close, not urgent, and worth doing carefully on its own.

Locations

  • openaev-model/.../database/model/Injector.java (equals/hashCode)
  • openaev-model/.../database/model/BaseConnectorEntity.java (@Data equality for Collector/Executor)
  • openaev-model/.../database/model/InjectorContract.java (addInjector contains dedup)

2. Observability for silent-null connector links (deferred, needs a targeted approach)

Two audit passes flagged that a connector link which does not resolve is dropped silently:

  • Inject.injector and InjectorInjectorContract.injector use @NotFound(action = IGNORE), so a dangling reference (connector uninstalled) becomes null with no log.
  • Injector.getContracts() and InjectorContract.getInjectors() filter nulls out of the derived views.

Why it was not patched in this PR: a naive WARN is either noisy or wrong.

  • Logging in the derived getters spams for any persistently-dangling link, because those getters are on the hot serialization path.
  • Logging at inject resolution (InjectUtils.resolveInjector) false-positives on contracts that legitimately have no per-tenant injector (for example OVH and OpenCTI contracts, copied to a tenant that has no matching built-in injector).
  • A blanket "every default contract must have a link" check has the same false-positive.

Recommendation: instrument this as a targeted signal that knows which contracts are expected to have an injector (the built-in-registered set), ideally a metric with tenant and contract-id context rather than a log line, or a health check run after provisioning. The provisioning gap itself is already guarded in CI by the strengthened test TenantServiceTest.should_provision_and_link_builtin_injector_contracts_for_new_tenant.

3. Asymmetric @NotFound on the join entity (minor)

InjectorInjectorContract.injector has @NotFound(action = IGNORE) but injectorContract does not. If a link's contract were ever missing, getContracts() would throw EntityNotFoundException before its null filter runs. Largely shielded today by the ON DELETE CASCADE on the join FK and the same-tenant guarantee. Not patched here because adding @NotFound to that side forces it EAGER, which risks a load cycle with the already-EAGER InjectorContract.injectorLinks. Worth a careful look if the asymmetry ever bites.

4. Strategic note (design level, for the record)

The composite-key approach is the right fix for connectors (built-in ids repeat across tenants, so id alone cannot be the primary key), and it removes real native-query workarounds that main still carries (cross-tenant contract leak, all-tenant UPDATE). But it makes connectors the only entities using composite keys, diverging from the platform norm (surrogate key plus tenantFilter), with a recurring Hibernate 6 cost. The equals/hashCode gap above is one symptom of that cost. Worth deciding whether composite keys stay a scoped exception for connectors or become a pattern, and documenting that choice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

filigran team Item from the Filigran team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants