Copilot commented on code in PR #391: URL: https://github.com/apache/pekko-persistence-jdbc/pull/391#discussion_r2616139018
########## integration-test/src/test/scala/org/apache/pekko/persistence/jdbc/integration/MigrationScriptSpec.scala: ########## @@ -0,0 +1,71 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.pekko.persistence.jdbc.integration + +import java.sql.Statement + +import com.typesafe.config.{ Config, ConfigFactory } +import org.apache.pekko +import pekko.Done +import pekko.actor.ActorSystem +import pekko.persistence.jdbc.state.scaladsl.StateSpecBase +import pekko.persistence.jdbc.testkit.internal.{ SchemaType, SqlServer } +import slick.jdbc.JdbcBackend.Database + +abstract class MigrationScriptSpec(config: Config, schemaType: SchemaType) extends StateSpecBase(config, schemaType) { + + implicit lazy val system: ActorSystem = ActorSystem("migration-test", config) + + protected def withStatement(database: Database)(f: Statement => Unit): Done = { + val session = database.createSession() + try session.withStatement()(f) + finally session.close() + Done + } + + protected def applyScriptWithSlick(script: String, database: Database): Done = { + withStatement(database) { stmt => + stmt.executeUpdate(script) + } + } + + protected def applyScriptWithSlick(script: String, separator: String, database: Database): Done = { + withStatement(database) { stmt => + val lines = script.split(separator).map(_.trim) + for { + line <- lines if line.nonEmpty + } yield { + stmt.executeUpdate(line) + } + } + } +} + +class SqlServerMigrationScriptSpec extends MigrationScriptSpec( + ConfigFactory.load("sqlserver-application.conf"), SqlServer) { + "SQL Server nvarchar migration script" must { + "apply without errors" in { + val script = getClass.getResource("/schema/sqlserver/sqlserver-nvarchar-migration.sql").getPath + val sql = scala.io.Source.fromFile(script).mkString + val parts = sql.split("(?<=END;)") + parts.length should be > 1 + applyScriptWithSlick(parts(0), db) // need to add procedure as a whole + parts.tail.foreach(part => applyScriptWithSlick(part, db)) + } + } +} Review Comment: Only SQL Server migration script has test coverage. The other migration scripts (Oracle, MySQL, MariaDB) are missing integration tests to verify they can be applied without errors. Consider adding test classes similar to SqlServerMigrationScriptSpec for the other databases. ########## core/src/main/resources/schema/oracle/oracle-number-boolean-migration.sql: ########## @@ -0,0 +1,11 @@ +-- SPDX-License-Identifier: Apache-2.0 + +-- see https://github.com/apache/pekko-persistence-jdbc/pull/323 + +-- Script is provided as an example only and only been partially tested. Review Comment: Inconsistent wording with other migration scripts. Line 5 says "only been partially tested" but should say "has only been partially tested" for consistency and grammatical correctness. ```suggestion -- Script is provided as an example only and has only been partially tested. ``` ########## core/src/main/resources/schema/mysql/mysql-durable-state-migration.sql: ########## @@ -0,0 +1,31 @@ +-- SPDX-License-Identifier: Apache-2.0 + +-- see https://github.com/apache/pekko-persistence-jdbc/pull/366 + +-- Script is provided as an example only and only been partially tested. Review Comment: Inconsistent wording with other migration scripts. Line 5 says "only been partially tested" but should say "has only been partially tested" for consistency and grammatical correctness. ```suggestion -- Script is provided as an example only and has only been partially tested. ``` ########## integration-test/src/test/scala/org/apache/pekko/persistence/jdbc/integration/MigrationScriptSpec.scala: ########## @@ -0,0 +1,71 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.pekko.persistence.jdbc.integration + +import java.sql.Statement + +import com.typesafe.config.{ Config, ConfigFactory } +import org.apache.pekko +import pekko.Done +import pekko.actor.ActorSystem +import pekko.persistence.jdbc.state.scaladsl.StateSpecBase +import pekko.persistence.jdbc.testkit.internal.{ SchemaType, SqlServer } +import slick.jdbc.JdbcBackend.Database + +abstract class MigrationScriptSpec(config: Config, schemaType: SchemaType) extends StateSpecBase(config, schemaType) { + + implicit lazy val system: ActorSystem = ActorSystem("migration-test", config) + + protected def withStatement(database: Database)(f: Statement => Unit): Done = { + val session = database.createSession() + try session.withStatement()(f) + finally session.close() + Done + } + + protected def applyScriptWithSlick(script: String, database: Database): Done = { + withStatement(database) { stmt => + stmt.executeUpdate(script) + } + } + + protected def applyScriptWithSlick(script: String, separator: String, database: Database): Done = { + withStatement(database) { stmt => + val lines = script.split(separator).map(_.trim) + for { + line <- lines if line.nonEmpty + } yield { + stmt.executeUpdate(line) + } + } + } Review Comment: The for comprehension in this method doesn't properly accumulate or handle the results of executeUpdate. The yield returns Unit values from executeUpdate which are then collected but not returned or checked. This could mask SQL errors during migration. Consider removing the for comprehension and using foreach instead, or properly collecting and checking the results. ########## core/src/main/resources/schema/sqlserver/sqlserver-nvarchar-migration.sql: ########## @@ -0,0 +1,92 @@ +-- SPDX-License-Identifier: Apache-2.0 + +-- see https://github.com/apache/pekko-persistence-jdbc/pull/382 + +-- Drop primary key constraint on event_journal to allow altering column types + +-- Script is provided as an example only and only been partially tested. +-- Please review and test thoroughly before using in production and +-- ideally, in a test environment first. +-- Always back up your database before running migration scripts. + +CREATE PROCEDURE DropPrimaryKey + @TableName NVARCHAR(255) +AS +BEGIN + SET NOCOUNT ON; + DECLARE @PKName NVARCHAR(1024); + + SELECT @PKName = name + FROM sys.key_constraints + WHERE parent_object_id = OBJECT_ID(@TableName) + AND type = 'PK'; + + IF @PKName IS NOT NULL + EXEC('ALTER TABLE ' + @TableName + ' DROP CONSTRAINT ' + @PKName); +END; + +EXEC DropPrimaryKey 'event_journal'; + +ALTER TABLE event_journal ALTER COLUMN + "persistence_id" NVARCHAR(255); Review Comment: The ALTER COLUMN statement is missing the NOT NULL constraint for persistence_id. According to the schema in sqlserver-create-schema.sql (line 4), persistence_id should be NOT NULL. This should be: "persistence_id" NVARCHAR(255) NOT NULL; ```suggestion "persistence_id" NVARCHAR(255) NOT NULL; ``` ########## core/src/main/resources/schema/mysql/mysql-durable-state-migration.sql: ########## @@ -0,0 +1,31 @@ +-- SPDX-License-Identifier: Apache-2.0 + +-- see https://github.com/apache/pekko-persistence-jdbc/pull/366 + +-- Script is provided as an example only and only been partially tested. +-- Please review and test thoroughly before using in production and +-- ideally, in a test environment first. +-- Always back up your database before running migration scripts. + +CREATE TABLE IF NOT EXISTS durable_state +( + global_offset SERIAL, + persistence_id VARCHAR(255) NOT NULL, + revision BIGINT NOT NULL, + state_payload BLOB NOT NULL, + state_serial_id INTEGER NOT NULL, + state_serial_manifest VARCHAR(255), + tag VARCHAR(255), + state_timestamp BIGINT NOT NULL, + PRIMARY KEY (persistence_id) +); +CREATE INDEX state_tag_idx on durable_state (tag); +CREATE INDEX state_global_offset_idx on durable_state (global_offset); + +CREATE TABLE IF NOT EXISTS durable_state_global_offset +( + singleton TINYINT NOT NULL, + current_offset BIGINT UNSIGNED NOT NULL UNIQUE, + PRIMARY KEY (singleton) +); +INSERT INTO durable_state_global_offset (singleton, current_offset) VALUES (0, 0); Review Comment: The last INSERT statement at line 31 will fail if the script is run multiple times due to the primary key constraint. Consider using INSERT IGNORE or INSERT ... ON DUPLICATE KEY UPDATE to make the script idempotent and safer to re-run. ```suggestion INSERT IGNORE INTO durable_state_global_offset (singleton, current_offset) VALUES (0, 0); ``` ########## core/src/main/resources/schema/mariadb/mariadb-durable-state-migration.sql: ########## @@ -0,0 +1,30 @@ +-- SPDX-License-Identifier: Apache-2.0 + +-- see https://github.com/apache/pekko-persistence-jdbc/pull/365 + +-- Script is provided as an example only and only been partially tested. Review Comment: Inconsistent wording with other migration scripts. Line 5 says "only been partially tested" but should say "has only been partially tested" for consistency and grammatical correctness. ```suggestion -- Script is provided as an example only and has only been partially tested. ``` ########## core/src/main/resources/schema/oracle/oracle-number-boolean-schema-legacy-migration.sql: ########## @@ -0,0 +1,11 @@ +-- SPDX-License-Identifier: Apache-2.0 + +-- see https://github.com/apache/pekko-persistence-jdbc/pull/323 + +-- Script is provided as an example only and only been partially tested. Review Comment: Inconsistent wording with other migration scripts. Line 5 says "only been partially tested" but other migration scripts say "has only been partially tested". Consider using "Script is provided as an example only and has only been partially tested." for consistency. ```suggestion -- Script is provided as an example only and has only been partially tested. ``` ########## core/src/main/resources/schema/sqlserver/sqlserver-nvarchar-migration.sql: ########## @@ -0,0 +1,92 @@ +-- SPDX-License-Identifier: Apache-2.0 + +-- see https://github.com/apache/pekko-persistence-jdbc/pull/382 + +-- Drop primary key constraint on event_journal to allow altering column types + +-- Script is provided as an example only and only been partially tested. +-- Please review and test thoroughly before using in production and +-- ideally, in a test environment first. +-- Always back up your database before running migration scripts. + +CREATE PROCEDURE DropPrimaryKey + @TableName NVARCHAR(255) +AS +BEGIN + SET NOCOUNT ON; + DECLARE @PKName NVARCHAR(1024); + + SELECT @PKName = name + FROM sys.key_constraints + WHERE parent_object_id = OBJECT_ID(@TableName) + AND type = 'PK'; + + IF @PKName IS NOT NULL + EXEC('ALTER TABLE ' + @TableName + ' DROP CONSTRAINT ' + @PKName); +END; + +EXEC DropPrimaryKey 'event_journal'; Review Comment: Inconsistent table name quoting. The table name 'event_journal' is passed as a string literal without quotes, while column names and table names in ALTER statements use double quotes. For consistency and to avoid potential SQL injection in the stored procedure, consider using quoted identifiers consistently throughout the script. ########## core/src/main/resources/schema/sqlserver/sqlserver-nvarchar-migration.sql: ########## @@ -0,0 +1,92 @@ +-- SPDX-License-Identifier: Apache-2.0 + +-- see https://github.com/apache/pekko-persistence-jdbc/pull/382 + +-- Drop primary key constraint on event_journal to allow altering column types + +-- Script is provided as an example only and only been partially tested. Review Comment: Inconsistent wording with grammatical error. Line 7 says "only been partially tested" but should say "has only been partially tested" for grammatical correctness. ```suggestion -- Script is provided as an example only and has only been partially tested. ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
