-
Notifications
You must be signed in to change notification settings - Fork 1
refactor: explicit per-repository query filtering + @Transactional cleanup (Option B) #1182
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: staging
Are you sure you want to change the base?
Changes from 3 commits
8263084
d20958d
16234a7
1557a7c
e6ab21f
0738587
e11d967
b9233f8
342ad2a
9db96af
57341d6
fc3f484
1dc0e04
c42405b
5be962f
b21463a
a543030
15dabfe
c9de350
ca5220e
fb41e47
9258efd
db87b28
677cd3e
628d45d
a667ee5
2d8d290
663d403
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,53 @@ | ||
| # Profile used by full-context integration tests (HeliosIntegrationTest). Supplies dummy values for | ||
| # every no-default @Value placeholder and disables all external integrations / schedulers so the | ||
| # context boots without credentials, network, NATS, or a running scheduler. Not used in any real | ||
| # deployment (prod/staging/dev activate their own profiles). | ||
| spring: | ||
| data: | ||
| jpa: | ||
| repositories: | ||
| bootstrap-mode: deferred | ||
| # deferred needs an async bootstrap executor not wired in the test context | ||
| bootstrap-mode: default | ||
| security: | ||
| oauth2: | ||
| resourceserver: | ||
| jwt: | ||
| issuer-uri: "url.invalid" | ||
| issuer-uri: "url.invalid" | ||
| flyway: | ||
| enabled: true | ||
|
|
||
| nats: | ||
| enabled: false | ||
| server: "nats://localhost:4222" | ||
| auth: | ||
| token: "dummy" | ||
| durableConsumerName: "test-consumer" | ||
| consumerAckWaitSeconds: 300 | ||
| consumerInactiveThresholdMinutes: 45 | ||
| timeframe: 1440 | ||
|
|
||
| github: | ||
| organizationName: "test-org" | ||
| authToken: "dummy-token" | ||
| appName: "test-app" | ||
| clientId: "dummy-client" | ||
| privateKeyPath: "dummy-key.pem" | ||
| tokenExchangeClientId: "dummy-exchange-client" | ||
| tokenExchangeClientSecret: "dummy-exchange-secret" | ||
|
|
||
| monitoring: | ||
| repositories: "" | ||
| timeframe: 14 | ||
| repository-sync-cron: "0 0 4 * * *" | ||
| runOnStartup: false | ||
| runOnStartupCooldownInMinutes: 1440 | ||
|
|
||
| notification: | ||
| enabled: false | ||
|
|
||
| reconciliation: | ||
| enabled: false | ||
|
|
||
| helios: | ||
| ai: | ||
| enabled: false |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| package de.tum.cit.aet.helios; | ||
|
|
||
| import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; | ||
| import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; | ||
|
|
||
| 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 { | ||
|
|
||
| @Test | ||
| void contextLoadsAndApiResponds() throws Exception { | ||
| mockMvc.perform(get("/api/repository")).andExpect(status().isOk()); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| package de.tum.cit.aet.helios; | ||
|
|
||
| import de.tum.cit.aet.helios.github.GitHubClientManager; | ||
| import de.tum.cit.aet.helios.github.GitHubFacade; | ||
| import de.tum.cit.aet.helios.github.GitHubService; | ||
| import io.zonky.test.db.AutoConfigureEmbeddedDatabase; | ||
| import javax.sql.DataSource; | ||
| import org.springframework.beans.factory.annotation.Autowired; | ||
| import org.springframework.boot.test.context.SpringBootTest; | ||
| import org.springframework.boot.webmvc.test.autoconfigure.AutoConfigureMockMvc; | ||
| import org.springframework.test.context.ActiveProfiles; | ||
| import org.springframework.test.context.bean.override.mockito.MockitoBean; | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.sizes.LineLengthCheck> reported by reviewdog 🐶 |
||
| * 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.sizes.LineLengthCheck> reported by reviewdog 🐶 |
||
| * 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.sizes.LineLengthCheck> reported by reviewdog 🐶 |
||
| * ({@code nats.enabled=false}), the schedulers/reconciliation/notifications/AI are turned off, and the | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.sizes.LineLengthCheck> reported by reviewdog 🐶 |
||
| * GitHub clients are mocked so no credentials or network are required. | ||
| */ | ||
| @SpringBootTest | ||
| @ActiveProfiles("test") | ||
| @AutoConfigureMockMvc | ||
| @AutoConfigureEmbeddedDatabase( | ||
| type = AutoConfigureEmbeddedDatabase.DatabaseType.POSTGRES, | ||
| provider = AutoConfigureEmbeddedDatabase.DatabaseProvider.DOCKER) | ||
| public abstract class HeliosIntegrationTest { | ||
|
|
||
| public static final String X_REPOSITORY_ID = "X-REPOSITORY-ID"; | ||
|
|
||
| @Autowired protected MockMvc mockMvc; | ||
| @Autowired protected DataSource dataSource; | ||
|
|
||
| // GitHub clients reach out to GitHub on construction/first use; mock them so the context boots | ||
| // without credentials or network. Read (GET) scoping tests do not exercise GitHub. | ||
| @MockitoBean protected GitHubClientManager gitHubClientManager; | ||
| @MockitoBean protected GitHubFacade gitHubFacade; | ||
| @MockitoBean protected GitHubService gitHubService; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| package de.tum.cit.aet.helios.environment; | ||
|
|
||
| import static org.hamcrest.Matchers.equalTo; | ||
| import static org.hamcrest.Matchers.everyItem; | ||
| import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; | ||
| import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; | ||
| import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; | ||
|
|
||
| import de.tum.cit.aet.helios.HeliosIntegrationTest; | ||
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.springframework.jdbc.core.JdbcTemplate; | ||
|
|
||
| /** | ||
| * Cross-refactor guard: {@code GET /api/environments*} must return only the current repository's | ||
| * environments (scoped by the {@code X-REPOSITORY-ID} header). This passes with the legacy | ||
| * 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.naming.AbbreviationAsWordInNameCheck> reported by reviewdog 🐶 |
||
|
|
||
| private static final long REPO_A = 1L; | ||
| private static final long REPO_B = 2L; | ||
| private static final long ENV_A1 = 11L; | ||
| private static final long ENV_A2 = 12L; | ||
| private static final long ENV_B1 = 21L; | ||
|
|
||
| @BeforeEach | ||
| void seed() { | ||
| JdbcTemplate jdbc = new JdbcTemplate(dataSource); | ||
| jdbc.execute("TRUNCATE TABLE repository CASCADE"); | ||
| insertRepo(jdbc, REPO_A, "ls1intum/repo-a"); | ||
| insertRepo(jdbc, REPO_B, "ls1intum/repo-b"); | ||
| insertEnabledEnv(jdbc, ENV_A1, REPO_A, "a-staging"); | ||
| insertEnabledEnv(jdbc, ENV_A2, REPO_A, "a-production"); | ||
| insertEnabledEnv(jdbc, ENV_B1, REPO_B, "b-production"); | ||
| } | ||
|
|
||
| @Test | ||
| void enabledEnvironmentsAreScopedToRepositoryA() throws Exception { | ||
| mockMvc | ||
| .perform(get("/api/environments/enabled").header(X_REPOSITORY_ID, String.valueOf(REPO_A))) | ||
| .andExpect(status().isOk()) | ||
| .andExpect(jsonPath("$.length()").value(2)) | ||
| .andExpect(jsonPath("$[*].repository.id", everyItem(equalTo((int) REPO_A)))); | ||
| } | ||
|
|
||
| @Test | ||
| void enabledEnvironmentsAreScopedToRepositoryB() throws Exception { | ||
| mockMvc | ||
| .perform(get("/api/environments/enabled").header(X_REPOSITORY_ID, String.valueOf(REPO_B))) | ||
| .andExpect(status().isOk()) | ||
| .andExpect(jsonPath("$.length()").value(1)) | ||
| .andExpect(jsonPath("$[0].repository.id").value((int) REPO_B)); | ||
| } | ||
|
|
||
| @Test | ||
| void allEnvironmentsAreScopedToCurrentRepository() throws Exception { | ||
| mockMvc | ||
| .perform(get("/api/environments").header(X_REPOSITORY_ID, String.valueOf(REPO_A))) | ||
| .andExpect(status().isOk()) | ||
| .andExpect(jsonPath("$.length()").value(2)) | ||
| .andExpect(jsonPath("$[*].repository.id", everyItem(equalTo((int) REPO_A)))); | ||
| } | ||
|
|
||
| @Test | ||
| void environmentByIdIsInvisibleFromAnotherRepository() throws Exception { | ||
| // ENV_B1 belongs to repo B: invisible when scoped to repo A, visible when scoped to repo B. | ||
| mockMvc | ||
| .perform( | ||
| get("/api/environments/{id}", ENV_B1).header(X_REPOSITORY_ID, String.valueOf(REPO_A))) | ||
| .andExpect(status().isNotFound()); | ||
| mockMvc | ||
| .perform( | ||
| get("/api/environments/{id}", ENV_B1).header(X_REPOSITORY_ID, String.valueOf(REPO_B))) | ||
| .andExpect(status().isOk()); | ||
| } | ||
|
|
||
| private static void insertRepo(JdbcTemplate jdbc, long id, String nameWithOwner) { | ||
| jdbc.update( | ||
| "INSERT INTO repository (repository_id, has_issues, has_projects, has_wiki, is_archived, " | ||
| + "is_disabled, is_private, stargazers_count, watchers_count, name_with_owner) " | ||
| + "VALUES (?, false, false, false, false, false, false, 0, 0, ?)", | ||
| id, | ||
| nameWithOwner); | ||
| } | ||
|
|
||
| private static void insertEnabledEnv(JdbcTemplate jdbc, long id, long repositoryId, String name) { | ||
| jdbc.update( | ||
| "INSERT INTO environment (id, repository_id, enabled, locked, name) " | ||
| + "VALUES (?, ?, true, false, ?)", | ||
| id, | ||
| repositoryId, | ||
| name); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.naming.AbbreviationAsWordInNameCheck> reported by reviewdog 🐶
Abbreviation in name 'ContextBootIT' must contain no more than '1' consecutive capital letters.