Merge pull request #42 from cmdscale/fix/#39_support_idempotent_scripts

sebastian-ederer · web-flow · commit c03c4788cd13 · 2026-04-03T18:41:21.000+02:00
fix: Fix sql generation for idempotent scripts
diff --git a/src/Eftdb/Generators/SqlBuilderHelper.cs b/src/Eftdb/Generators/SqlBuilderHelper.cs
@@ -7,7 +7,7 @@ public class SqlBuilderHelper(string quoteString)
     {
         private readonly string quoteString = quoteString;
 
-        public static void BuildQueryString(List<string> statements, MigrationCommandListBuilder builder, bool suppressTransaction = false)
+        public static void BuildQueryString(List<string> statements, MigrationCommandListBuilder builder, bool suppressTransaction = false, bool usePerform = false)
         {
             if (statements.Count == 0)
             {
@@ -39,13 +39,51 @@ public static void BuildQueryString(List<string> statements, MigrationCommandLis
             // Build each command group
             foreach (List<string> group in commandGroups)
             {
-                string command = string.Join("\n", group);
+                List<string> processedGroup = usePerform
+                    ? [.. group.Select(ReplaceSelectWithPerform)]
+                    : group;
+
+                string command = string.Join("\n", processedGroup);
                 builder
                     .Append(command)
                     .EndCommand(suppressTransaction: suppressTransaction);
             }
         }
 
+        /// <summary>
+        /// Replaces a leading SELECT keyword with PERFORM for use inside PL/pgSQL blocks.
+        /// In PL/pgSQL (e.g., idempotent migration scripts), bare SELECT statements that return
+        /// results fail with "query has no destination for result data". PERFORM discards the
+        /// results silently and is the standard PL/pgSQL equivalent of SELECT for this purpose.
+        /// </summary>
+        internal static string ReplaceSelectWithPerform(string sql)
+        {
+            string trimmed = sql.TrimStart();
+            if (trimmed.StartsWith("SELECT ", StringComparison.OrdinalIgnoreCase))
+            {
+                int leadingWhitespaceLength = sql.Length - trimmed.Length;
+                return string.Concat(sql.AsSpan(0, leadingWhitespaceLength), "PERFORM", trimmed.AsSpan("SELECT".Length));
+            }
+
+            return sql;
+        }
+
+        /// <summary>
+        /// Applies <see cref="ReplaceSelectWithPerform"/> to each line of a multi-line SQL string.
+        /// Handles multi-statement SQL blocks where each statement starts on its own line.
+        /// Continuation lines (FROM, WHERE, etc.) are not affected because they don't start with SELECT.
+        /// </summary>
+        internal static string ReplaceSelectWithPerformMultiLine(string sql)
+        {
+            string[] lines = sql.Split('\n');
+            for (int i = 0; i < lines.Length; i++)
+            {
+                lines[i] = ReplaceSelectWithPerform(lines[i]);
+            }
+
+            return string.Join('\n', lines);
+        }
+
         public static void BuildQueryString(List<string> statements, IndentedStringBuilder builder, bool suppressTransaction = false)
         {
             if (statements.Count > 0)
diff --git a/src/Eftdb/TimescaleDbMigrationsSqlGenerator.cs b/src/Eftdb/TimescaleDbMigrationsSqlGenerator.cs
@@ -97,9 +97,38 @@ protected override void Generate(
                     return;
             }
 
-            SqlBuilderHelper.BuildQueryString(statements, builder, suppressTransaction);
+            bool usePerform = Options.HasFlag(MigrationsSqlGenerationOptions.Idempotent);
+            SqlBuilderHelper.BuildQueryString(statements, builder, suppressTransaction, usePerform);
 
         }
+
+        /// <summary>
+        /// Handles raw SQL operations from migration files (migrationBuilder.Sql calls).
+        /// In idempotent mode, replaces SELECT with PERFORM because the SQL is wrapped
+        /// in a PL/pgSQL DO block where bare SELECT fails with "query has no destination for result data".
+        /// Skips replacement for DDL statements (CREATE, ALTER, DROP) where SELECT is part of the syntax.
+        /// </summary>
+        protected override void Generate(SqlOperation operation, IModel? model, MigrationCommandListBuilder builder)
+        {
+            if (Options.HasFlag(MigrationsSqlGenerationOptions.Idempotent)
+                && !IsDdlStatement(operation.Sql))
+            {
+                string sql = SqlBuilderHelper.ReplaceSelectWithPerformMultiLine(operation.Sql);
+                builder.Append(sql);
+                builder.EndCommand(suppressTransaction: operation.SuppressTransaction);
+                return;
+            }
+
+            base.Generate(operation, model, builder);
+        }
+
+        private static bool IsDdlStatement(string sql)
+        {
+            string trimmed = sql.TrimStart();
+            return trimmed.StartsWith("CREATE ", StringComparison.OrdinalIgnoreCase)
+                || trimmed.StartsWith("ALTER ", StringComparison.OrdinalIgnoreCase)
+                || trimmed.StartsWith("DROP ", StringComparison.OrdinalIgnoreCase);
+        }
     }
 #pragma warning disable IDE0079
 }
diff --git a/tests/Eftdb.Tests/Generators/SqlBuilderHelperTests.cs b/tests/Eftdb.Tests/Generators/SqlBuilderHelperTests.cs
@@ -164,6 +164,165 @@ public void BuildQueryString_MigrationCommandListBuilder_WritesNothingForEmptyLi
             mockBuilder.Verify(b => b.Append(It.IsAny<string>()), Times.Never);
             mockBuilder.Verify(b => b.EndCommand(It.IsAny<bool>()), Times.Never);
         }
