diff --git a/src/main/java/net/tfminecraft/vehicleframework/database/VehicleRepository.java b/src/main/java/net/tfminecraft/vehicleframework/database/VehicleRepository.java index 9eb88f3..b461a55 100644 --- a/src/main/java/net/tfminecraft/vehicleframework/database/VehicleRepository.java +++ b/src/main/java/net/tfminecraft/vehicleframework/database/VehicleRepository.java @@ -483,7 +483,7 @@ public void runInTransaction(Runnable work) { } try { database.runTransaction(connection -> work.run()); - } catch (RuntimeException failure) { + } catch (RuntimeException | Error failure) { // Row mutations update these caches eagerly; rollback must restore them too. try { rebuildOccupiedChunks(); diff --git a/src/main/java/net/tfminecraft/vehicleframework/events/VehicleOwnerClaimedEvent.java b/src/main/java/net/tfminecraft/vehicleframework/events/VehicleOwnerClaimedEvent.java deleted file mode 100644 index 50089cf..0000000 --- a/src/main/java/net/tfminecraft/vehicleframework/events/VehicleOwnerClaimedEvent.java +++ /dev/null @@ -1,64 +0,0 @@ -package net.tfminecraft.vehicleframework.events; - -import org.bukkit.entity.Player; -import org.bukkit.event.Cancellable; -import org.bukkit.event.HandlerList; -import org.bukkit.event.player.PlayerEvent; - -import net.tfminecraft.vehicleframework.vehicles.ActiveVehicle; - -/** - * Fired before ownership is written when a player would claim a previously - * unowned ({@code none}) vehicle via interact or admin takeover. Cancel to - * block the claim (owner stays {@code none}). - */ -public final class VehicleOwnerClaimedEvent extends PlayerEvent implements Cancellable { - private static final HandlerList HANDLERS = new HandlerList(); - - private final ActiveVehicle vehicle; - private final String previousOwner; - private final String newOwner; - private boolean cancelled; - - public VehicleOwnerClaimedEvent( - Player player, - ActiveVehicle vehicle, - String previousOwner, - String newOwner) { - super(player); - this.vehicle = vehicle; - this.previousOwner = previousOwner; - this.newOwner = newOwner; - } - - public ActiveVehicle getVehicle() { - return vehicle; - } - - public String getPreviousOwner() { - return previousOwner; - } - - public String getNewOwner() { - return newOwner; - } - - @Override - public boolean isCancelled() { - return cancelled; - } - - @Override - public void setCancelled(boolean cancelled) { - this.cancelled = cancelled; - } - - @Override - public HandlerList getHandlers() { - return HANDLERS; - } - - public static HandlerList getHandlerList() { - return HANDLERS; - } -} diff --git a/src/main/java/net/tfminecraft/vehicleframework/ownership/OwnershipActions.java b/src/main/java/net/tfminecraft/vehicleframework/ownership/OwnershipActions.java index 7f912a9..a6dc8d2 100644 --- a/src/main/java/net/tfminecraft/vehicleframework/ownership/OwnershipActions.java +++ b/src/main/java/net/tfminecraft/vehicleframework/ownership/OwnershipActions.java @@ -16,7 +16,6 @@ import net.tfminecraft.vehicleframework.api.VehicleOwners; import net.tfminecraft.vehicleframework.api.VehicleOwnershipAuthority; import net.tfminecraft.vehicleframework.data.OwnerData; -import net.tfminecraft.vehicleframework.events.VehicleOwnerClaimedEvent; import net.tfminecraft.vehicleframework.events.VehicleOwnershipChangeEvent; import net.tfminecraft.vehicleframework.events.VehicleOwnershipChangedEvent; import net.tfminecraft.vehicleframework.events.VehicleOwnershipEditCheckEvent; @@ -139,7 +138,7 @@ public record Change(Actor actor, ActiveVehicle vehicle, OwnershipReason reason, } /** - * Fires the pre-write event (and the legacy claim event for claims), writes, and always fires + * Fires the pre-write event, writes, and always fires * the post-write event under the same operation ID. * * @return the write result, or empty if a listener cancelled the change @@ -162,14 +161,6 @@ public static Optional perform(Change change) { if (pre.isCancelled()) { return Optional.empty(); } - if (change.reason() == OwnershipReason.CLAIM) { - VehicleOwnerClaimedEvent legacy = - new VehicleOwnerClaimedEvent(actor.player(), vehicle, previousOwner, newOwner); - Bukkit.getPluginManager().callEvent(legacy); - if (legacy.isCancelled()) { - return Optional.empty(); - } - } result = OwnershipStore.writeLive(vehicle, change.mutation()); return Optional.of(result); } finally { diff --git a/src/test/java/net/tfminecraft/vehicleframework/data/RuntimeValuesCoverageTest.java b/src/test/java/net/tfminecraft/vehicleframework/data/RuntimeValuesCoverageTest.java index f66fb13..9b856c4 100644 --- a/src/test/java/net/tfminecraft/vehicleframework/data/RuntimeValuesCoverageTest.java +++ b/src/test/java/net/tfminecraft/vehicleframework/data/RuntimeValuesCoverageTest.java @@ -369,16 +369,6 @@ void eventsPreservePayloadsCancellationAndStableHandlerLists() { assertSame(payload, remove.getPayload()); assertSame(VehicleRemoveEvent.getHandlerList(), remove.getHandlers()); assertTrue(new VehicleRemoveEvent(vehicle, null).getPayload().isRemove()); - VehicleOwnerClaimedEvent owner = - new VehicleOwnerClaimedEvent(player, vehicle, "none", "player_alex"); - assertSame(player, owner.getPlayer()); - assertSame(vehicle, owner.getVehicle()); - assertEquals("none", owner.getPreviousOwner()); - assertEquals("player_alex", owner.getNewOwner()); - assertFalse(owner.isCancelled()); - owner.setCancelled(true); - assertTrue(owner.isCancelled()); - assertSame(VehicleOwnerClaimedEvent.getHandlerList(), owner.getHandlers()); VehiclePreInteractEvent interact = new VehiclePreInteractEvent(player, vehicle); assertSame(vehicle, interact.getVehicle()); assertFalse(interact.isCancelled()); diff --git a/src/test/java/net/tfminecraft/vehicleframework/database/TrackedCarIndexTest.java b/src/test/java/net/tfminecraft/vehicleframework/database/TrackedCarIndexTest.java index 0caea97..7ae382c 100644 --- a/src/test/java/net/tfminecraft/vehicleframework/database/TrackedCarIndexTest.java +++ b/src/test/java/net/tfminecraft/vehicleframework/database/TrackedCarIndexTest.java @@ -1,16 +1,27 @@ package net.tfminecraft.vehicleframework.database; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.AdditionalAnswers.delegatesTo; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mockConstruction; +import static org.mockito.Mockito.withSettings; import java.nio.file.Path; +import java.sql.Connection; import java.util.List; +import java.util.function.Consumer; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; +import net.tfminecraft.tlibs.database.SqliteDatabase; + class TrackedCarIndexTest { private static final String SPLINE = "11111111-2222-3333-4444-555555555555"; @@ -94,6 +105,62 @@ void rolledBackChangesAreUndoneInTheIndex() { assertEquals(2, failure.getSuppressed().length); } + @Test + void errorRollbackRestoresBothIndexesAndRowsAcrossReopen() { + java.io.File file = tempDir.resolve("error-rollback.db").toFile(); + SqliteDatabase sqlite = new SqliteDatabase(file); + // Model the runtime provider's Error rollback contract with the older compile pin. + try (var databases = mockConstruction(SqliteDatabase.class, + withSettings().defaultAnswer(delegatesTo(sqlite)), (database, context) -> { + doAnswer(invocation -> { + Connection connection = sqlite.getConnection(); + connection.setAutoCommit(false); + try { + invocation.>getArgument(0).accept(connection); + connection.commit(); + } catch (RuntimeException | Error failure) { + connection.rollback(); + throw failure; + } finally { + connection.setAutoCommit(true); + } + return null; + }).when(database).runTransaction(any()); + })) { + VehicleRepository repository = VehicleRepository.open(file); + try { + assertTrue(repository.saveLive(snapshot("car-1", payload(7, null), false))); + Error failure = new AssertionError("rollback"); + assertSame(failure, assertThrows(Error.class, () -> repository.runInTransaction(() -> { + repository.tombstone("car-1"); + repository.saveLive(new VehicleSnapshot("car-2", "wagon", "world", 65.5, 64, 2.5, + 0f, 4, 0, payload(9, null), VehicleRepository.SCHEMA_VERSION, 1, false, + System.currentTimeMillis())); + throw failure; + }))); + assertErrorRollbackState(repository); + } finally { + repository.close(); + } + } finally { + sqlite.close(); + } + VehicleRepository reopened = VehicleRepository.open(file); + try { + assertErrorRollbackState(reopened); + } finally { + reopened.close(); + } + } + + private static void assertErrorRollbackState(VehicleRepository repository) { + assertTrue(repository.findLive("car-1").isPresent()); + assertTrue(repository.findLive("car-2").isEmpty()); + assertTrue(repository.hasLiveInChunk("world", 0, 0)); + assertFalse(repository.hasLiveInChunk("world", 4, 0)); + assertEquals(List.of(car("car-1", 7, null)), repository.trackedCars()); + } + private static TrackedCar car(String uuid, double s, String parent) { return new TrackedCar(uuid, SPLINE, s, parent, "world", 1.5, 64, 2.5); } diff --git a/src/test/java/net/tfminecraft/vehicleframework/events/VehicleOwnerClaimedEventTest.java b/src/test/java/net/tfminecraft/vehicleframework/events/VehicleOwnerClaimedEventTest.java deleted file mode 100644 index 6c47482..0000000 --- a/src/test/java/net/tfminecraft/vehicleframework/events/VehicleOwnerClaimedEventTest.java +++ /dev/null @@ -1,26 +0,0 @@ -package net.tfminecraft.vehicleframework.events; - -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertSame; -import static org.junit.jupiter.api.Assertions.assertTrue; - -import org.bukkit.event.Cancellable; -import org.junit.jupiter.api.Test; - -class VehicleOwnerClaimedEventTest { - @Test - void handlerList_isShared() { - assertNotNull(VehicleOwnerClaimedEvent.getHandlerList()); - assertSame(VehicleOwnerClaimedEvent.getHandlerList(), VehicleOwnerClaimedEvent.getHandlerList()); - } - - @Test - void implementsCancellable_defaultNotCancelled() { - assertTrue(Cancellable.class.isAssignableFrom(VehicleOwnerClaimedEvent.class)); - VehicleOwnerClaimedEvent event = new VehicleOwnerClaimedEvent(null, null, "none", "player_Test"); - assertFalse(event.isCancelled()); - event.setCancelled(true); - assertTrue(event.isCancelled()); - } -} diff --git a/src/test/java/net/tfminecraft/vehicleframework/managers/OwnershipManagerTest.java b/src/test/java/net/tfminecraft/vehicleframework/managers/OwnershipManagerTest.java index e76b8e9..7d6e9f9 100644 --- a/src/test/java/net/tfminecraft/vehicleframework/managers/OwnershipManagerTest.java +++ b/src/test/java/net/tfminecraft/vehicleframework/managers/OwnershipManagerTest.java @@ -52,7 +52,6 @@ import net.tfminecraft.vehicleframework.data.OwnerData; import net.tfminecraft.vehicleframework.database.VehiclePersistence; import net.tfminecraft.vehicleframework.enums.VFGUI; -import net.tfminecraft.vehicleframework.events.VehicleOwnerClaimedEvent; import net.tfminecraft.vehicleframework.events.VehicleOwnershipChangeEvent; import net.tfminecraft.vehicleframework.events.VehicleOwnershipChangedEvent; import net.tfminecraft.vehicleframework.events.VehicleOwnershipEditCheckEvent; @@ -238,7 +237,7 @@ void aClaimStampsTheCurrentCharacterAndDropsThePreviousCrew() { VehicleOwnershipChangeEvent pre = fired(VehicleOwnershipChangeEvent.class).getFirst(); assertEquals(OwnershipReason.CLAIM, pre.getReason()); assertEquals(kira.ref(), pre.getActor().orElseThrow()); assertEquals(kira.ref(), pre.getTarget().orElseThrow()); - assertEquals(owner(kira), fired(VehicleOwnerClaimedEvent.class).getFirst().getNewOwner()); + assertEquals(owner(kira), pre.getNewOwner()); assertTrue(fired(VehicleOwnershipChangedEvent.class).getFirst().isSuccess()); verify(persistence).saveLive(v); } @@ -610,7 +609,6 @@ void staffTakeoverOfAnOwnedVehicleGoesThroughTheTakeoverEvent() { VehicleOwnershipChangeEvent pre = fired(VehicleOwnershipChangeEvent.class).getFirst(); assertEquals(OwnershipReason.TAKEOVER, pre.getReason()); assertTrue(pre.isStaff()); assertEquals(warden.ref(), pre.getTarget().orElseThrow()); - assertTrue(fired(VehicleOwnerClaimedEvent.class).isEmpty()); verify(staff).sendMessage("§aYou are now the owner of §eCart§a."); } @@ -623,7 +621,7 @@ void staffTakeoverOfAnUnownedVehicleIsAClaimAndCanNameAnotherCharacter() { assertTrue(manager.takeover(staff, v)); assertEquals(owner(bren), v.getOwnerData().getOwner()); assertEquals(OwnershipReason.CLAIM, fired(VehicleOwnershipChangeEvent.class).getFirst().getReason()); - assertEquals(1, fired(VehicleOwnerClaimedEvent.class).size()); + assertEquals(1, fired(VehicleOwnershipChangeEvent.class).size()); verify(staff).sendMessage("§eCart§a now belongs to §eBren§a."); Lookup.results.clear(); manager.startTakeover(staff, "Bren"); diff --git a/src/test/java/net/tfminecraft/vehicleframework/managers/VehicleManagerCoverageTest.java b/src/test/java/net/tfminecraft/vehicleframework/managers/VehicleManagerCoverageTest.java index 3839501..db5fe32 100644 --- a/src/test/java/net/tfminecraft/vehicleframework/managers/VehicleManagerCoverageTest.java +++ b/src/test/java/net/tfminecraft/vehicleframework/managers/VehicleManagerCoverageTest.java @@ -732,10 +732,10 @@ void pendingEntitySeatSelectionChecksAvailabilityBeforeMounting(String outcome) @Test void interactionHonorsCancelledOwnershipCooldownAndItemIntegrations() throws Exception { ActiveVehicle v = vehicle("v"); Player player = player("Ryan"); register(v); - doAnswer(call -> { if (call.getArgument(0) instanceof VehicleOwnerClaimedEvent claim) claim.setCancelled(true); return null; }).when(plugins).callEvent(any(Event.class)); + doAnswer(call -> { if (call.getArgument(0) instanceof VehicleOwnershipChangeEvent claim) claim.setCancelled(true); return null; }).when(plugins).callEvent(any(Event.class)); manager.vehicleInteract(interact(player, v.getEntity())); assertEquals("none", v.getOwnerData().getOwner()); map("cooldown").put(player, Long.MAX_VALUE); - manager.vehicleInteract(interact(player, v.getEntity())); verify(plugins, times(1)).callEvent(isA(VehicleOwnerClaimedEvent.class)); + manager.vehicleInteract(interact(player, v.getEntity())); verify(plugins, times(1)).callEvent(isA(VehicleOwnershipChangeEvent.class)); map("cooldown").clear(); tapeInteractions.when(() -> TrainTapeInteract.handle(player, v)).thenReturn(true); PlayerInteractEntityEvent event = interact(player, v.getEntity()); manager.vehicleInteract(event); assertTrue(event.isCancelled()); map("cooldown").clear(); tapeInteractions.when(() -> TrainTapeInteract.handle(player, v)).thenReturn(false); diff --git a/src/test/java/net/tfminecraft/vehicleframework/ownership/OwnershipActionsTest.java b/src/test/java/net/tfminecraft/vehicleframework/ownership/OwnershipActionsTest.java index 3950185..86311ce 100644 --- a/src/test/java/net/tfminecraft/vehicleframework/ownership/OwnershipActionsTest.java +++ b/src/test/java/net/tfminecraft/vehicleframework/ownership/OwnershipActionsTest.java @@ -30,7 +30,6 @@ import net.tfminecraft.vehicleframework.api.VehicleOwnershipAuthority; import net.tfminecraft.vehicleframework.data.OwnerData; import net.tfminecraft.vehicleframework.database.VehiclePersistence; -import net.tfminecraft.vehicleframework.events.VehicleOwnerClaimedEvent; import net.tfminecraft.vehicleframework.events.VehicleOwnershipChangeEvent; import net.tfminecraft.vehicleframework.events.VehicleOwnershipChangedEvent; import net.tfminecraft.vehicleframework.events.VehicleOwnershipEditCheckEvent; @@ -184,17 +183,16 @@ private Optional claim(Consumer mutation) { } @Test - void aWrittenClaimFiresPreLegacyAndPostEventsUnderOneOperation() { + void aWrittenClaimFiresOnlyPreAndPostEventsUnderOneOperation() { String owner = "char_" + kira.ref().characterId(); assertEquals(Optional.of(Result.OK), claim(d -> d.setOwner(owner))); assertEquals(owner, data.getOwner()); - assertEquals(3, events.size()); + assertEquals(2, events.size()); VehicleOwnershipChangeEvent pre = (VehicleOwnershipChangeEvent) events.get(0); - VehicleOwnerClaimedEvent legacy = (VehicleOwnerClaimedEvent) events.get(1); - VehicleOwnershipChangedEvent post = (VehicleOwnershipChangedEvent) events.get(2); + VehicleOwnershipChangedEvent post = (VehicleOwnershipChangedEvent) events.get(1); assertEquals("none", pre.getPreviousOwner()); assertEquals(owner, pre.getNewOwner()); assertEquals(kira.ref(), pre.getTarget().orElseThrow()); - assertEquals(owner, legacy.getNewOwner()); assertSame(alex, legacy.getPlayer()); + assertSame(alex, pre.getPlayer()); assertTrue(post.isSuccess()); assertEquals(pre.getOperationId(), post.getOperationId()); assertEquals("none", post.getPrevious().owner()); assertEquals(owner, post.getCurrent().owner()); assertEquals(java.util.UUID.fromString(vehicle.getUUID()), post.getVehicleId()); @@ -208,24 +206,12 @@ void aCancelledChangeStillReportsFailureUnderItsOperation() { }).when(plugins).callEvent(any(Event.class)); assertTrue(claim(d -> d.setOwner("char_x")).isEmpty()); assertEquals("none", data.getOwner()); - assertTrue(fired(VehicleOwnerClaimedEvent.class).isEmpty()); VehicleOwnershipChangedEvent post = fired(VehicleOwnershipChangedEvent.class).getFirst(); assertFalse(post.isSuccess()); assertEquals(fired(VehicleOwnershipChangeEvent.class).getFirst().getOperationId(), post.getOperationId()); verify(persistence, never()).saveLive(any(ActiveVehicle.class)); } - @Test - void theLegacyClaimEventCanStillVetoAClaim() { - doAnswer(call -> { - if (call.getArgument(0) instanceof VehicleOwnerClaimedEvent legacy) legacy.setCancelled(true); - return events.add(call.getArgument(0)); - }).when(plugins).callEvent(any(Event.class)); - assertTrue(claim(d -> d.setOwner("char_x")).isEmpty()); - assertEquals("none", data.getOwner()); - assertFalse(fired(VehicleOwnershipChangedEvent.class).getFirst().isSuccess()); - } - @Test void aFailedSaveRestoresTheVehicleAndReportsFailure() { data.setWhiteList(new ArrayList<>(List.of("char_old")));