diff --git a/README.md b/README.md index 105266c..66ba448 100644 --- a/README.md +++ b/README.md @@ -88,7 +88,8 @@ The MVP implements the endpoints defined in `docs/openapi/viron-api.json` and do - Docker + Docker Compose (deployment) - Swagger/OpenAPI (API documentation) - JaCoCo (test coverage) -- Planned: Flyway (future migrations); schema currently comes from `db-scripts/setup/` +- Planned: Flyway (future migrations); schema currently comes from `db-scripts/setup/`, with + changes to an already-created schema kept as hand-run scripts in `db-scripts/migrations/` --- @@ -108,7 +109,7 @@ viron/ │ └── services/ # Business logic ├── src/main/python/ # Python client SDK ├── src/test/java/... # Unit and integration tests - ├── db-scripts/ # SQL schema setup scripts + ├── db-scripts/ # SQL schema setup scripts, and migrations for existing databases ├── docs/ │ ├── MVP.md # Implementation checklist for MVP │ └── openapi/ diff --git a/db-scripts/migrations/2026-08-12_entity_location_one_placement_per_entity.sql b/db-scripts/migrations/2026-08-12_entity_location_one_placement_per_entity.sql new file mode 100644 index 0000000..f524bd0 --- /dev/null +++ b/db-scripts/migrations/2026-08-12_entity_location_one_placement_per_entity.sql @@ -0,0 +1,47 @@ +-- Copyright (c) 2024 Preponderous Software +-- MIT License + +-- Migration for issue #200: one placement per entity. +-- +-- Databases created before this change key viron.entity_location on (entity_id, location_id), +-- which permits the same entity to occupy two locations at once. Two concurrent placements of an +-- unplaced entity at different locations both pass the controller's read-then-write guard and +-- both insert, so the invariant the rest of the service assumes is not actually enforced. +-- Keying on entity_id alone makes the database reject the second insert, which is what lets the +-- losing request be reported as a 409 instead of a 500. +-- +-- db-scripts/setup/create_tables.sql already creates new databases this way; this script brings +-- an existing one into line. It is not applied automatically (the setup scripts run only when +-- Postgres initialises an empty volume), so run it once against each existing database: +-- +-- psql -U "$DATABASE_DB_USERNAME" -d "$DATABASE_DB_NAME" \ +-- -f db-scripts/migrations/2026-08-12_entity_location_one_placement_per_entity.sql +-- +-- Nothing is deleted here. If any entity is already placed twice, the migration aborts and +-- reports the count, leaving it to an operator to decide which placement is the real one. + +BEGIN; + +DO $$ +DECLARE + duplicate_count INT; +BEGIN + SELECT count(*) INTO duplicate_count + FROM ( + SELECT entity_id + FROM viron.entity_location + GROUP BY entity_id + HAVING count(*) > 1 + ) duplicates; + + IF duplicate_count > 0 THEN + RAISE EXCEPTION + 'viron.entity_location places % entity/entities at more than one location; resolve them before applying this migration. To list them: SELECT entity_id, location_id FROM viron.entity_location WHERE entity_id IN (SELECT entity_id FROM viron.entity_location GROUP BY entity_id HAVING count(*) > 1) ORDER BY entity_id, location_id;', + duplicate_count; + END IF; +END $$; + +ALTER TABLE viron.entity_location DROP CONSTRAINT entity_location_pkey; +ALTER TABLE viron.entity_location ADD PRIMARY KEY (entity_id); + +COMMIT; diff --git a/db-scripts/setup/create_tables.sql b/db-scripts/setup/create_tables.sql index e5259c9..c4c66ae 100644 --- a/db-scripts/setup/create_tables.sql +++ b/db-scripts/setup/create_tables.sql @@ -34,10 +34,14 @@ CREATE TABLE IF NOT EXISTS viron.environment ( ); -- entity_location table (entity_id, location_id) +-- An entity occupies at most one location, so entity_id alone is the key: a location may hold +-- many entities, but a second placement of the same entity is rejected by the database rather +-- than only by the read-then-write guard in LocationController.addEntityToLocation, which two +-- concurrent requests can both pass. CREATE TABLE IF NOT EXISTS viron.entity_location ( entity_id INT NOT NULL, location_id INT NOT NULL, - PRIMARY KEY (entity_id, location_id), + PRIMARY KEY (entity_id), FOREIGN KEY (entity_id) REFERENCES viron.entity(entity_id), FOREIGN KEY (location_id) REFERENCES viron.location(location_id) ); diff --git a/docs/openapi/viron-api.json b/docs/openapi/viron-api.json index b7e2837..a9cfbb7 100644 --- a/docs/openapi/viron-api.json +++ b/docs/openapi/viron-api.json @@ -370,7 +370,7 @@ "/api/v1/locations/{locationId}/entity/{entityId}": { "put": { "summary": "Add entity to location", - "description": "Places an unplaced entity at the location. An entity occupies at most one location, so the request is a no-op when the entity is already at that location and a conflict when it is placed elsewhere.", + "description": "Places an unplaced entity at the location. An entity occupies at most one location, so the request is a no-op when the entity is already at that location and a conflict when it is placed elsewhere. The same answers are given when two placements of the same entity are made at once: exactly one of them takes effect, and the others are resolved against the placement it left behind.", "parameters": [ { "$ref": "#/components/parameters/LocationIdParam" }, { "$ref": "#/components/parameters/EntityIdParam" } @@ -384,7 +384,7 @@ } }, "409": { - "description": "Entity is already placed at a different location", + "description": "Entity is already placed at a different location, including when a concurrent request placed it there first", "content": { "application/json": { "schema": { "$ref": "#/components/schemas/ErrorResponse" } } } diff --git a/src/main/java/preponderous/viron/controllers/LocationController.java b/src/main/java/preponderous/viron/controllers/LocationController.java index d4b7bed..703582c 100644 --- a/src/main/java/preponderous/viron/controllers/LocationController.java +++ b/src/main/java/preponderous/viron/controllers/LocationController.java @@ -2,6 +2,7 @@ import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; +import org.springframework.dao.DuplicateKeyException; import org.springframework.http.HttpStatus; import org.springframework.validation.annotation.Validated; import org.springframework.web.bind.annotation.*; @@ -72,6 +73,13 @@ public LocationDto getLocationOfEntity(@PathVariable @Min(1) int entityId) { * Places an unplaced entity at {@code locationId}. An entity occupies at most one location, * so a request for an entity that is already placed elsewhere is a conflict; a request for an * entity already at the target is a no-op, keeping the {@code PUT} idempotent. + * + *

