Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 82 additions & 0 deletions .github/skills/activate-tenant-table/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -675,6 +675,72 @@ Do not defer this phase to "later regression pass" — an association missed
here degrades silently (200 OK, empty array) exactly like #7026, so nothing
in Phase 8's regression run will catch it unless the new test from 3b.3 exists.

**3b.4 — computed getters and DTO mappers that resolve the activated table
(the #7605 / #7621 shape).** An association accessor is not the only silent
reader. A COMPUTED getter on an unrelated entity — a `@JsonProperty` method
with no column of its own that walks a relation to `{Entity}` to derive a
scalar — reads the activated table on every serialization, and it is invisible
to all three greps above: it names neither `{EntityRepository}` nor `{table}`,
and it is not an `@OneToMany`/`@ManyToOne` field. `Inject#getType()` is the
reference: `@JsonProperty("inject_type")` resolving
`injectorContract.getFirstInjector().getType()` on the v2-scoped `injectors`
table. Its callers are three DTO mappers (`InjectMapper#toInjectOutput`,
`InjectMapper#toInjectResultOverviewOutput`,
`InjectStatusMapper#toInjectTestStatusOutput`) plus direct entity
serialization, so EVERY endpoint returning `Inject`, `InjectOutput`,
`InjectResultOverviewOutput`, `InjectResultOutput` or `InjectTestStatusOutput`
reads `injectors`. The activation wired the obvious inject endpoints and
missed the rest; they shipped 200 OK with `inject_type: null`, which the
frontend renders as the generic "unknown" icon on the whole Execution screen
(time-based AND chaining) — a silent regression found in production, not in
CI.

Walk it in two directions, and treat BOTH as part of the closure:

```bash
# 3b.4.a - computed @JsonProperty getters anywhere in the model that resolve
# {Entity} without naming {table} or {EntityRepository}
grep -rn -B3 "get{Entity}()\|get{Entities}()\|getFirst{Entity}()" \
openaev-model/src/main/java/io/openaev/database/model --include="*.java" | grep -n "@JsonProperty" -B3

# 3b.4.b - every caller of each computed getter found (mappers included)
grep -rn "\.{computedGetter}()" openaev-api/src/main/java openaev-model/src/main/java --include="*.java"

# 3b.4.c - THE SINK SWEEP: once a DTO/entity is known to carry the computed
# value, enumerate EVERY endpoint whose return type is that DTO/entity, and
# check TxCtx on each one. This is the step that was skipped in #7605.
grep -rn "public .*\b{SinkType}\b\|Page<{SinkType}>\|List<{SinkType}>\|Iterable<{SinkType}>" \
openaev-api/src/main/java --include="*Api.java"
```

Rules for this sub-phase:

- The unit of enumeration is the RESPONSE TYPE, not the API package. A
computed getter leaks through `TeamApi`, `PlayerApi`, `OrganizationApi`,
`AssetGroupApi`, `EndpointApi`, ... simply because they return the same DTO;
none of them mentions the activated table anywhere. Sweep by sink type
across all controllers, then diff that list against `TX_SCOPED_ENTRYPOINTS`:
every endpoint returning a sink type must appear in one of the two lists
(wired, or explicitly justified as never serializing the computed value).
- A criteria/JPA projection that SELECTs the derived column
(`injectorJoin.get("type").alias("inject_type")`) is the same sink: it joins
the activated table inside the query, so its endpoints need `TxCtx` exactly
like the lazy-getter path.
- `@Transactional(propagation = Propagation.SUPPORTS)` handlers (bulk
update/delete, massive-operation wrappers) are a trap: with no inbound
transaction the aspect has nothing to scope, so adding `TxCtx` alone does
NOT fix them. Either the service opens the scoped transaction, or the
handler is switched to a real `@Transactional` boundary — decide and write
it down, do not leave a `TxCtx` parameter that silently does nothing.
- Deprecated endpoints returning the sink type count (they still ship): the
`/api/exercise/{id}/injects/test` variant is as live as its
`/injects/test/search` successor.
- Pin the sweep: for each sink type, add one production-like test
(`@TestPropertySource(properties = "openaev.tenant.active-tables={table}")`)
asserting the computed field is NON-NULL on a representative endpoint per
controller family, not just on the table's own API. A null-valued scalar is
the failure mode; an empty-array assertion will not catch it.

### Phase 4 — RED then GREEN: write attribution

The inspector cannot attribute `INSERT ... VALUES`. Attribution is application
Expand Down Expand Up @@ -1209,6 +1275,22 @@ Before marking the issue done, write down:
- [ ] association-accessor scan run for every entity holding a reference to
the activated entity, regardless of whether the activated table has its
own API (eager/lazy loads bypass the repository grep either way)
- [ ] computed-getter scan run (Phase 3b.4): every `@JsonProperty` getter that
derives a scalar from the activated table (model: `Inject#getType()` →
`injectors`) found, its DTO mappers and JPA projections listed, and the
SINK SWEEP done — every endpoint returning one of those sink types
(entity or DTO), in ANY controller, diffed against
`TX_SCOPED_ENTRYPOINTS` so none is left unwired (#7605/#7621: the inject
endpoints were wired, the `InjectResultOutput` /
`InjectTestStatusOutput` / `InjectResultOverviewOutput` endpoints on
team, player, organization, asset-group and atomic-testing were not, and
shipped `inject_type: null`)
- [ ] every `@Transactional(propagation = SUPPORTS)` handler returning a sink
type is explicitly resolved: either the service opens the scoped
transaction or the handler gets a real transaction boundary — no
`TxCtx` parameter left on a SUPPORTS handler where the aspect cannot fire
- [ ] one production-like test per sink type asserts the computed field is
NON-NULL (a null scalar, not an empty array, is this regression's shape)
- [ ] isolation test written first and seen red for the mechanism, then green;
raw red/green outputs captured in the report
- [ ] reads: own row visible, cross-tenant 404, path and header selectors
Expand Down
11 changes: 11 additions & 0 deletions .github/skills/review-multi-tenancy/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,17 @@ Classify each touched table as one of:
- a native `@Query` that `JOIN`s a v2-active table anywhere in the codebase
(not just its own repository) is pulled into the fail-closed rewrite —
check its FROM/JOIN shape against `TenantStatementInspectorTest` (#7007)
- a new endpoint (in ANY controller) whose response type serializes a
computed value derived from a v2-active table needs `TxCtx` too, even
though nothing in the file names the table: `Inject#getType()`
(`@JsonProperty("inject_type")`) resolves `injectors`, so every endpoint
returning `Inject`, `InjectOutput`, `InjectResultOutput`,
`InjectResultOverviewOutput` or `InjectTestStatusOutput` reads it. Missing
`TxCtx` here is 🔴 CRITICAL and completely silent: 200 OK with the scalar
`null` (#7605/#7621 — missing injector icons across the Execution screen).
Sweep by RESPONSE TYPE, not by API package.
- `@Transactional(propagation = SUPPORTS)` + `TxCtx` is a false fix: with no
inbound transaction the aspect never fires. Flag it 🟠 HIGH.
- **v1 (still `@Filter`-based)** — not in `active-tables`. Isolation is
Hibernate `@Filter` + `TenantBaseListener`, ambient via
`TenantContext.getCurrentTenant()`. For these tables, Steps 2-7 below
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public class ScenarioInjectApi extends RestBehavior {
resourceType = ResourceType.SCENARIO)
@Transactional(readOnly = true)
public Iterable<InjectOutput> scenarioInjectsSimple(
@PathVariable @NotBlank final String scenarioId) {
TxCtx ctx, @PathVariable @NotBlank final String scenarioId) {
return injectSearchService.injects(fromScenario(scenarioId));
}

Expand All @@ -75,6 +75,7 @@ public Iterable<InjectOutput> scenarioInjectsSimple(
resourceType = ResourceType.SCENARIO)
@Transactional(readOnly = true)
public Iterable<InjectOutput> scenarioInjectsSimple(
TxCtx ctx,
@PathVariable @NotBlank final String scenarioId,
@RequestBody @Valid final SearchPaginationInput searchPaginationInput) {
Map<String, Join<Base, Base>> joinMap = new HashMap<>();
Expand All @@ -101,7 +102,8 @@ public Iterable<InjectOutput> scenarioInjectsSimple(
resourceId = "#scenarioId",
actionPerformed = Action.READ,
resourceType = ResourceType.SCENARIO)
public Iterable<Inject> scenarioInjects(@PathVariable @NotBlank final String scenarioId) {
public Iterable<Inject> scenarioInjects(
TxCtx ctx, @PathVariable @NotBlank final String scenarioId) {
Comment on lines +105 to +106
return this.injectRepository.findByScenarioId(scenarioId).stream()
.sorted(Inject.executionComparator)
.toList();
Expand All @@ -117,6 +119,7 @@ public Iterable<Inject> scenarioInjects(@PathVariable @NotBlank final String sce
actionPerformed = Action.READ,
resourceType = ResourceType.SCENARIO)
public Inject scenarioInject(
TxCtx ctx,
@PathVariable @NotBlank final String scenarioId,
@PathVariable @NotBlank final String injectId) {
Scenario scenario = this.scenarioService.scenario(scenarioId);
Expand Down Expand Up @@ -242,6 +245,7 @@ public InjectOutput updateInjectForScenario(
actionPerformed = Action.WRITE,
resourceType = ResourceType.INJECT)
public Inject updateInjectActivationForScenario(
TxCtx ctx,
@PathVariable @NotBlank final String scenarioId,
@PathVariable @NotBlank final String injectId,
@Valid @RequestBody InjectUpdateActivationInput input) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ public class SimulationInjectApi extends RestBehavior {
resourceType = ResourceType.SIMULATION)
@Transactional(readOnly = true)
public Iterable<InjectOutput> exerciseInjectsSimple(
@PathVariable @NotBlank final String exerciseId) {
TxCtx ctx, @PathVariable @NotBlank final String exerciseId) {
return injectSearchService.injects(fromSimulation(exerciseId));
}

Expand All @@ -112,6 +112,7 @@ public Iterable<InjectOutput> exerciseInjectsSimple(
resourceType = ResourceType.SIMULATION)
@Transactional(readOnly = true)
public Iterable<InjectOutput> exerciseInjectsSimple(
TxCtx ctx,
@PathVariable @NotBlank final String exerciseId,
@RequestBody @Valid final SearchPaginationInput searchPaginationInput) {
Map<String, Join<Base, Base>> joinMap = new HashMap<>();
Expand Down Expand Up @@ -139,7 +140,8 @@ public Iterable<InjectOutput> exerciseInjectsSimple(
resourceId = "#exerciseId",
actionPerformed = Action.READ,
resourceType = ResourceType.SIMULATION)
public Iterable<Inject> exerciseInjects(@PathVariable @NotBlank final String exerciseId) {
public Iterable<Inject> exerciseInjects(
TxCtx ctx, @PathVariable @NotBlank final String exerciseId) {
Comment on lines +143 to +144
return injectRepository.findByExerciseId(exerciseId).stream()
.sorted(Inject.executionComparator)
.toList();
Expand All @@ -156,6 +158,7 @@ public Iterable<Inject> exerciseInjects(@PathVariable @NotBlank final String exe
resourceType = ResourceType.SIMULATION)
@Transactional(readOnly = true)
public Page<InjectResultOutput> searchExerciseInjects(
TxCtx ctx,
@PathVariable final String exerciseId,
@RequestBody @Valid SearchPaginationInput searchPaginationInput) {
return injectSearchService.getPageOfInjectResults(exerciseId, searchPaginationInput);
Expand All @@ -171,7 +174,8 @@ public Page<InjectResultOutput> searchExerciseInjects(
actionPerformed = Action.READ,
resourceType = ResourceType.SIMULATION)
@Transactional(readOnly = true)
public List<InjectResultOutput> exerciseInjectsResults(@PathVariable final String exerciseId) {
public List<InjectResultOutput> exerciseInjectsResults(
TxCtx ctx, @PathVariable final String exerciseId) {
return injectSearchService.getListOfInjectResults(exerciseId);
}

Expand Down Expand Up @@ -354,6 +358,7 @@ public InjectStatus executeInject(
actionPerformed = Action.WRITE,
resourceType = ResourceType.INJECT)
public Inject updateInjectActivationForExercise(
TxCtx ctx,
@PathVariable String exerciseId,
@PathVariable String injectId,
@Valid @RequestBody InjectUpdateActivationInput input) {
Expand All @@ -370,7 +375,7 @@ public Inject updateInjectActivationForExercise(
actionPerformed = Action.WRITE,
resourceType = ResourceType.INJECT)
public Inject updateInjectTrigger(
@PathVariable String exerciseId, @PathVariable String injectId) {
TxCtx ctx, @PathVariable String exerciseId, @PathVariable String injectId) {
return simulationInjectService.triggerInjectForSimulation(exerciseId, injectId);
}

Expand All @@ -384,6 +389,7 @@ public Inject updateInjectTrigger(
actionPerformed = Action.WRITE,
resourceType = ResourceType.INJECT)
public Inject setInjectStatus(
TxCtx ctx,
@PathVariable String exerciseId,
@PathVariable String injectId,
@Valid @RequestBody InjectUpdateStatusInput input) {
Expand All @@ -400,6 +406,7 @@ public Inject setInjectStatus(
actionPerformed = Action.WRITE,
resourceType = ResourceType.INJECT)
public Inject updateInjectTeams(
TxCtx ctx,
@PathVariable String exerciseId,
@PathVariable String injectId,
@Valid @RequestBody InjectTeamsInput input) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,18 @@ class TenantScopedEntrypointsTxCtxArchTest {
"io.openaev.rest.inject.ScenarioInjectApi#createInjectForScenario",
"io.openaev.rest.inject.ScenarioInjectApi#duplicateInjectForScenario",
"io.openaev.rest.inject.ScenarioInjectApi#updateInjectForScenario",
"io.openaev.rest.inject.SimulationInjectApi#exerciseInjects",
"io.openaev.rest.inject.SimulationInjectApi#exerciseInjectsSimple",
"io.openaev.rest.inject.SimulationInjectApi#searchExerciseInjects",
"io.openaev.rest.inject.SimulationInjectApi#exerciseInjectsResults",
"io.openaev.rest.inject.SimulationInjectApi#updateInjectActivationForExercise",
"io.openaev.rest.inject.SimulationInjectApi#updateInjectTrigger",
"io.openaev.rest.inject.SimulationInjectApi#setInjectStatus",
"io.openaev.rest.inject.SimulationInjectApi#updateInjectTeams",
"io.openaev.rest.inject.ScenarioInjectApi#scenarioInjects",
"io.openaev.rest.inject.ScenarioInjectApi#scenarioInjectsSimple",
"io.openaev.rest.inject.ScenarioInjectApi#scenarioInject",
"io.openaev.rest.inject.ScenarioInjectApi#updateInjectActivationForScenario",
// health-check streams: runChecks -> securityPlatformCollectors
"io.openaev.rest.scenario.ScenarioApi#streamHealthChecks",
"io.openaev.rest.exercise.ExerciseApi#streamHealthChecks",
Expand Down
Loading