Contract Visit Town database column (#8646)

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,

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`

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 ++

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"
+            )
+          )
+        )
   }
 
   /**

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

src/test/scala/net/eagle0/eagle/service/PostgresHistorySchemaBatchTest.scala

@@ -67,7 +67,37 @@ class PostgresHistorySchemaBatchTest extends AnyFlatSpec with Matchers with Mock
     )
   }
 
-  it should "cut over an existing PostgreSQL v1 schema without changing its version" in {
+  "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]
@@ -76,7 +106,7 @@ class PostgresHistorySchemaBatchTest extends AnyFlatSpec with Matchers with Mock
     val legacyResult         = mock[ResultSet]
     val visitTownStatement   = mock[PreparedStatement]
     val visitTownResult      = mock[ResultSet]
-    val expansionStatement = mock[Statement]
+    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
     }

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()