From 91f02da9cfc7f78a0be181ad6cfdd561113dd6f7 Mon Sep 17 00:00:00 2001 From: Dan Crosby Date: Thu, 16 Jul 2026 10:00:45 -0700 Subject: [PATCH] Contract Visit Town database column (#8646) --- docs/SQLITE_ACTION_RESULT_DECOMPOSITION.md | 1 - .../service/ChangedProvinceTableWriter.scala | 12 +++- .../eagle/service/PostgresHistory.scala | 12 ++-- .../eagle0/eagle/service/SqliteHistory.scala | 21 +++++- .../service/PostgresGameSchemaPoolTest.scala | 10 +-- .../PostgresHistorySchemaBatchTest.scala | 68 +++++++++++++------ .../eagle/service/SqliteHistoryTest.scala | 24 +++---- 7 files changed, 101 insertions(+), 47 deletions(-) diff --git a/docs/SQLITE_ACTION_RESULT_DECOMPOSITION.md b/docs/SQLITE_ACTION_RESULT_DECOMPOSITION.md index bbba403268..3c75b10dd3 100644 --- a/docs/SQLITE_ACTION_RESULT_DECOMPOSITION.md +++ b/docs/SQLITE_ACTION_RESULT_DECOMPOSITION.md @@ -156,7 +156,6 @@ CREATE TABLE action_changed_provinces ( support_delta REAL, -- round properties set_has_acted INTEGER, -- 0/1/NULL - set_ruler_is_traveling INTEGER, -- legacy, retained until Visit Town contraction set_ruler_is_visiting_town INTEGER, -- ruling faction changes clear_ruling_faction_id INTEGER NOT NULL DEFAULT 0, diff --git a/src/main/scala/net/eagle0/eagle/service/ChangedProvinceTableWriter.scala b/src/main/scala/net/eagle0/eagle/service/ChangedProvinceTableWriter.scala index fe9f48cf98..d9a86cf6e2 100644 --- a/src/main/scala/net/eagle0/eagle/service/ChangedProvinceTableWriter.scala +++ b/src/main/scala/net/eagle0/eagle/service/ChangedProvinceTableWriter.scala @@ -60,7 +60,7 @@ private[service] object ChangedProvinceTableWriter { | infrastructure_devastation_delta REAL, | support_delta REAL, | set_has_acted INTEGER, - | set_ruler_is_traveling INTEGER, + | set_ruler_is_visiting_town INTEGER, | clear_ruling_faction_id INTEGER NOT NULL DEFAULT 0, | new_ruling_faction_id INTEGER, | new_locked_improvement_kind TEXT, @@ -89,6 +89,8 @@ private[service] object ChangedProvinceTableWriter { private val backfillLegacyTravelColumnStatement: String = "UPDATE action_changed_provinces SET set_ruler_is_traveling = set_ruler_is_visiting_town " + "WHERE set_ruler_is_traveling IS NULL" + private val dropLegacyTravelColumnStatement: String = + "ALTER TABLE action_changed_provinces DROP COLUMN set_ruler_is_traveling" private[service] def visitTownExpansionStatements( hasLegacyTravelColumn: Boolean, @@ -112,6 +114,14 @@ private[service] object ChangedProvinceTableWriter { Option.when(hasLegacyTravelColumn)(backfillVisitTownColumnStatement) else Option.when(hasLegacyTravelColumn)(backfillVisitTownColumnStatement).toVector + private[service] def visitTownContractionStatements( + hasLegacyTravelColumn: Boolean, + hasVisitTownColumn: Boolean + ): Vector[String] = + Option.when(!hasVisitTownColumn)(addVisitTownColumnStatement).toVector ++ + Option.when(hasLegacyTravelColumn)(backfillVisitTownColumnStatement) ++ + Option.when(hasLegacyTravelColumn)(dropLegacyTravelColumnStatement) + /** * Write one `ChangedProvince` for `(actionSeq, n, cp.id)`: the v2 scalar columns (denormalized for querying) plus the * full proto in `changed_province_proto`. `n` is the position within the source `ActionResult`'s `changedProvinces` diff --git a/src/main/scala/net/eagle0/eagle/service/PostgresHistory.scala b/src/main/scala/net/eagle0/eagle/service/PostgresHistory.scala index 9eb290853b..98044e17e2 100644 --- a/src/main/scala/net/eagle0/eagle/service/PostgresHistory.scala +++ b/src/main/scala/net/eagle0/eagle/service/PostgresHistory.scala @@ -686,7 +686,7 @@ object PostgresHistory { // Keep v1 during the additive phase so the old blue/green container can still lazy-load expanded schemas. private[service] val SchemaVersion = 1 // Prepared schemas need a new namespace even though the additive layout remains readable by schema-v1 servers. - private[service] val SchemaLayoutVersion = 2 + private[service] val SchemaLayoutVersion = 3 private val applier = SqliteHistory.applier val ActionResultInsertSql: String = @@ -887,7 +887,7 @@ object PostgresHistory { private[service] def initializeSchema(connection: Connection): Unit = { execEach(connection, Vector("CREATE TABLE IF NOT EXISTS metadata (key TEXT PRIMARY KEY, value TEXT NOT NULL)")) val version = readSchemaVersion(connection) - if version == SchemaVersion then ensureVisitTownCutover(connection) + if version == SchemaVersion then ensureVisitTownContraction(connection) else if version == 0 then { val originalAutoCommit = connection.getAutoCommit connection.setAutoCommit(false) @@ -907,8 +907,8 @@ object PostgresHistory { end if } - private def ensureVisitTownCutover(connection: Connection): Unit = { - val statements = ChangedProvinceTableWriter.visitTownCutoverStatements( + private def ensureVisitTownContraction(connection: Connection): Unit = { + val statements = ChangedProvinceTableWriter.visitTownContractionStatements( hasLegacyTravelColumn = columnExists(connection, "action_changed_provinces", "set_ruler_is_traveling"), hasVisitTownColumn = columnExists(connection, "action_changed_provinces", "set_ruler_is_visiting_town") ) @@ -1178,10 +1178,6 @@ object PostgresHistory { |)""".stripMargin ) ++ ChangedProvinceTableWriter.schemaStatements ++ - ChangedProvinceTableWriter.visitTownExpansionStatements( - hasLegacyTravelColumn = true, - hasVisitTownColumn = false - ) ++ ChangedHeroTableWriter.schemaStatements ++ ChangedFactionTableWriter.schemaStatements ++ ChangedBattalionTableWriter.schemaStatements ++ diff --git a/src/main/scala/net/eagle0/eagle/service/SqliteHistory.scala b/src/main/scala/net/eagle0/eagle/service/SqliteHistory.scala index 387d96babf..52a225a8b6 100644 --- a/src/main/scala/net/eagle0/eagle/service/SqliteHistory.scala +++ b/src/main/scala/net/eagle0/eagle/service/SqliteHistory.scala @@ -1744,7 +1744,8 @@ object SqliteHistory { Migration(23, applyV23DecisionRecords), Migration(24, applyV24RenameVisitTownColumn), Migration(25, applyV25ExpandVisitTownColumns), - Migration(26, applyV26CutOverVisitTownColumn) + Migration(26, applyV26CutOverVisitTownColumn), + Migration(27, applyV27ContractVisitTownColumn) ) val latestVersion: Int = all.lastOption.map(_.version).getOrElse(0) @@ -2117,6 +2118,24 @@ object SqliteHistory { ) ) ) + + private def applyV27ContractVisitTownColumn(connection: Connection): Unit = + if tableExists(connection, "action_changed_provinces") then + execEach( + connection, + ChangedProvinceTableWriter.visitTownContractionStatements( + hasLegacyTravelColumn = columnExists( + connection, + "action_changed_provinces", + "set_ruler_is_traveling" + ), + hasVisitTownColumn = columnExists( + connection, + "action_changed_provinces", + "set_ruler_is_visiting_town" + ) + ) + ) } /** diff --git a/src/test/scala/net/eagle0/eagle/service/PostgresGameSchemaPoolTest.scala b/src/test/scala/net/eagle0/eagle/service/PostgresGameSchemaPoolTest.scala index 8c8353fe43..0fccce28ea 100644 --- a/src/test/scala/net/eagle0/eagle/service/PostgresGameSchemaPoolTest.scala +++ b/src/test/scala/net/eagle0/eagle/service/PostgresGameSchemaPoolTest.scala @@ -44,8 +44,8 @@ class PostgresGameSchemaPoolTest extends AnyFlatSpec with Matchers with MockFact "poolSchemaNames" should "include history, layout, and client text schema versions" in { PostgresGameSchemaPool.poolSchemaNames(2) shouldBe Vector( - "eagle_pool_h1_l2_t2_0", - "eagle_pool_h1_l2_t2_1" + "eagle_pool_h1_l3_t2_0", + "eagle_pool_h1_l3_t2_1" ) } @@ -68,13 +68,13 @@ class PostgresGameSchemaPoolTest extends AnyFlatSpec with Matchers with MockFact (() => targetStatement.close()).expects().once(): Unit (connection.prepareStatement(_: String)).expects(schemaExistsSql).returning(firstStatement).once(): Unit - firstStatement.setString.expects(1, "eagle_pool_h1_l2_t2_0").once(): Unit + firstStatement.setString.expects(1, "eagle_pool_h1_l3_t2_0").once(): Unit (() => firstStatement.executeQuery()).expects().returning(firstResult).once(): Unit (() => firstResult.next()).expects().returning(false).once(): Unit (() => firstStatement.close()).expects().once(): Unit (connection.prepareStatement(_: String)).expects(schemaExistsSql).returning(secondStatement).once(): Unit - secondStatement.setString.expects(1, "eagle_pool_h1_l2_t2_1").once(): Unit + secondStatement.setString.expects(1, "eagle_pool_h1_l3_t2_1").once(): Unit (() => secondStatement.executeQuery()).expects().returning(secondResult).once(): Unit (() => secondResult.next()).expects().returning(true).once(): Unit (() => secondStatement.close()).expects().once(): Unit @@ -82,7 +82,7 @@ class PostgresGameSchemaPoolTest extends AnyFlatSpec with Matchers with MockFact (() => connection.createStatement()).expects().returning(renameStatement).once(): Unit (renameStatement .execute(_: String)) - .expects("ALTER SCHEMA eagle_pool_h1_l2_t2_1 RENAME TO eagle_game_1234") + .expects("ALTER SCHEMA eagle_pool_h1_l3_t2_1 RENAME TO eagle_game_1234") .returning(false) .once(): Unit (() => renameStatement.close()).expects().once(): Unit diff --git a/src/test/scala/net/eagle0/eagle/service/PostgresHistorySchemaBatchTest.scala b/src/test/scala/net/eagle0/eagle/service/PostgresHistorySchemaBatchTest.scala index 6bd5f33073..6e0c771b4c 100644 --- a/src/test/scala/net/eagle0/eagle/service/PostgresHistorySchemaBatchTest.scala +++ b/src/test/scala/net/eagle0/eagle/service/PostgresHistorySchemaBatchTest.scala @@ -67,17 +67,47 @@ class PostgresHistorySchemaBatchTest extends AnyFlatSpec with Matchers with Mock ) } - it should "cut over an existing PostgreSQL v1 schema without changing its version" in { - val connection = mock[Connection] - val metadataStatement = mock[Statement] - val versionStatement = mock[PreparedStatement] - val versionResult = mock[ResultSet] - val legacyStatement = mock[PreparedStatement] - val legacyResult = mock[ResultSet] - val visitTownStatement = mock[PreparedStatement] - val visitTownResult = mock[ResultSet] - val expansionStatement = mock[Statement] - val columnExistsSql = + "Visit Town schema contraction" should "backfill and drop the legacy column" in { + ChangedProvinceTableWriter.visitTownContractionStatements( + hasLegacyTravelColumn = true, + hasVisitTownColumn = true + ) shouldBe Vector( + "UPDATE action_changed_provinces SET set_ruler_is_visiting_town = set_ruler_is_traveling " + + "WHERE set_ruler_is_visiting_town IS NULL", + "ALTER TABLE action_changed_provinces DROP COLUMN set_ruler_is_traveling" + ) + } + + it should "leave a fully contracted schema unchanged" in { + ChangedProvinceTableWriter.visitTownContractionStatements( + hasLegacyTravelColumn = false, + hasVisitTownColumn = true + ) shouldBe empty + } + + it should "preserve legacy data when contracting an old-only schema" in { + ChangedProvinceTableWriter.visitTownContractionStatements( + hasLegacyTravelColumn = true, + hasVisitTownColumn = false + ) shouldBe Vector( + "ALTER TABLE action_changed_provinces ADD COLUMN set_ruler_is_visiting_town INTEGER", + "UPDATE action_changed_provinces SET set_ruler_is_visiting_town = set_ruler_is_traveling " + + "WHERE set_ruler_is_visiting_town IS NULL", + "ALTER TABLE action_changed_provinces DROP COLUMN set_ruler_is_traveling" + ) + } + + it should "contract an existing PostgreSQL v1 schema without changing its version" in { + val connection = mock[Connection] + val metadataStatement = mock[Statement] + val versionStatement = mock[PreparedStatement] + val versionResult = mock[ResultSet] + val legacyStatement = mock[PreparedStatement] + val legacyResult = mock[ResultSet] + val visitTownStatement = mock[PreparedStatement] + val visitTownResult = mock[ResultSet] + val contractionStatement = mock[Statement] + val columnExistsSql = "SELECT 1 FROM information_schema.columns\n" + "WHERE table_schema = current_schema() AND table_name = ? AND column_name = ?" @@ -110,23 +140,23 @@ class PostgresHistorySchemaBatchTest extends AnyFlatSpec with Matchers with Mock visitTownStatement.setString.expects(1, "action_changed_provinces").once(): Unit visitTownStatement.setString.expects(2, "set_ruler_is_visiting_town").once(): Unit (() => visitTownStatement.executeQuery()).expects().returning(visitTownResult).once(): Unit - (() => visitTownResult.next()).expects().returning(false).once(): Unit + (() => visitTownResult.next()).expects().returning(true).once(): Unit (() => visitTownStatement.close()).expects().once(): Unit (() => connection.getAutoCommit).expects().returning(true).once(): Unit connection.setAutoCommit.expects(false).once(): Unit - (() => connection.createStatement()).expects().returning(expansionStatement).once(): Unit - expansionStatement.addBatch - .expects("ALTER TABLE action_changed_provinces ADD COLUMN set_ruler_is_visiting_town INTEGER") - .once(): Unit - expansionStatement.addBatch + (() => connection.createStatement()).expects().returning(contractionStatement).once(): Unit + contractionStatement.addBatch .expects( "UPDATE action_changed_provinces SET set_ruler_is_visiting_town = set_ruler_is_traveling " + "WHERE set_ruler_is_visiting_town IS NULL" ) .once(): Unit - (() => expansionStatement.executeBatch()).expects().returning(Array(0, 0)).once(): Unit - (() => expansionStatement.close()).expects().once(): Unit + contractionStatement.addBatch + .expects("ALTER TABLE action_changed_provinces DROP COLUMN set_ruler_is_traveling") + .once(): Unit + (() => contractionStatement.executeBatch()).expects().returning(Array(0, 0)).once(): Unit + (() => contractionStatement.close()).expects().once(): Unit (() => connection.commit()).expects().once(): Unit connection.setAutoCommit.expects(true).once(): Unit } diff --git a/src/test/scala/net/eagle0/eagle/service/SqliteHistoryTest.scala b/src/test/scala/net/eagle0/eagle/service/SqliteHistoryTest.scala index 68eb99d898..4d130ccd41 100644 --- a/src/test/scala/net/eagle0/eagle/service/SqliteHistoryTest.scala +++ b/src/test/scala/net/eagle0/eagle/service/SqliteHistoryTest.scala @@ -1072,21 +1072,19 @@ class SqliteHistoryTest extends AnyFlatSpec with Matchers with BeforeAndAfterEac withRawConnection { conn => readSchemaVersion(conn) shouldBe Some(SqliteHistory.latestSchemaVersion) - columnsOf(conn, "action_changed_provinces") should contain allOf ( - "set_ruler_is_traveling", - "set_ruler_is_visiting_town" - ) + val columns = columnsOf(conn, "action_changed_provinces") + columns should contain("set_ruler_is_visiting_town") + columns should not contain "set_ruler_is_traveling" } } - it should "expand a v24 Visit Town schema before cutover" in { + it should "contract a v24 Visit Town schema through all pending phases" in { val history = SqliteHistory.create(dbFile, emptyStartingState) history.close() withRawConnection { conn => val stmt = conn.createStatement() try { - val _ = stmt.execute("ALTER TABLE action_changed_provinces DROP COLUMN set_ruler_is_traveling") val _ = stmt.execute("UPDATE metadata SET value = '24' WHERE key = 'schema_version'") } finally stmt.close() } @@ -1096,10 +1094,9 @@ class SqliteHistoryTest extends AnyFlatSpec with Matchers with BeforeAndAfterEac withRawConnection { conn => readSchemaVersion(conn) shouldBe Some(SqliteHistory.latestSchemaVersion) - columnsOf(conn, "action_changed_provinces") should contain allOf ( - "set_ruler_is_traveling", - "set_ruler_is_visiting_town" - ) + val columns = columnsOf(conn, "action_changed_provinces") + columns should contain("set_ruler_is_visiting_town") + columns should not contain "set_ruler_is_traveling" } } @@ -1114,6 +1111,9 @@ class SqliteHistoryTest extends AnyFlatSpec with Matchers with BeforeAndAfterEac withRawConnection { conn => val stmt = conn.createStatement() try { + val _ = stmt.execute( + "ALTER TABLE action_changed_provinces ADD COLUMN set_ruler_is_traveling INTEGER" + ) val _ = stmt.execute( "UPDATE action_changed_provinces " + "SET set_ruler_is_traveling = 1, set_ruler_is_visiting_town = NULL" @@ -1126,6 +1126,7 @@ class SqliteHistoryTest extends AnyFlatSpec with Matchers with BeforeAndAfterEac reloaded.close() withRawConnection { conn => + columnsOf(conn, "action_changed_provinces") should not contain "set_ruler_is_traveling" val stmt = conn.createStatement() try { val rs = stmt.executeQuery("SELECT set_ruler_is_visiting_town FROM action_changed_provinces") @@ -2418,11 +2419,10 @@ class SqliteHistoryTest extends AnyFlatSpec with Matchers with BeforeAndAfterEac emptyTables shouldBe Vector.empty[String] } val visitTownColumns = stmt.executeQuery( - "SELECT set_ruler_is_traveling, set_ruler_is_visiting_town FROM action_changed_provinces" + "SELECT set_ruler_is_visiting_town FROM action_changed_provinces" ) try { visitTownColumns.next() shouldBe true - visitTownColumns.getObject("set_ruler_is_traveling") shouldBe null visitTownColumns.getInt("set_ruler_is_visiting_town") shouldBe 1 } finally visitTownColumns.close() } finally stmt.close()