+        #region ReplaceSelectWithPerform
+
+        [Fact]
+        public void ReplaceSelectWithPerform_ReplacesLeadingSelect()
+        {
+            string input = "SELECT create_hypertable('public.\"Events\"', 'CapturedAt');";
+            string result = SqlBuilderHelper.ReplaceSelectWithPerform(input);
+            Assert.Equal("PERFORM create_hypertable('public.\"Events\"', 'CapturedAt');", result);
+        }
+
+        [Fact]
+        public void ReplaceSelectWithPerform_PreservesLeadingWhitespace()
+        {
+            string input = "    SELECT add_dimension('public.\"Events\"', by_range('sensor_id'));";
+            string result = SqlBuilderHelper.ReplaceSelectWithPerform(input);
+            Assert.Equal("    PERFORM add_dimension('public.\"Events\"', by_range('sensor_id'));", result);
+        }
+
+        [Fact]
+        public void ReplaceSelectWithPerform_PreservesNonSelectStatements()
+        {
+            string input = "ALTER TABLE \"public\".\"Events\" SET (timescaledb.compress = true);";
+            string result = SqlBuilderHelper.ReplaceSelectWithPerform(input);
+            Assert.Equal(input, result);
+        }
+
+        [Fact]
+        public void ReplaceSelectWithPerform_PreservesDoBlocks()
+        {
+            string input = "DO $$ BEGIN EXECUTE 'SELECT 1'; END $$;";
+            string result = SqlBuilderHelper.ReplaceSelectWithPerform(input);
+            Assert.Equal(input, result);
+        }
+
+        [Fact]
+        public void ReplaceSelectWithPerform_IsCaseInsensitive()
+        {
+            string input = "select remove_retention_policy('public.\"Events\"', if_exists => true);";
+            string result = SqlBuilderHelper.ReplaceSelectWithPerform(input);
+            Assert.StartsWith("PERFORM", result);
+        }
+
+        [Fact]
+        public void ReplaceSelectWithPerform_HandlesMultiLineAlterJob()
+        {
+            string input = @"
+                SELECT alter_job(job_id, schedule_interval => INTERVAL '2 days')
+                FROM timescaledb_information.jobs
+                WHERE proc_name = 'policy_retention' AND hypertable_schema = 'public' AND hypertable_name = 'TestTable';".Trim();
+
+            string result = SqlBuilderHelper.ReplaceSelectWithPerform(input);
+
+            Assert.StartsWith("PERFORM alter_job", result);
+            Assert.Contains("FROM timescaledb_information.jobs", result);
+            Assert.DoesNotContain("SELECT", result);
+        }
+
+        [Fact]
+        public void ReplaceSelectWithPerformMultiLine_ReplacesAllSelectStatements()
+        {
+            string input = """
+                SELECT create_hypertable('public."Events"', 'Time');
+                SELECT add_dimension('public."Events"', by_range('sensor_id', 100));
+                SELECT alter_job(job_id, schedule_interval => INTERVAL '1 day')
+                FROM timescaledb_information.jobs
+                WHERE proc_name = 'policy_retention';
+            """;
+
+            string result = SqlBuilderHelper.ReplaceSelectWithPerformMultiLine(input);
+
+            Assert.DoesNotContain("SELECT create_hypertable", result);
+            Assert.DoesNotContain("SELECT add_dimension", result);
+            Assert.DoesNotContain("SELECT alter_job", result);
+            Assert.Contains("PERFORM create_hypertable", result);
+            Assert.Contains("PERFORM add_dimension", result);
+            Assert.Contains("PERFORM alter_job", result);
+            Assert.Contains("FROM timescaledb_information.jobs", result);
+        }
+
+        [Fact]
+        public void ReplaceSelectWithPerformMultiLine_PreservesDoBlocks()
+        {
+            string input = """
+                SELECT create_hypertable('public."Events"', 'Time');
+                DO $$
+                BEGIN
+                    EXECUTE 'SELECT enable_chunk_skipping(...)';
+                END $$;
+            """;
+
+            string result = SqlBuilderHelper.ReplaceSelectWithPerformMultiLine(input);
+
+            Assert.Contains("PERFORM create_hypertable", result);
+            Assert.Contains("DO $$", result);
+            Assert.Contains("EXECUTE 'SELECT enable_chunk_skipping(...)'", result);
+        }
+
+        #endregion
+
+        #region BuildQueryString_MigrationCommandListBuilder_UsePerform
+
+        [Fact]
+        public void BuildQueryString_MigrationCommandListBuilder_UsePerform_ReplacesSelectWithPerform()
+        {
+            // Arrange
+            List<string> statements = ["SELECT create_hypertable('public.\"Events\"', 'Time');"];
+            MigrationsSqlGeneratorDependencies dependencies = new(
+                Mock.Of<IRelationalCommandBuilderFactory>(),
+                Mock.Of<IUpdateSqlGenerator>(),
+                Mock.Of<ISqlGenerationHelper>(),
+                Mock.Of<IRelationalTypeMappingSource>(),
+                Mock.Of<ICurrentDbContext>(),
+                Mock.Of<IModificationCommandFactory>(),
+                Mock.Of<ILoggingOptions>(),
+                Mock.Of<IRelationalCommandDiagnosticsLogger>(),
+                Mock.Of<IDiagnosticsLogger<DbLoggerCategory.Migrations>>()
+            );
+
+            Mock<MigrationCommandListBuilder> mockBuilder = new(dependencies);
+            mockBuilder.Setup(b => b.Append(It.IsAny<string>())).Returns(mockBuilder.Object);
+            mockBuilder.Setup(b => b.EndCommand(It.IsAny<bool>())).Returns(mockBuilder.Object);
+
+            // Act
+            SqlBuilderHelper.BuildQueryString(statements, mockBuilder.Object, usePerform: true);
+
+            // Assert
+            mockBuilder.Verify(b => b.Append(It.Is<string>(s => s.StartsWith("PERFORM"))), Times.Once);
+            mockBuilder.Verify(b => b.Append(It.Is<string>(s => s.StartsWith("SELECT"))), Times.Never);
+        }
+
+        [Fact]
+        public void BuildQueryString_MigrationCommandListBuilder_UsePerform_False_PreservesSelect()
+        {
+            // Arrange
+            List<string> statements = ["SELECT create_hypertable('public.\"Events\"', 'Time');"];
+            MigrationsSqlGeneratorDependencies dependencies = new(
+                Mock.Of<IRelationalCommandBuilderFactory>(),
+                Mock.Of<IUpdateSqlGenerator>(),
+                Mock.Of<ISqlGenerationHelper>(),
+                Mock.Of<IRelationalTypeMappingSource>(),
+                Mock.Of<ICurrentDbContext>(),
+                Mock.Of<IModificationCommandFactory>(),
+                Mock.Of<ILoggingOptions>(),
+                Mock.Of<IRelationalCommandDiagnosticsLogger>(),
+                Mock.Of<IDiagnosticsLogger<DbLoggerCategory.Migrations>>()
+            );
+
+            Mock<MigrationCommandListBuilder> mockBuilder = new(dependencies);
+            mockBuilder.Setup(b => b.Append(It.IsAny<string>())).Returns(mockBuilder.Object);
+            mockBuilder.Setup(b => b.EndCommand(It.IsAny<bool>())).Returns(mockBuilder.Object);
+
+            // Act
+            SqlBuilderHelper.BuildQueryString(statements, mockBuilder.Object, usePerform: false);
+
+            // Assert
+            mockBuilder.Verify(b => b.Append(It.Is<string>(s => s.StartsWith("SELECT"))), Times.Once);
+        }
+
+        #endregion
     }
 #pragma warning restore EF1001 // Internal EF Core API usage.
 }
diff --git a/tests/Eftdb.Tests/Generators/TimescaleDbMigrationsSqlGeneratorTests.cs b/tests/Eftdb.Tests/Generators/TimescaleDbMigrationsSqlGeneratorTests.cs
