Cut over Visit Town database writes (#8645)

docs/SQLITE_ACTION_RESULT_DECOMPOSITION.md

@@ -156,7 +156,7 @@ CREATE TABLE action_changed_provinces (
     support_delta                         REAL,
     -- round properties
     set_has_acted                         INTEGER,  -- 0/1/NULL
-    set_ruler_is_traveling                INTEGER,  -- legacy, dual-written during Visit Town migration
+    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

@@ -94,8 +94,7 @@ private[service] object ChangedProvinceTableWriter {
       hasLegacyTravelColumn: Boolean,
       hasVisitTownColumn: Boolean
   ): Vector[String] = {
-    // The expand release keeps both columns and dual-writes them. A later contract release can backfill once more,
-    // switch readers/writers to the Visit Town column only, and finally drop the legacy column.
+    // The expansion release keeps both columns available to old and new blue/green containers.
     val addStatements =
       Option.when(!hasLegacyTravelColumn)(addLegacyTravelColumnStatement).toVector ++
         Option.when(!hasVisitTownColumn)(addVisitTownColumnStatement)
@@ -103,6 +102,16 @@ private[service] object ChangedProvinceTableWriter {
     else addStatements ++ Vector(backfillVisitTownColumnStatement, backfillLegacyTravelColumnStatement)
   }
 
+  private[service] def visitTownCutoverStatements(
+      hasLegacyTravelColumn: Boolean,
+      hasVisitTownColumn: Boolean
+  ): Vector[String] =
+    // Do not restore a missing legacy column: the next deployment can safely drop it while this release is active.
+    if !hasVisitTownColumn then
+      Vector(addVisitTownColumnStatement) ++
+        Option.when(hasLegacyTravelColumn)(backfillVisitTownColumnStatement)
+    else Option.when(hasLegacyTravelColumn)(backfillVisitTownColumnStatement).toVector
+
   /**
    * 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`
@@ -129,12 +138,11 @@ private[service] object ChangedProvinceTableWriter {
       """INSERT INTO action_changed_provinces
         |(action_seq, n, province_id, gold_delta, food_delta, new_price_index, economy_delta, agriculture_delta,
         | infrastructure_delta, economy_devastation_delta, agriculture_devastation_delta,
-        | infrastructure_devastation_delta, support_delta, set_has_acted, set_ruler_is_traveling,
-        | set_ruler_is_visiting_town,
+        | infrastructure_devastation_delta, support_delta, set_has_acted, set_ruler_is_visiting_town,
         | clear_ruling_faction_id, new_ruling_faction_id, new_locked_improvement_kind, new_locked_improvement_value,
         | new_province_orders, clear_defending_army, cleared_pending_conquest_info, removed_deferred_change_index,
         | changed_province_proto)
-        |VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)""".stripMargin
+        |VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)""".stripMargin
     )
     try {
       ps.setInt(1, actionSeq)
@@ -152,16 +160,15 @@ private[service] object ChangedProvinceTableWriter {
       setDoubleOpt(ps, 13, cp.supportDelta)
       setBoolOpt(ps, 14, cp.setHasActed)
       setBoolOpt(ps, 15, cp.setRulerIsVisitingTown)
-      setBoolOpt(ps, 16, cp.setRulerIsVisitingTown)
-      ps.setInt(17, if cp.clearRulingFactionId then 1 else 0)
-      setIntOpt(ps, 18, cp.newRulingFactionId)
-      setStrOpt(ps, 19, lockedKind)
-      setIntOpt(ps, 20, lockedValue)
-      setIntOpt(ps, 21, provinceOrders)
-      ps.setInt(22, if cp.clearDefendingArmy then 1 else 0)
-      ps.setInt(23, if cp.clearPendingConquestInfo then 1 else 0)
-      setIntOpt(ps, 24, cp.removedDeferredChangeIndex)
-      ps.setBytes(25, cp.toByteArray)
+      ps.setInt(16, if cp.clearRulingFactionId then 1 else 0)
+      setIntOpt(ps, 17, cp.newRulingFactionId)
+      setStrOpt(ps, 18, lockedKind)
+      setIntOpt(ps, 19, lockedValue)
+      setIntOpt(ps, 20, provinceOrders)
+      ps.setInt(21, if cp.clearDefendingArmy then 1 else 0)
+      ps.setInt(22, if cp.clearPendingConquestInfo then 1 else 0)
+      setIntOpt(ps, 23, cp.removedDeferredChangeIndex)
+      ps.setBytes(24, cp.toByteArray)
       ps.executeUpdate(): Unit
     } finally ps.close()
     end try

src/main/scala/net/eagle0/eagle/service/PostgresHistory.scala

@@ -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 ensureVisitTownExpansion(connection)
+    if version == SchemaVersion then ensureVisitTownCutover(connection)
     else if version == 0 then {
       val originalAutoCommit = connection.getAutoCommit
       connection.setAutoCommit(false)
@@ -907,8 +907,8 @@ object PostgresHistory {
     end if
   }
 
-  private def ensureVisitTownExpansion(connection: Connection): Unit = {
-    val statements = ChangedProvinceTableWriter.visitTownExpansionStatements(
+  private def ensureVisitTownCutover(connection: Connection): Unit = {
+    val statements = ChangedProvinceTableWriter.visitTownCutoverStatements(
       hasLegacyTravelColumn = columnExists(connection, "action_changed_provinces", "set_ruler_is_traveling"),
       hasVisitTownColumn = columnExists(connection, "action_changed_provinces", "set_ruler_is_visiting_town")
     )

src/main/scala/net/eagle0/eagle/service/SqliteHistory.scala

@@ -1743,7 +1743,8 @@ object SqliteHistory {
       Migration(22, applyV22ShardokAppliedDeltas),
       Migration(23, applyV23DecisionRecords),
       Migration(24, applyV24RenameVisitTownColumn),
-      Migration(25, applyV25ExpandVisitTownColumns)
+      Migration(25, applyV25ExpandVisitTownColumns),
+      Migration(26, applyV26CutOverVisitTownColumn)
     )
 
     val latestVersion: Int = all.lastOption.map(_.version).getOrElse(0)
@@ -2098,6 +2099,24 @@ object SqliteHistory {
             )
           )
         )
+
+    private def applyV26CutOverVisitTownColumn(connection: Connection): Unit =
+      if tableExists(connection, "action_changed_provinces") then
+        execEach(
+          connection,
+          ChangedProvinceTableWriter.visitTownCutoverStatements(
+            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/PostgresHistorySchemaBatchTest.scala

@@ -39,7 +39,35 @@ class PostgresHistorySchemaBatchTest extends AnyFlatSpec with Matchers with Mock
     ) shouldBe empty
   }
 
-  it should "expand an existing PostgreSQL v1 schema without changing its version" in {
+  "Visit Town schema cutover" should "backfill the new column when both columns exist" in {
+    ChangedProvinceTableWriter.visitTownCutoverStatements(
+      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"
+    )
+  }
+
+  it should "not restore a missing legacy column" in {
+    ChangedProvinceTableWriter.visitTownCutoverStatements(
+      hasLegacyTravelColumn = false,
+      hasVisitTownColumn = true
+    ) shouldBe empty
+  }
+
+  it should "add and backfill the new column when only the legacy column exists" in {
+    ChangedProvinceTableWriter.visitTownCutoverStatements(
+      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"
+    )
+  }
+
+  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]
@@ -97,13 +125,7 @@ class PostgresHistorySchemaBatchTest extends AnyFlatSpec with Matchers with Mock
             "WHERE set_ruler_is_visiting_town IS NULL"
         )
         .once(): Unit
-      expansionStatement.addBatch
-        .expects(
-          "UPDATE action_changed_provinces SET set_ruler_is_traveling = set_ruler_is_visiting_town " +
-            "WHERE set_ruler_is_traveling IS NULL"
-        )
-        .once(): Unit
-      (() => expansionStatement.executeBatch()).expects().returning(Array(0, 0, 0)).once(): Unit
+      (() => expansionStatement.executeBatch()).expects().returning(Array(0, 0)).once(): Unit
       (() => expansionStatement.close()).expects().once(): Unit
       (() => connection.commit()).expects().once(): Unit
       connection.setAutoCommit.expects(true).once(): Unit

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

@@ -1079,7 +1079,7 @@ class SqliteHistoryTest extends AnyFlatSpec with Matchers with BeforeAndAfterEac
     }
   }
 
-  it should "expand a v24 Visit Town schema for dual writes" in {
+  it should "expand a v24 Visit Town schema before cutover" in {
     val history = SqliteHistory.create(dbFile, emptyStartingState)
     history.close()
 
@@ -1103,6 +1103,40 @@ class SqliteHistoryTest extends AnyFlatSpec with Matchers with BeforeAndAfterEac
     }
   }
 
+  it should "backfill legacy Visit Town writes when upgrading a v25 schema" in {
+    val source  = ActionResultC(
+      actionResultType = ActionResultType.UnknownActionUnspecified,
+      changedProvinces = Vector(ChangedProvinceC(provinceId = 11, setRulerIsVisitingTown = Some(true)))
+    )
+    val history = SqliteHistory.create(dbFile, emptyStartingState)
+    history.withNewResults(Vector(ActionResultWithResultingState(source, emptyStartingState))).close()
+
+    withRawConnection { conn =>
+      val stmt = conn.createStatement()
+      try {
+        val _ = stmt.execute(
+          "UPDATE action_changed_provinces " +
+            "SET set_ruler_is_traveling = 1, set_ruler_is_visiting_town = NULL"
+        )
+        val _ = stmt.execute("UPDATE metadata SET value = '25' WHERE key = 'schema_version'")
+      } finally stmt.close()
+    }
+
+    val reloaded = SqliteHistory.loaded(dbFile)
+    reloaded.close()
+
+    withRawConnection { conn =>
+      val stmt = conn.createStatement()
+      try {
+        val rs = stmt.executeQuery("SELECT set_ruler_is_visiting_town FROM action_changed_provinces")
+        try {
+          rs.next() shouldBe true
+          rs.getInt(1) shouldBe 1
+        } finally rs.close()
+      } finally stmt.close()
+    }
+  }
+
   it should "create the four changed-entity top-level tables in v2" in {
     val history = SqliteHistory.create(dbFile, emptyStartingState)
     history.close()
@@ -2388,7 +2422,7 @@ class SqliteHistoryTest extends AnyFlatSpec with Matchers with BeforeAndAfterEac
           )
           try {
             visitTownColumns.next() shouldBe true
-            visitTownColumns.getInt("set_ruler_is_traveling") shouldBe 1
+            visitTownColumns.getObject("set_ruler_is_traveling") shouldBe null
             visitTownColumns.getInt("set_ruler_is_visiting_town") shouldBe 1
           } finally visitTownColumns.close()
         } finally stmt.close()