The placement check below cannot decide the outcome on its own: two concurrent requests + * for the same unplaced entity both read no placement and both go on to insert. The primary + * key on {@code viron.entity_location.entity_id} is what settles which of them wins, so the + * loser is recognised by the {@link DuplicateKeyException} its insert raises and answered + * from the placement the winner committed — the same answer the check above would have given + * had the two requests arrived in sequence (#200). */ @PutMapping("/{locationId}/entity/{entityId}") public void addEntityToLocation(@PathVariable("entityId") @Min(1) int entityId, @PathVariable("locationId") @Min(1) int locationId) { @@ -83,15 +91,36 @@ public void addEntityToLocation(@PathVariable("entityId") @Min(1) int entityId, } Optional currentLocation = locationRepository.findByEntityId(entityId); if (currentLocation.isPresent()) { - if (currentLocation.get().getLocationId() == locationId) { - return; + reportPlacement(entityId, locationId, currentLocation.get()); + return; + } + try { + if (!locationRepository.addEntityToLocation(entityId, locationId)) { + throw new ServiceException("Failed to add entity " + entityId + " to location " + locationId); + } + } catch (DuplicateKeyException e) { + log.info("Entity {} was placed concurrently while adding it to location {}", entityId, locationId); + Optional winner = locationRepository.findByEntityId(entityId); + if (winner.isEmpty()) { + // The winning placement was removed again before it could be read back, so there + // is no location to name. The request still failed on a conflict, not a fault. + throw new ConflictException("Entity " + entityId + + " was placed by a concurrent request and could not be added to location " + locationId); } - throw new ConflictException("Entity " + entityId + " is already placed at location " - + currentLocation.get().getLocationId()); + reportPlacement(entityId, locationId, winner.get()); } - if (!locationRepository.addEntityToLocation(entityId, locationId)) { - throw new ServiceException("Failed to add entity " + entityId + " to location " + locationId); + } + + /** + * Answers a placement request for an entity that is already placed: silence when it is + * already where the request wanted it, a conflict naming its actual location otherwise. + */ + private static void reportPlacement(int entityId, int requestedLocationId, Location placement) { + if (placement.getLocationId() == requestedLocationId) { + return; } + throw new ConflictException("Entity " + entityId + " is already placed at location " + + placement.getLocationId()); } @DeleteMapping("/{locationId}/entity/{entityId}") diff --git a/src/main/java/preponderous/viron/database/DbInteractions.java b/src/main/java/preponderous/viron/database/DbInteractions.java index 1b1a580..5e9f18d 100644 --- a/src/main/java/preponderous/viron/database/DbInteractions.java +++ b/src/main/java/preponderous/viron/database/DbInteractions.java @@ -15,6 +15,7 @@ import lombok.extern.slf4j.Slf4j; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.dao.DuplicateKeyException; import org.springframework.jdbc.datasource.DataSourceUtils; import org.springframework.stereotype.Component; @@ -42,6 +43,10 @@ @Component @Slf4j public class DbInteractions { + + /** SQL state for a unique/primary-key violation, as reported by both Postgres and H2. */ + private static final String SQL_STATE_UNIQUE_VIOLATION = "23505"; + private final DataSource dataSource; @Autowired @@ -124,11 +129,46 @@ public Optional queryOne(String query, RowMapper mapper, Object... par * @return {@code true} if at least one row was affected, {@code false} otherwise (including on error) */ public boolean update(String query, Object... params) { + try { + return updateReportingDuplicateKey(query, params); + } catch (DuplicateKeyException e) { + // Flattened to false so this method keeps the contract every existing caller was + // written against; callers that need to tell a conflict apart from any other write + // failure call updateReportingDuplicateKey directly. + log.error("Error executing update: {}", e.getMessage()); + return false; + } + } + + /** + * Execute a parameterized INSERT/UPDATE/DELETE, reporting a unique or primary-key violation + * to the caller instead of flattening it to {@code false}. + * + *

Identical to {@link #update(String, Object...)} in every other respect. It exists because + * a request that loses a race against a concurrent write is a conflict, not a server fault, + * and the two are indistinguishable once the {@link SQLException} has been swallowed + * (#200): {@link preponderous.viron.repositories.LocationRepositoryImpl#addEntityToLocation} + * needs the distinction to answer 409 rather than 500. + * + *

Detection is by SQL state {@code 23505} (unique violation), which Postgres and the H2 + * database the tests run against both report. Every other failure is logged and reported as + * {@code false}, exactly as before. + * + * @param query SQL with {@code ?} placeholders for each parameter + * @param params values to bind to the placeholders, in order + * @return {@code true} if at least one row was affected, {@code false} otherwise (including on + * any error that is not a unique/primary-key violation) + * @throws DuplicateKeyException if the statement violated a unique or primary-key constraint + */ + public boolean updateReportingDuplicateKey(String query, Object... params) { Connection connection = DataSourceUtils.getConnection(dataSource); try (PreparedStatement statement = connection.prepareStatement(query)) { bindParameters(statement, params); return statement.executeUpdate() > 0; } catch (SQLException e) { + if (isUniqueViolation(e)) { + throw new DuplicateKeyException("Update violated a unique constraint: " + e.getMessage(), e); + } log.error("Error executing update: {}", e.getMessage()); } finally { DataSourceUtils.releaseConnection(connection, dataSource); @@ -136,6 +176,25 @@ public boolean update(String query, Object... params) { return false; } + /** + * True if {@code e}, or any exception chained behind it, reports SQL state {@code 23505}. + * + *

The chain is walked because a driver may wrap the violation it actually hit: the + * Postgres driver reports it directly, but batched or nested failures surface through + * {@link SQLException#getNextException()}. + */ + private static boolean isUniqueViolation(SQLException e) { + for (SQLException current = e; current != null; current = current.getNextException()) { + if (SQL_STATE_UNIQUE_VIOLATION.equals(current.getSQLState())) { + return true; + } + if (current == current.getNextException()) { + break; + } + } + return false; + } + private void bindParameters(PreparedStatement statement, Object... params) throws SQLException { for (int i = 0; i < params.length; i++) { statement.setObject(i + 1, params[i]); diff --git a/src/main/java/preponderous/viron/repositories/LocationRepository.java b/src/main/java/preponderous/viron/repositories/LocationRepository.java index a55c5d5..d484d6c 100644 --- a/src/main/java/preponderous/viron/repositories/LocationRepository.java +++ b/src/main/java/preponderous/viron/repositories/LocationRepository.java @@ -10,6 +10,14 @@ public interface LocationRepository { List findByEnvironmentId(int environmentId); List findByGridId(int gridId); Optional findByEntityId(int entityId); + /** + * Places an entity at a location. + * + * @throws org.springframework.dao.DuplicateKeyException if the entity is already placed — + * the database, not the caller's prior read, is what decides this, so a request that + * lost a race against a concurrent placement lands here rather than returning + * {@code false} + */ boolean addEntityToLocation(int entityId, int locationId); boolean removeEntityFromLocation(int entityId, int locationId); boolean removeEntityFromCurrentLocation(int entityId); diff --git a/src/main/java/preponderous/viron/repositories/LocationRepositoryImpl.java b/src/main/java/preponderous/viron/repositories/LocationRepositoryImpl.java index 6672ed5..889c208 100644 --- a/src/main/java/preponderous/viron/repositories/LocationRepositoryImpl.java +++ b/src/main/java/preponderous/viron/repositories/LocationRepositoryImpl.java @@ -57,10 +57,19 @@ public Optional findByEntityId(int entityId) { return dbInteractions.queryOne(query, this::mapResultSetToLocation, entityId); } + /** + * {@inheritDoc} + * + *

The insert is issued through + * {@link DbInteractions#updateReportingDuplicateKey(String, Object...)} so that the primary + * key on {@code entity_id} — the only thing that actually serialises two concurrent + * placements of the same unplaced entity — reaches the caller as a conflict rather than as + * an indistinguishable {@code false}. + */ @Override public boolean addEntityToLocation(int entityId, int locationId) { String query = "INSERT INTO viron.entity_location (entity_id, location_id) VALUES (?, ?)"; - return dbInteractions.update(query, entityId, locationId); + return dbInteractions.updateReportingDuplicateKey(query, entityId, locationId); } @Override diff --git a/src/test/java/preponderous/viron/controllers/ConcurrentPlacementTest.java b/src/test/java/preponderous/viron/controllers/ConcurrentPlacementTest.java new file mode 100644 index 0000000..b8e7e3d --- /dev/null +++ b/src/test/java/preponderous/viron/controllers/ConcurrentPlacementTest.java @@ -0,0 +1,203 @@ +// Copyright (c) 2024 Preponderous Software +// MIT License + +package preponderous.viron.controllers; + +import com.zaxxer.hikari.HikariDataSource; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import preponderous.viron.config.DataSourceConfig; +import preponderous.viron.config.DbConfig; +import preponderous.viron.database.DbInteractions; +import preponderous.viron.exceptions.ConflictException; +import preponderous.viron.mappers.LocationMapperImpl; +import preponderous.viron.models.Location; +import preponderous.viron.repositories.EntityRepositoryImpl; +import preponderous.viron.repositories.LocationRepositoryImpl; + +import javax.sql.DataSource; + +import java.util.ArrayList; +import java.util.List; +import java.util.Optional; +import java.util.concurrent.CyclicBarrier; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.function.IntUnaryOperator; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Exercises the concurrent path through + * {@link LocationController#addEntityToLocation(int, int)} (#200) rather than only its + * sequential guard: several requests race to place the same unplaced entity, and the invariant + * that an entity occupies at most one location has to survive. + * + *

The controller is assembled over real repositories and an in-memory H2 database, because + * what settles the race is the primary key on {@code entity_location.entity_id} — nothing a + * mocked repository could reproduce. The schema mirrors the Postgres one in + * {@code db-scripts/setup/create_tables.sql}, including that key. + * + *

The interleaving is forced rather than hoped for. Simply releasing threads together does + * not reproduce it: the winner commits so quickly that every other request's guard read already + * sees the placement and is refused by the sequential check. {@link GuardReadRacer} therefore + * holds every request at a barrier immediately after its placement read, so all of them observe + * an unplaced entity and only then go on to insert. + */ +class ConcurrentPlacementTest { + + private static final int ENTITY_ID = 1; + private static final int REQUESTS = 8; + private static final int TIMEOUT_SECONDS = 30; + + private DataSource dataSource; + private DbInteractions dbInteractions; + + @BeforeEach + void setUp() { + dataSource = new DataSourceConfig().dataSource(h2Config()); + dbInteractions = new DbInteractions(dataSource); + + dbInteractions.update("DROP TABLE IF EXISTS viron.entity_location"); + dbInteractions.update("DROP TABLE IF EXISTS viron.entity"); + dbInteractions.update("DROP TABLE IF EXISTS viron.location"); + 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.location (location_id INT PRIMARY KEY, x INT, y INT)"); + dbInteractions.update("CREATE TABLE viron.entity_location (" + + "entity_id INT NOT NULL, location_id INT NOT NULL, PRIMARY KEY (entity_id), " + + "FOREIGN KEY (entity_id) REFERENCES viron.entity(entity_id), " + + "FOREIGN KEY (location_id) REFERENCES viron.location(location_id))"); + + dbInteractions.update("INSERT INTO viron.entity (entity_id, name, creation_date) VALUES (?, ?, ?)", + ENTITY_ID, "Alice", "2026-01-01"); + for (int locationId = 1; locationId <= REQUESTS; locationId++) { + dbInteractions.update("INSERT INTO viron.location (location_id, x, y) VALUES (?, ?, ?)", + locationId, locationId, 0); + } + } + + @AfterEach + void tearDown() { + ((HikariDataSource) dataSource).close(); + } + + /** + * Every request asks for a different location, so exactly one placement may survive and every + * other request has to be told, as a conflict, where the entity actually ended up. + */ + @Test + void concurrentPlacementsAtDifferentLocations_leaveExactlyOnePlacement_andReportTheLosersAsConflicts() + throws Exception { + List outcomes = placeConcurrently(request -> request); + + List placements = placedLocationIds(); + assertThat(placements).hasSize(1); + + int winningLocationId = placements.get(0); + List failures = outcomes.stream().filter(outcome -> outcome != null).toList(); + assertThat(failures).hasSize(REQUESTS - 1); + assertThat(failures).allSatisfy(failure -> assertThat(failure) + .isInstanceOf(ConflictException.class) + .hasMessage("Entity " + ENTITY_ID + " is already placed at location " + winningLocationId)); + } + + /** + * Every request asks for the same location, so every one of them got the outcome it wanted: + * the endpoint is idempotent whether the requests arrive together or in sequence. + */ + @Test + void concurrentPlacementsAtTheSameLocation_allSucceed_andLeaveExactlyOnePlacement() throws Exception { + List outcomes = placeConcurrently(request -> 1); + + assertThat(placedLocationIds()).containsExactly(1); + assertThat(outcomes).containsOnlyNulls(); + } + + /** + * Runs {@link #REQUESTS} placements of the same entity at once, each against the location + * {@code targetLocation} derives from its request number. Returns the exception each request + * ended with, or {@code null} where it succeeded, in request order. + */ + private List placeConcurrently(IntUnaryOperator targetLocation) throws Exception { + CyclicBarrier afterGuardRead = new CyclicBarrier(REQUESTS); + LocationController controller = new LocationController( + new GuardReadRacer(dbInteractions, afterGuardRead), + new EntityRepositoryImpl(dbInteractions), + new LocationMapperImpl()); + + ExecutorService executor = Executors.newFixedThreadPool(REQUESTS); + List> futures = new ArrayList<>(); + for (int request = 1; request <= REQUESTS; request++) { + int locationId = targetLocation.applyAsInt(request); + futures.add(executor.submit(() -> { + try { + controller.addEntityToLocation(ENTITY_ID, locationId); + return null; + } catch (Throwable t) { + return t; + } + })); + } + + try { + List outcomes = new ArrayList<>(); + for (Future future : futures) { + outcomes.add(future.get(TIMEOUT_SECONDS, TimeUnit.SECONDS)); + } + return outcomes; + } finally { + executor.shutdownNow(); + } + } + + private List placedLocationIds() { + return dbInteractions.query("SELECT location_id FROM viron.entity_location WHERE entity_id = ?", + rs -> rs.getInt("location_id"), ENTITY_ID); + } + + /** + * The real repository, with every request held at a barrier the first time it reads a + * placement. That is the window the controller's guard cannot cover on its own, and holding + * it open makes the race a certainty instead of a matter of timing. + * + *

Only the first read per thread waits: a request that loses the race reads the placement + * a second time to find out where the entity ended up, and by then the other requests have + * long since left the barrier. + */ + private static class GuardReadRacer extends LocationRepositoryImpl { + private final CyclicBarrier afterGuardRead; + private final ThreadLocal hasWaited = ThreadLocal.withInitial(() -> false); + + GuardReadRacer(DbInteractions dbInteractions, CyclicBarrier afterGuardRead) { + super(dbInteractions); + this.afterGuardRead = afterGuardRead; + } + + @Override + public Optional findByEntityId(int entityId) { + Optional placement = super.findByEntityId(entityId); + if (!hasWaited.get()) { + hasWaited.set(true); + try { + afterGuardRead.await(TIMEOUT_SECONDS, TimeUnit.SECONDS); + } catch (Exception e) { + throw new IllegalStateException("Timed out waiting for the other placements to read", e); + } + } + return placement; + } + } + + private static DbConfig h2Config() { + DbConfig config = new DbConfig(); + config.setDbUrl("jdbc:h2:mem:viron_concurrent_placement;DB_CLOSE_DELAY=-1"); + config.setDbUsername("sa"); + config.setDbPassword(""); + return config; + } +} diff --git a/src/test/java/preponderous/viron/controllers/LocationControllerTest.java b/src/test/java/preponderous/viron/controllers/LocationControllerTest.java index f89ba18..5b68c17 100644 --- a/src/test/java/preponderous/viron/controllers/LocationControllerTest.java +++ b/src/test/java/preponderous/viron/controllers/LocationControllerTest.java @@ -7,6 +7,7 @@ import org.springframework.boot.test.context.SpringBootTest; import org.springframework.boot.test.mock.mockito.MockBean; +import org.springframework.dao.DuplicateKeyException; import org.springframework.test.web.servlet.MockMvc; import preponderous.viron.config.DbConfig; import preponderous.viron.database.DbInteractions; @@ -411,6 +412,57 @@ void addEntityToLocation_UpdateFails() throws Exception { .andExpect(jsonPath("$.message").value("Failed to add entity 1 to location 2")); } + // #200: the guard read no placement, a concurrent request won the race, and the insert was + // rejected by the primary key on entity_id. The loser is told where the entity actually is. + @Test + void addEntityToLocation_LostRaceWithConcurrentPlacementElsewhere_Conflict() throws Exception { + when(locationRepository.findById(2)).thenReturn(Optional.of(new Location(2, 10, 20))); + when(entityRepository.findById(1)).thenReturn(Optional.of(new Entity(1, "Entity1", "2024-01-01"))); + when(locationRepository.findByEntityId(1)) + .thenReturn(Optional.empty()) + .thenReturn(Optional.of(new Location(7, 30, 40))); + when(locationRepository.addEntityToLocation(1, 2)) + .thenThrow(new DuplicateKeyException("entity 1 is already placed")); + + mockMvc.perform(put("/api/v1/locations/2/entity/1")) + .andExpect(status().isConflict()) + .andExpect(jsonPath("$.status").value(409)) + .andExpect(jsonPath("$.message").value("Entity 1 is already placed at location 7")); + } + + // Both requests wanted the same location, so the loser got the outcome it asked for: the + // endpoint stays idempotent whether the two arrive concurrently or in sequence. + @Test + void addEntityToLocation_LostRaceWithConcurrentPlacementAtSameLocation_IsNoOp() throws Exception { + when(locationRepository.findById(2)).thenReturn(Optional.of(new Location(2, 10, 20))); + when(entityRepository.findById(1)).thenReturn(Optional.of(new Entity(1, "Entity1", "2024-01-01"))); + when(locationRepository.findByEntityId(1)) + .thenReturn(Optional.empty()) + .thenReturn(Optional.of(new Location(2, 10, 20))); + when(locationRepository.addEntityToLocation(1, 2)) + .thenThrow(new DuplicateKeyException("entity 1 is already placed")); + + mockMvc.perform(put("/api/v1/locations/2/entity/1")) + .andExpect(status().isOk()); + } + + // The winning placement was removed again before it could be read back: still a conflict + // rather than a fault, but with no location left to name. + @Test + void addEntityToLocation_LostRaceAndWinningPlacementIsGone_Conflict() throws Exception { + when(locationRepository.findById(2)).thenReturn(Optional.of(new Location(2, 10, 20))); + when(entityRepository.findById(1)).thenReturn(Optional.of(new Entity(1, "Entity1", "2024-01-01"))); + when(locationRepository.findByEntityId(1)).thenReturn(Optional.empty()); + when(locationRepository.addEntityToLocation(1, 2)) + .thenThrow(new DuplicateKeyException("entity 1 is already placed")); + + mockMvc.perform(put("/api/v1/locations/2/entity/1")) + .andExpect(status().isConflict()) + .andExpect(jsonPath("$.status").value(409)) + .andExpect(jsonPath("$.message").value( + "Entity 1 was placed by a concurrent request and could not be added to location 2")); + } + @Test void addEntityToLocation_RepositoryThrowsException() throws Exception { when(locationRepository.findById(2)).thenReturn(Optional.of(new Location(2, 10, 20))); diff --git a/src/test/java/preponderous/viron/database/DbInteractionsTest.java b/src/test/java/preponderous/viron/database/DbInteractionsTest.java index cbf6ab5..54223ba 100644 --- a/src/test/java/preponderous/viron/database/DbInteractionsTest.java +++ b/src/test/java/preponderous/viron/database/DbInteractionsTest.java @@ -4,6 +4,7 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.springframework.dao.DuplicateKeyException; import preponderous.viron.config.DataSourceConfig; import preponderous.viron.config.DbConfig; @@ -19,6 +20,7 @@ import java.util.concurrent.TimeUnit; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; /** * Exercises {@link DbInteractions} against an in-memory H2 database to verify @@ -144,6 +146,65 @@ void update_affectingNoRows_returnsFalse() { assertThat(dbInteractions.update("UPDATE person SET name = ? WHERE id = ?", "Nobody", 999)).isFalse(); } + // #200: a duplicate key is a conflict, and updateReportingDuplicateKey is the only way a + // caller can tell it apart from any other failed write. + @Test + void updateReportingDuplicateKey_onDuplicateKey_throws() { + assertThat(dbInteractions.update("INSERT INTO person (id, name) VALUES (?, ?)", 8, "Heidi")).isTrue(); + + assertThatThrownBy(() -> + dbInteractions.updateReportingDuplicateKey("INSERT INTO person (id, name) VALUES (?, ?)", 8, "Ivan")) + .isInstanceOf(DuplicateKeyException.class); + + // The row that was already there is untouched. + assertThat(dbInteractions.queryOne("SELECT id, name FROM person WHERE id = ?", PERSON_MAPPER, 8)) + .contains(new Person(8, "Heidi")); + } + + @Test + void updateReportingDuplicateKey_onAnyOtherFailure_returnsFalseAndDoesNotThrow() { + assertThat(dbInteractions.updateReportingDuplicateKey("UPDATE does_not_exist SET name = ?", "x")).isFalse(); + } + + @Test + void updateReportingDuplicateKey_onSuccess_returnsTrue() { + assertThat(dbInteractions.updateReportingDuplicateKey("INSERT INTO person (id, name) VALUES (?, ?)", 9, "Judy")) + .isTrue(); + assertThat(dbInteractions.queryOne("SELECT id, name FROM person WHERE id = ?", PERSON_MAPPER, 9)).isPresent(); + } + + // The contract every pre-existing caller was written against: update() still flattens a + // duplicate key to false rather than throwing at code that cannot handle it. + @Test + void update_onDuplicateKey_stillReturnsFalse() { + assertThat(dbInteractions.update("INSERT INTO person (id, name) VALUES (?, ?)", 10, "Karl")).isTrue(); + + assertThat(dbInteractions.update("INSERT INTO person (id, name) VALUES (?, ?)", 10, "Liam")).isFalse(); + } + + // A duplicate key leaves nothing checked out: the throwing path returns its connection too. + @Test + void updateReportingDuplicateKey_onDuplicateKey_returnsItsConnectionToThePool() { + HikariDataSource singleConnectionPool = (HikariDataSource) new DataSourceConfig().dataSource(h2Config()); + singleConnectionPool.setMaximumPoolSize(1); + singleConnectionPool.setConnectionTimeout(1000); + + DbInteractions pooled = new DbInteractions(singleConnectionPool); + try { + assertThat(pooled.update("INSERT INTO person (id, name) VALUES (?, ?)", 11, "Mona")).isTrue(); + + for (int i = 0; i < 5; i++) { + assertThatThrownBy(() -> + pooled.updateReportingDuplicateKey("INSERT INTO person (id, name) VALUES (?, ?)", 11, "Nina")) + .isInstanceOf(DuplicateKeyException.class); + } + + assertThat(pooled.queryOne("SELECT id, name FROM person WHERE id = ?", PERSON_MAPPER, 11)).isPresent(); + } finally { + singleConnectionPool.close(); + } + } + // #194: every path must hand its connection back to the pool. A pool of one with a short // acquisition timeout turns any leak — including one on an error path — into a failure on // the very next call. diff --git a/src/test/java/preponderous/viron/repositories/LocationRepositoryImplTest.java b/src/test/java/preponderous/viron/repositories/LocationRepositoryImplTest.java index 21f6049..cebef21 100644 --- a/src/test/java/preponderous/viron/repositories/LocationRepositoryImplTest.java +++ b/src/test/java/preponderous/viron/repositories/LocationRepositoryImplTest.java @@ -4,6 +4,7 @@ import org.mockito.Mockito; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.boot.test.mock.mockito.MockBean; +import org.springframework.dao.DuplicateKeyException; import preponderous.viron.database.DbInteractions; import preponderous.viron.models.Location; @@ -14,6 +15,7 @@ import java.util.Optional; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static preponderous.viron.database.ResultSetAnswers.mapsAllRows; @@ -249,24 +251,38 @@ public void testFindByEntityId_ReturnsEmptyWhenQueryFails() { @Test public void testAddEntityToLocation_DelegatesToUpdateAndReturnsResult() { String query = "INSERT INTO viron.entity_location (entity_id, location_id) VALUES (?, ?)"; - Mockito.when(dbInteractions.update(query, 42, 8)).thenReturn(true); + Mockito.when(dbInteractions.updateReportingDuplicateKey(query, 42, 8)).thenReturn(true); LocationRepositoryImpl repository = new LocationRepositoryImpl(dbInteractions); assertThat(repository.addEntityToLocation(42, 8)).isTrue(); - Mockito.verify(dbInteractions).update(query, 42, 8); + Mockito.verify(dbInteractions).updateReportingDuplicateKey(query, 42, 8); } @Test public void testAddEntityToLocation_ReturnsFalseWhenUpdateFails() { String query = "INSERT INTO viron.entity_location (entity_id, location_id) VALUES (?, ?)"; - Mockito.when(dbInteractions.update(query, 42, 8)).thenReturn(false); + Mockito.when(dbInteractions.updateReportingDuplicateKey(query, 42, 8)).thenReturn(false); LocationRepositoryImpl repository = new LocationRepositoryImpl(dbInteractions); assertThat(repository.addEntityToLocation(42, 8)).isFalse(); } + // #200: a placement rejected by the primary key on entity_id is a conflict the caller has to + // be able to see, so it must not be flattened into the false above. + @Test + public void testAddEntityToLocation_PropagatesDuplicateKeyException() { + String query = "INSERT INTO viron.entity_location (entity_id, location_id) VALUES (?, ?)"; + Mockito.when(dbInteractions.updateReportingDuplicateKey(query, 42, 8)) + .thenThrow(new DuplicateKeyException("entity 42 is already placed")); + + LocationRepositoryImpl repository = new LocationRepositoryImpl(dbInteractions); + + assertThatThrownBy(() -> repository.addEntityToLocation(42, 8)) + .isInstanceOf(DuplicateKeyException.class); + } + @Test public void testRemoveEntityFromLocation_DelegatesToUpdateAndReturnsResult() { String query = "DELETE FROM viron.entity_location WHERE entity_id = ? AND location_id = ?";