The pool is built from {@link DbConfig} rather than Spring Boot's * {@code spring.datasource.*} properties, so the existing {@code database.*} property @@ -39,4 +42,22 @@ public DataSource dataSource(DbConfig dbConfig) { dataSource.setPassword(dbConfig.getDbPassword()); return dataSource; } + + /** + * Manages the transactions that {@code @Transactional} write paths run in. + * + *
This is the manager {@code DataSourceUtils} cooperates with: it binds one pooled + * connection to the current thread with auto-commit off for the length of the transaction, + * so every {@link preponderous.viron.database.DbInteractions} call made inside the boundary + * joins it and a thrown runtime exception discards the whole sequence rather than leaving + * a half-applied cascade behind. + * + *
Spring Boot would auto-configure an equivalent manager, but only while exactly one + * {@link DataSource} candidate exists and no manager is declared. Declaring it next to the + * pool it wraps keeps the boundaries from depending on that condition continuing to hold. + */ + @Bean + public PlatformTransactionManager transactionManager(DataSource dataSource) { + return new DataSourceTransactionManager(dataSource); + } } diff --git a/src/main/java/preponderous/viron/controllers/EntityController.java b/src/main/java/preponderous/viron/controllers/EntityController.java index 6f8d20d..8440d57 100644 --- a/src/main/java/preponderous/viron/controllers/EntityController.java +++ b/src/main/java/preponderous/viron/controllers/EntityController.java @@ -3,6 +3,7 @@ import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.http.HttpStatus; +import org.springframework.transaction.annotation.Transactional; import org.springframework.validation.annotation.Validated; import org.springframework.web.bind.annotation.*; import preponderous.viron.dto.CreateEntityRequest; @@ -73,8 +74,17 @@ public EntityDto createEntity(@Valid @RequestBody CreateEntityRequest request) { return entityMapper.toDto(newEntity); } + /** + * Deletes an entity and its placement. + * + *
{@link EntityRepository#deleteById(int)} clears the placement before deleting the + * entity, so the two statements run in one transaction: the {@link ServiceException} raised + * when the entity delete does not take effect rolls the placement back, rather than leaving + * an entity that has silently lost where it was. + */ @DeleteMapping("/{id}") @ResponseStatus(HttpStatus.NO_CONTENT) + @Transactional public void deleteEntity(@PathVariable @Min(1) int id) { if (entityRepository.findById(id).isEmpty()) { throw new NotFoundException("Entity not found with id: " + id); diff --git a/src/main/java/preponderous/viron/controllers/EnvironmentController.java b/src/main/java/preponderous/viron/controllers/EnvironmentController.java index 5d7773b..a6af65b 100644 --- a/src/main/java/preponderous/viron/controllers/EnvironmentController.java +++ b/src/main/java/preponderous/viron/controllers/EnvironmentController.java @@ -7,6 +7,7 @@ import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.http.HttpStatus; +import org.springframework.transaction.annotation.Transactional; import org.springframework.validation.annotation.Validated; import org.springframework.web.bind.annotation.*; import preponderous.viron.dto.CreateEnvironmentRequest; @@ -70,8 +71,17 @@ public EnvironmentDto createEnvironment(@Valid @RequestBody CreateEnvironmentReq return environmentMapper.toDto(newEnvironment); } + /** + * Deletes an environment and everything it contains. + * + *
The cascade is a long sequence of dependent deletes, so it runs in one transaction: + * the {@link ServiceException} thrown on the first failure discards the deletes already + * performed instead of leaving a partially deleted environment that no request could have + * produced deliberately. + */ @DeleteMapping("/{id}") @ResponseStatus(HttpStatus.NO_CONTENT) + @Transactional public void deleteEnvironment(@PathVariable @Min(1) int id) { if (environmentRepository.findById(id).isEmpty()) { throw new NotFoundException("Environment not found with id: " + id); diff --git a/src/main/java/preponderous/viron/factories/EnvironmentFactory.java b/src/main/java/preponderous/viron/factories/EnvironmentFactory.java index cf4ba3a..75989b6 100644 --- a/src/main/java/preponderous/viron/factories/EnvironmentFactory.java +++ b/src/main/java/preponderous/viron/factories/EnvironmentFactory.java @@ -7,6 +7,7 @@ import lombok.extern.slf4j.Slf4j; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Component; +import org.springframework.transaction.annotation.Transactional; import preponderous.viron.database.DbInteractions; import preponderous.viron.exceptions.EnvironmentCreationException; @@ -26,11 +27,19 @@ public EnvironmentFactory(DbInteractions dbInteractions) { * Creates an environment containing {@code numGrids} grids, each {@code numRows} by * {@code numColumns} locations. Rows and columns are independent — grids need not be square. * + *
The environment, its grids, its locations and every association between them are + * inserted in one transaction, so the {@link EnvironmentCreationException} thrown partway + * through discards the rows already written instead of leaving an environment that is + * missing some of its grids or a grid that is missing some of its locations. The sequences + * the ids come from are not transactional, so a rolled-back attempt still consumes the ids + * it drew — the gap is expected and harmless. + * * @param name name of the environment * @param numGrids number of grids to create in the environment * @param numRows number of rows in each grid * @param numColumns number of columns in each grid */ + @Transactional public Environment createEnvironment(String name, int numGrids, int numRows, int numColumns) throws EnvironmentCreationException { log.info("Attempting to create environment: '{}' with {} grids of size {}x{}", name, numGrids, numRows, numColumns); diff --git a/src/main/java/preponderous/viron/repositories/EntityRepositoryImpl.java b/src/main/java/preponderous/viron/repositories/EntityRepositoryImpl.java index f140e52..39b6abc 100644 --- a/src/main/java/preponderous/viron/repositories/EntityRepositoryImpl.java +++ b/src/main/java/preponderous/viron/repositories/EntityRepositoryImpl.java @@ -91,6 +91,11 @@ public Entity save(Entity entity) { * placement row has to be cleared first or the entity delete is rejected by the constraint. * An entity that is not placed has no row to clear, which is why the first statement's result * is not part of the outcome — only the entity delete itself is reported. + * + *
The two statements are not atomic on their own: a {@code false} return means the entity + * survived while its placement was already cleared. Callers that need the pair to be + * all-or-nothing put a transaction boundary around the call and fail on {@code false} — see + * {@link preponderous.viron.controllers.EntityController#deleteEntity(int)}. */ @Override public boolean deleteById(int id) { diff --git a/src/test/java/preponderous/viron/config/TransactionBoundaryWiringTest.java b/src/test/java/preponderous/viron/config/TransactionBoundaryWiringTest.java new file mode 100644 index 0000000..6a960ec --- /dev/null +++ b/src/test/java/preponderous/viron/config/TransactionBoundaryWiringTest.java @@ -0,0 +1,83 @@ +// Copyright (c) 2024 Preponderous Software +// MIT License + +package preponderous.viron.config; + +import java.lang.reflect.Method; + +import javax.sql.DataSource; + +import org.junit.jupiter.api.Test; +import org.springframework.aop.support.AopUtils; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.context.ApplicationContext; +import org.springframework.jdbc.datasource.DataSourceTransactionManager; +import org.springframework.transaction.PlatformTransactionManager; +import org.springframework.transaction.interceptor.TransactionAttributeSource; +import preponderous.viron.controllers.EntityController; +import preponderous.viron.controllers.EnvironmentController; +import preponderous.viron.factories.EnvironmentFactory; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Verifies that the write paths annotated for #194 are genuinely advised at runtime: an + * {@code @Transactional} annotation only takes effect through a proxy, and only while a + * transaction manager is present. Rollback behaviour itself is proved against a real database in + * {@code preponderous.viron.database.TransactionRollbackTest}. + */ +@SpringBootTest +class TransactionBoundaryWiringTest { + + @Autowired + private ApplicationContext applicationContext; + + @Autowired + private TransactionAttributeSource transactionAttributeSource; + + @Test + void contextExposesOneTransactionManagerBoundToThePooledDataSource() { + assertThat(applicationContext.getBeanNamesForType(PlatformTransactionManager.class)).hasSize(1); + + DataSourceTransactionManager transactionManager = + (DataSourceTransactionManager) applicationContext.getBean(PlatformTransactionManager.class); + assertThat(transactionManager.getDataSource()).isSameAs(applicationContext.getBean(DataSource.class)); + } + + @Test + void environmentCascadeDeleteRunsInATransaction() { + assertTransactional(EnvironmentController.class, "deleteEnvironment", int.class); + } + + @Test + void entityDeleteRunsInATransaction() { + assertTransactional(EntityController.class, "deleteEntity", int.class); + } + + @Test + void environmentCreationRunsInATransaction() { + assertTransactional(EnvironmentFactory.class, "createEnvironment", + String.class, int.class, int.class, int.class); + } + + private void assertTransactional(Class beanType, String methodName, Class... parameterTypes) { + Object bean = applicationContext.getBean(beanType); + assertThat(AopUtils.isAopProxy(bean)) + .as("%s must be proxied for its transaction boundary to apply", beanType.getSimpleName()) + .isTrue(); + + Method method = findMethod(beanType, methodName, parameterTypes); + assertThat(transactionAttributeSource.getTransactionAttribute(method, AopUtils.getTargetClass(bean))) + .as("%s.%s must carry a transaction attribute", beanType.getSimpleName(), methodName) + .isNotNull(); + } + + private static Method findMethod(Class beanType, String methodName, Class... parameterTypes) { + try { + return beanType.getMethod(methodName, parameterTypes); + } catch (NoSuchMethodException e) { + throw new AssertionError(beanType.getSimpleName() + "." + methodName + " no longer exists", e); + } + } +} diff --git a/src/test/java/preponderous/viron/controllers/EntityControllerTest.java b/src/test/java/preponderous/viron/controllers/EntityControllerTest.java index 99b873a..9b45c25 100644 --- a/src/test/java/preponderous/viron/controllers/EntityControllerTest.java +++ b/src/test/java/preponderous/viron/controllers/EntityControllerTest.java @@ -9,7 +9,6 @@ import org.springframework.boot.test.mock.mockito.MockBean; import org.springframework.http.MediaType; import org.springframework.test.web.servlet.MockMvc; -import preponderous.viron.config.DbConfig; import preponderous.viron.database.DbInteractions; import preponderous.viron.dto.EntityDto; import preponderous.viron.exceptions.EntityCreationException; @@ -50,8 +49,9 @@ class EntityControllerTest { @MockBean private DbInteractions dbInteractions; - @MockBean - private DbConfig dbConfig; + // DbConfig is left real, unlike the collaborators above: the transaction boundary on + // deleteEntity opens a connection from the pool DbConfig configures, and a mock would + // supply it a null JDBC URL. // --- GET /api/v1/entities --- diff --git a/src/test/java/preponderous/viron/controllers/EnvironmentControllerTest.java b/src/test/java/preponderous/viron/controllers/EnvironmentControllerTest.java index c26780a..20c5009 100644 --- a/src/test/java/preponderous/viron/controllers/EnvironmentControllerTest.java +++ b/src/test/java/preponderous/viron/controllers/EnvironmentControllerTest.java @@ -9,7 +9,6 @@ import org.springframework.http.MediaType; import org.springframework.test.annotation.DirtiesContext; import org.springframework.test.web.servlet.MockMvc; -import preponderous.viron.config.DbConfig; import preponderous.viron.database.DbInteractions; import preponderous.viron.dto.EnvironmentDto; import preponderous.viron.exceptions.EnvironmentCreationException; @@ -50,8 +49,9 @@ class EnvironmentControllerTest { @MockBean private DbInteractions dbInteractions; - @MockBean - private DbConfig dbConfig; + // DbConfig is left real, unlike the collaborators above: the transaction boundary on + // deleteEnvironment opens a connection from the pool DbConfig configures, and a mock would + // supply it a null JDBC URL. @Test void getAllEnvironments_Success() throws Exception { diff --git a/src/test/java/preponderous/viron/database/TransactionRollbackTest.java b/src/test/java/preponderous/viron/database/TransactionRollbackTest.java new file mode 100644 index 0000000..0c4ef67 --- /dev/null +++ b/src/test/java/preponderous/viron/database/TransactionRollbackTest.java @@ -0,0 +1,133 @@ +// Copyright (c) 2024 Preponderous Software +// MIT License + +package preponderous.viron.database; + +import com.zaxxer.hikari.HikariDataSource; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.transaction.PlatformTransactionManager; +import org.springframework.transaction.support.TransactionTemplate; +import preponderous.viron.config.DataSourceConfig; +import preponderous.viron.config.DbConfig; +import preponderous.viron.exceptions.ServiceException; +import preponderous.viron.repositories.EntityRepositoryImpl; + +import javax.sql.DataSource; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** + * Proves the transaction boundaries added for #194 actually roll back, by running the real + * {@link EntityRepositoryImpl#deleteById(int)} — two dependent statements — against an in-memory + * H2 database through the production {@link DataSourceConfig} wiring. + * + *
The schema is a minimal stand-in for the Postgres one: the entity table, the placement table + * whose row has to be cleared first, and a guard table that exists only so the second statement + * can be made to fail on demand. + */ +class TransactionRollbackTest { + + private static final int ENTITY_ID = 1; + private static final int LOCATION_ID = 7; + + private DataSource dataSource; + private DbInteractions dbInteractions; + private EntityRepositoryImpl entityRepository; + private TransactionTemplate transactionTemplate; + + @BeforeEach + void setUp() { + dataSource = new DataSourceConfig().dataSource(h2Config()); + dbInteractions = new DbInteractions(dataSource); + entityRepository = new EntityRepositoryImpl(dbInteractions); + + PlatformTransactionManager transactionManager = new DataSourceConfig().transactionManager(dataSource); + transactionTemplate = new TransactionTemplate(transactionManager); + + dbInteractions.update("DROP TABLE IF EXISTS viron.entity_delete_guard"); + dbInteractions.update("DROP TABLE IF EXISTS viron.entity_location"); + dbInteractions.update("DROP TABLE IF EXISTS viron.entity"); + dbInteractions.update("CREATE SCHEMA IF NOT EXISTS viron"); + dbInteractions.update( + "CREATE TABLE viron.entity (entity_id INT PRIMARY KEY, name VARCHAR(255), creation_date VARCHAR(255))"); + dbInteractions.update( + "CREATE TABLE viron.entity_location (entity_id INT, location_id INT, PRIMARY KEY (entity_id, location_id))"); + dbInteractions.update( + "CREATE TABLE viron.entity_delete_guard (id INT PRIMARY KEY, entity_id INT NOT NULL REFERENCES viron.entity(entity_id))"); + + dbInteractions.update("INSERT INTO viron.entity (entity_id, name, creation_date) VALUES (?, ?, ?)", + ENTITY_ID, "Alice", "2026-01-01"); + dbInteractions.update("INSERT INTO viron.entity_location (entity_id, location_id) VALUES (?, ?)", + ENTITY_ID, LOCATION_ID); + } + + @AfterEach + void tearDown() { + ((HikariDataSource) dataSource).close(); + } + + // #194: the placement must come back when the entity delete it was cleared for does not happen. + @Test + void failedEntityDelete_insideATransaction_restoresTheClearedPlacement() { + blockEntityDeletion(); + + assertThatThrownBy(() -> transactionTemplate.executeWithoutResult(status -> { + if (!entityRepository.deleteById(ENTITY_ID)) { + throw new ServiceException("Failed to delete entity with id: " + ENTITY_ID); + } + })).isInstanceOf(ServiceException.class); + + assertThat(entityExists()).isTrue(); + assertThat(placementExists()).isTrue(); + } + + // The behaviour the boundary replaces: without one, the first delete commits on its own and + // the surviving entity silently loses where it was. + @Test + void failedEntityDelete_withoutATransaction_leavesTheEntityWithoutItsPlacement() { + blockEntityDeletion(); + + assertThat(entityRepository.deleteById(ENTITY_ID)).isFalse(); + + assertThat(entityExists()).isTrue(); + assertThat(placementExists()).isFalse(); + } + + @Test + void successfulDelete_insideATransaction_commitsBothStatements() { + transactionTemplate.executeWithoutResult(status -> { + if (!entityRepository.deleteById(ENTITY_ID)) { + throw new ServiceException("Failed to delete entity with id: " + ENTITY_ID); + } + }); + + assertThat(entityExists()).isFalse(); + assertThat(placementExists()).isFalse(); + } + + /** Makes {@code DELETE FROM viron.entity} fail by pointing a foreign key at the row. */ + private void blockEntityDeletion() { + dbInteractions.update("INSERT INTO viron.entity_delete_guard (id, entity_id) VALUES (?, ?)", 1, ENTITY_ID); + } + + private boolean entityExists() { + return dbInteractions.queryOne("SELECT entity_id FROM viron.entity WHERE entity_id = ?", + rs -> rs.getInt(1), ENTITY_ID).isPresent(); + } + + private boolean placementExists() { + return dbInteractions.queryOne("SELECT location_id FROM viron.entity_location WHERE entity_id = ?", + rs -> rs.getInt(1), ENTITY_ID).isPresent(); + } + + private static DbConfig h2Config() { + DbConfig config = new DbConfig(); + config.setDbUrl("jdbc:h2:mem:viron_transactionrollback;DB_CLOSE_DELAY=-1"); + config.setDbUsername("sa"); + config.setDbPassword(""); + return config; + } +} diff --git a/src/test/resources/application.properties b/src/test/resources/application.properties index 64dc403..65175ec 100644 --- a/src/test/resources/application.properties +++ b/src/test/resources/application.properties @@ -1,15 +1,20 @@ # Copyright (c) 2024 Preponderous Software # MIT License # -# Test configuration. Mirrors the runtime properties and supplies a fixed JWT secret -# so the Spring context (which now requires app.jwt.secret) starts in tests. +# Test configuration. Mirrors the runtime properties apart from the datasource, which points at +# an in-memory database, and supplies a fixed JWT secret so the Spring context (which now +# requires app.jwt.secret) starts in tests. spring.application.name=viron server.port=9999 -database.dbUrl=jdbc:postgresql://localhost:5432/postgres -database.dbUsername=postgres -database.dbPassword=postgres +# An in-memory database rather than the runtime Postgres. Context tests mock DbInteractions, so +# no SQL is issued against it, but the transaction boundaries on the write paths open a real +# connection before the mocked call — against an unreachable Postgres those requests fail with a +# 500 instead of exercising the controller. +database.dbUrl=jdbc:h2:mem:viron_test;DB_CLOSE_DELAY=-1 +database.dbUsername=sa +database.dbPassword= # Mirrors the runtime setting: no database contributor on /actuator/health, so # SecurityConfigTest's health check does not depend on a reachable database.