refactor: explicit per-repository query filtering + @Transactional cleanup (Option B)#1182
Draft
krusche wants to merge 22 commits into
Draft
refactor: explicit per-repository query filtering + @Transactional cleanup (Option B)#1182krusche wants to merge 22 commits into
krusche wants to merge 22 commits into
Conversation
Adds HeliosIntegrationTest — the first full-context integration-test base — booting the whole application against a real embedded Postgres (zonky) with the real Flyway schema and MockMvc, so tests drive the actual servlet stack (interceptors, OSIV, repositories). This is the harness for the tenant-isolation guard tests in the Option-B refactor: a scoping test written here exercises the same request path production uses, so it passes both with the legacy gitRepositoryFilter and with explicit per-query filtering after the migration. Completes the test profile in application-test.yml (dummy values for every no-default @value placeholder; NATS/schedulers/reconciliation/notifications/AI disabled) and mocks the GitHub clients so the context boots without credentials or network. ContextBootIT smoke-tests that the context loads and a public GET responds. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tory Adds EnvironmentScopingIT (full-context MockMvc): seeds two repositories and asserts the enabled / all environment list endpoints return only the current repository's environments (via the X-REPOSITORY-ID header). Passes on the current gitRepositoryFilter-based code — the pre-migration baseline for the Environments endpoints. (getEnvironmentById scoping is deliberately not asserted here: Hibernate's @filter does not apply to findById/PK loads, so it currently leaks cross-repo; that assertion + fix land together in the explicit-filtering commit.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replaces reliance on the ambient gitRepositoryFilter in EnvironmentService with explicit RepositoryContext-based scoping (conditional: scoped when a repo context is present, unscoped when absent — preserving today's header-conditional behavior). getAllEnvironments / getAllEnabledEnvironments now use new scoped finders; getEnvironmentById / getEnvironmentTypeById go through findScopedById, which also closes a latent cross-repo read (Hibernate @filter never applied to findById/PK loads). EnvironmentScopingIT's list guards stay green (behavior preserved) and the re-added environmentByIdIsInvisibleFromAnotherRepository test now passes (leak fixed). Existing EnvironmentServiceTest still green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| * Hibernate {@code gitRepositoryFilter} and must keep passing after the switch to explicit | ||
| * per-query filtering — it exercises the real request path (interceptor → OSIV → repository). | ||
| */ | ||
| class EnvironmentScopingIT extends HeliosIntegrationTest { |
There was a problem hiding this comment.
🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.naming.AbbreviationAsWordInNameCheck> reported by reviewdog 🐶
Abbreviation in name 'EnvironmentScopingIT' must contain no more than '1' consecutive capital letters.
| import org.junit.jupiter.api.Test; | ||
|
|
||
| /** Smoke test: the full application context boots and the servlet stack answers a public GET. */ | ||
| class ContextBootIT extends HeliosIntegrationTest { |
There was a problem hiding this comment.
🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.naming.AbbreviationAsWordInNameCheck> reported by reviewdog 🐶
Abbreviation in name 'ContextBootIT' must contain no more than '1' consecutive capital letters.
| import org.springframework.test.web.servlet.MockMvc; | ||
|
|
||
| /** | ||
| * Base class for full-context integration tests. Boots the whole application against a real embedded |
There was a problem hiding this comment.
🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.sizes.LineLengthCheck> reported by reviewdog 🐶
Line is longer than 100 characters (found 101).
| * PostgreSQL (zonky, via Docker) with the real Flyway schema, and exposes {@link MockMvc} so tests | ||
| * drive the actual servlet stack — interceptors, Open-Session-In-View, and the repository layer. | ||
| * | ||
| * <p>This is the harness for the tenant-isolation guard tests: it exercises the same request path that |
There was a problem hiding this comment.
🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.sizes.LineLengthCheck> reported by reviewdog 🐶
Line is longer than 100 characters (found 103).
| * production uses, so a scoping test written here passes both with the legacy Hibernate | ||
| * {@code gitRepositoryFilter} and with explicit per-query filtering after the migration. | ||
| * | ||
| * <p>External integrations that would otherwise reach out on startup are neutralised: NATS is disabled |
There was a problem hiding this comment.
🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.sizes.LineLengthCheck> reported by reviewdog 🐶
Line is longer than 100 characters (found 103).
| * {@code gitRepositoryFilter} and with explicit per-query filtering after the migration. | ||
| * | ||
| * <p>External integrations that would otherwise reach out on startup are neutralised: NATS is disabled | ||
| * ({@code nats.enabled=false}), the schedulers/reconciliation/notifications/AI are turned off, and the |
There was a problem hiding this comment.
🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.sizes.LineLengthCheck> reported by reviewdog 🐶
Line is longer than 100 characters (found 103).
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| CodeStyle | 12 minor |
🔴 Metrics 44 complexity
Metric Results Complexity ⚠️ 44 (≤ 20 complexity)
🟢 Coverage 85.27% diff coverage · +1.82% coverage variation
Metric Results Coverage variation ✅ +1.82% coverage variation (-1.00%) Diff coverage ✅ 85.27% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (9eeada2) 15441 7617 49.33% Head commit (9258efd) 15514 (+73) 7936 (+319) 51.15% (+1.82%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#1182) 129 110 85.27% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
…ches BranchService.getAllBranches now scopes explicitly via RepositoryContext (conditional: scoped when a repo context is present, unscoped otherwise) using the existing findByRepositoryRepositoryId finder, instead of relying on the ambient gitRepositoryFilter over findAll(). Adds BranchScopingIT (green on the old filter-based code and after the migration); existing BranchServiceTest still green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ng repo context Per review: findAll() loads every repository's rows and must never back a tenant-scoped read. Drops the findAll() fallback from EnvironmentService.getAll(Enabled)Environments and BranchService.getAllBranches; when no repository context is present these now return an empty list (the scoped finder is the only DB path). Adds no-context guard tests locking that behavior, and updates the service unit tests to the new contract (set a RepositoryContext, mock the scoped finder, clear in @AfterEach). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
WorkflowService.getAllWorkflows / getWorkflowsByState now scope explicitly via RepositoryContext (empty when no repo context, never findAll) using new scoped finders; getWorkflowById goes through findScopedById (closing the findById/PK cross-repo leak). Adds WorkflowScopingIT covering the list, state, by-id, and no-context cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t reads DeploymentService.getAllDeployments (empty on no repo context, never findAll) and getDeploymentById (scoped findById, closing the PK cross-repo leak) now filter explicitly via RepositoryContext using new scoped finders. DeploymentServiceTest updated to the new contract. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
getAllWorkflowRuns() (findAll, no repository scope) had no callers. The live workflow-run reads (getWorkflowRunById, getPaginatedWorkflowRuns, getLatestWorkflowRunsBy*) are already repository-scoped via RepositoryContext, so no migration is needed there — just delete the dead method. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…quest reads getAllPullRequests (empty on no context, never findAll) and getPullRequestById (scoped findById, closing the PK cross-repo leak) now filter explicitly via RepositoryContext. getPaginatedPullRequests now adds an explicit repository predicate to the non-pinned Specification (it previously leaned on the ambient filter) and short-circuits to an empty page when there is no repo context. PullRequestServiceTest updated to set/clear a RepositoryContext and mock the scoped finder. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…eleaseInfos Replaces the unscoped findAllByOrderByCreatedAtDesc with a repository-scoped finder via RepositoryContext (empty on no context, never findAll). Other ReleaseInfoService reads already scope explicitly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… is now fully explicit With every repository-scoped web read migrated to explicit RepositoryContext-based filtering, the ambient Hibernate filter is no longer relied upon anywhere, so remove it entirely: - drop @filter from RepositoryFilterEntity and the six directly-annotated entities (Branch, Label, GitRepository, ReleaseCandidate, TestCaseStatistics, TestCaseFlakiness); - delete the @FilterDef (package-info.java) and RepositoryFilterTransactionConfig; - RepositoryInterceptor now only publishes/clears the repository id (from X-REPOSITORY-ID); it clears the context when no header is present (correct semantics + defends against a stale ThreadLocal on a reused worker thread), and SecurityConfig no longer needs the OSIV ordering; - delete the obsolete RepositoryTenantFilterIntegrationTest (the endpoint scoping ITs cover this); - add a missing RepositoryContext @AfterEach clear in WorkflowRunServiceTest (test hygiene). RepositoryContext + RepositoryFilterEntity (as the base holding the repository association) + the X-REPOSITORY-ID header remain. Full server test suite (432 tests) green with the filter gone. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…te-on-GET)
GET /api/settings/{repositoryId}/settings called getOrCreateGitRepoSettingsByRepositoryId, which
INSERTs a defaults row when none exists — a write on a read path. Add a read-only
getGitRepoSettingsByRepositoryId that returns the persisted settings or transient defaults without
writing, and point the GET at it. getOrCreate stays for the write callers (lock/reservation calc,
scheduler). New test asserts the read path never calls save().
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
GitHubService is a pure GitHub-API gateway with zero JPA repositories injected — the class-level @transactional made every GitHub network call hold an open DB transaction for its duration, an antipattern. Removing it (and the now-unused jakarta.transaction.Transactional import) has no effect on persistence and frees the connection during remote calls. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Drop the class-level @transactional (which wrapped every read in a write-capable transaction) from the 9 read-heavy services. Reads now run under Open-Session-in-View only (no transaction). Writes are handled explicitly: - Single repository writes (save/saveAndFlush) keep no annotation — Spring Data's own per-call transaction commits them (e.g. WorkflowService.updateWorkflowLabel, UserService.updateUserSettings, BranchService.setBranchPinned..., ReleaseInfoService.create/evaluate/updateNotes/updateName, EnvironmentService.markStatusAsChanged). - Genuinely-atomic multi-write methods keep/gain a method-level @transactional so they stay all-or-nothing: DeploymentService.deployToEnvironment/cancelDeployment, EnvironmentService.lock/extend/unlock/updateEnvironment/updateLockExpirationAndReservation/ syncRepositoryEnvironments, WorkflowRunService.reRunWorkflow/reRunFailedJobs, UserService.handleFirstLogin, NotificationPreferenceService.initialize/updatePreferences, PullRequestService.setPrPinned... (conditional create + collection mutation + save). - Derived delete queries, which REQUIRE an active transaction, keep it: BranchService.deleteBranchByNameAndRepositoryId, ReleaseInfoService.deleteReleaseCandidateByName. No behavior change for writes (atomicity preserved where it existed); reads simply stop opening a write transaction. Full server suite green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ervice Last class-level @transactional in the service layer. The read (getGitRepoSettingsByRepositoryId, used by GET /settings) and the single-save getOrCreate no longer run in a write transaction; updateGitRepoSettings keeps a method-level @transactional because it persists the settings row AND cascades a cross-service locked-environment expiry recalculation that must be atomic. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Now that every tenant query filters by repository explicitly, these unscoped finders have zero callers (verified across main + test): BranchRepository.findByName, LabelRepository.findByName, PullRequestRepository.findAllByState, EnvironmentRepository.findAllByOrderByNameAsc / findByEnabledTrueOrderByNameAsc, ReleaseCandidateRepository.findAllByOrderByCreatedAtDesc, and the no-repository TestCaseStatisticsRepository.findByTestNameAndClassNameAndTestSuiteNameAndBranchName. Also removes the empty, never-injected IssueRepository. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A transaction boundary is not free: it holds a DB connection (and any row locks) for the method's whole duration — actively harmful when the method spans a GitHub/NATS network call — and its commit-time dirty-check flushes every modified managed entity, risking unintended writes. Since writes almost always succeed, "atomicity just in case" rarely pays for that cost. Every write in these services already calls save()/saveAndFlush()/delete(entity) explicitly (Spring Data's per-call transaction persists each one), so removing the method-level @transactional changes nothing about whether data is written — only whether unrelated operations roll back together. Removed it from EnvironmentService (lock/extend/unlock/updateEnvironment/updateLockExpirationAndReservation/ syncRepositoryEnvironments + the self-invoked, already-bypassed lock-expiry helpers), DeploymentService (deployToEnvironment/cancelDeployment), WorkflowRunService (reRunWorkflow/reRunFailedJobs), UserService (handleFirstLogin), NotificationPreferenceService, PullRequestService (setPrPinned...), ReleaseInfoService (publishReleaseDraft), GitRepoSettingsService (updateGitRepoSettings). Several previously held the transaction across a GitHub API call. Kept ONLY where Spring throws without a transaction: the two derived delete queries (BranchService.deleteBranchByNameAndRepositoryId, ReleaseInfoService.deleteReleaseCandidateByName). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| * branches (scoped by {@code X-REPOSITORY-ID}). Passes with the legacy gitRepositoryFilter and must | ||
| * keep passing after the switch to explicit per-query filtering. | ||
| */ | ||
| class BranchScopingIT extends HeliosIntegrationTest { |
There was a problem hiding this comment.
🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.naming.AbbreviationAsWordInNameCheck> reported by reviewdog 🐶
Abbreviation in name 'BranchScopingIT' must contain no more than '1' consecutive capital letters.
| /** | ||
| * Cross-refactor guard: {@code GET /api/workflows*} must return only the current repository's | ||
| * workflows (scoped by {@code X-REPOSITORY-ID}). Passes with the legacy gitRepositoryFilter and | ||
| * after the switch to explicit per-query filtering; also asserts the {@code findById} path no longer |
There was a problem hiding this comment.
🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.sizes.LineLengthCheck> reported by reviewdog 🐶
Line is longer than 100 characters (found 101).
| * after the switch to explicit per-query filtering; also asserts the {@code findById} path no longer | ||
| * leaks cross-repo and that a missing repository context yields an empty list (never findAll). | ||
| */ | ||
| class WorkflowScopingIT extends HeliosIntegrationTest { |
There was a problem hiding this comment.
🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.naming.AbbreviationAsWordInNameCheck> reported by reviewdog 🐶
Abbreviation in name 'WorkflowScopingIT' must contain no more than '1' consecutive capital letters.
| // requests, which would silently apply the previous user's tenant filter to the next. | ||
| // Intentionally empty — clearing happens in afterCompletion so the ThreadLocal is wiped even | ||
| // when the handler threw (postHandle is skipped on exception, afterCompletion runs | ||
| // unconditionally). Otherwise a Tomcat worker thread keeps a stale repositoryId across requests. |
There was a problem hiding this comment.
🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.sizes.LineLengthCheck> reported by reviewdog 🐶
Line is longer than 100 characters (found 101).
Deep review found reads/writes that were no longer scoped once the Hibernate gitRepositoryFilter was removed. Since GET /api/** is permitAll, these are anonymously exploitable. Fixes: - Detail-by-id reads no longer fall back to an unscoped findById when the X-REPOSITORY-ID header is absent (anonymous IDOR): EnvironmentService.findScopedById, DeploymentService.getDeploymentById, PullRequestService.getPullRequestById, WorkflowService.findScopedById now return empty on null context (mirrors WorkflowRunService.getWorkflowRunById). - Deployment reads keyed by environment id (list / latest / activity-history) and by pull-request id are gated to the current repository — REGRESSIONS: the filter used to scope these derived Deployment queries. - TestResultService.getLatestTestResultsForPr scopes the PR lookup (a foreign PR id no longer leaks its test results), matching the run/branch variants — also a regression. - EnvironmentService.getDeploymentReadiness/getLockHistoryByEnvironmentId/getEnvironmentReviewers and DeploymentService.getWorkflowJobStatus (resolveWorkflowRunRepositoryName) scope to the current repo. - Cross-repo WRITES by id are closed: lock/extend/unlock/updateEnvironment load the environment via findByIdAndRepositoryRepositoryId, so a maintainer of one repo can no longer mutate another repo's environment by id (@EnforceAtLeast* only checks the header repo). Tests: new DeploymentScopingIT (real request path: list/by-id/by-env/no-header), EnvironmentScopingIT no-header-by-id guard, and scoping unit tests (Environment lock-history gate + cross-repo write, Deployment env gate, TestResult cross-repo PR). Existing unit tests updated to the scoped finders. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
GET /settings no longer persists a settings row (write-on-GET fix), so updateGitRepoSettings could throw IllegalArgumentException → HTTP 400 when Save ran for a repo whose row was never lazily created (new repo, no locked env, no workflow group). It now creates-on-absent; the orElseThrow only fires when the repository itself does not exist. New test covers the create-on-absent path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…Desc No callers remain after getPullRequests switched to the repository-scoped finder. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| when(authService.getUserFromGithubId()).thenReturn(user); | ||
| when(environmentRepository.findById(1L)).thenReturn(Optional.of(environment)); | ||
| when(environmentRepository.findByIdAndRepositoryRepositoryId(1L, 1L)) | ||
| .thenReturn(Optional.of(environment)); |
There was a problem hiding this comment.
🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.indentation.IndentationCheck> reported by reviewdog 🐶
'method call' child has incorrect indentation level 8, expected level should be 10.
| * now enforce it explicitly. Because {@code GET /api/**} is unauthenticated, the no-header case is | ||
| * exercised too (a missing {@code X-REPOSITORY-ID} must not fall back to an unscoped PK load). | ||
| */ | ||
| class DeploymentScopingIT extends HeliosIntegrationTest { |
There was a problem hiding this comment.
🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.naming.AbbreviationAsWordInNameCheck> reported by reviewdog 🐶
Abbreviation in name 'DeploymentScopingIT' must contain no more than '1' consecutive capital letters.
…Large Objects need a tx Staging smoke test caught a regression: GET /api/environments (and other reads) returned HTTP 500 with `org.postgresql.util.PSQLException: Large Objects may not be used in auto-commit mode`. The PR body (@lob Issue.body) and the release/release-candidate `body` oid columns are PostgreSQL Large Objects, which can only be read inside a transaction. Removing @transactional from the read methods made them run in auto-commit; Open-Session-in-View keeps the EntityManager open but does NOT start a transaction, so it doesn't help. Fix: mark the pure-read methods @transactional(readOnly = true) across the service layer (swapping the jakarta→Spring import where needed). This is a mechanical requirement (like the derived-delete transactions), not the write-transaction antipattern — it's read-only, holds no write lock, and spans no network call. Write methods remain transaction-free (except the two derived deletes), preserving the intended @transactional cleanup. Verified: full server suite green; staging redeploy + smoke test confirm the reads recover. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Staging smoke test: GET /api/tests/pr/{id} for a valid same-repo PR returned 500
("Large Objects may not be used in auto-commit mode") — the test-result graph reads PR/release
Large Objects, which require a transaction. getLatestTestResultsForPr/ForBranch/ForWorkflowRun now
run readOnly. Verified no real writes in the service (the flakiness/status setters target @transient
fields, so readOnly is safe).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
krusche
force-pushed
the
refactor/explicit-repo-filter-and-tx-cleanup
branch
from
bf1f7d1 to
9258efd
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Large, incremental, test-first refactor implementing the approved plan (
docs: splendid-forging-pascal). It replaces the fragile HibernategitRepositoryFilter"magic" — the root cause of the 1.7.0→1.7.2 cross-repo data leak — with explicit per-query repository filtering, and strips@Transactionalback to only where it is mechanically required.Every step is its own commit and keeps the full server suite green; one commit temporarily injects a leak to prove the guards catch it (reverted in the same investigation, never committed).
Why
The old per-tenant isolation relied on a Hibernate
@Filtersilently enabled per request. After a refactor it stopped applying under Open-Session-in-View and leaked every repository's data into every project view — invisible in single-repo staging. Ambient, transaction-scoped filtering is too easy to break silently. This PR makes tenant scoping explicit at each query and removes the magic entirely.It also closes a latent cross-repo read: the Hibernate
@Filternever applied tofindById/PK loads, sogetEnvironmentById(idFromAnotherRepo)(and the equivalents for workflows/deployments/PRs/…) leaked before this change. The newfindByIdAndRepositoryRepositoryIdfinders scope those explicitly.What changed
A — Test harness + guards
HeliosIntegrationTest(@SpringBootTest+ MockMvc on zonky Postgres + real Flyway; externals disabled, GitHub clients mocked).X-REPOSITORY-IDrepo, no header → empty, cross-repofindById→ 404. These pass on both the old (filter) and new (explicit) code — the true cross-refactor guard.B — Explicit per-query filtering (one commit per service)
Environment,Branch,Workflow,WorkflowRun,Deployment,PullRequest(incl. the paginatedSpecification),ReleaseInforeads now pass the repo id fromRepositoryContextto a scoped finder.findAll(): no repo context → empty list (per maintainer guidance —findAllis too expensive and leak-prone).C — Fix write-on-GET
GET /settingsno longer INSERTs a settings row. New read-onlygetGitRepoSettingsByRepositoryId(returns transient defaults, nosave);getOrCreateretained for the write callers.D — Remove the Hibernate filter infra
@FilterDef/@Filterfrom all entities +package-info.java, deletedRepositoryFilterTransactionConfigand the filter-mechanism test.RepositoryInterceptornow only sets/clearsRepositoryContext(and clears on no-header, defending against stale ThreadLocals on reused worker threads). OSIV kept on (disabling it is a separate, larger effort).E —
@Transactionalcleanup@TransactionalfromGitHubService(a pure API gateway with zero JPA — it was holding a DB tx across GitHub network calls) and from all read-heavy services. Every write already callssave()/saveAndFlush()/delete(entity)explicitly (Spring Data's per-call transaction persists each), so removal changes nothing about whether data is written.@Transactionalis kept only where Spring throws without it — the two derived delete queries (BranchService.deleteBranchByNameAndRepositoryId,ReleaseInfoService.deleteReleaseCandidateByName).F — Delete dead code
findByName,findAllByState,findAllByOrderBy…, the no-repoTestCaseStatisticsfinder) and the emptyIssueRepository(verified zero callers across main + test).Verification
:application-serverserver suite green.findAll()madeEnvironmentScopingITfail (2/5); restoring the scoped finder returned it to green.Behavior preserved / flagged (not changed here)
RepositoryService.getAllRepositoriesstays intentionally cross-repo (repo picker).Userlookups are globally unique (unique_user_login), so global lookups after filter removal are safe.Remaining (owner)
GET /api/environments/enabledwithX-REPOSITORY-IDreturns only that repo's rows) and nogitRepositoryFiltererrors.🤖 Generated with Claude Code