From 257f6e57f128ba2b369e85f6568b060505cddbd2 Mon Sep 17 00:00:00 2001 From: Christian Findlay <16697547+MelbourneDeveloper@users.noreply.github.com> Date: Thu, 2 Jul 2026 18:06:28 +1000 Subject: [PATCH 1/5] fixes --- .gitignore | 3 + .../MigrationRunner.cs | 1 + .../RlsPredicateTranspiler.TriggerGuard.cs | 354 ++++++++++++++++++ .../RlsPredicateTranspiler.cs | 2 +- .../SchemaDefinition.cs | 6 + .../SchemaDiff.Triggers.cs | 57 +++ .../SchemaDiff.cs | 8 + .../SchemaOperation.cs | 18 + .../SchemaYamlSerializer.cs | 44 +++ .../TriggerDdlSupport.cs | 51 +++ .../TriggerDefinition.cs | 67 ++++ .../PostgresDdlGenerator.cs | 2 + .../PostgresSchemaInspector.cs | 6 + .../PostgresTriggerDdlBuilder.cs | 80 ++++ .../PostgresTriggerSchemaInspector.cs | 79 ++++ .../SqliteDdlGenerator.cs | 2 + .../SqliteRlsSchemaInspector.cs | 72 +--- .../SqliteSchemaInspector.cs | 1 + .../SqliteTriggerDdlBuilder.cs | 81 ++++ .../SqliteTriggerNames.cs | 58 +++ .../SqliteTriggerSchemaInspector.cs | 42 +++ .../PostgresDeclarativeTriggerE2ETests.cs | 161 ++++++++ .../PostgresRlsE2ETests.cs | 19 +- .../PostgresTestDb.cs | 36 ++ .../SqliteDeclarativeTriggerTests.cs | 232 ++++++++++++ .../SqliteRlsMigrationTests.cs | 155 +++----- .../SqliteTestDb.cs | 95 +++++ .../TriggerYamlSerializerTests.cs | 93 +++++ docs/specs/declarative-triggers-spec.md | 69 ++++ 29 files changed, 1709 insertions(+), 185 deletions(-) create mode 100644 Migration/Nimblesite.DataProvider.Migration.Core/RlsPredicateTranspiler.TriggerGuard.cs create mode 100644 Migration/Nimblesite.DataProvider.Migration.Core/SchemaDiff.Triggers.cs create mode 100644 Migration/Nimblesite.DataProvider.Migration.Core/TriggerDdlSupport.cs create mode 100644 Migration/Nimblesite.DataProvider.Migration.Core/TriggerDefinition.cs create mode 100644 Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresTriggerDdlBuilder.cs create mode 100644 Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresTriggerSchemaInspector.cs create mode 100644 Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteTriggerDdlBuilder.cs create mode 100644 Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteTriggerNames.cs create mode 100644 Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteTriggerSchemaInspector.cs create mode 100644 Migration/Nimblesite.DataProvider.Migration.Tests/PostgresDeclarativeTriggerE2ETests.cs create mode 100644 Migration/Nimblesite.DataProvider.Migration.Tests/PostgresTestDb.cs create mode 100644 Migration/Nimblesite.DataProvider.Migration.Tests/SqliteDeclarativeTriggerTests.cs create mode 100644 Migration/Nimblesite.DataProvider.Migration.Tests/SqliteTestDb.cs create mode 100644 Migration/Nimblesite.DataProvider.Migration.Tests/TriggerYamlSerializerTests.cs create mode 100644 docs/specs/declarative-triggers-spec.md diff --git a/.gitignore b/.gitignore index 3f440c9..c4bd593 100644 --- a/.gitignore +++ b/.gitignore @@ -498,3 +498,6 @@ Reporting/Nimblesite.Reporting.React/wwwroot/js/h5.meta.js Reporting/Nimblesite.Reporting.React/wwwroot/js/h5.min.js Reporting/Nimblesite.Reporting.React/wwwroot/js/h5.meta.min.js Reporting/Nimblesite.Reporting.React/wwwroot/js/index.html + + +.deslop-cache/ \ No newline at end of file diff --git a/Migration/Nimblesite.DataProvider.Migration.Core/MigrationRunner.cs b/Migration/Nimblesite.DataProvider.Migration.Core/MigrationRunner.cs index f6a3a8c..8f6479f 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Core/MigrationRunner.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Core/MigrationRunner.cs @@ -140,6 +140,7 @@ or DropForeignKeyOperation or DropFunctionOperation or RevokePrivilegesOperation or DropRlsPolicyOperation + or DropTriggerOperation or DisableRlsOperation or DisableForceRlsOperation; } diff --git a/Migration/Nimblesite.DataProvider.Migration.Core/RlsPredicateTranspiler.TriggerGuard.cs b/Migration/Nimblesite.DataProvider.Migration.Core/RlsPredicateTranspiler.TriggerGuard.cs new file mode 100644 index 0000000..f8f0200 --- /dev/null +++ b/Migration/Nimblesite.DataProvider.Migration.Core/RlsPredicateTranspiler.TriggerGuard.cs @@ -0,0 +1,354 @@ +using System.Text; +using GuardSqlError = Outcome.Result< + string, + Nimblesite.DataProvider.Migration.Core.MigrationError +>.Error; +using GuardSqlOk = Outcome.Result.Ok< + string, + Nimblesite.DataProvider.Migration.Core.MigrationError +>; +using GuardSqlResult = Outcome.Result< + string, + Nimblesite.DataProvider.Migration.Core.MigrationError +>; + +namespace Nimblesite.DataProvider.Migration.Core; + +// Implements [MIG-TRIGGER-GUARD-LQL] from +// docs/specs/declarative-triggers-spec.md (GitHub issue 82). + +/// +/// Trigger guard predicate translation. Extends the RLS predicate transpiler +/// with old./new. row references, and/or +/// composition, and [not] exists(pipeline) subqueries embedded in a +/// larger predicate. +/// +public static partial class RlsPredicateTranspiler +{ + private const string OldRowSentinelPrefix = "__TRG_OLD_"; + private const string NewRowSentinelPrefix = "__TRG_NEW_"; + private const string ExistsMarkerPrefix = "__TRG_EXISTS_"; + + /// + /// Translate a trigger guard LQL predicate to platform-specific SQL. + /// Row column references use old./new. prefixes + /// (case-insensitive) and become platform-quoted OLD./NEW. + /// references. exists(pipeline) and not exists(pipeline) + /// subqueries may appear anywhere in the predicate. + /// + /// LQL guard predicate. + /// Target platform. + /// Trigger name -- used for error messages. + public static GuardSqlResult TranslateGuardPredicate( + string lql, + RlsPlatform platform, + string triggerName + ) + { + if (string.IsNullOrWhiteSpace(lql)) + { + return new GuardSqlError(MigrationError.RlsEmptyPredicate(triggerName)); + } + + var withRowSentinels = SubstituteRowReferences(lql.Trim()); + var (remainder, segments) = ExtractExistsSegments(withRowSentinels); + + var simple = TranslateSimplePredicate(remainder, platform, triggerName); + if (simple is GuardSqlError simpleError) + { + return simpleError; + } + if (simple is not GuardSqlOk simpleOk) + { + return new GuardSqlError( + MigrationError.RlsLqlParse(triggerName, "unknown guard predicate failure") + ); + } + + var sql = simpleOk.Value; + for (var index = 0; index < segments.Count; index++) + { + var segment = segments[index]; + var subquery = TranslateExistsSubquery(segment.InnerLql, platform, triggerName); + if (subquery is GuardSqlError subqueryError) + { + return subqueryError; + } + if (subquery is not GuardSqlOk subqueryOk) + { + return new GuardSqlError( + MigrationError.RlsLqlParse(triggerName, "unknown guard subquery failure") + ); + } + var replacement = segment.Negated ? $"NOT {subqueryOk.Value}" : subqueryOk.Value; + sql = sql.Replace(ExistsMarker(index), replacement, StringComparison.Ordinal); + } + + return new GuardSqlOk(RestoreRowReferences(sql, platform)); + } + + private static string ExistsMarker(int index) => $"'{ExistsMarkerPrefix}{index}__'"; + + /// + /// Replaces old.col/new.col references with sentinel string + /// literals that survive LQL transpilation. Skips string literals. + /// + private static string SubstituteRowReferences(string source) + { + var sb = new StringBuilder(source.Length + 32); + var i = 0; + while (i < source.Length) + { + if (source[i] == '\'') + { + i = AppendStringLiteral(source, i, sb); + continue; + } + if (IsIdentifierStart(source, i)) + { + var start = i; + i = ReadIdentifierEnd(source, i); + var word = source[start..i]; + var isRowRef = + ( + word.Equals("old", StringComparison.OrdinalIgnoreCase) + || word.Equals("new", StringComparison.OrdinalIgnoreCase) + ) + && i + 1 < source.Length + && source[i] == '.' + && (char.IsLetter(source[i + 1]) || source[i + 1] == '_'); + if (isRowRef) + { + var columnStart = i + 1; + var columnEnd = ReadIdentifierEnd(source, columnStart); + var prefix = word.Equals("old", StringComparison.OrdinalIgnoreCase) + ? OldRowSentinelPrefix + : NewRowSentinelPrefix; + sb.Append('\'') + .Append(prefix) + .Append(source[columnStart..columnEnd]) + .Append("__'"); + i = columnEnd; + continue; + } + sb.Append(word); + continue; + } + sb.Append(source[i]); + i++; + } + return sb.ToString(); + } + + /// + /// Extracts every top-level [not] exists(...) subquery, replacing + /// each with a marker literal so the remainder can be translated as a + /// simple predicate. + /// + private static ( + string Remainder, + IReadOnlyList Segments + ) ExtractExistsSegments(string predicate) + { + var segments = new List(); + var sb = new StringBuilder(predicate.Length); + var i = 0; + while (i < predicate.Length) + { + if (predicate[i] == '\'') + { + i = AppendStringLiteral(predicate, i, sb); + continue; + } + if (IsIdentifierStart(predicate, i)) + { + var start = i; + i = ReadIdentifierEnd(predicate, i); + var word = predicate[start..i]; + if (word.Equals("not", StringComparison.OrdinalIgnoreCase)) + { + var afterNot = SkipWhitespace(predicate, i); + var keywordEnd = ReadIdentifierEnd(predicate, afterNot); + var next = predicate[afterNot..keywordEnd]; + if ( + next.Equals("exists", StringComparison.OrdinalIgnoreCase) + && TryReadSubquery(predicate, keywordEnd, out var negatedInner, out var end) + ) + { + segments.Add(new GuardExistsSegment(negatedInner, Negated: true)); + sb.Append(ExistsMarker(segments.Count - 1)); + i = end; + continue; + } + } + if ( + word.Equals("exists", StringComparison.OrdinalIgnoreCase) + && TryReadSubquery(predicate, i, out var inner, out var afterSubquery) + ) + { + segments.Add(new GuardExistsSegment(inner, Negated: false)); + sb.Append(ExistsMarker(segments.Count - 1)); + i = afterSubquery; + continue; + } + sb.Append(word); + continue; + } + sb.Append(predicate[i]); + i++; + } + return (sb.ToString(), segments); + } + + /// + /// Reads a balanced-paren subquery starting at the first ( after + /// . Returns false when no subquery follows. + /// + private static bool TryReadSubquery(string source, int from, out string inner, out int end) + { + inner = string.Empty; + end = from; + var i = SkipWhitespace(source, from); + if (i >= source.Length || source[i] != '(') + { + return false; + } + var openIndex = i; + var depth = 1; + i++; + while (i < source.Length && depth > 0) + { + switch (source[i]) + { + case '\'': + i = SkipStringLiteral(source, i); + continue; + case '(': + depth++; + break; + case ')': + depth--; + if (depth == 0) + { + inner = source[(openIndex + 1)..i]; + end = i + 1; + return true; + } + break; + } + i++; + } + return false; + } + + /// + /// Replaces row-reference sentinel literals with platform-quoted + /// OLD./NEW. column references. + /// + private static string RestoreRowReferences(string sql, RlsPlatform platform) + { + var (open, close) = platform == RlsPlatform.Postgres ? ('"', '"') : ('[', ']'); + var restored = ReplaceRowSentinels(sql, OldRowSentinelPrefix, "OLD", open, close); + return ReplaceRowSentinels(restored, NewRowSentinelPrefix, "NEW", open, close); + } + + private static string ReplaceRowSentinels( + string sql, + string sentinelPrefix, + string rowAlias, + char open, + char close + ) + { + var marker = $"'{sentinelPrefix}"; + var sb = new StringBuilder(sql.Length); + var i = 0; + while (i < sql.Length) + { + var idx = sql.IndexOf(marker, i, StringComparison.Ordinal); + if (idx < 0) + { + sb.Append(sql, i, sql.Length - i); + break; + } + sb.Append(sql, i, idx - i); + var columnStart = idx + marker.Length; + var terminator = sql.IndexOf("__'", columnStart, StringComparison.Ordinal); + if (terminator < 0) + { + sb.Append(sql, idx, sql.Length - idx); + break; + } + sb.Append(rowAlias) + .Append('.') + .Append(open) + .Append(sql[columnStart..terminator]) + .Append(close); + i = terminator + 3; + } + return sb.ToString(); + } + + private static bool IsIdentifierStart(string source, int i) => + (char.IsLetter(source[i]) || source[i] == '_') + && ( + i == 0 + || ( + !char.IsLetterOrDigit(source[i - 1]) && source[i - 1] != '_' && source[i - 1] != '.' + ) + ); + + private static int ReadIdentifierEnd(string source, int start) + { + var i = start; + while (i < source.Length && (char.IsLetterOrDigit(source[i]) || source[i] == '_')) + { + i++; + } + return i; + } + + private static int SkipWhitespace(string source, int start) + { + var i = start; + while (i < source.Length && char.IsWhiteSpace(source[i])) + { + i++; + } + return i; + } + + private static int SkipStringLiteral(string source, int start) + { + // start points at the opening quote; returns index after the closing + // quote (handles '' escapes). + var i = start + 1; + while (i < source.Length) + { + if (source[i] == '\'') + { + if (i + 1 < source.Length && source[i + 1] == '\'') + { + i += 2; + continue; + } + return i + 1; + } + i++; + } + return i; + } + + private static int AppendStringLiteral(string source, int start, StringBuilder sb) + { + var end = SkipStringLiteral(source, start); + sb.Append(source, start, end - start); + return end; + } +} + +/// +/// One extracted [not] exists(...) subquery segment of a guard +/// predicate. +/// +internal sealed record GuardExistsSegment(string InnerLql, bool Negated); diff --git a/Migration/Nimblesite.DataProvider.Migration.Core/RlsPredicateTranspiler.cs b/Migration/Nimblesite.DataProvider.Migration.Core/RlsPredicateTranspiler.cs index c011a5c..5951a60 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Core/RlsPredicateTranspiler.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Core/RlsPredicateTranspiler.cs @@ -32,7 +32,7 @@ public enum RlsPlatform /// SQL Server: SESSION_CONTEXT) and delegates exists(pipeline) /// subquery wrappers to the LQL pipeline transpiler. /// -public static class RlsPredicateTranspiler +public static partial class RlsPredicateTranspiler { /// /// Sentinel placeholder used to mark current_user_id() calls diff --git a/Migration/Nimblesite.DataProvider.Migration.Core/SchemaDefinition.cs b/Migration/Nimblesite.DataProvider.Migration.Core/SchemaDefinition.cs index f26ac97..d1f1475 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Core/SchemaDefinition.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Core/SchemaDefinition.cs @@ -181,6 +181,12 @@ public sealed record TableDefinition /// table and each contained policy is applied. Implements [RLS-CORE-POLICY]. /// public RlsPolicySetDefinition? RowLevelSecurity { get; init; } + + /// + /// Declarative trigger guards on this table. Implements + /// [MIG-TRIGGER-MODEL] (GitHub issue 82). + /// + public IReadOnlyList Triggers { get; init; } = []; } /// diff --git a/Migration/Nimblesite.DataProvider.Migration.Core/SchemaDiff.Triggers.cs b/Migration/Nimblesite.DataProvider.Migration.Core/SchemaDiff.Triggers.cs new file mode 100644 index 0000000..9cde8b8 --- /dev/null +++ b/Migration/Nimblesite.DataProvider.Migration.Core/SchemaDiff.Triggers.cs @@ -0,0 +1,57 @@ +namespace Nimblesite.DataProvider.Migration.Core; + +// Implements [MIG-TRIGGER-DIFF] from docs/specs/declarative-triggers-spec.md +// (GitHub issue 82). Mirrors the RLS policy diff: triggers are matched by +// name per table; drops are destructive and gated behind allowDestructive. + +public static partial class SchemaDiff +{ + private static IEnumerable CalculateTriggerDiff( + TableDefinition? current, + TableDefinition desired, + bool allowDestructive, + ILogger? logger + ) + { + var currentNames = + current?.Triggers.Select(t => t.Name).ToHashSet(StringComparer.OrdinalIgnoreCase) + ?? new HashSet(StringComparer.OrdinalIgnoreCase); + + foreach (var trigger in desired.Triggers) + { + if (!currentNames.Contains(trigger.Name)) + { + logger?.LogDebug( + "Creating trigger {Trigger} on {Schema}.{Table}", + trigger.Name, + desired.Schema, + desired.Name + ); + yield return new CreateTriggerOperation(desired.Schema, desired.Name, trigger); + } + } + + if (!allowDestructive || current is null) + { + yield break; + } + + var desiredNames = desired + .Triggers.Select(t => t.Name) + .ToHashSet(StringComparer.OrdinalIgnoreCase); + + foreach (var trigger in current.Triggers) + { + if (!desiredNames.Contains(trigger.Name)) + { + logger?.LogWarning( + "Trigger {Trigger} on {Schema}.{Table} will be DROPPED", + trigger.Name, + desired.Schema, + desired.Name + ); + yield return new DropTriggerOperation(desired.Schema, desired.Name, trigger.Name); + } + } + } +} diff --git a/Migration/Nimblesite.DataProvider.Migration.Core/SchemaDiff.cs b/Migration/Nimblesite.DataProvider.Migration.Core/SchemaDiff.cs index a3e01fa..9b22364 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Core/SchemaDiff.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Core/SchemaDiff.cs @@ -53,6 +53,7 @@ public static OperationsResult Calculate( { var operations = new List(); var rlsOperations = new List(); + var triggerOperations = new List(); operations.AddRange(CalculateRoleDiff(current, desired, logger)); @@ -92,6 +93,9 @@ public static OperationsResult Calculate( rlsOperations.AddRange( CalculateRlsDiff(null, desiredTable, allowDestructive, logger) ); + triggerOperations.AddRange( + CalculateTriggerDiff(null, desiredTable, allowDestructive, logger) + ); } else { @@ -136,6 +140,9 @@ public static OperationsResult Calculate( rlsOperations.AddRange( CalculateRlsDiff(currentTable, desiredTable, allowDestructive, logger) ); + triggerOperations.AddRange( + CalculateTriggerDiff(currentTable, desiredTable, allowDestructive, logger) + ); } } @@ -172,6 +179,7 @@ public static OperationsResult Calculate( operations.AddRange(functionOps.Where(op => !IsSupportCleanupOperation(op))); operations.AddRange(grantOps.Where(op => !IsSupportCleanupOperation(op))); operations.AddRange(rlsOperations); + operations.AddRange(triggerOperations); operations.AddRange(functionOps.Where(IsSupportCleanupOperation)); operations.AddRange(grantOps.Where(IsSupportCleanupOperation)); diff --git a/Migration/Nimblesite.DataProvider.Migration.Core/SchemaOperation.cs b/Migration/Nimblesite.DataProvider.Migration.Core/SchemaOperation.cs index e11f809..4fa85fb 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Core/SchemaOperation.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Core/SchemaOperation.cs @@ -100,6 +100,16 @@ public sealed record CreateRlsPolicyOperation( RlsPolicyDefinition Policy ) : SchemaOperation; +/// +/// Create a declarative trigger guard. Additive. Implements +/// [MIG-TRIGGER-DIFF] (GitHub issue 82). +/// +public sealed record CreateTriggerOperation( + string Schema, + string TableName, + TriggerDefinition Trigger +) : SchemaOperation; + // ═══════════════════════════════════════════════════════════════════ // DESTRUCTIVE OPERATIONS - Require explicit opt-in // ═══════════════════════════════════════════════════════════════════ @@ -162,6 +172,14 @@ public sealed record RevokePrivilegesOperation(PostgresGrantDefinition Grant) : public sealed record DropRlsPolicyOperation(string Schema, string TableName, string PolicyName) : SchemaOperation; +/// +/// Drop a declarative trigger guard -- removes an enforcement rule. +/// DESTRUCTIVE - requires explicit opt-in. Implements [MIG-TRIGGER-DIFF] +/// (GitHub issue 82). +/// +public sealed record DropTriggerOperation(string Schema, string TableName, string TriggerName) + : SchemaOperation; + /// /// Disable row-level security on a table. DESTRUCTIVE - requires explicit /// opt-in because rows previously hidden by policies become visible. diff --git a/Migration/Nimblesite.DataProvider.Migration.Core/SchemaYamlSerializer.cs b/Migration/Nimblesite.DataProvider.Migration.Core/SchemaYamlSerializer.cs index ebbf0fc..1f4f52a 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Core/SchemaYamlSerializer.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Core/SchemaYamlSerializer.cs @@ -18,6 +18,7 @@ public static class SchemaYamlSerializer .WithTypeConverter(new PortableTypeYamlConverter()) .WithTypeConverter(new ForeignKeyActionYamlConverter()) .WithTypeConverter(new RlsOperationYamlConverter()) + .WithTypeConverter(new TriggerEnumYamlConverter()) .WithTypeConverter(new PostgresGrantTargetYamlConverter()) .ConfigureDefaultValuesHandling( DefaultValuesHandling.OmitDefaults @@ -35,6 +36,7 @@ public static class SchemaYamlSerializer .WithTypeConverter(new PortableTypeYamlConverter()) .WithTypeConverter(new ForeignKeyActionYamlConverter()) .WithTypeConverter(new RlsOperationYamlConverter()) + .WithTypeConverter(new TriggerEnumYamlConverter()) .WithTypeConverter(new PostgresGrantTargetYamlConverter()) .WithTypeMapping, List>() .WithTypeMapping, List>() @@ -60,6 +62,8 @@ public static class SchemaYamlSerializer >() .WithTypeMapping, List>() .WithTypeMapping, List>() + .WithTypeMapping, List>() + .WithTypeMapping, List>() .WithTypeMapping, List>() .Build(); @@ -352,6 +356,44 @@ public void WriteYaml(IEmitter emitter, object? value, Type type, ObjectSerializ } } +/// +/// YAML type converter for the and +/// enums. Implements [MIG-TRIGGER-YAML]. +/// +internal sealed class TriggerEnumYamlConverter : IYamlTypeConverter +{ + /// + public bool Accepts(Type type) => type == typeof(TriggerTiming) || type == typeof(TriggerEvent); + + /// + public object? ReadYaml(IParser parser, Type type, ObjectDeserializer rootDeserializer) + { + var scalar = parser.Consume(); + return type == typeof(TriggerTiming) ? ParseTiming(scalar.Value) : ParseEvent(scalar.Value); + } + + /// + public void WriteYaml(IEmitter emitter, object? value, Type type, ObjectSerializer serializer) + { + emitter.Emit(new Scalar(value?.ToString() ?? string.Empty)); + } + + private static object ParseTiming(string value) => + value.ToUpperInvariant() switch + { + "AFTER" => TriggerTiming.After, + _ => TriggerTiming.Before, + }; + + private static object ParseEvent(string value) => + value.ToUpperInvariant() switch + { + "UPDATE" => TriggerEvent.Update, + "DELETE" => TriggerEvent.Delete, + _ => TriggerEvent.Insert, + }; +} + /// /// YAML type converter for the enum. /// Implements [RLS-PG-SUPPORT-DDL]. @@ -409,6 +451,8 @@ internal sealed class PropertyDefaultValueFilter(IObjectGraphVisitor n // RlsPolicySetDefinition / RlsPolicyDefinition semantic defaults { "enabled", (typeof(bool), true) }, { "isPermissive", (typeof(bool), true) }, + // TriggerDefinition semantic defaults ([MIG-TRIGGER-YAML]) + { "forEachRow", (typeof(bool), true) }, // PostgreSQL support object semantic defaults { "language", (typeof(string), "sql") }, { "volatility", (typeof(string), "stable") }, diff --git a/Migration/Nimblesite.DataProvider.Migration.Core/TriggerDdlSupport.cs b/Migration/Nimblesite.DataProvider.Migration.Core/TriggerDdlSupport.cs new file mode 100644 index 0000000..e8bc57f --- /dev/null +++ b/Migration/Nimblesite.DataProvider.Migration.Core/TriggerDdlSupport.cs @@ -0,0 +1,51 @@ +namespace Nimblesite.DataProvider.Migration.Core; + +// Implements [MIG-TRIGGER-SQLITE] and [MIG-TRIGGER-PG] shared DDL support +// from docs/specs/declarative-triggers-spec.md (GitHub issue 82). + +/// +/// Shared helpers for platform trigger DDL builders: validation, error +/// message defaulting/escaping, and keyword mapping. +/// +public static class TriggerDdlSupport +{ + /// Returns the guard predicate or throws when missing. + public static string RequireRaiseWhen(TriggerDefinition trigger) => + trigger.RaiseWhenLql is { } lql && !string.IsNullOrWhiteSpace(lql) + ? lql + : throw new InvalidOperationException( + $"Trigger '{trigger.Name}' has no raiseWhen predicate" + ); + + /// Returns the distinct declared events or throws when empty. + public static IReadOnlyList RequireEvents(TriggerDefinition trigger) => + trigger.Events.Count > 0 + ? trigger.Events.Distinct().ToList() + : throw new InvalidOperationException($"Trigger '{trigger.Name}' declares no events"); + + /// + /// Error message (defaulted from the trigger name when absent) with + /// single quotes escaped for embedding in a SQL string literal. + /// + public static string EscapedMessage(TriggerDefinition trigger) + { + var message = string.IsNullOrWhiteSpace(trigger.ErrorMessage) + ? $"TRIGGER-GUARD: {trigger.Name}" + : trigger.ErrorMessage; + return message.Replace("'", "''", StringComparison.Ordinal); + } + + /// SQL verb for a trigger event. + public static string EventVerb(TriggerEvent triggerEvent) => + triggerEvent switch + { + TriggerEvent.Insert => "INSERT", + TriggerEvent.Update => "UPDATE", + TriggerEvent.Delete => "DELETE", + _ => throw new NotSupportedException($"Unknown trigger event: {triggerEvent}"), + }; + + /// SQL timing keyword for a trigger timing. + public static string TimingKeyword(TriggerTiming timing) => + timing == TriggerTiming.After ? "AFTER" : "BEFORE"; +} diff --git a/Migration/Nimblesite.DataProvider.Migration.Core/TriggerDefinition.cs b/Migration/Nimblesite.DataProvider.Migration.Core/TriggerDefinition.cs new file mode 100644 index 0000000..5e1b394 --- /dev/null +++ b/Migration/Nimblesite.DataProvider.Migration.Core/TriggerDefinition.cs @@ -0,0 +1,67 @@ +using YamlDotNet.Serialization; + +namespace Nimblesite.DataProvider.Migration.Core; + +// Implements [MIG-TRIGGER-MODEL] from docs/specs/declarative-triggers-spec.md +// (GitHub issue 82). + +/// +/// Declarative trigger guard attached to a table. The trigger raises +/// and aborts the statement when +/// evaluates true for the affected row. Diffed by +/// name and materialised by the platform DDL generator, like RLS policies. +/// +public sealed record TriggerDefinition +{ + /// Trigger name -- unique within the table. + public string Name { get; init; } = string.Empty; + + /// When the trigger fires relative to the event. Default Before. + public TriggerTiming Timing { get; init; } = TriggerTiming.Before; + + /// Events the trigger fires on. At least one is required. + public IReadOnlyList Events { get; init; } = []; + + /// True to fire once per affected row (default). + [YamlMember(DefaultValuesHandling = DefaultValuesHandling.Preserve)] + public bool ForEachRow { get; init; } = true; + + /// + /// LQL guard predicate. Row column references use old./new. + /// prefixes; supports and/or composition and + /// [not] exists(pipeline) subqueries. Implements + /// [MIG-TRIGGER-GUARD-LQL]. + /// + [YamlMember(Alias = "raiseWhen")] + public string? RaiseWhenLql { get; init; } + + /// Error message raised when the guard predicate holds. + public string? ErrorMessage { get; init; } +} + +/// +/// When a trigger fires relative to its event. +/// +public enum TriggerTiming +{ + /// Fires before the row change is applied. + Before, + + /// Fires after the row change is applied. + After, +} + +/// +/// DML event a trigger fires on. +/// +public enum TriggerEvent +{ + /// Fires on INSERT. + Insert, + + /// Fires on UPDATE. + Update, + + /// Fires on DELETE. + Delete, +} diff --git a/Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresDdlGenerator.cs b/Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresDdlGenerator.cs index f366a00..3ba2185 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresDdlGenerator.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresDdlGenerator.cs @@ -130,6 +130,8 @@ public static string Generate(SchemaOperation operation) => DropRlsPolicyOperation op => $"DROP POLICY IF EXISTS \"{op.PolicyName}\" ON \"{op.Schema}\".\"{op.TableName}\"", CreateRlsPolicyOperation op => GenerateCreateRlsPolicy(op), + CreateTriggerOperation op => PostgresTriggerDdlBuilder.GenerateCreate(op), + DropTriggerOperation op => PostgresTriggerDdlBuilder.GenerateDrop(op), _ => throw new NotSupportedException( $"Unknown operation type: {operation.GetType().Name}" ), diff --git a/Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresSchemaInspector.cs b/Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresSchemaInspector.cs index 5943597..e79861b 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresSchemaInspector.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresSchemaInspector.cs @@ -344,6 +344,12 @@ JOIN information_schema.referential_constraints rc UniqueConstraints = uniqueConstraints.AsReadOnly(), CheckConstraints = checkConstraints.AsReadOnly(), RowLevelSecurity = rls, + // [MIG-TRIGGER-PG] read declarative trigger guards by name. + Triggers = PostgresTriggerSchemaInspector.Inspect( + connection, + schemaName, + tableName + ), } ); } diff --git a/Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresTriggerDdlBuilder.cs b/Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresTriggerDdlBuilder.cs new file mode 100644 index 0000000..5d0a460 --- /dev/null +++ b/Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresTriggerDdlBuilder.cs @@ -0,0 +1,80 @@ +using GuardTranspileError = Outcome.Result< + string, + Nimblesite.DataProvider.Migration.Core.MigrationError +>.Error; +using GuardTranspileOk = Outcome.Result< + string, + Nimblesite.DataProvider.Migration.Core.MigrationError +>.Ok; + +namespace Nimblesite.DataProvider.Migration.Postgres; + +// Implements [MIG-TRIGGER-PG] from docs/specs/declarative-triggers-spec.md +// (GitHub issue 82). + +/// +/// Emits PostgreSQL DDL for declarative trigger guards: one plpgsql guard +/// function ({table}_{trigger}_trgfn) that raises the declared error +/// when the guard predicate holds, plus one trigger wired to the declared +/// events. RAISE ... USING MESSAGE avoids format-string +/// interpretation of % in user messages. +/// +internal static class PostgresTriggerDdlBuilder +{ + public static string GenerateCreate(CreateTriggerOperation op) + { + var predicate = Translate(TriggerDdlSupport.RequireRaiseWhen(op.Trigger), op.Trigger.Name); + var functionName = FunctionName(op.TableName, op.Trigger.Name); + var events = string.Join( + " OR ", + TriggerDdlSupport.RequireEvents(op.Trigger).Select(TriggerDdlSupport.EventVerb) + ); + var level = op.Trigger.ForEachRow ? "ROW" : "STATEMENT"; + return $""" + CREATE OR REPLACE FUNCTION "{op.Schema}"."{functionName}"() + RETURNS trigger + LANGUAGE plpgsql + AS $trigger_guard$ + BEGIN + IF {predicate} THEN + RAISE EXCEPTION USING MESSAGE = '{TriggerDdlSupport.EscapedMessage(op.Trigger)}'; + END IF; + IF TG_OP = 'DELETE' THEN + RETURN OLD; + END IF; + RETURN NEW; + END + $trigger_guard$; + DROP TRIGGER IF EXISTS "{op.Trigger.Name}" ON "{op.Schema}"."{op.TableName}"; + CREATE TRIGGER "{op.Trigger.Name}" + {TriggerDdlSupport.TimingKeyword( + op.Trigger.Timing + )} {events} ON "{op.Schema}"."{op.TableName}" + FOR EACH {level} + EXECUTE FUNCTION "{op.Schema}"."{functionName}"() + """; + } + + public static string GenerateDrop(DropTriggerOperation op) => + $""" + DROP TRIGGER IF EXISTS "{op.TriggerName}" ON "{op.Schema}"."{op.TableName}"; + DROP FUNCTION IF EXISTS "{op.Schema}"."{FunctionName(op.TableName, op.TriggerName)}"() + """; + + private static string Translate(string lql, string triggerName) + { + var result = RlsPredicateTranspiler.TranslateGuardPredicate( + lql, + RlsPlatform.Postgres, + triggerName + ); + return result switch + { + GuardTranspileOk ok => ok.Value, + GuardTranspileError error => throw new InvalidOperationException(error.Value.Message), + }; + } + + private static string FunctionName(string tableName, string triggerName) => + $"{tableName}_{triggerName}_trgfn"; +} diff --git a/Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresTriggerSchemaInspector.cs b/Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresTriggerSchemaInspector.cs new file mode 100644 index 0000000..a457e04 --- /dev/null +++ b/Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresTriggerSchemaInspector.cs @@ -0,0 +1,79 @@ +namespace Nimblesite.DataProvider.Migration.Postgres; + +// Implements [MIG-TRIGGER-PG] read-back from +// docs/specs/declarative-triggers-spec.md (GitHub issue 82). + +/// +/// Reads triggers back from information_schema.triggers (one row per +/// event) grouped by trigger name so the diff can match declarative trigger +/// guards by name. +/// +internal static class PostgresTriggerSchemaInspector +{ + public static IReadOnlyList Inspect( + NpgsqlConnection connection, + string schemaName, + string tableName + ) + { + using var command = connection.CreateCommand(); + command.CommandText = """ + SELECT trigger_name, event_manipulation, action_timing, action_orientation + FROM information_schema.triggers + WHERE event_object_schema = @schema AND event_object_table = @table + ORDER BY trigger_name + """; + command.Parameters.AddWithValue("@schema", schemaName); + command.Parameters.AddWithValue("@table", tableName); + + var rows = new List(); + using (var reader = command.ExecuteReader()) + { + while (reader.Read()) + { + rows.Add( + new PostgresTriggerRow( + Name: reader.GetString(0), + Event: reader.GetString(1), + Timing: reader.GetString(2), + Orientation: reader.GetString(3) + ) + ); + } + } + + return rows.GroupBy(r => r.Name, StringComparer.OrdinalIgnoreCase) + .Select(ToTrigger) + .OrderBy(t => t.Name, StringComparer.OrdinalIgnoreCase) + .ToList(); + } + + private static TriggerDefinition ToTrigger(IGrouping group) => + new() + { + Name = group.Key, + Timing = group.Any(r => r.Timing.Equals("AFTER", StringComparison.OrdinalIgnoreCase)) + ? TriggerTiming.After + : TriggerTiming.Before, + ForEachRow = group.All(r => + r.Orientation.Equals("ROW", StringComparison.OrdinalIgnoreCase) + ), + Events = group.Select(r => ToEvent(r.Event)).Distinct().ToList(), + }; + + private static TriggerEvent ToEvent(string eventManipulation) => + eventManipulation.ToUpperInvariant() switch + { + "INSERT" => TriggerEvent.Insert, + "UPDATE" => TriggerEvent.Update, + _ => TriggerEvent.Delete, + }; +} + +/// One information_schema.triggers row. +internal sealed record PostgresTriggerRow( + string Name, + string Event, + string Timing, + string Orientation +); diff --git a/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteDdlGenerator.cs b/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteDdlGenerator.cs index 15756f1..c1fdff5 100644 --- a/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteDdlGenerator.cs +++ b/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteDdlGenerator.cs @@ -40,6 +40,8 @@ public static string Generate(SchemaOperation operation) => CreateRlsPolicyOperation op => SqliteRlsDdlBuilder.GenerateCreatePolicy(op), DropRlsPolicyOperation op => SqliteRlsDdlBuilder.GenerateDropPolicy(op), DisableRlsOperation op => SqliteRlsDdlBuilder.GenerateDisable(op), + CreateTriggerOperation op => SqliteTriggerDdlBuilder.GenerateCreate(op), + DropTriggerOperation op => SqliteTriggerDdlBuilder.GenerateDrop(op), _ => throw new NotSupportedException( $"Unknown operation type: {operation.GetType().Name}" ), diff --git a/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteRlsSchemaInspector.cs b/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteRlsSchemaInspector.cs index 0add70a..e04aee3 100644 --- a/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteRlsSchemaInspector.cs +++ b/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteRlsSchemaInspector.cs @@ -4,9 +4,11 @@ namespace Nimblesite.DataProvider.Migration.SQLite; internal static class SqliteRlsSchemaInspector { + private static readonly string[] EventTokens = ["insert", "update", "delete"]; + public static RlsPolicySetDefinition? Inspect(SqliteConnection connection, string tableName) { - var triggers = ReadTriggerNames(connection, tableName); + var triggers = SqliteTriggerNames.Read(connection, tableName, $"rls_%_{tableName}"); if (triggers.Count == 0) { return null; @@ -23,70 +25,24 @@ internal static class SqliteRlsSchemaInspector return policies.Count == 0 ? null : new RlsPolicySetDefinition { Policies = policies }; } - private static List ReadTriggerNames(SqliteConnection connection, string tableName) - { - using var command = connection.CreateCommand(); - command.CommandText = """ - SELECT name FROM sqlite_master - WHERE type = 'trigger' AND tbl_name = @table AND name LIKE @pattern - ORDER BY name - """; - command.Parameters.AddWithValue("@table", tableName); - command.Parameters.AddWithValue("@pattern", $"rls_%_{tableName}"); - using var reader = command.ExecuteReader(); - var names = new List(); - while (reader.Read()) - { - names.Add(reader.GetString(0)); - } - return names; - } - private static SqliteRlsTriggerPolicy? ToTriggerPolicy(string name, string tableName) { - var suffix = $"_{tableName}"; - if ( - !name.StartsWith("rls_", StringComparison.Ordinal) - || !name.EndsWith(suffix, StringComparison.Ordinal) - ) - { - return null; - } - - var body = name[4..^suffix.Length]; - var operation = ReadOperation(body); - return operation is null + var parsed = SqliteTriggerNames.Parse(name, "rls_", tableName, EventTokens); + return parsed is null ? null - : new SqliteRlsTriggerPolicy(body[(operation.SqlName.Length + 1)..], operation); + : new SqliteRlsTriggerPolicy(parsed.BaseName, ToOperation(parsed.EventToken)); } - private static SqliteRlsOperationName? ReadOperation(string body) - { - foreach (var op in OperationNames()) + private static RlsOperation ToOperation(string eventToken) => + eventToken switch { - if (body.StartsWith($"{op.SqlName}_", StringComparison.Ordinal)) - { - return op; - } - } - return null; - } - - private static RlsPolicyDefinition ToPolicy(IGrouping group) => - new() - { - Name = group.Key, - Operations = group.Select(p => p.Operation.RlsOperation).Distinct().ToList(), + "insert" => RlsOperation.Insert, + "update" => RlsOperation.Update, + _ => RlsOperation.Delete, }; - private static IEnumerable OperationNames() => - [ - new("insert", RlsOperation.Insert), - new("update", RlsOperation.Update), - new("delete", RlsOperation.Delete), - ]; + private static RlsPolicyDefinition ToPolicy(IGrouping group) => + new() { Name = group.Key, Operations = group.Select(p => p.Operation).Distinct().ToList() }; } -internal sealed record SqliteRlsTriggerPolicy(string Name, SqliteRlsOperationName Operation); - -internal sealed record SqliteRlsOperationName(string SqlName, RlsOperation RlsOperation); +internal sealed record SqliteRlsTriggerPolicy(string Name, RlsOperation Operation); diff --git a/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteSchemaInspector.cs b/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteSchemaInspector.cs index 536b8a9..1c40477 100644 --- a/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteSchemaInspector.cs +++ b/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteSchemaInspector.cs @@ -254,6 +254,7 @@ SELECT sql FROM sqlite_master ForeignKeys = foreignKeys.AsReadOnly(), PrimaryKey = primaryKey, RowLevelSecurity = SqliteRlsSchemaInspector.Inspect(connection, tableName), + Triggers = SqliteTriggerSchemaInspector.Inspect(connection, tableName), } ); } diff --git a/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteTriggerDdlBuilder.cs b/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteTriggerDdlBuilder.cs new file mode 100644 index 0000000..c53e673 --- /dev/null +++ b/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteTriggerDdlBuilder.cs @@ -0,0 +1,81 @@ +using GuardTranspileError = Outcome.Result< + string, + Nimblesite.DataProvider.Migration.Core.MigrationError +>.Error; +using GuardTranspileOk = Outcome.Result< + string, + Nimblesite.DataProvider.Migration.Core.MigrationError +>.Ok; + +namespace Nimblesite.DataProvider.Migration.SQLite; + +// Implements [MIG-TRIGGER-SQLITE] from docs/specs/declarative-triggers-spec.md +// (GitHub issue 82). + +/// +/// Emits SQLite DDL for declarative trigger guards. One trigger per declared +/// event, named usr_{event}_{trigger}_{table} so the inspector can +/// read the definition back by name. SQLite only supports row-level +/// triggers, so FOR EACH ROW is always emitted. +/// +internal static class SqliteTriggerDdlBuilder +{ + private static readonly string[] EventTokens = ["insert", "update", "delete"]; + + public static string GenerateCreate(CreateTriggerOperation op) + { + var predicate = Translate(TriggerDdlSupport.RequireRaiseWhen(op.Trigger), op.Trigger.Name); + var message = TriggerDdlSupport.EscapedMessage(op.Trigger); + return string.Join( + ";\n", + TriggerDdlSupport + .RequireEvents(op.Trigger) + .Select(triggerEvent => Trigger(op, triggerEvent, predicate, message)) + ); + } + + public static string GenerateDrop(DropTriggerOperation op) => + string.Join( + ";\n", + EventTokens.Select(token => + $"DROP TRIGGER IF EXISTS [{TriggerName(token, op.TriggerName, op.TableName)}]" + ) + ); + + private static string Trigger( + CreateTriggerOperation op, + TriggerEvent triggerEvent, + string predicate, + string message + ) + { + var verb = TriggerDdlSupport.EventVerb(triggerEvent); + var name = TriggerName(verb.ToLowerInvariant(), op.Trigger.Name, op.TableName); + return $""" + CREATE TRIGGER IF NOT EXISTS [{name}] + {TriggerDdlSupport.TimingKeyword(op.Trigger.Timing)} {verb} ON [{op.TableName}] + FOR EACH ROW + BEGIN + SELECT RAISE(ABORT, '{message}') + WHERE {predicate}; + END + """; + } + + private static string Translate(string lql, string triggerName) + { + var result = RlsPredicateTranspiler.TranslateGuardPredicate( + lql, + RlsPlatform.Sqlite, + triggerName + ); + return result switch + { + GuardTranspileOk ok => ok.Value, + GuardTranspileError error => throw new InvalidOperationException(error.Value.Message), + }; + } + + private static string TriggerName(string eventToken, string triggerName, string tableName) => + $"usr_{eventToken}_{triggerName}_{tableName}"; +} diff --git a/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteTriggerNames.cs b/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteTriggerNames.cs new file mode 100644 index 0000000..39621d8 --- /dev/null +++ b/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteTriggerNames.cs @@ -0,0 +1,58 @@ +namespace Nimblesite.DataProvider.Migration.SQLite; + +/// +/// Shared reading/parsing for migration-managed SQLite trigger names of the +/// form {prefix}{event}_{name}_{table}. Used by the RLS and +/// declarative trigger inspectors. +/// +internal static class SqliteTriggerNames +{ + public static List Read(SqliteConnection connection, string tableName, string pattern) + { + using var command = connection.CreateCommand(); + command.CommandText = """ + SELECT name FROM sqlite_master + WHERE type = 'trigger' AND tbl_name = @table AND name LIKE @pattern + ORDER BY name + """; + command.Parameters.AddWithValue("@table", tableName); + command.Parameters.AddWithValue("@pattern", pattern); + using var reader = command.ExecuteReader(); + var names = new List(); + while (reader.Read()) + { + names.Add(reader.GetString(0)); + } + return names; + } + + public static ParsedTriggerName? Parse( + string name, + string prefix, + string tableName, + IReadOnlyList eventTokens + ) + { + var suffix = $"_{tableName}"; + if ( + !name.StartsWith(prefix, StringComparison.Ordinal) + || !name.EndsWith(suffix, StringComparison.Ordinal) + ) + { + return null; + } + + var body = name[prefix.Length..^suffix.Length]; + foreach (var token in eventTokens) + { + if (body.StartsWith($"{token}_", StringComparison.Ordinal)) + { + return new ParsedTriggerName(token, body[(token.Length + 1)..]); + } + } + return null; + } +} + +/// Trigger name split into its event token and base name. +internal sealed record ParsedTriggerName(string EventToken, string BaseName); diff --git a/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteTriggerSchemaInspector.cs b/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteTriggerSchemaInspector.cs new file mode 100644 index 0000000..df62c4b --- /dev/null +++ b/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteTriggerSchemaInspector.cs @@ -0,0 +1,42 @@ +namespace Nimblesite.DataProvider.Migration.SQLite; + +// Implements [MIG-TRIGGER-SQLITE] read-back from +// docs/specs/declarative-triggers-spec.md (GitHub issue 82). + +/// +/// Reads declarative trigger guards back from sqlite_master so the +/// diff can match them by name. Only migration-managed usr_ triggers +/// are considered; rls_ triggers belong to the RLS inspector. +/// +internal static class SqliteTriggerSchemaInspector +{ + private static readonly string[] EventTokens = ["insert", "update", "delete"]; + + public static IReadOnlyList Inspect( + SqliteConnection connection, + string tableName + ) => + SqliteTriggerNames + .Read(connection, tableName, $"usr_%_{tableName}") + .Select(name => SqliteTriggerNames.Parse(name, "usr_", tableName, EventTokens)) + .OfType() + .GroupBy(p => p.BaseName, StringComparer.OrdinalIgnoreCase) + .Select(ToTrigger) + .OrderBy(t => t.Name, StringComparer.OrdinalIgnoreCase) + .ToList(); + + private static TriggerDefinition ToTrigger(IGrouping group) => + new() + { + Name = group.Key, + Events = group.Select(p => ToEvent(p.EventToken)).Distinct().ToList(), + }; + + private static TriggerEvent ToEvent(string eventToken) => + eventToken switch + { + "insert" => TriggerEvent.Insert, + "update" => TriggerEvent.Update, + _ => TriggerEvent.Delete, + }; +} diff --git a/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresDeclarativeTriggerE2ETests.cs b/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresDeclarativeTriggerE2ETests.cs new file mode 100644 index 0000000..d6e92fe --- /dev/null +++ b/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresDeclarativeTriggerE2ETests.cs @@ -0,0 +1,161 @@ +namespace Nimblesite.DataProvider.Migration.Tests; + +// Implements [MIG-TRIGGER-PG] and [MIG-TRIGGER-DIFF] from +// docs/specs/declarative-triggers-spec.md (GitHub issue 82). + +/// +/// E2E tests for declarative BEFORE-trigger guards against a real +/// Testcontainers postgres instance. The schema declares a last-owner guard +/// on tenant_members: demoting or deleting the only 'owner' row of a tenant +/// must fail with the declared error message. +/// +[Collection(PostgresTestSuite.Name)] +[System.Diagnostics.CodeAnalysis.SuppressMessage( + "Usage", + "CA1001:Types that own disposable fields should be disposable", + Justification = "Disposed via IAsyncLifetime.DisposeAsync" +)] +public sealed class PostgresDeclarativeTriggerE2ETests(PostgresContainerFixture fixture) + : IAsyncLifetime +{ + private NpgsqlConnection _connection = null!; + private readonly ILogger _logger = NullLogger.Instance; + + public async Task InitializeAsync() + { + _connection = await fixture.CreateDatabaseAsync("trigger_e2e").ConfigureAwait(false); + } + + public async Task DisposeAsync() + { + await _connection.DisposeAsync().ConfigureAwait(false); + } + + private const string GuardYaml = """ + name: tenant_app + tables: + - name: tenant_members + columns: + - name: id + type: Uuid + isNullable: false + - name: tenant_id + type: Uuid + isNullable: false + - name: role + type: VarChar(50) + isNullable: false + primaryKey: + columns: + - id + triggers: + - name: assert_not_last_owner + events: [Update, Delete] + raiseWhen: | + old.role = 'owner' and not exists( + tenant_members + |> filter(fn(m) => m.tenant_id = old.tenant_id and m.role = 'owner' and m.id <> old.id) + ) + errorMessage: cannot remove the last owner of a tenant + """; + + [Fact] + public void PgTriggerGuard_BlocksDeleteOfLastOwner() + { + ApplyGuardSchema(); + var owner = Guid.NewGuid(); + var tenant = Guid.NewGuid(); + InsertMember(owner, tenant, "owner"); + InsertMember(Guid.NewGuid(), tenant, "member"); + + var ex = Assert.Throws(() => DeleteMember(owner)); + + Assert.Contains("last owner", ex.Message, StringComparison.Ordinal); + Assert.Equal(2, CountMembers()); + } + + [Fact] + public void PgTriggerGuard_AllowsDeleteWhenAnotherOwnerExists() + { + ApplyGuardSchema(); + var ownerA = Guid.NewGuid(); + var tenant = Guid.NewGuid(); + InsertMember(ownerA, tenant, "owner"); + InsertMember(Guid.NewGuid(), tenant, "owner"); + + DeleteMember(ownerA); + + Assert.Equal(1, CountMembers()); + } + + [Fact] + public void PgTriggerGuard_BlocksDemoteOfLastOwner() + { + ApplyGuardSchema(); + var owner = Guid.NewGuid(); + var tenant = Guid.NewGuid(); + InsertMember(owner, tenant, "owner"); + InsertMember(Guid.NewGuid(), tenant, "member"); + + var ex = Assert.Throws(() => DemoteMember(owner)); + + Assert.Contains("last owner", ex.Message, StringComparison.Ordinal); + } + + [Fact] + public void PgTrigger_RediffAfterApply_ProducesNoOperations() + { + ApplyGuardSchema(); + + var current = ( + (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) + ).Value; + var ops = ( + (OperationsResultOk) + SchemaDiff.Calculate( + current, + SchemaYamlSerializer.FromYaml(GuardYaml), + logger: _logger + ) + ).Value; + + Assert.Empty(ops); + } + + private void ApplyGuardSchema() => + PostgresTestDb.ApplySchema(_connection, SchemaYamlSerializer.FromYaml(GuardYaml), _logger); + + private void InsertMember(Guid id, Guid tenantId, string role) + { + using var cmd = _connection.CreateCommand(); + cmd.CommandText = + "INSERT INTO \"public\".\"tenant_members\"(id, tenant_id, role) VALUES (@i, @t, @r)"; + cmd.Parameters.AddWithValue("@i", id); + cmd.Parameters.AddWithValue("@t", tenantId); + cmd.Parameters.AddWithValue("@r", role); + cmd.ExecuteNonQuery(); + } + + private void DeleteMember(Guid id) + { + using var cmd = _connection.CreateCommand(); + cmd.CommandText = "DELETE FROM \"public\".\"tenant_members\" WHERE id = @i"; + cmd.Parameters.AddWithValue("@i", id); + cmd.ExecuteNonQuery(); + } + + private void DemoteMember(Guid id) + { + using var cmd = _connection.CreateCommand(); + cmd.CommandText = "UPDATE \"public\".\"tenant_members\" SET role = 'member' WHERE id = @i"; + cmd.Parameters.AddWithValue("@i", id); + cmd.ExecuteNonQuery(); + } + + private long CountMembers() + { + using var cmd = _connection.CreateCommand(); + cmd.CommandText = "SELECT COUNT(*) FROM \"public\".\"tenant_members\""; + return Assert.IsType(cmd.ExecuteScalar()); + } +} diff --git a/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresRlsE2ETests.cs b/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresRlsE2ETests.cs index b897dba..a09f953 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresRlsE2ETests.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresRlsE2ETests.cs @@ -164,24 +164,7 @@ private static SchemaDefinition BuildGroupMembershipSchema() => private void ApplyAndForceRls(SchemaDefinition desired, params string[] tableNames) { - var current = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; - var ops = ( - (OperationsResultOk)SchemaDiff.Calculate(current, desired, logger: _logger) - ).Value; - - var apply = MigrationRunner.Apply( - _connection, - ops, - PostgresDdlGenerator.Generate, - MigrationOptions.Default, - _logger - ); - Assert.True( - apply is MigrationApplyResultOk, - $"Migration failed: {(apply as MigrationApplyResultError)?.Value}" - ); + PostgresTestDb.ApplySchema(_connection, desired, _logger); // Testcontainers postgres connects as a superuser with BYPASSRLS. // To exercise policies we need a non-bypassrls role and grant CRUD. diff --git a/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresTestDb.cs b/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresTestDb.cs new file mode 100644 index 0000000..6a63233 --- /dev/null +++ b/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresTestDb.cs @@ -0,0 +1,36 @@ +namespace Nimblesite.DataProvider.Migration.Tests; + +/// +/// Shared PostgreSQL E2E test helper: applies a desired schema via +/// inspect → diff → apply and asserts the migration succeeded. +/// +internal static class PostgresTestDb +{ + public static void ApplySchema( + NpgsqlConnection connection, + SchemaDefinition desired, + ILogger logger, + bool allowDestructive = false + ) + { + var current = ( + (SchemaResultOk)PostgresSchemaInspector.Inspect(connection, "public", logger) + ).Value; + var ops = ( + (OperationsResultOk) + SchemaDiff.Calculate(current, desired, allowDestructive, logger: logger) + ).Value; + + var apply = MigrationRunner.Apply( + connection, + ops, + PostgresDdlGenerator.Generate, + allowDestructive ? MigrationOptions.Destructive : MigrationOptions.Default, + logger + ); + Assert.True( + apply is MigrationApplyResultOk, + $"Migration failed: {(apply as MigrationApplyResultError)?.Value}" + ); + } +} diff --git a/Migration/Nimblesite.DataProvider.Migration.Tests/SqliteDeclarativeTriggerTests.cs b/Migration/Nimblesite.DataProvider.Migration.Tests/SqliteDeclarativeTriggerTests.cs new file mode 100644 index 0000000..fe6c375 --- /dev/null +++ b/Migration/Nimblesite.DataProvider.Migration.Tests/SqliteDeclarativeTriggerTests.cs @@ -0,0 +1,232 @@ +namespace Nimblesite.DataProvider.Migration.Tests; + +// Implements [MIG-TRIGGER-SQLITE] and [MIG-TRIGGER-DIFF] from +// docs/specs/declarative-triggers-spec.md (GitHub issue 82). + +/// +/// E2E tests for declarative BEFORE-trigger guards on SQLite. The schema +/// declares a last-owner guard on TenantMembers: demoting or deleting the +/// only 'owner' row of a tenant must fail with the declared error message. +/// +public sealed class SqliteDeclarativeTriggerTests +{ + private static readonly ILogger Logger = NullLogger.Instance; + private static readonly string OwnerA = Guid.NewGuid().ToString(); + private static readonly string OwnerB = Guid.NewGuid().ToString(); + private static readonly string MemberC = Guid.NewGuid().ToString(); + private static readonly string Tenant1 = Guid.NewGuid().ToString(); + private static readonly string Tenant2 = Guid.NewGuid().ToString(); + + private const string GuardYaml = """ + name: tenant_app + tables: + - name: TenantMembers + schema: main + columns: + - name: Id + type: Text + isNullable: false + - name: TenantId + type: Text + isNullable: false + - name: Role + type: Text + isNullable: false + primaryKey: + columns: + - Id + triggers: + - name: assert_not_last_owner + timing: Before + events: [Update, Delete] + forEachRow: true + raiseWhen: | + old.Role = 'owner' and not exists( + TenantMembers + |> filter(fn(m) => m.TenantId = old.TenantId and m.Role = 'owner' and m.Id <> old.Id) + ) + errorMessage: cannot remove the last owner of a tenant + """; + + private const string PlainYaml = """ + name: tenant_app + tables: + - name: TenantMembers + schema: main + columns: + - name: Id + type: Text + isNullable: false + - name: TenantId + type: Text + isNullable: false + - name: Role + type: Text + isNullable: false + primaryKey: + columns: + - Id + """; + + [Fact] + public void Sqlite_TriggerGuard_BlocksDeleteOfLastOwner() + { + SqliteTestDb.WithDb(connection => + { + ApplyGuardSchema(connection); + InsertMember(connection, OwnerA, Tenant1, "owner"); + InsertMember(connection, MemberC, Tenant1, "member"); + + var ex = Assert.Throws(() => DeleteMember(connection, OwnerA)); + + Assert.Contains("last owner", ex.Message, StringComparison.Ordinal); + Assert.Equal(2, SqliteTestDb.CountRows(connection, "TenantMembers")); + }); + } + + [Fact] + public void Sqlite_TriggerGuard_AllowsDeleteWhenAnotherOwnerExists() + { + SqliteTestDb.WithDb(connection => + { + ApplyGuardSchema(connection); + InsertMember(connection, OwnerA, Tenant1, "owner"); + InsertMember(connection, OwnerB, Tenant1, "owner"); + + DeleteMember(connection, OwnerA); + + Assert.Equal(1, SqliteTestDb.CountRows(connection, "TenantMembers")); + }); + } + + [Fact] + public void Sqlite_TriggerGuard_BlocksDemoteOfLastOwner() + { + SqliteTestDb.WithDb(connection => + { + ApplyGuardSchema(connection); + InsertMember(connection, OwnerA, Tenant1, "owner"); + InsertMember(connection, MemberC, Tenant1, "member"); + + var ex = Assert.Throws(() => DemoteMember(connection, OwnerA)); + + Assert.Contains("last owner", ex.Message, StringComparison.Ordinal); + }); + } + + [Fact] + public void Sqlite_TriggerGuard_AllowsDemoteWhenAnotherOwnerExists() + { + SqliteTestDb.WithDb(connection => + { + ApplyGuardSchema(connection); + InsertMember(connection, OwnerA, Tenant1, "owner"); + InsertMember(connection, OwnerB, Tenant1, "owner"); + + DemoteMember(connection, OwnerA); + + Assert.Equal( + 1, + SqliteTestDb.Count( + connection, + "SELECT COUNT(*) FROM [TenantMembers] WHERE [Role]='owner'" + ) + ); + }); + } + + [Fact] + public void Sqlite_TriggerGuard_IsScopedToTenant() + { + SqliteTestDb.WithDb(connection => + { + ApplyGuardSchema(connection); + InsertMember(connection, OwnerA, Tenant1, "owner"); + InsertMember(connection, OwnerB, Tenant2, "owner"); + + // OwnerB owns a different tenant, so OwnerA is still the last + // owner of Tenant1 and must not be deletable. + var ex = Assert.Throws(() => DeleteMember(connection, OwnerA)); + + Assert.Contains("last owner", ex.Message, StringComparison.Ordinal); + }); + } + + [Fact] + public void Sqlite_Trigger_RediffAfterApply_ProducesNoOperations() + { + SqliteTestDb.WithDb(connection => + { + ApplyGuardSchema(connection); + + var current = SqliteTestDb.Inspect(connection); + var ops = ( + (OperationsResultOk) + SchemaDiff.Calculate( + current, + SchemaYamlSerializer.FromYaml(GuardYaml), + logger: Logger + ) + ).Value; + + Assert.Empty(ops); + }); + } + + [Fact] + public void Sqlite_TriggerRemoval_WithoutDestructive_KeepsGuard() + { + SqliteTestDb.WithDb(connection => + { + ApplyGuardSchema(connection); + SqliteTestDb.ApplySchema(connection, SchemaYamlSerializer.FromYaml(PlainYaml)); + InsertMember(connection, OwnerA, Tenant1, "owner"); + + var ex = Assert.Throws(() => DeleteMember(connection, OwnerA)); + + Assert.Contains("last owner", ex.Message, StringComparison.Ordinal); + }); + } + + [Fact] + public void Sqlite_TriggerRemoval_WithDestructive_DropsGuard() + { + SqliteTestDb.WithDb(connection => + { + ApplyGuardSchema(connection); + SqliteTestDb.ApplySchema( + connection, + SchemaYamlSerializer.FromYaml(PlainYaml), + allowDestructive: true + ); + InsertMember(connection, OwnerA, Tenant1, "owner"); + + DeleteMember(connection, OwnerA); + + Assert.Equal(0, SqliteTestDb.CountRows(connection, "TenantMembers")); + }); + } + + private static void ApplyGuardSchema(SqliteConnection connection) => + SqliteTestDb.ApplySchema(connection, SchemaYamlSerializer.FromYaml(GuardYaml)); + + private static void InsertMember( + SqliteConnection connection, + string id, + string tenantId, + string role + ) => + SqliteTestDb.Execute( + connection, + $"INSERT INTO [TenantMembers]([Id], [TenantId], [Role]) VALUES ('{id}', '{tenantId}', '{role}')" + ); + + private static void DeleteMember(SqliteConnection connection, string id) => + SqliteTestDb.Execute(connection, $"DELETE FROM [TenantMembers] WHERE [Id]='{id}'"); + + private static void DemoteMember(SqliteConnection connection, string id) => + SqliteTestDb.Execute( + connection, + $"UPDATE [TenantMembers] SET [Role]='member' WHERE [Id]='{id}'" + ); +} diff --git a/Migration/Nimblesite.DataProvider.Migration.Tests/SqliteRlsMigrationTests.cs b/Migration/Nimblesite.DataProvider.Migration.Tests/SqliteRlsMigrationTests.cs index a3004e3..4eec9bb 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Tests/SqliteRlsMigrationTests.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Tests/SqliteRlsMigrationTests.cs @@ -1,5 +1,3 @@ -using System.Globalization; - namespace Nimblesite.DataProvider.Migration.Tests; // Implements [RLS-SQLITE] tests from docs/specs/rls-spec.md. @@ -9,26 +7,27 @@ namespace Nimblesite.DataProvider.Migration.Tests; /// public sealed class SqliteRlsMigrationTests { - private static readonly ILogger Logger = NullLogger.Instance; - [Fact] public void Sqlite_EnableRls_CreatesRlsContextTable() { - WithDb(connection => + SqliteTestDb.WithDb(connection => { - Apply(connection, [new EnableRlsOperation("main", "Documents")]); + SqliteTestDb.Apply(connection, [new EnableRlsOperation("main", "Documents")]); - Assert.Equal(1, CountMasterRows(connection, "table", "__rls_context")); + Assert.Equal(1, SqliteTestDb.CountMasterRows(connection, "table", "__rls_context")); }); } [Fact] public void Sqlite_CreatePolicy_Insert_TriggerBlocksCrossOwnerInsert() { - WithDb(connection => + SqliteTestDb.WithDb(connection => { - ApplySchema(connection, DocumentsSchema(OwnerPolicy([RlsOperation.Insert]))); - SetUser(connection, "user-a"); + SqliteTestDb.ApplySchema( + connection, + DocumentsSchema(OwnerPolicy([RlsOperation.Insert])) + ); + SqliteTestDb.SetUser(connection, "user-a"); InsertDocument(connection, "doc-a", "user-a"); @@ -42,14 +41,17 @@ public void Sqlite_CreatePolicy_Insert_TriggerBlocksCrossOwnerInsert() [Fact] public void Sqlite_CreatePolicy_Update_TriggerBlocksCrossOwnerUpdate() { - WithDb(connection => + SqliteTestDb.WithDb(connection => { - ApplySchema(connection, DocumentsSchema(OwnerPolicy([RlsOperation.Update]))); - SetUser(connection, "user-a"); + SqliteTestDb.ApplySchema( + connection, + DocumentsSchema(OwnerPolicy([RlsOperation.Update])) + ); + SqliteTestDb.SetUser(connection, "user-a"); InsertDocument(connection, "doc-a", "user-a"); var ex = Assert.Throws(() => - Execute(connection, "UPDATE [Documents] SET [OwnerId]='user-b'") + SqliteTestDb.Execute(connection, "UPDATE [Documents] SET [OwnerId]='user-b'") ); Assert.Contains("RLS-SQLITE", ex.Message, StringComparison.Ordinal); }); @@ -58,15 +60,18 @@ public void Sqlite_CreatePolicy_Update_TriggerBlocksCrossOwnerUpdate() [Fact] public void Sqlite_CreatePolicy_Delete_TriggerBlocksCrossOwnerDelete() { - WithDb(connection => + SqliteTestDb.WithDb(connection => { - ApplySchema(connection, DocumentsSchema(OwnerPolicy([RlsOperation.Delete]))); - SetUser(connection, "user-a"); + SqliteTestDb.ApplySchema( + connection, + DocumentsSchema(OwnerPolicy([RlsOperation.Delete])) + ); + SqliteTestDb.SetUser(connection, "user-a"); InsertDocument(connection, "doc-a", "user-a"); - SetUser(connection, "user-b"); + SqliteTestDb.SetUser(connection, "user-b"); var ex = Assert.Throws(() => - Execute(connection, "DELETE FROM [Documents] WHERE [Id]='doc-a'") + SqliteTestDb.Execute(connection, "DELETE FROM [Documents] WHERE [Id]='doc-a'") ); Assert.Contains("RLS-SQLITE", ex.Message, StringComparison.Ordinal); }); @@ -75,43 +80,44 @@ public void Sqlite_CreatePolicy_Delete_TriggerBlocksCrossOwnerDelete() [Fact] public void Sqlite_CreatePolicy_GroupMembership_TriggerUsesSubquery() { - WithDb(connection => + SqliteTestDb.WithDb(connection => { - ApplySchema(connection, GroupMembershipSchema()); - SetUser(connection, "user-a"); + SqliteTestDb.ApplySchema(connection, GroupMembershipSchema()); + SqliteTestDb.SetUser(connection, "user-a"); Assert.Throws(() => InsertDocument(connection, "doc-a", "user-a")); InsertMembership(connection, "membership-a", "user-a"); InsertDocument(connection, "doc-b", "user-a"); - Assert.Equal(1, CountRows(connection, "Documents")); + Assert.Equal(1, SqliteTestDb.CountRows(connection, "Documents")); }); } [Fact] public void Sqlite_SelectPolicy_CreatesSecureView() { - WithDb(connection => + SqliteTestDb.WithDb(connection => { - ApplySchema(connection, DocumentsSchema(OwnerPolicy([RlsOperation.Select]))); + SqliteTestDb.ApplySchema( + connection, + DocumentsSchema(OwnerPolicy([RlsOperation.Select])) + ); InsertDocument(connection, "doc-a", "user-a"); InsertDocument(connection, "doc-b", "user-b"); - SetUser(connection, "user-a"); + SqliteTestDb.SetUser(connection, "user-a"); - Assert.Equal(1, CountRows(connection, "Documents_secure")); + Assert.Equal(1, SqliteTestDb.CountRows(connection, "Documents_secure")); }); } [Fact] public void Sqlite_SchemaInspector_ReadsBackTriggers() { - WithDb(connection => + SqliteTestDb.WithDb(connection => { - ApplySchema(connection, DocumentsSchema(OwnerPolicy([RlsOperation.All]))); + SqliteTestDb.ApplySchema(connection, DocumentsSchema(OwnerPolicy([RlsOperation.All]))); - var inspected = ( - (SchemaResultOk)SqliteSchemaInspector.Inspect(connection, Logger) - ).Value; + var inspected = SqliteTestDb.Inspect(connection); var rls = inspected.Tables.Single(t => t.Name == "Documents").RowLevelSecurity; Assert.NotNull(rls); @@ -139,17 +145,20 @@ public void Sqlite_RestrictivePolicy_EmitsWarning() [Fact] public void Sqlite_DisableRls_DropsSecureView() { - WithDb(connection => + SqliteTestDb.WithDb(connection => { - ApplySchema(connection, DocumentsSchema(OwnerPolicy([RlsOperation.Select]))); + SqliteTestDb.ApplySchema( + connection, + DocumentsSchema(OwnerPolicy([RlsOperation.Select])) + ); - Apply( + SqliteTestDb.Apply( connection, [new DisableRlsOperation("main", "Documents")], MigrationOptions.Destructive ); - Assert.Equal(0, CountMasterRows(connection, "view", "Documents_secure")); + Assert.Equal(0, SqliteTestDb.CountMasterRows(connection, "view", "Documents_secure")); }); } @@ -221,85 +230,15 @@ private static RlsPolicyDefinition GroupMembershipPolicy() => """, }; - private static void ApplySchema(SqliteConnection connection, SchemaDefinition schema) - { - var current = ((SchemaResultOk)SqliteSchemaInspector.Inspect(connection, Logger)).Value; - var ops = ((OperationsResultOk)SchemaDiff.Calculate(current, schema, logger: Logger)).Value; - Apply(connection, ops); - } - - private static void Apply( - SqliteConnection connection, - IReadOnlyList ops, - MigrationOptions? options = null - ) - { - var result = MigrationRunner.Apply( - connection, - ops, - SqliteDdlGenerator.Generate, - options ?? MigrationOptions.Default, - Logger - ); - Assert.True(result is MigrationApplyResultOk); - } - - private static void SetUser(SqliteConnection connection, string userId) - { - Execute(connection, "DELETE FROM [__rls_context]"); - Execute(connection, $"INSERT INTO [__rls_context]([current_user_id]) VALUES ('{userId}')"); - } - private static void InsertDocument(SqliteConnection connection, string id, string ownerId) => - Execute( + SqliteTestDb.Execute( connection, $"INSERT INTO [Documents]([Id], [OwnerId], [Title]) VALUES ('{id}', '{ownerId}', 't')" ); private static void InsertMembership(SqliteConnection connection, string id, string userId) => - Execute( + SqliteTestDb.Execute( connection, $"INSERT INTO [UserGroupMemberships]([Id], [UserId]) VALUES ('{id}', '{userId}')" ); - - private static void Execute(SqliteConnection connection, string sql) - { - using var command = connection.CreateCommand(); - command.CommandText = sql; - command.ExecuteNonQuery(); - } - - private static int CountRows(SqliteConnection connection, string tableName) => - Count(connection, $"SELECT COUNT(*) FROM [{tableName}]"); - - private static int CountMasterRows(SqliteConnection connection, string type, string name) => - Count( - connection, - $"SELECT COUNT(*) FROM sqlite_master WHERE type='{type}' AND name='{name}'" - ); - - private static int Count(SqliteConnection connection, string sql) - { - using var command = connection.CreateCommand(); - command.CommandText = sql; - return Convert.ToInt32(command.ExecuteScalar(), CultureInfo.InvariantCulture); - } - - private static void WithDb(Action test) - { - var dbPath = Path.Combine(Path.GetTempPath(), $"sqliterls_{Guid.NewGuid()}.db"); - using var connection = new SqliteConnection($"Data Source={dbPath}"); - connection.Open(); - try - { - test(connection); - } - finally - { - if (File.Exists(dbPath)) - { - File.Delete(dbPath); - } - } - } } diff --git a/Migration/Nimblesite.DataProvider.Migration.Tests/SqliteTestDb.cs b/Migration/Nimblesite.DataProvider.Migration.Tests/SqliteTestDb.cs new file mode 100644 index 0000000..7c3e83c --- /dev/null +++ b/Migration/Nimblesite.DataProvider.Migration.Tests/SqliteTestDb.cs @@ -0,0 +1,95 @@ +using System.Globalization; + +namespace Nimblesite.DataProvider.Migration.Tests; + +/// +/// Shared SQLite E2E test helpers: temp-file databases, schema application +/// via inspect → diff → apply, and row counting. +/// +internal static class SqliteTestDb +{ + private static readonly ILogger Logger = NullLogger.Instance; + + public static void WithDb(Action test) + { + var dbPath = Path.Combine(Path.GetTempPath(), $"sqlitemig_{Guid.NewGuid()}.db"); + using var connection = new SqliteConnection($"Data Source={dbPath}"); + connection.Open(); + try + { + test(connection); + } + finally + { + if (File.Exists(dbPath)) + { + File.Delete(dbPath); + } + } + } + + public static SchemaDefinition Inspect(SqliteConnection connection) => + ((SchemaResultOk)SqliteSchemaInspector.Inspect(connection, Logger)).Value; + + public static void ApplySchema( + SqliteConnection connection, + SchemaDefinition schema, + bool allowDestructive = false + ) + { + var current = Inspect(connection); + var ops = ( + (OperationsResultOk) + SchemaDiff.Calculate(current, schema, allowDestructive, logger: Logger) + ).Value; + Apply(connection, ops, allowDestructive ? MigrationOptions.Destructive : null); + } + + public static void Apply( + SqliteConnection connection, + IReadOnlyList ops, + MigrationOptions? options = null + ) + { + var result = MigrationRunner.Apply( + connection, + ops, + SqliteDdlGenerator.Generate, + options ?? MigrationOptions.Default, + Logger + ); + Assert.True( + result is MigrationApplyResultOk, + $"Migration failed: {(result as MigrationApplyResultError)?.Value}" + ); + } + + public static void SetUser(SqliteConnection connection, string userId) + { + Execute(connection, "DELETE FROM [__rls_context]"); + Execute(connection, $"INSERT INTO [__rls_context]([current_user_id]) VALUES ('{userId}')"); + } + + public static void Execute(SqliteConnection connection, string sql) + { + using var command = connection.CreateCommand(); + command.CommandText = sql; + command.ExecuteNonQuery(); + } + + public static int CountRows(SqliteConnection connection, string tableName) => + Count(connection, $"SELECT COUNT(*) FROM [{tableName}]"); + + public static int CountMasterRows(SqliteConnection connection, string type, string name) => + Count( + connection, + $"SELECT COUNT(*) FROM sqlite_master WHERE type='{type}' AND name='{name}'" + ); + + public static int Count(SqliteConnection connection, string sql) + { + using var command = connection.CreateCommand(); + command.CommandText = sql; + return Convert.ToInt32(command.ExecuteScalar(), CultureInfo.InvariantCulture); + } +} diff --git a/Migration/Nimblesite.DataProvider.Migration.Tests/TriggerYamlSerializerTests.cs b/Migration/Nimblesite.DataProvider.Migration.Tests/TriggerYamlSerializerTests.cs new file mode 100644 index 0000000..dd3c8f1 --- /dev/null +++ b/Migration/Nimblesite.DataProvider.Migration.Tests/TriggerYamlSerializerTests.cs @@ -0,0 +1,93 @@ +namespace Nimblesite.DataProvider.Migration.Tests; + +// Implements [MIG-TRIGGER-YAML] from docs/specs/declarative-triggers-spec.md +// (GitHub issue 82). + +/// +/// YAML round-trip tests for declarative trigger definitions. +/// +public sealed class TriggerYamlSerializerTests +{ + private const string TriggerYaml = """ + name: tenant_app + tables: + - name: tenant_members + columns: + - name: id + type: Uuid + isNullable: false + - name: tenant_id + type: Uuid + isNullable: false + - name: role + type: VarChar(50) + isNullable: false + primaryKey: + columns: + - id + triggers: + - name: assert_not_last_owner + timing: Before + events: [Update, Delete] + forEachRow: true + raiseWhen: | + old.role = 'owner' and not exists( + tenant_members + |> filter(fn(m) => m.tenant_id = old.tenant_id and m.role = 'owner' and m.id <> old.id) + ) + errorMessage: cannot remove the last owner of a tenant + """; + + [Fact] + public void TriggerDefinition_YamlRoundTrip_PreservesDeclaration() + { + var schema = SchemaYamlSerializer.FromYaml(TriggerYaml); + var back = SchemaYamlSerializer.ToYaml(schema); + + Assert.Contains("triggers:", back, StringComparison.Ordinal); + Assert.Contains("assert_not_last_owner", back, StringComparison.Ordinal); + Assert.Contains("Update", back, StringComparison.Ordinal); + Assert.Contains("Delete", back, StringComparison.Ordinal); + Assert.Contains("raiseWhen", back, StringComparison.Ordinal); + Assert.Contains("not exists", back, StringComparison.Ordinal); + Assert.Contains("errorMessage", back, StringComparison.Ordinal); + Assert.Contains("cannot remove the last owner", back, StringComparison.Ordinal); + } + + [Fact] + public void TriggerDefinition_YamlRoundTrip_OmitsDefaults() + { + // Defaults: timing Before, forEachRow true. These must not appear in + // serialized YAML, and a second round-trip must preserve the trigger. + var schema = SchemaYamlSerializer.FromYaml(TriggerYaml); + var back = SchemaYamlSerializer.ToYaml(schema); + + Assert.DoesNotContain("timing:", back, StringComparison.Ordinal); + Assert.DoesNotContain("forEachRow:", back, StringComparison.Ordinal); + + var reparsed = SchemaYamlSerializer.FromYaml(back); + var rereserialized = SchemaYamlSerializer.ToYaml(reparsed); + Assert.Contains("assert_not_last_owner", rereserialized, StringComparison.Ordinal); + } + + [Fact] + public void Triggers_Absent_DoNotAppearInYaml() + { + var schema = new SchemaDefinition + { + Name = "t", + Tables = + [ + new TableDefinition + { + Name = "plain", + Columns = [new ColumnDefinition { Name = "id", Type = new UuidType() }], + }, + ], + }; + + var yaml = SchemaYamlSerializer.ToYaml(schema); + + Assert.DoesNotContain("triggers", yaml, StringComparison.Ordinal); + } +} diff --git a/docs/specs/declarative-triggers-spec.md b/docs/specs/declarative-triggers-spec.md new file mode 100644 index 0000000..e8f3dca --- /dev/null +++ b/docs/specs/declarative-triggers-spec.md @@ -0,0 +1,69 @@ +# Declarative Trigger Guards (GitHub issue 82) + +User-declared BEFORE triggers as first-class schema objects: diffed, inspected, +and dropped destructively, exactly like RLS policies. Motivating case: +last-owner protection on `tenant_members` (refuse to demote/delete the only +`owner` of a tenant), which RLS predicates cannot express. + +## [MIG-TRIGGER-YAML] YAML shape + +Triggers are declared per table under `triggers:`. + +```yaml +tables: + - name: tenant_members + triggers: + - name: assert_not_last_owner + timing: Before # Before (default) or After + events: [Update, Delete] # Insert | Update | Delete + forEachRow: true # default true + raiseWhen: | # LQL guard predicate; row refs via old./new. + old.role = 'owner' and not exists( + tenant_members + |> filter(fn(m) => m.tenant_id = old.tenant_id and m.role = 'owner' and m.id <> old.id) + ) + errorMessage: cannot remove the last owner of a tenant +``` + +The trigger raises `errorMessage` and aborts the statement when `raiseWhen` +evaluates true for the affected row. `raiseWhen` + `errorMessage` is the +declarative, platform-independent form of the issue's `bodyLql` proposal. + +## [MIG-TRIGGER-MODEL] Model + +`TriggerDefinition` record (Name, Timing, Events, ForEachRow, RaiseWhenLql, +ErrorMessage) on `TableDefinition.Triggers`. Enums `TriggerTiming` +(Before/After) and `TriggerEvent` (Insert/Update/Delete). Closed, immutable, +YAML round-trippable with defaults omitted. + +## [MIG-TRIGGER-DIFF] Diff semantics + +Triggers are diffed by name per table (mirror of RLS policy diff): +missing in current → `CreateTriggerOperation`; present in current but removed +from desired → `DropTriggerOperation` only when `allowDestructive` (the drop +is logged at Warning), otherwise skipped. `DropTriggerOperation` is +destructive and gated by `MigrationOptions.AllowDestructive`. + +## [MIG-TRIGGER-GUARD-LQL] Guard predicate LQL + +`raiseWhen` is an LQL boolean expression over the affected row and the +database. Row column references use `old.` / `new.` +(case-insensitive prefix). Supports simple comparisons, `and`/`or` +composition, and `exists(...)` / `not exists(...)` pipeline subqueries. +Transpiles identically (same semantics) on every supported platform. + +## [MIG-TRIGGER-SQLITE] SQLite emission and inspection + +One trigger per event named `usr_{event}_{trigger}_{table}`: +`CREATE TRIGGER ... BEFORE {EVENT} ON [table] FOR EACH ROW BEGIN +SELECT RAISE(ABORT, '{errorMessage}') WHERE {predicate}; END`. +Inspector reads `usr_`-prefixed triggers back from `sqlite_master` (grouped by +trigger name, `rls_` triggers excluded) so re-diff is a no-op. + +## [MIG-TRIGGER-PG] PostgreSQL emission and inspection + +One plpgsql function `{table}_{trigger}_trgfn` raising `errorMessage` when the +predicate holds, plus one trigger `{trigger}` (`BEFORE {events} ... FOR EACH +ROW EXECUTE FUNCTION`). Drop removes trigger and function. Inspector reads +triggers back from `information_schema.triggers` grouped by trigger name so +re-diff is a no-op. From 66cffea36693c5cd8ad237f67d086258fb5dd0d4 Mon Sep 17 00:00:00 2001 From: Christian Findlay <16697547+MelbourneDeveloper@users.noreply.github.com> Date: Thu, 2 Jul 2026 21:51:16 +1000 Subject: [PATCH 2/5] fixes --- .../RlsPredicateTranspiler.TriggerGuard.cs | 294 +++++++++++++----- .../SchemaYamlSerializer.cs | 38 --- .../TriggerDdlSupport.cs | 110 ++++++- .../TriggerEnumYamlConverter.cs | 44 +++ .../PostgresSupportSchemaInspector.cs | 8 + .../PostgresTriggerDdlBuilder.cs | 88 +++--- .../PostgresTriggerSchemaInspector.cs | 17 +- .../SqliteRlsSchemaInspector.cs | 9 +- .../SqliteTriggerDdlBuilder.cs | 31 +- .../SqliteTriggerNames.cs | 3 + .../SqliteTriggerSchemaInspector.cs | 13 +- .../GlobalUsings.cs | 4 + .../PostgresDeclarativeTriggerE2ETests.cs | 111 +++++++ .../PostgresTestDb.cs | 19 +- .../PostgresTriggerDdlTests.cs | 73 +++++ .../SqliteDeclarativeTriggerTests.cs | 90 ++++++ .../SqliteTestDb.cs | 38 ++- docs/specs/declarative-triggers-spec.md | 31 +- 18 files changed, 802 insertions(+), 219 deletions(-) create mode 100644 Migration/Nimblesite.DataProvider.Migration.Core/TriggerEnumYamlConverter.cs create mode 100644 Migration/Nimblesite.DataProvider.Migration.Tests/PostgresTriggerDdlTests.cs diff --git a/Migration/Nimblesite.DataProvider.Migration.Core/RlsPredicateTranspiler.TriggerGuard.cs b/Migration/Nimblesite.DataProvider.Migration.Core/RlsPredicateTranspiler.TriggerGuard.cs index f8f0200..dd96713 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Core/RlsPredicateTranspiler.TriggerGuard.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Core/RlsPredicateTranspiler.TriggerGuard.cs @@ -25,6 +25,7 @@ namespace Nimblesite.DataProvider.Migration.Core; /// public static partial class RlsPredicateTranspiler { + private const string ReservedGuardTokenPrefix = "__TRG_"; private const string OldRowSentinelPrefix = "__TRG_OLD_"; private const string NewRowSentinelPrefix = "__TRG_NEW_"; private const string ExistsMarkerPrefix = "__TRG_EXISTS_"; @@ -34,7 +35,9 @@ public static partial class RlsPredicateTranspiler /// Row column references use old./new. prefixes /// (case-insensitive) and become platform-quoted OLD./NEW. /// references. exists(pipeline) and not exists(pipeline) - /// subqueries may appear anywhere in the predicate. + /// subqueries may appear anywhere in the predicate. Predicates containing + /// the reserved __TRG_ token are rejected so user literals can + /// never collide with internal sentinels. /// /// LQL guard predicate. /// Target platform. @@ -49,46 +52,92 @@ string triggerName { return new GuardSqlError(MigrationError.RlsEmptyPredicate(triggerName)); } + if (lql.Contains(ReservedGuardTokenPrefix, StringComparison.Ordinal)) + { + return new GuardSqlError( + MigrationError.FromMessage( + $"Trigger '{triggerName}': guard predicate must not contain the reserved token '{ReservedGuardTokenPrefix}'" + ) + ); + } var withRowSentinels = SubstituteRowReferences(lql.Trim()); var (remainder, segments) = ExtractExistsSegments(withRowSentinels); var simple = TranslateSimplePredicate(remainder, platform, triggerName); - if (simple is GuardSqlError simpleError) - { - return simpleError; - } if (simple is not GuardSqlOk simpleOk) { - return new GuardSqlError( - MigrationError.RlsLqlParse(triggerName, "unknown guard predicate failure") - ); + return simple; } - var sql = simpleOk.Value; - for (var index = 0; index < segments.Count; index++) + var expanded = ExpandExistsSegments(simpleOk.Value, segments, platform, triggerName); + return expanded is GuardSqlOk expandedOk + ? new GuardSqlOk(RestoreRowReferences(expandedOk.Value, platform)) + : expanded; + } + + /// + /// True when the guard predicate references the given row word + /// (old or new) as a row.column prefix outside + /// string literals. Used to validate row references against the declared + /// trigger events. + /// + internal static bool GuardReferencesRow(string lql, string rowWord) + { + var i = 0; + while (i < lql.Length) { - var segment = segments[index]; - var subquery = TranslateExistsSubquery(segment.InnerLql, platform, triggerName); - if (subquery is GuardSqlError subqueryError) + if (lql[i] == '\'') { - return subqueryError; + i = SkipStringLiteral(lql, i); + continue; } - if (subquery is not GuardSqlOk subqueryOk) + if (IsIdentifierStart(lql, i)) { - return new GuardSqlError( - MigrationError.RlsLqlParse(triggerName, "unknown guard subquery failure") - ); + var start = i; + i = ReadIdentifierEnd(lql, i); + if (IsRowReference(lql, i, lql[start..i], rowWord)) + { + return true; + } + continue; } - var replacement = segment.Negated ? $"NOT {subqueryOk.Value}" : subqueryOk.Value; - sql = sql.Replace(ExistsMarker(index), replacement, StringComparison.Ordinal); + i++; } - - return new GuardSqlOk(RestoreRowReferences(sql, platform)); + return false; } private static string ExistsMarker(int index) => $"'{ExistsMarkerPrefix}{index}__'"; + /// + /// Transpiles each extracted exists-segment and splices it back over its + /// marker. Postgres pipeline output gets an identifier-quoting pass so + /// mixed-case columns survive Postgres case folding. + /// + private static GuardSqlResult ExpandExistsSegments( + string sql, + IReadOnlyList segments, + RlsPlatform platform, + string triggerName + ) + { + for (var index = 0; index < segments.Count; index++) + { + var subquery = TranslateExistsSubquery(segments[index].InnerLql, platform, triggerName); + if (subquery is not GuardSqlOk subqueryOk) + { + return subquery; + } + var body = + platform == RlsPlatform.Postgres + ? QuotePipelineIdentifiersForPostgres(subqueryOk.Value) + : subqueryOk.Value; + var replacement = segments[index].Negated ? $"NOT {body}" : body; + sql = sql.Replace(ExistsMarker(index), replacement, StringComparison.Ordinal); + } + return new GuardSqlOk(sql); + } + /// /// Replaces old.col/new.col references with sentinel string /// literals that survive LQL transpilation. Skips string literals. @@ -108,30 +157,7 @@ private static string SubstituteRowReferences(string source) { var start = i; i = ReadIdentifierEnd(source, i); - var word = source[start..i]; - var isRowRef = - ( - word.Equals("old", StringComparison.OrdinalIgnoreCase) - || word.Equals("new", StringComparison.OrdinalIgnoreCase) - ) - && i + 1 < source.Length - && source[i] == '.' - && (char.IsLetter(source[i + 1]) || source[i + 1] == '_'); - if (isRowRef) - { - var columnStart = i + 1; - var columnEnd = ReadIdentifierEnd(source, columnStart); - var prefix = word.Equals("old", StringComparison.OrdinalIgnoreCase) - ? OldRowSentinelPrefix - : NewRowSentinelPrefix; - sb.Append('\'') - .Append(prefix) - .Append(source[columnStart..columnEnd]) - .Append("__'"); - i = columnEnd; - continue; - } - sb.Append(word); + i = AppendWordOrRowSentinel(source, i, source[start..i], sb); continue; } sb.Append(source[i]); @@ -140,6 +166,29 @@ private static string SubstituteRowReferences(string source) return sb.ToString(); } + private static int AppendWordOrRowSentinel(string source, int i, string word, StringBuilder sb) + { + var isOld = IsRowReference(source, i, word, "old"); + if (!isOld && !IsRowReference(source, i, word, "new")) + { + sb.Append(word); + return i; + } + var columnStart = i + 1; + var columnEnd = ReadIdentifierEnd(source, columnStart); + sb.Append('\'') + .Append(isOld ? OldRowSentinelPrefix : NewRowSentinelPrefix) + .Append(source[columnStart..columnEnd]) + .Append("__'"); + return columnEnd; + } + + private static bool IsRowReference(string source, int wordEnd, string word, string rowWord) => + word.Equals(rowWord, StringComparison.OrdinalIgnoreCase) + && wordEnd + 1 < source.Length + && source[wordEnd] == '.' + && (char.IsLetter(source[wordEnd + 1]) || source[wordEnd + 1] == '_'); + /// /// Extracts every top-level [not] exists(...) subquery, replacing /// each with a marker literal so the remainder can be translated as a @@ -164,34 +213,7 @@ IReadOnlyList Segments { var start = i; i = ReadIdentifierEnd(predicate, i); - var word = predicate[start..i]; - if (word.Equals("not", StringComparison.OrdinalIgnoreCase)) - { - var afterNot = SkipWhitespace(predicate, i); - var keywordEnd = ReadIdentifierEnd(predicate, afterNot); - var next = predicate[afterNot..keywordEnd]; - if ( - next.Equals("exists", StringComparison.OrdinalIgnoreCase) - && TryReadSubquery(predicate, keywordEnd, out var negatedInner, out var end) - ) - { - segments.Add(new GuardExistsSegment(negatedInner, Negated: true)); - sb.Append(ExistsMarker(segments.Count - 1)); - i = end; - continue; - } - } - if ( - word.Equals("exists", StringComparison.OrdinalIgnoreCase) - && TryReadSubquery(predicate, i, out var inner, out var afterSubquery) - ) - { - segments.Add(new GuardExistsSegment(inner, Negated: false)); - sb.Append(ExistsMarker(segments.Count - 1)); - i = afterSubquery; - continue; - } - sb.Append(word); + i = AppendWordOrExistsMarker(predicate, i, predicate[start..i], segments, sb); continue; } sb.Append(predicate[i]); @@ -200,6 +222,41 @@ IReadOnlyList Segments return (sb.ToString(), segments); } + private static int AppendWordOrExistsMarker( + string predicate, + int i, + string word, + List segments, + StringBuilder sb + ) + { + if (word.Equals("not", StringComparison.OrdinalIgnoreCase)) + { + var afterNot = SkipWhitespace(predicate, i); + var keywordEnd = ReadIdentifierEnd(predicate, afterNot); + if ( + predicate[afterNot..keywordEnd].Equals("exists", StringComparison.OrdinalIgnoreCase) + && TryReadSubquery(predicate, keywordEnd, out var negatedInner, out var end) + ) + { + segments.Add(new GuardExistsSegment(negatedInner, Negated: true)); + sb.Append(ExistsMarker(segments.Count - 1)); + return end; + } + } + if ( + word.Equals("exists", StringComparison.OrdinalIgnoreCase) + && TryReadSubquery(predicate, i, out var inner, out var afterSubquery) + ) + { + segments.Add(new GuardExistsSegment(inner, Negated: false)); + sb.Append(ExistsMarker(segments.Count - 1)); + return afterSubquery; + } + sb.Append(word); + return i; + } + /// /// Reads a balanced-paren subquery starting at the first ( after /// . Returns false when no subquery follows. @@ -241,6 +298,95 @@ private static bool TryReadSubquery(string source, int from, out string inner, o return false; } + /// + /// Quotes bare identifiers (including alias-qualified members) in a + /// transpiled Postgres pipeline subquery so mixed-case column names are + /// not case-folded by Postgres. Skips string literals, already-quoted + /// identifiers, SQL keywords, and function calls. + /// + private static string QuotePipelineIdentifiersForPostgres(string sql) + { + var sb = new StringBuilder(sql.Length + 16); + var i = 0; + while (i < sql.Length) + { + var c = sql[i]; + if (c == '\'') + { + i = AppendStringLiteral(sql, i, sb); + continue; + } + if (c == '"') + { + i = AppendQuotedIdentifier(sql, i, sb); + continue; + } + if (char.IsLetter(c) || c == '_') + { + var start = i; + i = ReadIdentifierEnd(sql, i); + var word = sql[start..i]; + sb.Append(ShouldQuotePipelineWord(sql, i, word) ? $"\"{word}\"" : word); + continue; + } + sb.Append(c); + i++; + } + return sb.ToString(); + } + + private static bool ShouldQuotePipelineWord(string sql, int wordEnd, string word) + { + if (IsKeyword(word) || IsPipelineSqlKeyword(word) || IsBuiltinExpr(word)) + { + return false; + } + var next = SkipWhitespace(sql, wordEnd); + return next >= sql.Length || sql[next] != '('; + } + + private static bool IsPipelineSqlKeyword(string word) + { + var u = word.ToUpperInvariant(); + return u + is "JOIN" + or "INNER" + or "LEFT" + or "RIGHT" + or "FULL" + or "OUTER" + or "CROSS" + or "GROUP" + or "BY" + or "ORDER" + or "HAVING" + or "LIMIT" + or "OFFSET" + or "DISTINCT" + or "UNION" + or "INTERSECT" + or "EXCEPT" + or "ALL" + or "ASC" + or "DESC"; + } + + private static int AppendQuotedIdentifier(string source, int start, StringBuilder sb) + { + sb.Append(source[start]); + var i = start + 1; + while (i < source.Length) + { + sb.Append(source[i]); + if (source[i] == '"') + { + return i + 1; + } + i++; + } + return i; + } + /// /// Replaces row-reference sentinel literals with platform-quoted /// OLD./NEW. column references. diff --git a/Migration/Nimblesite.DataProvider.Migration.Core/SchemaYamlSerializer.cs b/Migration/Nimblesite.DataProvider.Migration.Core/SchemaYamlSerializer.cs index 1f4f52a..c6e8eb7 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Core/SchemaYamlSerializer.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Core/SchemaYamlSerializer.cs @@ -356,44 +356,6 @@ public void WriteYaml(IEmitter emitter, object? value, Type type, ObjectSerializ } } -/// -/// YAML type converter for the and -/// enums. Implements [MIG-TRIGGER-YAML]. -/// -internal sealed class TriggerEnumYamlConverter : IYamlTypeConverter -{ - /// - public bool Accepts(Type type) => type == typeof(TriggerTiming) || type == typeof(TriggerEvent); - - /// - public object? ReadYaml(IParser parser, Type type, ObjectDeserializer rootDeserializer) - { - var scalar = parser.Consume(); - return type == typeof(TriggerTiming) ? ParseTiming(scalar.Value) : ParseEvent(scalar.Value); - } - - /// - public void WriteYaml(IEmitter emitter, object? value, Type type, ObjectSerializer serializer) - { - emitter.Emit(new Scalar(value?.ToString() ?? string.Empty)); - } - - private static object ParseTiming(string value) => - value.ToUpperInvariant() switch - { - "AFTER" => TriggerTiming.After, - _ => TriggerTiming.Before, - }; - - private static object ParseEvent(string value) => - value.ToUpperInvariant() switch - { - "UPDATE" => TriggerEvent.Update, - "DELETE" => TriggerEvent.Delete, - _ => TriggerEvent.Insert, - }; -} - /// /// YAML type converter for the enum. /// Implements [RLS-PG-SUPPORT-DDL]. diff --git a/Migration/Nimblesite.DataProvider.Migration.Core/TriggerDdlSupport.cs b/Migration/Nimblesite.DataProvider.Migration.Core/TriggerDdlSupport.cs index e8bc57f..9a05338 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Core/TriggerDdlSupport.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Core/TriggerDdlSupport.cs @@ -1,21 +1,59 @@ +using GuardTranspileError = Outcome.Result< + string, + Nimblesite.DataProvider.Migration.Core.MigrationError +>.Error; +using GuardTranspileOk = Outcome.Result< + string, + Nimblesite.DataProvider.Migration.Core.MigrationError +>.Ok; + namespace Nimblesite.DataProvider.Migration.Core; // Implements [MIG-TRIGGER-SQLITE] and [MIG-TRIGGER-PG] shared DDL support // from docs/specs/declarative-triggers-spec.md (GitHub issue 82). /// -/// Shared helpers for platform trigger DDL builders: validation, error -/// message defaulting/escaping, and keyword mapping. +/// Shared helpers for platform trigger DDL builders: guard predicate +/// validation and translation, error message defaulting/escaping, and +/// keyword mapping. All failures throw, matching the established +/// DDL-generator pattern; MigrationRunner converts them to error +/// results. /// public static class TriggerDdlSupport { - /// Returns the guard predicate or throws when missing. - public static string RequireRaiseWhen(TriggerDefinition trigger) => - trigger.RaiseWhenLql is { } lql && !string.IsNullOrWhiteSpace(lql) - ? lql - : throw new InvalidOperationException( - $"Trigger '{trigger.Name}' has no raiseWhen predicate" - ); + /// + /// Name prefix identifying migration-managed trigger objects on every + /// platform. Inspectors ignore triggers without this prefix so + /// destructive runs never drop triggers created outside the migration + /// tool (e.g. Sync change tracking). + /// + public const string ManagedTriggerPrefix = "usr_"; + + /// + /// Name suffix identifying PostgreSQL trigger guard functions. These are + /// owned by the trigger lifecycle and excluded from support-function + /// read-back so destructive runs do not try to drop them independently. + /// + public const string GuardFunctionSuffix = "_trgfn"; + + /// + /// Validates the trigger declaration and translates its guard predicate + /// for the target platform. Throws + /// on invalid declarations and for + /// statement-level triggers. + /// + public static string BuildGuardPredicate(TriggerDefinition trigger, RlsPlatform platform) + { + RequireForEachRow(trigger); + var lql = RequireRaiseWhen(trigger); + ValidateRowReferences(trigger, lql, RequireEvents(trigger)); + var result = RlsPredicateTranspiler.TranslateGuardPredicate(lql, platform, trigger.Name); + return result switch + { + GuardTranspileOk ok => ok.Value, + GuardTranspileError error => throw new InvalidOperationException(error.Value.Message), + }; + } /// Returns the distinct declared events or throws when empty. public static IReadOnlyList RequireEvents(TriggerDefinition trigger) => @@ -48,4 +86,58 @@ public static string EventVerb(TriggerEvent triggerEvent) => /// SQL timing keyword for a trigger timing. public static string TimingKeyword(TriggerTiming timing) => timing == TriggerTiming.After ? "AFTER" : "BEFORE"; + + private static string RequireRaiseWhen(TriggerDefinition trigger) => + trigger.RaiseWhenLql is { } lql && !string.IsNullOrWhiteSpace(lql) + ? lql + : throw new InvalidOperationException( + $"Trigger '{trigger.Name}' has no raiseWhen predicate" + ); + + /// + /// Guards are row-level on every platform. SQLite has no statement-level + /// triggers and Postgres statement-level triggers see NULL OLD/NEW, which + /// would silently disable the guard -- so forEachRow: false fails + /// loudly instead of diverging per platform. + /// + private static void RequireForEachRow(TriggerDefinition trigger) + { + if (!trigger.ForEachRow) + { + throw new NotSupportedException( + $"Trigger '{trigger.Name}': forEachRow: false (statement-level) is not supported; guards are row-level on every platform" + ); + } + } + + /// + /// Rejects row references that do not exist for a declared event + /// (old. on INSERT, new. on DELETE). Without this, SQLite + /// fails at DML time while Postgres silently never fires the guard. + /// + private static void ValidateRowReferences( + TriggerDefinition trigger, + string lql, + IReadOnlyList events + ) + { + if ( + events.Contains(TriggerEvent.Insert) + && RlsPredicateTranspiler.GuardReferencesRow(lql, "old") + ) + { + throw new InvalidOperationException( + $"Trigger '{trigger.Name}' references old. but fires on Insert; OLD row values do not exist for INSERT" + ); + } + if ( + events.Contains(TriggerEvent.Delete) + && RlsPredicateTranspiler.GuardReferencesRow(lql, "new") + ) + { + throw new InvalidOperationException( + $"Trigger '{trigger.Name}' references new. but fires on Delete; NEW row values do not exist for DELETE" + ); + } + } } diff --git a/Migration/Nimblesite.DataProvider.Migration.Core/TriggerEnumYamlConverter.cs b/Migration/Nimblesite.DataProvider.Migration.Core/TriggerEnumYamlConverter.cs new file mode 100644 index 0000000..3b710ff --- /dev/null +++ b/Migration/Nimblesite.DataProvider.Migration.Core/TriggerEnumYamlConverter.cs @@ -0,0 +1,44 @@ +using YamlDotNet.Core; +using YamlDotNet.Core.Events; +using YamlDotNet.Serialization; + +namespace Nimblesite.DataProvider.Migration.Core; + +/// +/// YAML type converter for the and +/// enums. Implements [MIG-TRIGGER-YAML] +/// (GitHub issue 82). +/// +internal sealed class TriggerEnumYamlConverter : IYamlTypeConverter +{ + /// + public bool Accepts(Type type) => type == typeof(TriggerTiming) || type == typeof(TriggerEvent); + + /// + public object? ReadYaml(IParser parser, Type type, ObjectDeserializer rootDeserializer) + { + var scalar = parser.Consume(); + return type == typeof(TriggerTiming) ? ParseTiming(scalar.Value) : ParseEvent(scalar.Value); + } + + /// + public void WriteYaml(IEmitter emitter, object? value, Type type, ObjectSerializer serializer) + { + emitter.Emit(new Scalar(value?.ToString() ?? string.Empty)); + } + + private static object ParseTiming(string value) => + value.ToUpperInvariant() switch + { + "AFTER" => TriggerTiming.After, + _ => TriggerTiming.Before, + }; + + private static object ParseEvent(string value) => + value.ToUpperInvariant() switch + { + "UPDATE" => TriggerEvent.Update, + "DELETE" => TriggerEvent.Delete, + _ => TriggerEvent.Insert, + }; +} diff --git a/Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresSupportSchemaInspector.cs b/Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresSupportSchemaInspector.cs index 1da3dbb..4060cf1 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresSupportSchemaInspector.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresSupportSchemaInspector.cs @@ -87,9 +87,17 @@ FROM pg_proc p JOIN pg_namespace n ON n.oid = p.pronamespace JOIN pg_language l ON l.oid = p.prolang WHERE n.nspname = @schema + -- [MIG-TRIGGER-PG]: trigger guard functions are owned by the + -- trigger lifecycle; reading them back would make destructive + -- runs try to drop them while their trigger still depends on them. + AND p.proname NOT LIKE @guardFunctionPattern ESCAPE '\' ORDER BY n.nspname, p.proname, p.oid """; command.Parameters.AddWithValue("@schema", schemaName); + command.Parameters.AddWithValue( + "@guardFunctionPattern", + $"%{TriggerDdlSupport.GuardFunctionSuffix.Replace("_", "\\_", StringComparison.Ordinal)}" + ); using var reader = command.ExecuteReader(); var functions = new List(); diff --git a/Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresTriggerDdlBuilder.cs b/Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresTriggerDdlBuilder.cs index 5d0a460..7f2c269 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresTriggerDdlBuilder.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresTriggerDdlBuilder.cs @@ -1,12 +1,3 @@ -using GuardTranspileError = Outcome.Result< - string, - Nimblesite.DataProvider.Migration.Core.MigrationError ->.Error; -using GuardTranspileOk = Outcome.Result< - string, - Nimblesite.DataProvider.Migration.Core.MigrationError ->.Ok; - namespace Nimblesite.DataProvider.Migration.Postgres; // Implements [MIG-TRIGGER-PG] from docs/specs/declarative-triggers-spec.md @@ -15,66 +6,93 @@ namespace Nimblesite.DataProvider.Migration.Postgres; /// /// Emits PostgreSQL DDL for declarative trigger guards: one plpgsql guard /// function ({table}_{trigger}_trgfn) that raises the declared error -/// when the guard predicate holds, plus one trigger wired to the declared -/// events. RAISE ... USING MESSAGE avoids format-string -/// interpretation of % in user messages. +/// when the guard predicate holds, plus one trigger named +/// usr_{trigger} wired to the declared events. The usr_ prefix +/// marks the trigger as migration-managed so the inspector never reads (and +/// destructive runs never drop) triggers created outside the tool. +/// RAISE ... USING MESSAGE avoids format-string interpretation of +/// % in user messages, and the dollar-quote tag is chosen to never +/// collide with the function body. /// internal static class PostgresTriggerDdlBuilder { + private const int MaxIdentifierBytes = 63; + public static string GenerateCreate(CreateTriggerOperation op) { - var predicate = Translate(TriggerDdlSupport.RequireRaiseWhen(op.Trigger), op.Trigger.Name); - var functionName = FunctionName(op.TableName, op.Trigger.Name); + var predicate = TriggerDdlSupport.BuildGuardPredicate(op.Trigger, RlsPlatform.Postgres); + var message = TriggerDdlSupport.EscapedMessage(op.Trigger); + var triggerName = ValidateIdentifier(ObjectName(op.Trigger.Name), "trigger name"); + var functionName = ValidateIdentifier( + FunctionName(op.TableName, op.Trigger.Name), + "trigger guard function name" + ); var events = string.Join( " OR ", TriggerDdlSupport.RequireEvents(op.Trigger).Select(TriggerDdlSupport.EventVerb) ); - var level = op.Trigger.ForEachRow ? "ROW" : "STATEMENT"; + var tag = DollarTag($"{predicate}\n{message}"); return $""" CREATE OR REPLACE FUNCTION "{op.Schema}"."{functionName}"() RETURNS trigger LANGUAGE plpgsql - AS $trigger_guard$ + AS {tag} BEGIN IF {predicate} THEN - RAISE EXCEPTION USING MESSAGE = '{TriggerDdlSupport.EscapedMessage(op.Trigger)}'; + RAISE EXCEPTION USING MESSAGE = '{message}'; END IF; IF TG_OP = 'DELETE' THEN RETURN OLD; END IF; RETURN NEW; END - $trigger_guard$; - DROP TRIGGER IF EXISTS "{op.Trigger.Name}" ON "{op.Schema}"."{op.TableName}"; - CREATE TRIGGER "{op.Trigger.Name}" + {tag}; + DROP TRIGGER IF EXISTS "{triggerName}" ON "{op.Schema}"."{op.TableName}"; + CREATE TRIGGER "{triggerName}" {TriggerDdlSupport.TimingKeyword( op.Trigger.Timing )} {events} ON "{op.Schema}"."{op.TableName}" - FOR EACH {level} + FOR EACH ROW EXECUTE FUNCTION "{op.Schema}"."{functionName}"() """; } public static string GenerateDrop(DropTriggerOperation op) => $""" - DROP TRIGGER IF EXISTS "{op.TriggerName}" ON "{op.Schema}"."{op.TableName}"; + DROP TRIGGER IF EXISTS "{ObjectName(op.TriggerName)}" ON "{op.Schema}"."{op.TableName}"; DROP FUNCTION IF EXISTS "{op.Schema}"."{FunctionName(op.TableName, op.TriggerName)}"() """; - private static string Translate(string lql, string triggerName) + private static string ObjectName(string triggerName) => + $"{TriggerDdlSupport.ManagedTriggerPrefix}{triggerName}"; + + private static string FunctionName(string tableName, string triggerName) => + $"{tableName}_{triggerName}{TriggerDdlSupport.GuardFunctionSuffix}"; + + /// + /// PostgreSQL silently truncates identifiers to 63 bytes, which would + /// desynchronise create/drop/inspect names -- fail loudly instead. + /// + private static string ValidateIdentifier(string name, string description) => + Encoding.UTF8.GetByteCount(name) <= MaxIdentifierBytes + ? name + : throw new InvalidOperationException( + $"PostgreSQL {description} '{name}' exceeds the 63-byte identifier limit" + ); + + /// + /// Picks a dollar-quote tag that does not occur in the function body so + /// user-supplied predicates/messages can never terminate the quoting. + /// + private static string DollarTag(string body) { - var result = RlsPredicateTranspiler.TranslateGuardPredicate( - lql, - RlsPlatform.Postgres, - triggerName - ); - return result switch + var suffix = 0; + var tag = "$trigger_guard$"; + while (body.Contains(tag, StringComparison.Ordinal)) { - GuardTranspileOk ok => ok.Value, - GuardTranspileError error => throw new InvalidOperationException(error.Value.Message), - }; + suffix++; + tag = $"$trigger_guard{suffix}$"; + } + return tag; } - - private static string FunctionName(string tableName, string triggerName) => - $"{tableName}_{triggerName}_trgfn"; } diff --git a/Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresTriggerSchemaInspector.cs b/Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresTriggerSchemaInspector.cs index a457e04..56852b5 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresTriggerSchemaInspector.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresTriggerSchemaInspector.cs @@ -4,9 +4,12 @@ namespace Nimblesite.DataProvider.Migration.Postgres; // docs/specs/declarative-triggers-spec.md (GitHub issue 82). /// -/// Reads triggers back from information_schema.triggers (one row per -/// event) grouped by trigger name so the diff can match declarative trigger -/// guards by name. +/// Reads migration-managed triggers back from +/// information_schema.triggers (one row per event) grouped by trigger +/// name so the diff can match declarative trigger guards by name. Only +/// usr_-prefixed triggers are read: triggers created outside the +/// migration tool (e.g. Sync change tracking) stay invisible to the diff so +/// destructive runs never drop them. /// internal static class PostgresTriggerSchemaInspector { @@ -21,10 +24,15 @@ string tableName SELECT trigger_name, event_manipulation, action_timing, action_orientation FROM information_schema.triggers WHERE event_object_schema = @schema AND event_object_table = @table + AND trigger_name LIKE @managedPattern ESCAPE '\' ORDER BY trigger_name """; command.Parameters.AddWithValue("@schema", schemaName); command.Parameters.AddWithValue("@table", tableName); + command.Parameters.AddWithValue( + "@managedPattern", + $"{TriggerDdlSupport.ManagedTriggerPrefix.Replace("_", "\\_", StringComparison.Ordinal)}%" + ); var rows = new List(); using (var reader = command.ExecuteReader()) @@ -51,7 +59,8 @@ ORDER BY trigger_name private static TriggerDefinition ToTrigger(IGrouping group) => new() { - Name = group.Key, + // Strip the managed prefix: the model holds the declared name. + Name = group.Key[TriggerDdlSupport.ManagedTriggerPrefix.Length..], Timing = group.Any(r => r.Timing.Equals("AFTER", StringComparison.OrdinalIgnoreCase)) ? TriggerTiming.After : TriggerTiming.Before, diff --git a/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteRlsSchemaInspector.cs b/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteRlsSchemaInspector.cs index e04aee3..3ee0ff2 100644 --- a/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteRlsSchemaInspector.cs +++ b/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteRlsSchemaInspector.cs @@ -4,8 +4,6 @@ namespace Nimblesite.DataProvider.Migration.SQLite; internal static class SqliteRlsSchemaInspector { - private static readonly string[] EventTokens = ["insert", "update", "delete"]; - public static RlsPolicySetDefinition? Inspect(SqliteConnection connection, string tableName) { var triggers = SqliteTriggerNames.Read(connection, tableName, $"rls_%_{tableName}"); @@ -27,7 +25,12 @@ internal static class SqliteRlsSchemaInspector private static SqliteRlsTriggerPolicy? ToTriggerPolicy(string name, string tableName) { - var parsed = SqliteTriggerNames.Parse(name, "rls_", tableName, EventTokens); + var parsed = SqliteTriggerNames.Parse( + name, + "rls_", + tableName, + SqliteTriggerNames.DmlEventTokens + ); return parsed is null ? null : new SqliteRlsTriggerPolicy(parsed.BaseName, ToOperation(parsed.EventToken)); diff --git a/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteTriggerDdlBuilder.cs b/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteTriggerDdlBuilder.cs index c53e673..e008330 100644 --- a/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteTriggerDdlBuilder.cs +++ b/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteTriggerDdlBuilder.cs @@ -1,12 +1,3 @@ -using GuardTranspileError = Outcome.Result< - string, - Nimblesite.DataProvider.Migration.Core.MigrationError ->.Error; -using GuardTranspileOk = Outcome.Result< - string, - Nimblesite.DataProvider.Migration.Core.MigrationError ->.Ok; - namespace Nimblesite.DataProvider.Migration.SQLite; // Implements [MIG-TRIGGER-SQLITE] from docs/specs/declarative-triggers-spec.md @@ -20,11 +11,9 @@ namespace Nimblesite.DataProvider.Migration.SQLite; /// internal static class SqliteTriggerDdlBuilder { - private static readonly string[] EventTokens = ["insert", "update", "delete"]; - public static string GenerateCreate(CreateTriggerOperation op) { - var predicate = Translate(TriggerDdlSupport.RequireRaiseWhen(op.Trigger), op.Trigger.Name); + var predicate = TriggerDdlSupport.BuildGuardPredicate(op.Trigger, RlsPlatform.Sqlite); var message = TriggerDdlSupport.EscapedMessage(op.Trigger); return string.Join( ";\n", @@ -37,7 +26,7 @@ public static string GenerateCreate(CreateTriggerOperation op) public static string GenerateDrop(DropTriggerOperation op) => string.Join( ";\n", - EventTokens.Select(token => + SqliteTriggerNames.DmlEventTokens.Select(token => $"DROP TRIGGER IF EXISTS [{TriggerName(token, op.TriggerName, op.TableName)}]" ) ); @@ -62,20 +51,6 @@ SELECT RAISE(ABORT, '{message}') """; } - private static string Translate(string lql, string triggerName) - { - var result = RlsPredicateTranspiler.TranslateGuardPredicate( - lql, - RlsPlatform.Sqlite, - triggerName - ); - return result switch - { - GuardTranspileOk ok => ok.Value, - GuardTranspileError error => throw new InvalidOperationException(error.Value.Message), - }; - } - private static string TriggerName(string eventToken, string triggerName, string tableName) => - $"usr_{eventToken}_{triggerName}_{tableName}"; + $"{TriggerDdlSupport.ManagedTriggerPrefix}{eventToken}_{triggerName}_{tableName}"; } diff --git a/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteTriggerNames.cs b/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteTriggerNames.cs index 39621d8..441c3ba 100644 --- a/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteTriggerNames.cs +++ b/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteTriggerNames.cs @@ -7,6 +7,9 @@ namespace Nimblesite.DataProvider.Migration.SQLite; /// internal static class SqliteTriggerNames { + /// DML event tokens used in managed trigger names. + public static IReadOnlyList DmlEventTokens { get; } = ["insert", "update", "delete"]; + public static List Read(SqliteConnection connection, string tableName, string pattern) { using var command = connection.CreateCommand(); diff --git a/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteTriggerSchemaInspector.cs b/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteTriggerSchemaInspector.cs index df62c4b..74f2aec 100644 --- a/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteTriggerSchemaInspector.cs +++ b/Migration/Nimblesite.DataProvider.Migration.SQLite/SqliteTriggerSchemaInspector.cs @@ -10,15 +10,20 @@ namespace Nimblesite.DataProvider.Migration.SQLite; /// internal static class SqliteTriggerSchemaInspector { - private static readonly string[] EventTokens = ["insert", "update", "delete"]; - public static IReadOnlyList Inspect( SqliteConnection connection, string tableName ) => SqliteTriggerNames - .Read(connection, tableName, $"usr_%_{tableName}") - .Select(name => SqliteTriggerNames.Parse(name, "usr_", tableName, EventTokens)) + .Read(connection, tableName, $"{TriggerDdlSupport.ManagedTriggerPrefix}%_{tableName}") + .Select(name => + SqliteTriggerNames.Parse( + name, + TriggerDdlSupport.ManagedTriggerPrefix, + tableName, + SqliteTriggerNames.DmlEventTokens + ) + ) .OfType() .GroupBy(p => p.BaseName, StringComparer.OrdinalIgnoreCase) .Select(ToTrigger) diff --git a/Migration/Nimblesite.DataProvider.Migration.Tests/GlobalUsings.cs b/Migration/Nimblesite.DataProvider.Migration.Tests/GlobalUsings.cs index 3c05eaf..d90fb25 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Tests/GlobalUsings.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Tests/GlobalUsings.cs @@ -7,6 +7,10 @@ global using Nimblesite.TestSupport; global using Npgsql; global using Xunit; +global using MigrationApplyResult = Outcome.Result< + bool, + Nimblesite.DataProvider.Migration.Core.MigrationError +>; global using MigrationApplyResultError = Outcome.Result< bool, Nimblesite.DataProvider.Migration.Core.MigrationError diff --git a/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresDeclarativeTriggerE2ETests.cs b/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresDeclarativeTriggerE2ETests.cs index d6e92fe..429a202 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresDeclarativeTriggerE2ETests.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresDeclarativeTriggerE2ETests.cs @@ -122,6 +122,117 @@ public void PgTrigger_RediffAfterApply_ProducesNoOperations() Assert.Empty(ops); } + private const string PascalCaseGuardYaml = """ + name: tenant_app + tables: + - name: TenantMembers + columns: + - name: Id + type: Uuid + isNullable: false + - name: TenantId + type: Uuid + isNullable: false + - name: Role + type: VarChar(50) + isNullable: false + primaryKey: + columns: + - Id + triggers: + - name: assert_not_last_owner + events: [Update, Delete] + raiseWhen: | + old.Role = 'owner' and not exists( + TenantMembers + |> filter(fn(m) => m.TenantId = old.TenantId and m.Role = 'owner' and m.Id <> old.Id) + ) + errorMessage: cannot remove the last owner of a tenant + """; + + [Fact] + public void PgTriggerGuard_PascalCaseIdentifiers_EnforcedAtRuntime() + { + // Implements [MIG-TRIGGER-GUARD-LQL]: exists() pipeline columns must + // be quoted on Postgres or mixed-case columns fold to lowercase and + // every UPDATE/DELETE fails with 42703 at trigger-fire time. + PostgresTestDb.ApplySchema( + _connection, + SchemaYamlSerializer.FromYaml(PascalCaseGuardYaml), + _logger + ); + var ownerA = Guid.NewGuid(); + var ownerB = Guid.NewGuid(); + var tenant = Guid.NewGuid(); + InsertPascalMember(ownerA, tenant, "owner"); + InsertPascalMember(ownerB, tenant, "owner"); + + // Two owners: deleting one must succeed (trigger fires cleanly). + Execute("DELETE FROM \"public\".\"TenantMembers\" WHERE \"Id\" = @i", ownerA); + + // Last owner: must be blocked by the declared guard message. + var ex = Assert.Throws(() => + Execute("DELETE FROM \"public\".\"TenantMembers\" WHERE \"Id\" = @i", ownerB) + ); + Assert.Contains("last owner", ex.Message, StringComparison.Ordinal); + } + + [Fact] + public void PgTrigger_DestructiveRerun_PreservesUnmanagedTriggerAndGuardFunction() + { + // Implements [MIG-TRIGGER-PG]: only usr_-prefixed triggers are + // migration-managed. Unmanaged triggers (e.g. Sync change tracking) + // must survive destructive runs, and the guard function must not be + // dropped out from under its trigger. + ApplyGuardSchema(); + Execute( + """ + CREATE TRIGGER zz_unmanaged_touch BEFORE UPDATE ON "public"."tenant_members" + FOR EACH ROW EXECUTE FUNCTION suppress_redundant_updates_trigger() + """ + ); + + PostgresTestDb.ApplySchema( + _connection, + SchemaYamlSerializer.FromYaml(GuardYaml), + _logger, + allowDestructive: true + ); + + using var cmd = _connection.CreateCommand(); + cmd.CommandText = "SELECT COUNT(*) FROM pg_trigger WHERE tgname = 'zz_unmanaged_touch'"; + Assert.Equal(1L, Assert.IsType(cmd.ExecuteScalar())); + + // Guard still enforced after the destructive rerun. + var owner = Guid.NewGuid(); + var tenant = Guid.NewGuid(); + InsertMember(owner, tenant, "owner"); + var ex = Assert.Throws(() => DeleteMember(owner)); + Assert.Contains("last owner", ex.Message, StringComparison.Ordinal); + } + + private void Execute(string sql, Guid? id = null) + { + using var cmd = _connection.CreateCommand(); + cmd.CommandText = sql; + if (id is { } value) + { + cmd.Parameters.AddWithValue("@i", value); + } + cmd.ExecuteNonQuery(); + } + + private void InsertPascalMember(Guid id, Guid tenantId, string role) + { + using var cmd = _connection.CreateCommand(); + cmd.CommandText = + "INSERT INTO \"public\".\"TenantMembers\"(\"Id\", \"TenantId\", \"Role\") VALUES (@i, @t, @r)"; + cmd.Parameters.AddWithValue("@i", id); + cmd.Parameters.AddWithValue("@t", tenantId); + cmd.Parameters.AddWithValue("@r", role); + cmd.ExecuteNonQuery(); + } + private void ApplyGuardSchema() => PostgresTestDb.ApplySchema(_connection, SchemaYamlSerializer.FromYaml(GuardYaml), _logger); diff --git a/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresTestDb.cs b/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresTestDb.cs index 6a63233..9a6321a 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresTestDb.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresTestDb.cs @@ -13,13 +13,14 @@ public static void ApplySchema( bool allowDestructive = false ) { - var current = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(connection, "public", logger) - ).Value; - var ops = ( - (OperationsResultOk) + var current = Assert + .IsType(PostgresSchemaInspector.Inspect(connection, "public", logger)) + .Value; + var ops = Assert + .IsType( SchemaDiff.Calculate(current, desired, allowDestructive, logger: logger) - ).Value; + ) + .Value; var apply = MigrationRunner.Apply( connection, @@ -28,9 +29,7 @@ public static void ApplySchema( allowDestructive ? MigrationOptions.Destructive : MigrationOptions.Default, logger ); - Assert.True( - apply is MigrationApplyResultOk, - $"Migration failed: {(apply as MigrationApplyResultError)?.Value}" - ); + var error = apply is MigrationApplyResultError failure ? failure.Value.Message : null; + Assert.True(apply is MigrationApplyResultOk, $"Migration failed: {error}"); } } diff --git a/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresTriggerDdlTests.cs b/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresTriggerDdlTests.cs new file mode 100644 index 0000000..7d7d1af --- /dev/null +++ b/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresTriggerDdlTests.cs @@ -0,0 +1,73 @@ +namespace Nimblesite.DataProvider.Migration.Tests; + +// Implements [MIG-TRIGGER-PG] DDL emission rules from +// docs/specs/declarative-triggers-spec.md (GitHub issue 82). + +/// +/// Generator-level tests for PostgreSQL trigger guard DDL emission rules +/// that do not need a live database. +/// +public sealed class PostgresTriggerDdlTests +{ + [Fact] + public void PgTriggerDdl_DollarTagCollision_PicksAlternateTag() + { + var ddl = PostgresDdlGenerator.Generate( + new CreateTriggerOperation( + "public", + "tenant_members", + Guard() with + { + ErrorMessage = "do not $trigger_guard$ terminate me", + } + ) + ); + + Assert.Contains("$trigger_guard1$", ddl, StringComparison.Ordinal); + Assert.Contains("do not $trigger_guard$ terminate me", ddl, StringComparison.Ordinal); + } + + [Fact] + public void PgTriggerDdl_ManagedPrefix_AppliedToTriggerObjectName() + { + var ddl = PostgresDdlGenerator.Generate( + new CreateTriggerOperation("public", "tenant_members", Guard()) + ); + + Assert.Contains( + "CREATE TRIGGER \"usr_assert_not_last_owner\"", + ddl, + StringComparison.Ordinal + ); + } + + [Fact] + public void PgTriggerDdl_IdentifierOver63Bytes_FailsLoudly() + { + // PostgreSQL silently truncates identifiers to 63 bytes, which would + // desynchronise create/drop/inspect names. + var ex = Assert.Throws(() => + PostgresDdlGenerator.Generate( + new CreateTriggerOperation( + "public", + "tenant_members", + Guard() with + { + Name = new string('x', 80), + } + ) + ) + ); + + Assert.Contains("63-byte", ex.Message, StringComparison.Ordinal); + } + + private static TriggerDefinition Guard() => + new() + { + Name = "assert_not_last_owner", + Events = [TriggerEvent.Delete], + RaiseWhenLql = "old.role = 'owner'", + ErrorMessage = "cannot remove the last owner of a tenant", + }; +} diff --git a/Migration/Nimblesite.DataProvider.Migration.Tests/SqliteDeclarativeTriggerTests.cs b/Migration/Nimblesite.DataProvider.Migration.Tests/SqliteDeclarativeTriggerTests.cs index fe6c375..f03a82b 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Tests/SqliteDeclarativeTriggerTests.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Tests/SqliteDeclarativeTriggerTests.cs @@ -207,6 +207,96 @@ public void Sqlite_TriggerRemoval_WithDestructive_DropsGuard() }); } + [Fact] + public void Sqlite_TriggerGuard_OldReferenceOnInsertEvent_FailsLoudly() + { + // Implements [MIG-TRIGGER-GUARD-LQL]: old. does not exist on INSERT. + // Without validation SQLite fails at DML time while Postgres silently + // never fires the guard. + SqliteTestDb.WithDb(connection => + { + var result = TryApplyGuardVariant( + connection, + events: "[Insert]", + raiseWhen: "old.Role = 'owner'" + ); + + var error = Assert.IsType(result); + Assert.Contains("fires on Insert", error.Value.Message, StringComparison.Ordinal); + }); + } + + [Fact] + public void Sqlite_TriggerGuard_ForEachRowFalse_FailsLoudly() + { + // Implements [MIG-TRIGGER-GUARD-LQL]: guards are row-level on every + // platform; statement-level triggers see no OLD/NEW row on Postgres. + SqliteTestDb.WithDb(connection => + { + var result = TryApplyGuardVariant( + connection, + events: "[Delete]", + raiseWhen: "old.Role = 'owner'", + extraTriggerYaml: "forEachRow: false" + ); + + var error = Assert.IsType(result); + Assert.Contains("statement-level", error.Value.Message, StringComparison.Ordinal); + }); + } + + [Fact] + public void Sqlite_TriggerGuard_ReservedTokenInPredicate_FailsLoudly() + { + // Implements [MIG-TRIGGER-GUARD-LQL]: __TRG_ is reserved for internal + // sentinels; user literals containing it would be silently rewritten. + SqliteTestDb.WithDb(connection => + { + var result = TryApplyGuardVariant( + connection, + events: "[Delete]", + raiseWhen: "old.Role = '__TRG_OLD_Role__'" + ); + + var error = Assert.IsType(result); + Assert.Contains("reserved token", error.Value.Message, StringComparison.Ordinal); + }); + } + + private static MigrationApplyResult TryApplyGuardVariant( + SqliteConnection connection, + string events, + string raiseWhen, + string? extraTriggerYaml = null + ) + { + var extra = extraTriggerYaml is null ? string.Empty : $"\n {extraTriggerYaml}"; + var yaml = $""" + name: tenant_app + tables: + - name: TenantMembers + schema: main + columns: + - name: Id + type: Text + isNullable: false + - name: TenantId + type: Text + isNullable: false + - name: Role + type: Text + isNullable: false + primaryKey: + columns: + - Id + triggers: + - name: guard_variant + events: {events} + raiseWhen: "{raiseWhen}"{extra} + """; + return SqliteTestDb.TryApplySchema(connection, SchemaYamlSerializer.FromYaml(yaml)); + } + private static void ApplyGuardSchema(SqliteConnection connection) => SqliteTestDb.ApplySchema(connection, SchemaYamlSerializer.FromYaml(GuardYaml)); diff --git a/Migration/Nimblesite.DataProvider.Migration.Tests/SqliteTestDb.cs b/Migration/Nimblesite.DataProvider.Migration.Tests/SqliteTestDb.cs index 7c3e83c..13529b6 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Tests/SqliteTestDb.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Tests/SqliteTestDb.cs @@ -29,20 +29,42 @@ public static void WithDb(Action test) } public static SchemaDefinition Inspect(SqliteConnection connection) => - ((SchemaResultOk)SqliteSchemaInspector.Inspect(connection, Logger)).Value; + Assert.IsType(SqliteSchemaInspector.Inspect(connection, Logger)).Value; public static void ApplySchema( SqliteConnection connection, SchemaDefinition schema, bool allowDestructive = false ) + { + var result = TryApplySchema(connection, schema, allowDestructive); + var error = result is MigrationApplyResultError failure ? failure.Value.Message : null; + Assert.True(result is MigrationApplyResultOk, $"Migration failed: {error}"); + } + + /// + /// Inspect → diff → apply without asserting success, for tests that + /// expect the migration to fail loudly. + /// + public static MigrationApplyResult TryApplySchema( + SqliteConnection connection, + SchemaDefinition schema, + bool allowDestructive = false + ) { var current = Inspect(connection); - var ops = ( - (OperationsResultOk) + var ops = Assert + .IsType( SchemaDiff.Calculate(current, schema, allowDestructive, logger: Logger) - ).Value; - Apply(connection, ops, allowDestructive ? MigrationOptions.Destructive : null); + ) + .Value; + return MigrationRunner.Apply( + connection, + ops, + SqliteDdlGenerator.Generate, + allowDestructive ? MigrationOptions.Destructive : MigrationOptions.Default, + Logger + ); } public static void Apply( @@ -58,10 +80,8 @@ public static void Apply( options ?? MigrationOptions.Default, Logger ); - Assert.True( - result is MigrationApplyResultOk, - $"Migration failed: {(result as MigrationApplyResultError)?.Value}" - ); + var error = result is MigrationApplyResultError failure ? failure.Value.Message : null; + Assert.True(result is MigrationApplyResultOk, $"Migration failed: {error}"); } public static void SetUser(SqliteConnection connection, string userId) diff --git a/docs/specs/declarative-triggers-spec.md b/docs/specs/declarative-triggers-spec.md index e8f3dca..b7bf738 100644 --- a/docs/specs/declarative-triggers-spec.md +++ b/docs/specs/declarative-triggers-spec.md @@ -16,7 +16,7 @@ tables: - name: assert_not_last_owner timing: Before # Before (default) or After events: [Update, Delete] # Insert | Update | Delete - forEachRow: true # default true + forEachRow: true # default true; false is rejected (guards are row-level) raiseWhen: | # LQL guard predicate; row refs via old./new. old.role = 'owner' and not exists( tenant_members @@ -52,6 +52,20 @@ database. Row column references use `old.` / `new.` composition, and `exists(...)` / `not exists(...)` pipeline subqueries. Transpiles identically (same semantics) on every supported platform. +Validation (all fail the migration loudly rather than diverging at runtime): + +- `old.` references require every event to be Update/Delete; `new.` + references require Insert/Update (OLD does not exist on INSERT, NEW does + not exist on DELETE). +- `forEachRow: false` is rejected -- guards are row-level on every platform + (SQLite has no statement triggers; Postgres statement triggers see NULL + OLD/NEW and would silently never fire). +- The token `__TRG_` is reserved for internal sentinels; predicates + containing it are rejected so user literals can never be rewritten. + +On Postgres, transpiled `exists()` pipeline SQL gets an identifier-quoting +pass so mixed-case column names are not case-folded. + ## [MIG-TRIGGER-SQLITE] SQLite emission and inspection One trigger per event named `usr_{event}_{trigger}_{table}`: @@ -63,7 +77,14 @@ trigger name, `rls_` triggers excluded) so re-diff is a no-op. ## [MIG-TRIGGER-PG] PostgreSQL emission and inspection One plpgsql function `{table}_{trigger}_trgfn` raising `errorMessage` when the -predicate holds, plus one trigger `{trigger}` (`BEFORE {events} ... FOR EACH -ROW EXECUTE FUNCTION`). Drop removes trigger and function. Inspector reads -triggers back from `information_schema.triggers` grouped by trigger name so -re-diff is a no-op. +predicate holds (`RAISE ... USING MESSAGE` avoids `%` format interpretation; +the dollar-quote tag is chosen to never occur in the body), plus one trigger +`usr_{trigger}` (`BEFORE {events} ... FOR EACH ROW EXECUTE FUNCTION`). Drop +removes trigger and function. Identifiers over PostgreSQL's 63-byte limit are +rejected loudly (silent truncation would desynchronise create/drop/inspect). + +Inspector reads only `usr_`-prefixed triggers back from +`information_schema.triggers` grouped by trigger name (prefix stripped) so +re-diff is a no-op and triggers created outside the migration tool are never +dropped by destructive runs. `*_trgfn` guard functions are excluded from +support-function read-back because they are owned by the trigger lifecycle. From 24966f8931553fb2671cf71529bde47f69a45cd5 Mon Sep 17 00:00:00 2001 From: Christian Findlay <16697547+MelbourneDeveloper@users.noreply.github.com> Date: Thu, 2 Jul 2026 22:03:37 +1000 Subject: [PATCH 3/5] fix: VS Code test host user-data-dir + ratchet Migration coverage to 87% The default user-data-dir under the repo produced an IPC socket path over the macOS 103-char AF_UNIX limit (listen EINVAL), failing make ci locally. The test host now uses a short dir under the OS temp folder. The trigger guard tests raised Migration.Core coverage, ratcheting its threshold 86->87. Co-Authored-By: Claude Fable 5 --- Lql/LqlExtension/src/test/runTest.ts | 8 +++++++- coverage-thresholds.json | 2 +- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/Lql/LqlExtension/src/test/runTest.ts b/Lql/LqlExtension/src/test/runTest.ts index 161eab1..698df4a 100644 --- a/Lql/LqlExtension/src/test/runTest.ts +++ b/Lql/LqlExtension/src/test/runTest.ts @@ -1,3 +1,5 @@ +import * as fs from "fs"; +import * as os from "os"; import * as path from "path"; import { runTests } from "@vscode/test-electron"; @@ -5,11 +7,15 @@ async function main() { try { const extensionDevelopmentPath = path.resolve(__dirname, "../../"); const extensionTestsPath = path.resolve(__dirname, "./suite/index"); + // The default user-data-dir lives under the repo; on deep checkouts its + // IPC socket path exceeds the macOS 103-char AF_UNIX limit (EINVAL). + const userDataDir = path.join(os.tmpdir(), "lql-ext-test"); + fs.mkdirSync(userDataDir, { recursive: true }); await runTests({ extensionDevelopmentPath, extensionTestsPath, - launchArgs: ["--disable-extensions"], + launchArgs: ["--disable-extensions", `--user-data-dir=${userDataDir}`], }); } catch (err: unknown) { const message = err instanceof Error ? err.message : String(err); diff --git a/coverage-thresholds.json b/coverage-thresholds.json index 7289367..feada56 100644 --- a/coverage-thresholds.json +++ b/coverage-thresholds.json @@ -18,7 +18,7 @@ "include": "[Nimblesite.Lql.TypeProvider.FSharp]*,[Nimblesite.Lql.Core]*,[Nimblesite.Lql.SQLite]*,[Nimblesite.Sql.Model]*" }, "Migration/Nimblesite.DataProvider.Migration.Core": { - "threshold": 86, + "threshold": 87, "include": "[Nimblesite.DataProvider.Migration.Core]*,[Nimblesite.DataProvider.Migration.SQLite]*,[Nimblesite.DataProvider.Migration.Postgres]*" }, "Sync/Nimblesite.Sync.Core": { From d0a49a42e04dffe6e862f5c4a066d946d9c80c02 Mon Sep 17 00:00:00 2001 From: Christian Findlay <16697547+MelbourneDeveloper@users.noreply.github.com> Date: Sat, 4 Jul 2026 11:51:13 +1000 Subject: [PATCH 4/5] fixes --- .vscode/settings.json | 3 +- DataProvider/DataProvider/PostgresCli.cs | 139 ++++----------- DataProvider/DataProvider/SqliteProgram.cs | 86 +++++----- .../Nimblesite.DataProvider.SQLite.csproj | 1 + .../SqliteCodeGenerator.cs | 36 +--- .../SqliteTypeMapper.cs | 41 +++++ .../LqlStatementValidator.cs | 24 +++ .../SqlStatementExtensionsPostgreSQL.cs | 11 +- .../SqlStatementExtensionsSQLite.cs | 11 +- .../SqlStatementExtensionsSqlServer.cs | 11 +- .../crates/lql-analyzer/src/scope.rs | 55 +++--- Lql/lql-lsp-rust/crates/lql-lsp/src/main.rs | 42 ++--- .../PostgresSyncClientRepository.cs | 24 ++- .../ChangeApplierSQLite.cs | 34 ++-- .../SubscriptionRepository.cs | 161 ++++++++---------- coverage-thresholds.json | 2 +- 16 files changed, 297 insertions(+), 384 deletions(-) create mode 100644 DataProvider/Nimblesite.DataProvider.SQLite/SqliteTypeMapper.cs create mode 100644 Lql/Nimblesite.Lql.Core/LqlStatementValidator.cs diff --git a/.vscode/settings.json b/.vscode/settings.json index 26577f3..3c08f12 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -15,5 +15,6 @@ "titleBar.activeForeground": "#F4F7FB", "titleBar.inactiveBackground": "#070D1F", "titleBar.inactiveForeground": "#F4F7FBcc" - } + }, + "deslop.topOffenders.splitByLanguage": true } \ No newline at end of file diff --git a/DataProvider/DataProvider/PostgresCli.cs b/DataProvider/DataProvider/PostgresCli.cs index 94468f8..edc59dd 100644 --- a/DataProvider/DataProvider/PostgresCli.cs +++ b/DataProvider/DataProvider/PostgresCli.cs @@ -595,81 +595,8 @@ string outDir ); } - if (columns.Count == 0) - { - return new Result.Error( - new SqlError($"No columns found for table {table.Schema}.{table.Name}") - ); - } - - var sb = new StringBuilder(); - // Use the literal table name (e.g. `gk_user`) instead of - // PascalCasing it. The SQLite.Cli emits methods like - // `Insertgk_userAsync`, and consumers reference them by that - // exact name. PascalCasing here breaks consumer call sites. - var pascalName = table.Name; - - // Header - _ = sb.AppendLine("// "); - _ = sb.AppendLine("#nullable enable"); - _ = sb.AppendLine(); - _ = sb.AppendLine("using Npgsql;"); - _ = sb.AppendLine("using Outcome;"); - _ = sb.AppendLine("using Nimblesite.Sql.Model;"); - _ = sb.AppendLine(); - _ = sb.AppendLine("namespace Generated;"); - _ = sb.AppendLine(); - - // Extension class - _ = sb.AppendLine("/// "); - _ = sb.AppendLine( - CultureInfo.InvariantCulture, - $"/// Generated CRUD operations for {table.Name} table." - ); - _ = sb.AppendLine("/// "); - _ = sb.AppendLine( - CultureInfo.InvariantCulture, - $"public static class {pascalName}Extensions" - ); - _ = sb.AppendLine("{"); - - // Generate INSERT method - if (table.GenerateInsert) - { - GenerateInsertMethod(sb, table, columns, pascalName); - } - - // Generate UPDATE method - if (table.GenerateUpdate) - { - GenerateUpdateMethod(sb, table, columns, pascalName); - } - - // Generate DELETE method - if (table.GenerateDelete) - { - GenerateDeleteMethod(sb, table, columns, pascalName); - } - - // Generate bulk INSERT method - if (table.GenerateBulkInsert) - { - GenerateBulkInsertMethod(sb, table, columns, pascalName); - } - - // Generate bulk UPSERT method - if (table.GenerateBulkUpsert) - { - GenerateBulkUpsertMethod(sb, table, columns, pascalName); - } - - _ = sb.AppendLine("}"); - - var target = Path.Combine(outDir, $"{pascalName}Operations.g.cs"); - await File.WriteAllTextAsync(target, sb.ToString()).ConfigureAwait(false); - Console.WriteLine($"✅ Generated {target}"); - - return new Result.Ok(sb.ToString()); + return await AssembleAndWriteTableOperationsAsync(table, columns, outDir) + .ConfigureAwait(false); } private static async Task> GenerateTableOperationsAsync( @@ -736,6 +663,16 @@ ORDER BY c.ordinal_position } } + return await AssembleAndWriteTableOperationsAsync(table, columns, outDir) + .ConfigureAwait(false); + } + + private static async Task> AssembleAndWriteTableOperationsAsync( + TableConfigItem table, + List columns, + string outDir + ) + { if (columns.Count == 0) { return new Result.Error( @@ -750,6 +687,23 @@ ORDER BY c.ordinal_position // exact name. PascalCasing here breaks consumer call sites. var pascalName = table.Name; + AppendExtensionsHeader(sb, table, pascalName); + AppendCrudMethods(sb, table, columns, pascalName); + _ = sb.AppendLine("}"); + + var target = Path.Combine(outDir, $"{pascalName}Operations.g.cs"); + await File.WriteAllTextAsync(target, sb.ToString()).ConfigureAwait(false); + Console.WriteLine($"✅ Generated {target}"); + + return new Result.Ok(sb.ToString()); + } + + private static void AppendExtensionsHeader( + StringBuilder sb, + TableConfigItem table, + string pascalName + ) + { // Header _ = sb.AppendLine("// "); _ = sb.AppendLine("#nullable enable"); @@ -773,7 +727,15 @@ ORDER BY c.ordinal_position $"public static class {pascalName}Extensions" ); _ = sb.AppendLine("{"); + } + private static void AppendCrudMethods( + StringBuilder sb, + TableConfigItem table, + List columns, + string pascalName + ) + { // Generate INSERT method if (table.GenerateInsert) { @@ -803,14 +765,6 @@ ORDER BY c.ordinal_position { GenerateBulkUpsertMethod(sb, table, columns, pascalName); } - - _ = sb.AppendLine("}"); - - var target = Path.Combine(outDir, $"{pascalName}Operations.g.cs"); - await File.WriteAllTextAsync(target, sb.ToString()).ConfigureAwait(false); - Console.WriteLine($"✅ Generated {target}"); - - return new Result.Ok(sb.ToString()); } private static void GenerateInsertMethod( @@ -2302,25 +2256,6 @@ private static string ToPascalCase(string name) return sb.ToString(); } - private static string ToCamelCase(string name) - { - if (string.IsNullOrEmpty(name)) - return name; - - // Preserve existing camel/Pascal input, just lowercase the first - // letter. For snake_case input, fall through to PascalCase then - // lowercase the leading letter. - if (!name.Contains('_', StringComparison.Ordinal)) - { - return char.ToLowerInvariant(name[0]) + name[1..]; - } - - var pascal = ToPascalCase(name); - if (string.IsNullOrEmpty(pascal)) - return pascal; - return char.ToLowerInvariant(pascal[0]) + pascal[1..]; - } - private static string EscapeForPreprocessor(string message) { if (string.IsNullOrEmpty(message)) diff --git a/DataProvider/DataProvider/SqliteProgram.cs b/DataProvider/DataProvider/SqliteProgram.cs index 656fc2d..e6b8147 100644 --- a/DataProvider/DataProvider/SqliteProgram.cs +++ b/DataProvider/DataProvider/SqliteProgram.cs @@ -265,10 +265,14 @@ as Result, SqlError>.Error< // Also emit an MSBuild-formatted error so IDE Problem Matchers pick it up Console.Error.WriteLine($"{sqlPath}(1,1): error DP0001: {prettyMeta}"); // Emit a compile-time error file so MSBuild surfaces the exact problem - var errorFile = Path.Combine(outDir.FullName, baseName + ".g.cs"); - var content = - $"// Auto-generated due to SQL error in {sqlPath}\n#error {EscapeForPreprocessor(prettyMeta)}\n"; - await File.WriteAllTextAsync(errorFile, content).ConfigureAwait(false); + await WriteCompileTimeErrorFileAsync( + outDir, + baseName, + sqlPath, + reason: "SQL error", + message: prettyMeta + ) + .ConfigureAwait(false); hadErrors = true; continue; } @@ -304,10 +308,14 @@ as Result, SqlError>.Error< // Also emit an MSBuild-formatted error so IDE Problem Matchers pick it up Console.Error.WriteLine($"{sqlPath}(1,1): error DL0002: {prettyGen}"); // Emit a compile-time error file so MSBuild surfaces the exact problem - var errorFile = Path.Combine(outDir.FullName, baseName + ".g.cs"); - var content = - $"// Auto-generated due to SQL generation error in {sqlPath}\n#error {EscapeForPreprocessor(prettyGen)}\n"; - await File.WriteAllTextAsync(errorFile, content).ConfigureAwait(false); + await WriteCompileTimeErrorFileAsync( + outDir, + baseName, + sqlPath, + reason: "SQL generation error", + message: prettyGen + ) + .ConfigureAwait(false); hadErrors = true; } } @@ -320,11 +328,15 @@ as Result, SqlError>.Error< { baseName = baseName[..^".generated".Length]; } - var errorFile = Path.Combine(outDir.FullName, baseName + ".g.cs"); - var content = - $"// Auto-generated due to unexpected error in {sqlPath}\n#error {EscapeForPreprocessor(ex.Message)}\n"; Console.Error.WriteLine($"{sqlPath}(1,1): error DL0003: {ex.Message}"); - await File.WriteAllTextAsync(errorFile, content).ConfigureAwait(false); + await WriteCompileTimeErrorFileAsync( + outDir, + baseName, + sqlPath, + reason: "unexpected error", + message: ex.Message + ) + .ConfigureAwait(false); hadErrors = true; } } @@ -361,7 +373,10 @@ as Result, SqlError>.Error< var notNull = reader.GetInt32(3) == 1; // notnull column var isPrimaryKey = reader.GetInt32(5) > 0; // pk column - var csharpType = MapSqliteTypeToCSharpType(sqliteType, !notNull); + var csharpType = SqliteTypeMapper.MapSqliteTypeToCSharpType( + sqliteType, + !notNull + ); columns.Add( new DatabaseColumn @@ -464,6 +479,20 @@ is Result.Error operationsFailure } } + private static async Task WriteCompileTimeErrorFileAsync( + DirectoryInfo outDir, + string baseName, + string sqlPath, + string reason, + string message + ) + { + var errorFile = Path.Combine(outDir.FullName, baseName + ".g.cs"); + var content = + $"// Auto-generated due to {reason} in {sqlPath}\n#error {EscapeForPreprocessor(message)}\n"; + await File.WriteAllTextAsync(errorFile, content).ConfigureAwait(false); + } + /// /// Generates a source file with the correct using statement for the specified connection type. /// @@ -615,35 +644,4 @@ private static string MakeConnectionStringAbsolute(string connectionString, stri var suffix = semicolonIdx >= 0 ? connectionString[semicolonIdx..] : string.Empty; return $"{prefix}{dataSourcePrefix}{absolutePath}{suffix}"; } - - /// - /// Maps SQLite types to C# types - /// - private static string MapSqliteTypeToCSharpType(string sqliteType, bool isNullable) - { - var baseType = sqliteType.ToUpperInvariant() switch - { - var t when t.Contains("INT", StringComparison.OrdinalIgnoreCase) => "long", - var t - when t.Contains("REAL", StringComparison.OrdinalIgnoreCase) - || t.Contains("FLOAT", StringComparison.OrdinalIgnoreCase) - || t.Contains("DOUBLE", StringComparison.OrdinalIgnoreCase) => "double", - var t - when t.Contains("DECIMAL", StringComparison.OrdinalIgnoreCase) - || t.Contains("NUMERIC", StringComparison.OrdinalIgnoreCase) => "double", - var t when t.Contains("BOOL", StringComparison.OrdinalIgnoreCase) => "bool", - var t - when t.Contains("DATE", StringComparison.OrdinalIgnoreCase) - || t.Contains("TIME", StringComparison.OrdinalIgnoreCase) => "string", // SQLite stores dates as text - var t when t.Contains("BLOB", StringComparison.OrdinalIgnoreCase) => "byte[]", - _ => "string", - }; - - if (isNullable && baseType != "string" && baseType != "byte[]") - { - return baseType + "?"; - } - - return baseType; - } } diff --git a/DataProvider/Nimblesite.DataProvider.SQLite/Nimblesite.DataProvider.SQLite.csproj b/DataProvider/Nimblesite.DataProvider.SQLite/Nimblesite.DataProvider.SQLite.csproj index 3ef4e67..6f2105c 100644 --- a/DataProvider/Nimblesite.DataProvider.SQLite/Nimblesite.DataProvider.SQLite.csproj +++ b/DataProvider/Nimblesite.DataProvider.SQLite/Nimblesite.DataProvider.SQLite.csproj @@ -54,6 +54,7 @@ SqlParserContractTests which need to walk the raw parse tree. --> + diff --git a/DataProvider/Nimblesite.DataProvider.SQLite/SqliteCodeGenerator.cs b/DataProvider/Nimblesite.DataProvider.SQLite/SqliteCodeGenerator.cs index 86c0895..e7babb4 100644 --- a/DataProvider/Nimblesite.DataProvider.SQLite/SqliteCodeGenerator.cs +++ b/DataProvider/Nimblesite.DataProvider.SQLite/SqliteCodeGenerator.cs @@ -140,7 +140,10 @@ string tableName var notNull = reader.GetInt32(3) == 1; // notnull column var isPrimaryKey = reader.GetInt32(5) > 0; // pk column - var csharpType = MapSqliteTypeToCSharpType(sqliteType, !notNull); + var csharpType = SqliteTypeMapper.MapSqliteTypeToCSharpType( + sqliteType, + !notNull + ); columns.Add( new DatabaseColumn @@ -186,37 +189,6 @@ string tableName } } - /// - /// Maps SQLite types to C# types - /// - private static string MapSqliteTypeToCSharpType(string sqliteType, bool isNullable) - { - var baseType = sqliteType.ToUpperInvariant() switch - { - var t when t.Contains("INT", StringComparison.OrdinalIgnoreCase) => "long", - var t - when t.Contains("REAL", StringComparison.OrdinalIgnoreCase) - || t.Contains("FLOAT", StringComparison.OrdinalIgnoreCase) - || t.Contains("DOUBLE", StringComparison.OrdinalIgnoreCase) => "double", - var t - when t.Contains("DECIMAL", StringComparison.OrdinalIgnoreCase) - || t.Contains("NUMERIC", StringComparison.OrdinalIgnoreCase) => "double", - var t when t.Contains("BOOL", StringComparison.OrdinalIgnoreCase) => "bool", - var t - when t.Contains("DATE", StringComparison.OrdinalIgnoreCase) - || t.Contains("TIME", StringComparison.OrdinalIgnoreCase) => "string", // SQLite stores dates as text - var t when t.Contains("BLOB", StringComparison.OrdinalIgnoreCase) => "byte[]", - _ => "string", - }; - - if (isNullable && baseType != "string" && baseType != "byte[]") - { - return baseType + "?"; - } - - return baseType; - } - // ============================= // Incremental generator wiring // ============================= diff --git a/DataProvider/Nimblesite.DataProvider.SQLite/SqliteTypeMapper.cs b/DataProvider/Nimblesite.DataProvider.SQLite/SqliteTypeMapper.cs new file mode 100644 index 0000000..1180dbe --- /dev/null +++ b/DataProvider/Nimblesite.DataProvider.SQLite/SqliteTypeMapper.cs @@ -0,0 +1,41 @@ +namespace Nimblesite.DataProvider.SQLite; + +/// +/// Maps SQLite declared column types to C# type names. +/// +internal static class SqliteTypeMapper +{ + /// + /// Maps a SQLite declared type to a C# type name, appending "?" when nullable. + /// + /// The SQLite declared column type. + /// Whether the column is nullable. + /// The mapped C# type name. + internal static string MapSqliteTypeToCSharpType(string sqliteType, bool isNullable) + { + var baseType = sqliteType.ToUpperInvariant() switch + { + var t when t.Contains("INT", StringComparison.OrdinalIgnoreCase) => "long", + var t + when t.Contains("REAL", StringComparison.OrdinalIgnoreCase) + || t.Contains("FLOAT", StringComparison.OrdinalIgnoreCase) + || t.Contains("DOUBLE", StringComparison.OrdinalIgnoreCase) => "double", + var t + when t.Contains("DECIMAL", StringComparison.OrdinalIgnoreCase) + || t.Contains("NUMERIC", StringComparison.OrdinalIgnoreCase) => "double", + var t when t.Contains("BOOL", StringComparison.OrdinalIgnoreCase) => "bool", + var t + when t.Contains("DATE", StringComparison.OrdinalIgnoreCase) + || t.Contains("TIME", StringComparison.OrdinalIgnoreCase) => "string", // SQLite stores dates as text + var t when t.Contains("BLOB", StringComparison.OrdinalIgnoreCase) => "byte[]", + _ => "string", + }; + + if (isNullable && baseType != "string" && baseType != "byte[]") + { + return baseType + "?"; + } + + return baseType; + } +} diff --git a/Lql/Nimblesite.Lql.Core/LqlStatementValidator.cs b/Lql/Nimblesite.Lql.Core/LqlStatementValidator.cs new file mode 100644 index 0000000..0452862 --- /dev/null +++ b/Lql/Nimblesite.Lql.Core/LqlStatementValidator.cs @@ -0,0 +1,24 @@ +using Nimblesite.Sql.Model; + +namespace Nimblesite.Lql.Core; + +/// +/// Validates an before dialect conversion. +/// Shared by all platform-specific To*Sql extension methods to avoid duplicating +/// the parse-error and missing-AST guards. +/// +public static class LqlStatementValidator +{ + /// + /// Returns the reason the statement cannot be converted, or null when it is valid. + /// + /// The statement to validate. + /// + /// The parse error when present, a "no AST node" error when the statement has no AST node, + /// or null when the statement has a usable AST node. + /// + public static SqlError? Validate(LqlStatement statement) => + statement.ParseError is SqlError parseError ? parseError + : statement.AstNode is null ? new SqlError("No AST node found in statement") + : null; +} diff --git a/Lql/Nimblesite.Lql.Postgres/SqlStatementExtensionsPostgreSQL.cs b/Lql/Nimblesite.Lql.Postgres/SqlStatementExtensionsPostgreSQL.cs index f146a17..33f5ce1 100644 --- a/Lql/Nimblesite.Lql.Postgres/SqlStatementExtensionsPostgreSQL.cs +++ b/Lql/Nimblesite.Lql.Postgres/SqlStatementExtensionsPostgreSQL.cs @@ -17,16 +17,9 @@ public static Result ToPostgreSql(this LqlStatement statement) { ArgumentNullException.ThrowIfNull(statement); - if (statement.ParseError != null) + if (LqlStatementValidator.Validate(statement) is SqlError error) { - return new Result.Error(statement.ParseError); - } - - if (statement.AstNode == null) - { - return new Result.Error( - new SqlError("No AST node found in statement") - ); + return new Result.Error(error); } try diff --git a/Lql/Nimblesite.Lql.SQLite/SqlStatementExtensionsSQLite.cs b/Lql/Nimblesite.Lql.SQLite/SqlStatementExtensionsSQLite.cs index ba2fe08..9037afa 100644 --- a/Lql/Nimblesite.Lql.SQLite/SqlStatementExtensionsSQLite.cs +++ b/Lql/Nimblesite.Lql.SQLite/SqlStatementExtensionsSQLite.cs @@ -18,16 +18,9 @@ public static Result ToSQLite(this LqlStatement statement) { ArgumentNullException.ThrowIfNull(statement); - if (statement.ParseError != null) + if (LqlStatementValidator.Validate(statement) is SqlError error) { - return new Result.Error(statement.ParseError); - } - - if (statement.AstNode == null) - { - return new Result.Error( - new SqlError("No AST node found in statement") - ); + return new Result.Error(error); } try diff --git a/Lql/Nimblesite.Lql.SqlServer/SqlStatementExtensionsSqlServer.cs b/Lql/Nimblesite.Lql.SqlServer/SqlStatementExtensionsSqlServer.cs index ca5cb2e..988d70e 100644 --- a/Lql/Nimblesite.Lql.SqlServer/SqlStatementExtensionsSqlServer.cs +++ b/Lql/Nimblesite.Lql.SqlServer/SqlStatementExtensionsSqlServer.cs @@ -17,16 +17,9 @@ public static Result ToSqlServer(this LqlStatement statement) { ArgumentNullException.ThrowIfNull(statement); - if (statement.ParseError != null) + if (LqlStatementValidator.Validate(statement) is SqlError error) { - return new Result.Error(statement.ParseError); - } - - if (statement.AstNode == null) - { - return new Result.Error( - new SqlError("No AST node found in statement") - ); + return new Result.Error(error); } try diff --git a/Lql/lql-lsp-rust/crates/lql-analyzer/src/scope.rs b/Lql/lql-lsp-rust/crates/lql-analyzer/src/scope.rs index b425b6f..ff67686 100644 --- a/Lql/lql-lsp-rust/crates/lql-analyzer/src/scope.rs +++ b/Lql/lql-lsp-rust/crates/lql-analyzer/src/scope.rs @@ -1,7 +1,8 @@ -use antlr_rust::tree::ParseTree; +use antlr_rust::tree::{ParseTree, TerminalNode}; use lql_parser::{ parse_lql, ArgContextAttrs, ArgListContextAttrs, ExprContextAttrs, FunctionCallContextAttrs, - LetStmtContextAttrs, PipeExprContextAttrs, ProgramContextAttrs, StatementContextAttrs, + LetStmtContextAttrs, LqlParserContextType, PipeExprContextAttrs, ProgramContextAttrs, + StatementContextAttrs, }; use std::collections::HashMap; @@ -174,6 +175,23 @@ fn collect_fn_calls_from_pipe<'input>( } } +/// Build a `FunctionCallInfo` from an IDENT terminal node and append it to `calls`. +fn push_fn_call<'input>( + ident: &TerminalNode<'input, LqlParserContextType>, + calls: &mut Vec, +) { + let name = ident.symbol.text.to_string(); + let line = (ident.symbol.line - 1) as u32; + let col = ident.symbol.column as u32; + let end_col = col + name.len() as u32; + calls.push(FunctionCallInfo { + name, + line, + col, + end_col, + }); +} + /// Collect function calls from an expr node. fn collect_fn_calls_from_expr<'input>( expr: &lql_parser::ExprContext<'input>, @@ -182,16 +200,7 @@ fn collect_fn_calls_from_expr<'input>( // expr with IDENT + argList is a function call form if let Some(ident) = expr.IDENT() { if expr.argList().is_some() { - let name = ident.symbol.text.to_string(); - let line = (ident.symbol.line - 1) as u32; - let col = ident.symbol.column as u32; - let end_col = col + name.len() as u32; - calls.push(FunctionCallInfo { - name, - line, - col, - end_col, - }); + push_fn_call(&ident, calls); } } @@ -215,16 +224,7 @@ fn collect_fn_calls_from_arg_list<'input>( // Direct functionCall rule in arg if let Some(fc) = arg.functionCall() { if let Some(ident) = fc.IDENT() { - let name = ident.symbol.text.to_string(); - let line = (ident.symbol.line - 1) as u32; - let col = ident.symbol.column as u32; - let end_col = col + name.len() as u32; - calls.push(FunctionCallInfo { - name, - line, - col, - end_col, - }); + push_fn_call(&ident, calls); } // Recurse into functionCall's own argList if let Some(inner_args) = fc.argList() { @@ -237,16 +237,7 @@ fn collect_fn_calls_from_arg_list<'input>( use lql_parser::ColumnAliasContextAttrs; if let Some(fc) = col_alias.functionCall() { if let Some(ident) = fc.IDENT() { - let name = ident.symbol.text.to_string(); - let line = (ident.symbol.line - 1) as u32; - let col = ident.symbol.column as u32; - let end_col = col + name.len() as u32; - calls.push(FunctionCallInfo { - name, - line, - col, - end_col, - }); + push_fn_call(&ident, calls); } if let Some(inner_args) = fc.argList() { collect_fn_calls_from_arg_list(&inner_args, calls); diff --git a/Lql/lql-lsp-rust/crates/lql-lsp/src/main.rs b/Lql/lql-lsp-rust/crates/lql-lsp/src/main.rs index 15d97d8..1077f49 100644 --- a/Lql/lql-lsp-rust/crates/lql-lsp/src/main.rs +++ b/Lql/lql-lsp-rust/crates/lql-lsp/src/main.rs @@ -138,6 +138,12 @@ impl LqlBackend { (Some(word), None) } + /// Returns a clone of the current source text for `uri`, or `None` if the + /// document is not open. Centralizes the documents-map lock and lookup. + fn document_source(&self, uri: &Url) -> Option { + self.documents.lock().unwrap().get(uri).cloned() + } + fn compute_completion_context(source: &str, position: Position) -> CompletionContext { let line = source.lines().nth(position.line as usize).unwrap_or(""); let col = (position.character as usize).min(line.len()); @@ -340,12 +346,9 @@ impl LanguageServer for LqlBackend { let uri = ¶ms.text_document_position.text_document.uri; let position = params.text_document_position.position; - let source = { - let docs = self.documents.lock().unwrap(); - match docs.get(uri) { - Some(s) => s.clone(), - None => return Ok(None), - } + let source = match self.document_source(uri) { + Some(s) => s, + None => return Ok(None), }; let scope = build_scope(&source); @@ -449,12 +452,9 @@ impl LanguageServer for LqlBackend { let uri = ¶ms.text_document_position_params.text_document.uri; let position = params.text_document_position_params.position; - let source = { - let docs = self.documents.lock().unwrap(); - match docs.get(uri) { - Some(s) => s.clone(), - None => return Ok(None), - } + let source = match self.document_source(uri) { + Some(s) => s, + None => return Ok(None), }; let (word, qualified) = Self::get_qualified_at_position(&source, position); @@ -489,12 +489,9 @@ impl LanguageServer for LqlBackend { ) -> Result> { let uri = ¶ms.text_document.uri; - let source = { - let docs = self.documents.lock().unwrap(); - match docs.get(uri) { - Some(s) => s.clone(), - None => return Ok(None), - } + let source = match self.document_source(uri) { + Some(s) => s, + None => return Ok(None), }; let symbols = extract_symbols(&source); @@ -530,12 +527,9 @@ impl LanguageServer for LqlBackend { async fn formatting(&self, params: DocumentFormattingParams) -> Result>> { let uri = ¶ms.text_document.uri; - let source = { - let docs = self.documents.lock().unwrap(); - match docs.get(uri) { - Some(s) => s.clone(), - None => return Ok(None), - } + let source = match self.document_source(uri) { + Some(s) => s, + None => return Ok(None), }; let formatted = format_lql(&source); diff --git a/Sync/Nimblesite.Sync.Postgres/PostgresSyncClientRepository.cs b/Sync/Nimblesite.Sync.Postgres/PostgresSyncClientRepository.cs index 8b1d738..cc5f24f 100644 --- a/Sync/Nimblesite.Sync.Postgres/PostgresSyncClientRepository.cs +++ b/Sync/Nimblesite.Sync.Postgres/PostgresSyncClientRepository.cs @@ -27,14 +27,7 @@ ORDER BY last_sync_version ASC while (reader.Read()) { - clients.Add( - new SyncClient( - OriginId: reader.GetString(0), - LastSyncVersion: reader.GetInt64(1), - LastSyncTimestamp: reader.GetString(2), - CreatedAt: reader.GetString(3) - ) - ); + clients.Add(MapClient(reader)); } return new SyncClientListOk(clients); @@ -72,12 +65,7 @@ FROM _sync_clients return new SyncClientOk(null); } - var client = new SyncClient( - OriginId: reader.GetString(0), - LastSyncVersion: reader.GetInt64(1), - LastSyncTimestamp: reader.GetString(2), - CreatedAt: reader.GetString(3) - ); + var client = MapClient(reader); return new SyncClientOk(client); } @@ -171,4 +159,12 @@ public static LongSyncResult GetMinVersion(NpgsqlConnection connection) ); } } + + private static SyncClient MapClient(NpgsqlDataReader reader) => + new( + OriginId: reader.GetString(0), + LastSyncVersion: reader.GetInt64(1), + LastSyncTimestamp: reader.GetString(2), + CreatedAt: reader.GetString(3) + ); } diff --git a/Sync/Nimblesite.Sync.SQLite/ChangeApplierSQLite.cs b/Sync/Nimblesite.Sync.SQLite/ChangeApplierSQLite.cs index 315df80..a17040f 100644 --- a/Sync/Nimblesite.Sync.SQLite/ChangeApplierSQLite.cs +++ b/Sync/Nimblesite.Sync.SQLite/ChangeApplierSQLite.cs @@ -44,11 +44,6 @@ public static BoolSyncResult ApplyChange(SqliteConnection connection, SyncLogEnt } } - [SuppressMessage( - "Security", - "CA2100:Review SQL queries for security vulnerabilities", - Justification = "Table names come from internal sync log, not user input" - )] private static BoolSyncResult ApplyInsert(SqliteConnection connection, SyncLogEntry entry) { if (string.IsNullOrEmpty(entry.Payload)) @@ -62,6 +57,20 @@ private static BoolSyncResult ApplyInsert(SqliteConnection connection, SyncLogEn return new BoolSyncError(new SyncErrorDatabase("Invalid payload JSON")); } + return ApplyUpsert(connection, entry, data); + } + + [SuppressMessage( + "Security", + "CA2100:Review SQL queries for security vulnerabilities", + Justification = "Table names come from internal sync log, not user input" + )] + private static BoolSyncResult ApplyUpsert( + SqliteConnection connection, + SyncLogEntry entry, + Dictionary data + ) + { var columns = string.Join(", ", data.Keys); var parameters = string.Join(", ", data.Keys.Select(k => $"@{k}")); @@ -140,20 +149,7 @@ private static BoolSyncResult ApplyUpdate(SqliteConnection connection, SyncLogEn } // Apply the update using UPSERT - var columns = string.Join(", ", data.Keys); - var parameters = string.Join(", ", data.Keys.Select(k => $"@{k}")); - - using var cmd = connection.CreateCommand(); - cmd.CommandText = - $"INSERT OR REPLACE INTO {entry.TableName} ({columns}) VALUES ({parameters})"; - - foreach (var kvp in data) - { - cmd.Parameters.AddWithValue($"@{kvp.Key}", JsonElementToValue(kvp.Value)); - } - - cmd.ExecuteNonQuery(); - return new BoolSyncOk(true); + return ApplyUpsert(connection, entry, data); } [SuppressMessage( diff --git a/Sync/Nimblesite.Sync.SQLite/SubscriptionRepository.cs b/Sync/Nimblesite.Sync.SQLite/SubscriptionRepository.cs index f99e80d..e8be957 100644 --- a/Sync/Nimblesite.Sync.SQLite/SubscriptionRepository.cs +++ b/Sync/Nimblesite.Sync.SQLite/SubscriptionRepository.cs @@ -1,3 +1,4 @@ +using System.Diagnostics.CodeAnalysis; using Microsoft.Data.Sqlite; namespace Nimblesite.Sync.SQLite; @@ -13,34 +14,15 @@ public static class SubscriptionRepository /// /// SQLite connection. /// List of subscriptions or database error. - public static SubscriptionListResult GetAll(SqliteConnection connection) - { - try - { - using var cmd = connection.CreateCommand(); - cmd.CommandText = """ - SELECT subscription_id, origin_id, subscription_type, table_name, filter, created_at, expires_at - FROM _sync_subscriptions - ORDER BY created_at ASC - """; - - var subscriptions = new List(); - using var reader = cmd.ExecuteReader(); - - while (reader.Read()) - { - subscriptions.Add(ReadSubscription(reader)); - } - - return new SubscriptionListOk(subscriptions); - } - catch (SqliteException ex) - { - return new SubscriptionListError( - new SyncErrorDatabase($"Failed to get subscriptions: {ex.Message}") - ); - } - } + public static SubscriptionListResult GetAll(SqliteConnection connection) => + QueryList( + connection, + """ + SELECT subscription_id, origin_id, subscription_type, table_name, filter, created_at, expires_at + FROM _sync_subscriptions + ORDER BY created_at ASC + """ + ); /// /// Gets a subscription by ID. @@ -83,36 +65,20 @@ FROM _sync_subscriptions /// SQLite connection. /// Table name to filter by. /// List of subscriptions for the table or database error. - public static SubscriptionListResult GetByTable(SqliteConnection connection, string tableName) - { - try - { - using var cmd = connection.CreateCommand(); - cmd.CommandText = """ - SELECT subscription_id, origin_id, subscription_type, table_name, filter, created_at, expires_at - FROM _sync_subscriptions - WHERE table_name = @tableName - ORDER BY created_at ASC - """; - cmd.Parameters.AddWithValue("@tableName", tableName); - - var subscriptions = new List(); - using var reader = cmd.ExecuteReader(); - - while (reader.Read()) - { - subscriptions.Add(ReadSubscription(reader)); - } - - return new SubscriptionListOk(subscriptions); - } - catch (SqliteException ex) - { - return new SubscriptionListError( - new SyncErrorDatabase($"Failed to get subscriptions: {ex.Message}") - ); - } - } + public static SubscriptionListResult GetByTable( + SqliteConnection connection, + string tableName + ) => + QueryList( + connection, + """ + SELECT subscription_id, origin_id, subscription_type, table_name, filter, created_at, expires_at + FROM _sync_subscriptions + WHERE table_name = @tableName + ORDER BY created_at ASC + """, + cmd => cmd.Parameters.AddWithValue("@tableName", tableName) + ); /// /// Gets subscriptions for a specific origin. @@ -120,36 +86,20 @@ ORDER BY created_at ASC /// SQLite connection. /// Origin ID to filter by. /// List of subscriptions for the origin or database error. - public static SubscriptionListResult GetByOrigin(SqliteConnection connection, string originId) - { - try - { - using var cmd = connection.CreateCommand(); - cmd.CommandText = """ - SELECT subscription_id, origin_id, subscription_type, table_name, filter, created_at, expires_at - FROM _sync_subscriptions - WHERE origin_id = @originId - ORDER BY created_at ASC - """; - cmd.Parameters.AddWithValue("@originId", originId); - - var subscriptions = new List(); - using var reader = cmd.ExecuteReader(); - - while (reader.Read()) - { - subscriptions.Add(ReadSubscription(reader)); - } - - return new SubscriptionListOk(subscriptions); - } - catch (SqliteException ex) - { - return new SubscriptionListError( - new SyncErrorDatabase($"Failed to get subscriptions: {ex.Message}") - ); - } - } + public static SubscriptionListResult GetByOrigin( + SqliteConnection connection, + string originId + ) => + QueryList( + connection, + """ + SELECT subscription_id, origin_id, subscription_type, table_name, filter, created_at, expires_at + FROM _sync_subscriptions + WHERE origin_id = @originId + ORDER BY created_at ASC + """, + cmd => cmd.Parameters.AddWithValue("@originId", originId) + ); /// /// Inserts a subscription into _sync_subscriptions. @@ -271,6 +221,41 @@ WHERE expires_at IS NOT NULL AND expires_at < @currentTimestamp } } + [SuppressMessage( + "Security", + "CA2100:Review SQL queries for security vulnerabilities", + Justification = "SQL comes from internal constant literals, not user input" + )] + private static SubscriptionListResult QueryList( + SqliteConnection connection, + string sql, + Action? bindParameters = null + ) + { + try + { + using var cmd = connection.CreateCommand(); + cmd.CommandText = sql; + bindParameters?.Invoke(cmd); + + var subscriptions = new List(); + using var reader = cmd.ExecuteReader(); + + while (reader.Read()) + { + subscriptions.Add(ReadSubscription(reader)); + } + + return new SubscriptionListOk(subscriptions); + } + catch (SqliteException ex) + { + return new SubscriptionListError( + new SyncErrorDatabase($"Failed to get subscriptions: {ex.Message}") + ); + } + } + private static SyncSubscription ReadSubscription(SqliteDataReader reader) => new( SubscriptionId: reader.GetString(0), diff --git a/coverage-thresholds.json b/coverage-thresholds.json index feada56..b51f76b 100644 --- a/coverage-thresholds.json +++ b/coverage-thresholds.json @@ -38,7 +38,7 @@ "include": "[Nimblesite.Sync.Core]*,[Nimblesite.Sync.SQLite]*,[Nimblesite.Sync.Postgres]*,[Nimblesite.Sync.Http]*" }, "Sync/Nimblesite.Sync.Http": { - "threshold": 33, + "threshold": 34, "include": "[Nimblesite.Sync.Core]*,[Nimblesite.Sync.SQLite]*,[Nimblesite.Sync.Postgres]*,[Nimblesite.Sync.Http]*" }, "Reporting/Nimblesite.Reporting": { From 7f4202478a84a1071bf802f9a62bc290dde38dad Mon Sep 17 00:00:00 2001 From: Christian Findlay <16697547+MelbourneDeveloper@users.noreply.github.com> Date: Sat, 4 Jul 2026 15:31:55 +1000 Subject: [PATCH 5/5] Deslopping --- .../CodeGeneration/DataAccessGenerator.cs | 58 ++--- .../CodeGeneration/ModelGenerator.cs | 30 +-- .../DbConnectionExtensions.cs | 51 ++--- .../DbTransact.cs | 20 +- .../DbTransactionExtensions.cs | 65 +++--- .../SqliteCodeGenerator.cs | 34 ++- .../Fakes/FakeDbConnectionTests.cs | 43 ++-- .../SQLiteContextE2ETests.cs | 40 ++-- .../Services/FileDialogService.cs | 112 +++++----- .../Parsing/LqlToAstVisitor.cs | 34 ++- .../crates/lql-analyzer/src/completion.rs | 69 +++--- .../crates/lql-analyzer/src/scope.rs | 28 +-- Lql/lql-lsp-rust/crates/lql-lsp/src/ai.rs | 68 ++---- Lql/lql-lsp-rust/crates/lql-lsp/src/main.rs | 133 ++--------- .../PostgresCheckExpressionNormalizer.cs | 32 +-- .../LqlDefaultsTests.cs | 36 +-- ...sCheckConstraintReplacementIssue57Tests.cs | 71 ++---- .../PostgresColumnCheckConstraintTests.cs | 64 ++---- .../PostgresDropConstraintBackedIndexTests.cs | 88 +++----- .../PostgresGrantRunAsE2ETests.cs | 60 ++--- .../PostgresLqlOnlyE2ETests.cs | 57 +++-- .../PostgresMigrationTests.cs | 151 ++++--------- .../PostgresRlsNapShapeTests.cs | 43 ++-- .../PostgresTestDb.cs | 70 ++++++ ...PostgresUniqueConstraintIssue55E2ETests.cs | 76 ++----- .../SchemaDiffCheckConstraintIssue57Tests.cs | 80 +++---- .../SchemaDiffTests.cs | 31 +-- .../SchemaDiffUniqueConstraintIssue55Tests.cs | 66 ++---- .../SchemaVerifier.cs | 4 +- .../SqliteMigrationTests.cs | 143 +++--------- .../Components/BarChartComponent.cs | 16 +- .../Components/ColumnLookup.cs | 23 ++ .../Components/TableComponent.cs | 14 +- .../LqlExpressionEvaluator.cs | 37 ++- .../HttpEndpointTests.cs | 31 ++- .../HttpMappingE2ETests.cs | 30 +-- Sync/Nimblesite.Sync.Http/SyncHelpers.cs | 31 ++- .../HttpMappingSyncTests.cs | 211 +++--------------- .../PostgresChangeApplier.cs | 41 ++-- .../SpecComplianceTests.cs | 15 +- .../ChangeApplierSQLite.cs | 24 +- .../SqliteConnectionSyncExtensions.cs | 24 +- .../SubscriptionRepository.cs | 4 +- .../BatchManagerTests.cs | 17 +- .../ChangeApplierTests.cs | 13 +- .../LqlExpressionEvaluatorTests.cs | 23 +- .../SyncCoordinatorTests.cs | 24 +- Sync/Nimblesite.Sync.Tests/SyncStateTests.cs | 18 +- Sync/Nimblesite.Sync.Tests/TestDb.cs | 7 + 49 files changed, 900 insertions(+), 1560 deletions(-) create mode 100644 Reporting/Nimblesite.Reporting.React/Components/ColumnLookup.cs diff --git a/DataProvider/Nimblesite.DataProvider.Core/CodeGeneration/DataAccessGenerator.cs b/DataProvider/Nimblesite.DataProvider.Core/CodeGeneration/DataAccessGenerator.cs index 267dcf3..8e4001f 100644 --- a/DataProvider/Nimblesite.DataProvider.Core/CodeGeneration/DataAccessGenerator.cs +++ b/DataProvider/Nimblesite.DataProvider.Core/CodeGeneration/DataAccessGenerator.cs @@ -122,6 +122,26 @@ public static string GenerateParameterList(IReadOnlyList paramete return string.Join(", ", parameters.Select(p => $"object {p.Name}")); } + /// + /// Appends AddWithValue parameter-binding lines for the given columns. + /// Nullable columns coalesce to DBNull.Value; non-nullable bind directly. + /// + private static void AppendParameterBindings( + StringBuilder sb, + IReadOnlyList columns + ) + { + foreach (var column in columns) + { + var escaped = EscapeReservedKeyword(column.Name); + var value = column.IsNullable ? $"{escaped} ?? (object)DBNull.Value" : escaped; + sb.AppendLine( + CultureInfo.InvariantCulture, + $" command.Parameters.AddWithValue(\"@{column.Name}\", {value});" + ); + } + } + /// /// Generates a data access extension method for querying /// @@ -383,24 +403,7 @@ public static Result GenerateInsertMethod( sb.AppendLine(" {"); // Add parameters - foreach (var column in insertableColumns) - { - var paramName = EscapeReservedKeyword(column.Name); - if (column.IsNullable) - { - sb.AppendLine( - CultureInfo.InvariantCulture, - $" command.Parameters.AddWithValue(\"@{column.Name}\", {paramName} ?? (object)DBNull.Value);" - ); - } - else - { - sb.AppendLine( - CultureInfo.InvariantCulture, - $" command.Parameters.AddWithValue(\"@{column.Name}\", {paramName});" - ); - } - } + AppendParameterBindings(sb, insertableColumns); sb.AppendLine(); sb.AppendLine( @@ -620,24 +623,7 @@ public static Result GenerateUpdateMethod( sb.AppendLine(" {"); // Add parameters (nullable types use null-coalescing to DBNull.Value) - foreach (var column in allColumns) - { - var escaped = EscapeReservedKeyword(column.Name); - if (column.IsNullable) - { - sb.AppendLine( - CultureInfo.InvariantCulture, - $" command.Parameters.AddWithValue(\"@{column.Name}\", {escaped} ?? (object)DBNull.Value);" - ); - } - else - { - sb.AppendLine( - CultureInfo.InvariantCulture, - $" command.Parameters.AddWithValue(\"@{column.Name}\", {escaped});" - ); - } - } + AppendParameterBindings(sb, allColumns); sb.AppendLine(); sb.AppendLine( diff --git a/DataProvider/Nimblesite.DataProvider.Core/CodeGeneration/ModelGenerator.cs b/DataProvider/Nimblesite.DataProvider.Core/CodeGeneration/ModelGenerator.cs index c6567e2..aa687e2 100644 --- a/DataProvider/Nimblesite.DataProvider.Core/CodeGeneration/ModelGenerator.cs +++ b/DataProvider/Nimblesite.DataProvider.Core/CodeGeneration/ModelGenerator.cs @@ -21,15 +21,8 @@ public static Result GenerateRecordType( IReadOnlyList columns ) { - if (string.IsNullOrWhiteSpace(typeName)) - return new Result.Error( - new SqlError("typeName cannot be null or empty") - ); - - if (columns == null || columns.Count == 0) - return new Result.Error( - new SqlError("columns cannot be null or empty") - ); + if (ValidateTypeNameAndColumns(typeName, columns) is { } error) + return new Result.Error(error); var sb = new StringBuilder(); @@ -277,15 +270,8 @@ public static Result GenerateRawRecordType( IReadOnlyList columns ) { - if (string.IsNullOrWhiteSpace(typeName)) - return new Result.Error( - new SqlError("typeName cannot be null or empty") - ); - - if (columns == null || columns.Count == 0) - return new Result.Error( - new SqlError("columns cannot be null or empty") - ); + if (ValidateTypeNameAndColumns(typeName, columns) is { } error) + return new Result.Error(error); var sb = new StringBuilder(); sb.AppendLine(CultureInfo.InvariantCulture, $"internal record {typeName}("); @@ -303,4 +289,12 @@ IReadOnlyList columns return new Result.Ok(sb.ToString()); } + + private static SqlError? ValidateTypeNameAndColumns( + string typeName, + IReadOnlyList? columns + ) => + string.IsNullOrWhiteSpace(typeName) ? new SqlError("typeName cannot be null or empty") + : columns == null || columns.Count == 0 ? new SqlError("columns cannot be null or empty") + : null; } diff --git a/DataProvider/Nimblesite.DataProvider.Core/DbConnectionExtensions.cs b/DataProvider/Nimblesite.DataProvider.Core/DbConnectionExtensions.cs index 8b2799e..7f1e49b 100644 --- a/DataProvider/Nimblesite.DataProvider.Core/DbConnectionExtensions.cs +++ b/DataProvider/Nimblesite.DataProvider.Core/DbConnectionExtensions.cs @@ -70,16 +70,7 @@ public static Result, SqlError> Query( try { - using var command = connection.CreateCommand(); - command.CommandText = sql; - - if (parameters != null) - { - foreach (var parameter in parameters) - { - command.Parameters.Add(parameter); - } - } + using var command = connection.CreateCommandWith(sql, parameters); var results = new List(); using var reader = command.ExecuteReader(); @@ -143,16 +134,7 @@ public static Result Execute( try { - using var command = connection.CreateCommand(); - command.CommandText = sql; - - if (parameters != null) - { - foreach (var parameter in parameters) - { - command.Parameters.Add(parameter); - } - } + using var command = connection.CreateCommandWith(sql, parameters); var rowsAffected = command.ExecuteNonQuery(); return new Result.Ok(rowsAffected); @@ -189,16 +171,7 @@ public static Result Execute( try { - using var command = connection.CreateCommand(); - command.CommandText = sql; - - if (parameters != null) - { - foreach (var parameter in parameters) - { - command.Parameters.Add(parameter); - } - } + using var command = connection.CreateCommandWith(sql, parameters); var result = command.ExecuteScalar(); return new Result.Ok(result is T value ? value : default); @@ -255,4 +228,22 @@ public static Result, SqlError> GetRecords( var sql = ((Result.Ok)sqlResult).Value; return connection.Query(sql, parameters, mapper); } + + private static IDbCommand CreateCommandWith( + this IDbConnection connection, + string sql, + IEnumerable? parameters + ) + { + var command = connection.CreateCommand(); + command.CommandText = sql; + if (parameters != null) + { + foreach (var parameter in parameters) + { + command.Parameters.Add(parameter); + } + } + return command; + } } diff --git a/DataProvider/Nimblesite.DataProvider.Core/DbTransact.cs b/DataProvider/Nimblesite.DataProvider.Core/DbTransact.cs index 4d05293..d4d8a34 100644 --- a/DataProvider/Nimblesite.DataProvider.Core/DbTransact.cs +++ b/DataProvider/Nimblesite.DataProvider.Core/DbTransact.cs @@ -34,28 +34,16 @@ public static class DbTransact /// The database connection. It will be opened if not already open. /// The asynchronous delegate to execute within the transaction. /// A task that completes when the transactional operation finishes. - public static async Task Transact(this DbConnection cn, Func body) + public static Task Transact(this DbConnection cn, Func body) { ArgumentNullException.ThrowIfNull(cn); ArgumentNullException.ThrowIfNull(body); - if (cn.State != ConnectionState.Open) - await cn.OpenAsync().ConfigureAwait(false); - -#pragma warning disable CA2007 - await using var tx = await cn.BeginTransactionAsync().ConfigureAwait(false); -#pragma warning restore CA2007 - - try + return cn.Transact(async tx => { await body(tx).ConfigureAwait(false); - await tx.CommitAsync().ConfigureAwait(false); - } - catch - { - await tx.RollbackAsync().ConfigureAwait(false); - throw; - } + return true; + }); } /// diff --git a/DataProvider/Nimblesite.DataProvider.Core/DbTransactionExtensions.cs b/DataProvider/Nimblesite.DataProvider.Core/DbTransactionExtensions.cs index e6a5e58..3a85a94 100644 --- a/DataProvider/Nimblesite.DataProvider.Core/DbTransactionExtensions.cs +++ b/DataProvider/Nimblesite.DataProvider.Core/DbTransactionExtensions.cs @@ -37,17 +37,12 @@ public static Result, SqlError> Query( try { - using var command = transaction.Connection.CreateCommand(); - command.Transaction = transaction; - command.CommandText = sql; + if (transaction.Connection is not { } connection) + return new Result, SqlError>.Error, SqlError>( + SqlError.Create("Transaction or connection is null") + ); - if (parameters != null) - { - foreach (var parameter in parameters) - { - command.Parameters.Add(parameter); - } - } + using var command = CreateConfiguredCommand(connection, transaction, sql, parameters); var results = new List(); using var reader = command.ExecuteReader(); @@ -97,17 +92,12 @@ public static Result Execute( try { - using var command = transaction.Connection.CreateCommand(); - command.Transaction = transaction; - command.CommandText = sql; + if (transaction.Connection is not { } connection) + return new Result.Error( + SqlError.Create("Transaction or connection is null") + ); - if (parameters != null) - { - foreach (var parameter in parameters) - { - command.Parameters.Add(parameter); - } - } + using var command = CreateConfiguredCommand(connection, transaction, sql, parameters); var rowsAffected = command.ExecuteNonQuery(); return new Result.Ok(rowsAffected); @@ -144,17 +134,12 @@ public static Result Execute( try { - using var command = transaction.Connection.CreateCommand(); - command.Transaction = transaction; - command.CommandText = sql; + if (transaction.Connection is not { } connection) + return new Result.Error( + SqlError.Create("Transaction or connection is null") + ); - if (parameters != null) - { - foreach (var parameter in parameters) - { - command.Parameters.Add(parameter); - } - } + using var command = CreateConfiguredCommand(connection, transaction, sql, parameters); var result = command.ExecuteScalar(); return new Result.Ok(result is T value ? value : default); @@ -164,4 +149,24 @@ public static Result Execute( return new Result.Error(SqlError.FromException(ex)); } } + + private static IDbCommand CreateConfiguredCommand( + IDbConnection connection, + IDbTransaction transaction, + string sql, + IEnumerable? parameters + ) + { + var command = connection.CreateCommand(); + command.Transaction = transaction; + command.CommandText = sql; + if (parameters != null) + { + foreach (var parameter in parameters) + { + command.Parameters.Add(parameter); + } + } + return command; + } } diff --git a/DataProvider/Nimblesite.DataProvider.SQLite/SqliteCodeGenerator.cs b/DataProvider/Nimblesite.DataProvider.SQLite/SqliteCodeGenerator.cs index e7babb4..eab7eb1 100644 --- a/DataProvider/Nimblesite.DataProvider.SQLite/SqliteCodeGenerator.cs +++ b/DataProvider/Nimblesite.DataProvider.SQLite/SqliteCodeGenerator.cs @@ -486,12 +486,7 @@ as Result, SqlError>.Error< var text = sqlFile.GetText(context.CancellationToken) ?? SourceText.From(sqlText, Encoding.UTF8); - var span = new TextSpan(0, Math.Min(1, text.Length)); - var lineSpan = new LinePositionSpan( - new LinePosition(0, 0), - new LinePosition(0, Math.Min(1, text.Length)) - ); - var location = Location.Create(sqlFile.Path, span, lineSpan); + var location = CreateSqlFileLocation(sqlFile, text); var diagCol = Diagnostic.Create( new DiagnosticDescriptor( @@ -567,12 +562,7 @@ as Result, SqlError>.Error< var text = sqlFile.GetText(context.CancellationToken) ?? SourceText.From(sqlText, Encoding.UTF8); - var span = new TextSpan(0, Math.Min(1, text.Length)); - var lineSpan = new LinePositionSpan( - new LinePosition(0, 0), - new LinePosition(0, Math.Min(1, text.Length)) - ); - var location = Location.Create(sqlFile.Path, span, lineSpan); + var location = CreateSqlFileLocation(sqlFile, text); var diagGen = Diagnostic.Create( new DiagnosticDescriptor( @@ -596,12 +586,7 @@ as Result, SqlError>.Error< var text = sqlFile.GetText(context.CancellationToken) ?? SourceText.From(string.Empty, Encoding.UTF8); - var span = new TextSpan(0, Math.Min(1, text.Length)); - var lineSpan = new LinePositionSpan( - new LinePosition(0, 0), - new LinePosition(0, Math.Min(1, text.Length)) - ); - var location = Location.Create(sqlFile.Path, span, lineSpan); + var location = CreateSqlFileLocation(sqlFile, text); var diag = Diagnostic.Create( new DiagnosticDescriptor( @@ -742,4 +727,17 @@ is Result.Error operationsFailure } } } + + /// + /// Builds a diagnostic pointing at the start of a .sql additional file. + /// + private static Location CreateSqlFileLocation(AdditionalText sqlFile, SourceText text) + { + var span = new TextSpan(0, Math.Min(1, text.Length)); + var lineSpan = new LinePositionSpan( + new LinePosition(0, 0), + new LinePosition(0, Math.Min(1, text.Length)) + ); + return Location.Create(sqlFile.Path, span, lineSpan); + } } diff --git a/DataProvider/Nimblesite.DataProvider.Tests/Fakes/FakeDbConnectionTests.cs b/DataProvider/Nimblesite.DataProvider.Tests/Fakes/FakeDbConnectionTests.cs index c68c954..f47bbe2 100644 --- a/DataProvider/Nimblesite.DataProvider.Tests/Fakes/FakeDbConnectionTests.cs +++ b/DataProvider/Nimblesite.DataProvider.Tests/Fakes/FakeDbConnectionTests.cs @@ -2,6 +2,8 @@ #pragma warning disable CA2000 // Dispose objects before losing scope #pragma warning disable CA1849 // Synchronous blocking calls +using System.Data.Common; + namespace Nimblesite.DataProvider.Tests.Fakes; public class FakeDbConnectionTests @@ -134,20 +136,8 @@ var s when s.Contains("SELECT COUNT(*) FROM Users", StringComparison.Ordinal) => [Fact] public void FakeCommand_UsesDataReaderFactory() { - // Arrange - var connection = new FakeDbConnection(sql => new FakeDataReader( - ["Id", "Name"], - [typeof(int), typeof(string)], - [ - [1, "Test User"], - ] - )); - - connection.Open(); - - // Act - using var command = connection.CreateCommand(); - command.CommandText = "SELECT * FROM Users"; + // Arrange & Act + using var command = CreateTestUserCommand(); using var reader = command.ExecuteReader(); // Assert @@ -184,7 +174,19 @@ public void FakeDataReader_HandlesNullValues() [Fact] public async Task FakeDataReader_SupportsAsyncOperations() { - // Arrange + // Arrange & Act + using var command = CreateTestUserCommand(); + using var reader = await command.ExecuteReaderAsync(); + + // Assert + Assert.True(await reader.ReadAsync()); + Assert.Equal(1, reader.GetInt32(0)); + Assert.Equal("Test User", reader.GetString(1)); + Assert.False(await reader.IsDBNullAsync(0)); + } + + private static DbCommand CreateTestUserCommand() + { var connection = new FakeDbConnection(sql => new FakeDataReader( ["Id", "Name"], [typeof(int), typeof(string)], @@ -195,15 +197,8 @@ public async Task FakeDataReader_SupportsAsyncOperations() connection.Open(); - // Act - using var command = connection.CreateCommand(); + var command = connection.CreateCommand(); command.CommandText = "SELECT * FROM Users"; - using var reader = await command.ExecuteReaderAsync(); - - // Assert - Assert.True(await reader.ReadAsync()); - Assert.Equal(1, reader.GetInt32(0)); - Assert.Equal("Test User", reader.GetString(1)); - Assert.False(await reader.IsDBNullAsync(0)); + return command; } } diff --git a/DataProvider/Nimblesite.DataProvider.Tests/SQLiteContextE2ETests.cs b/DataProvider/Nimblesite.DataProvider.Tests/SQLiteContextE2ETests.cs index 4fb845b..64c064c 100644 --- a/DataProvider/Nimblesite.DataProvider.Tests/SQLiteContextE2ETests.cs +++ b/DataProvider/Nimblesite.DataProvider.Tests/SQLiteContextE2ETests.cs @@ -42,6 +42,22 @@ private static Pipeline WithIdentity(string table) private static Identifier Id(string name) => new(name); + private static Pipeline CountByCountryPipeline() + { + var pipeline = WithIdentity("users"); + pipeline.Steps.Add( + new SelectStep([ + ColumnInfo.Named("country"), + ColumnInfo.FromExpression("COUNT(*)", alias: "cnt"), + ]) + { + Base = Id("users"), + } + ); + pipeline.Steps.Add(new GroupByStep(["country"]) { Base = Id("users") }); + return pipeline; + } + private void CreateSchemaAndSeed() { using var cmd = _connection.CreateCommand(); @@ -119,17 +135,7 @@ public void ProcessPipeline_SelectFilterOrderLimit_GeneratesCorrectSql() [Fact] public void PipelineProcessor_GroupByWithHaving_GeneratesGroupByAndHavingSql() { - var pipeline = WithIdentity("users"); - pipeline.Steps.Add( - new SelectStep([ - ColumnInfo.Named("country"), - ColumnInfo.FromExpression("COUNT(*)", alias: "cnt"), - ]) - { - Base = Id("users"), - } - ); - pipeline.Steps.Add(new GroupByStep(["country"]) { Base = Id("users") }); + var pipeline = CountByCountryPipeline(); pipeline.Steps.Add(new HavingStep { Base = Id("users"), Condition = "COUNT(*) > 1" }); var context = new SQLiteContext(); @@ -346,17 +352,7 @@ is not Result, SqlError>.Ok, SqlErro [Fact] public void PipelineProcessor_GroupByExecute_ReturnsAggregatedRows() { - var pipeline = WithIdentity("users"); - pipeline.Steps.Add( - new SelectStep([ - ColumnInfo.Named("country"), - ColumnInfo.FromExpression("COUNT(*)", alias: "cnt"), - ]) - { - Base = Id("users"), - } - ); - pipeline.Steps.Add(new GroupByStep(["country"]) { Base = Id("users") }); + var pipeline = CountByCountryPipeline(); pipeline.Steps.Add(new OrderByStep([("cnt", "DESC")]) { Base = Id("users") }); var context = new SQLiteContext(); diff --git a/Lql/Nimblesite.Lql.Browser/Services/FileDialogService.cs b/Lql/Nimblesite.Lql.Browser/Services/FileDialogService.cs index 68e3772..0ec4e15 100644 --- a/Lql/Nimblesite.Lql.Browser/Services/FileDialogService.cs +++ b/Lql/Nimblesite.Lql.Browser/Services/FileDialogService.cs @@ -97,33 +97,22 @@ public static async Task> ShowSaveFileDialogAsync( string suggestedFileName ) { - try - { - var dialog = new FilePickerSaveOptions - { - Title = "Save File As", - SuggestedFileName = suggestedFileName, - FileTypeChoices = - [ - new FilePickerFileType("LQL Files") { Patterns = ["*.lql"] }, - new FilePickerFileType("SQL Files") { Patterns = ["*.sql"] }, - ], - }; - - var file = await storageProvider.SaveFilePickerAsync(dialog); - if (file != null) - { - return new Result.Ok(file.Path.LocalPath); - } - - return new Result.Error("No file selected"); - } - catch (Exception ex) + var dialog = new FilePickerSaveOptions { - return new Result.Error( - $"Error saving file: {ex.Message}" - ); - } + Title = "Save File As", + SuggestedFileName = suggestedFileName, + FileTypeChoices = + [ + new FilePickerFileType("LQL Files") { Patterns = ["*.lql"] }, + new FilePickerFileType("SQL Files") { Patterns = ["*.sql"] }, + ], + }; + + return await RunSavePickerAsync( + storageProvider: storageProvider, + dialog: dialog, + errorContext: "Error saving file" + ); } /// @@ -135,29 +124,18 @@ public static async Task> ShowExportCsvDialogAsync( IStorageProvider storageProvider ) { - try + var dialog = new FilePickerSaveOptions { - var dialog = new FilePickerSaveOptions - { - Title = "Export CSV", - SuggestedFileName = "export.csv", - FileTypeChoices = [new FilePickerFileType("CSV Files") { Patterns = ["*.csv"] }], - }; - - var file = await storageProvider.SaveFilePickerAsync(dialog); - if (file != null) - { - return new Result.Ok(file.Path.LocalPath); - } - - return new Result.Error("No file selected"); - } - catch (Exception ex) - { - return new Result.Error( - $"Error selecting export file: {ex.Message}" - ); - } + Title = "Export CSV", + SuggestedFileName = "export.csv", + FileTypeChoices = [new FilePickerFileType("CSV Files") { Patterns = ["*.csv"] }], + }; + + return await RunSavePickerAsync( + storageProvider: storageProvider, + dialog: dialog, + errorContext: "Error selecting export file" + ); } /// @@ -169,27 +147,37 @@ public static async Task> ShowExportJsonDialogAsync( IStorageProvider storageProvider ) { - try + var dialog = new FilePickerSaveOptions { - var dialog = new FilePickerSaveOptions - { - Title = "Export JSON", - SuggestedFileName = "export.json", - FileTypeChoices = [new FilePickerFileType("JSON Files") { Patterns = ["*.json"] }], - }; + Title = "Export JSON", + SuggestedFileName = "export.json", + FileTypeChoices = [new FilePickerFileType("JSON Files") { Patterns = ["*.json"] }], + }; + + return await RunSavePickerAsync( + storageProvider: storageProvider, + dialog: dialog, + errorContext: "Error selecting export file" + ); + } + private static async Task> RunSavePickerAsync( + IStorageProvider storageProvider, + FilePickerSaveOptions dialog, + string errorContext + ) + { + try + { var file = await storageProvider.SaveFilePickerAsync(dialog); - if (file != null) - { - return new Result.Ok(file.Path.LocalPath); - } - - return new Result.Error("No file selected"); + return file != null + ? new Result.Ok(file.Path.LocalPath) + : new Result.Error("No file selected"); } catch (Exception ex) { return new Result.Error( - $"Error selecting export file: {ex.Message}" + $"{errorContext}: {ex.Message}" ); } } diff --git a/Lql/Nimblesite.Lql.Core/Parsing/LqlToAstVisitor.cs b/Lql/Nimblesite.Lql.Core/Parsing/LqlToAstVisitor.cs index d79f85f..4119389 100644 --- a/Lql/Nimblesite.Lql.Core/Parsing/LqlToAstVisitor.cs +++ b/Lql/Nimblesite.Lql.Core/Parsing/LqlToAstVisitor.cs @@ -1310,35 +1310,33 @@ private static string ExtractLambdaCondition(LqlParser.LambdaExprContext lambdaE // Check for IDENT first if (namedArg.IDENT()?.GetText() == name) { - var comparisonText = - namedArg.comparison() != null - ? ProcessComparisonToSql(namedArg.comparison(), null) - : null; - var logicalText = - namedArg.logicalExpr() != null - ? ProcessLogicalExpressionToSql(namedArg.logicalExpr(), null) - : null; - return comparisonText ?? logicalText; + return ResolveNamedArgValue(namedArg); } // Check for ON keyword if (name == "on" && namedArg.ON() != null) { - var comparisonText = - namedArg.comparison() != null - ? ProcessComparisonToSql(namedArg.comparison(), null) - : null; - var logicalText = - namedArg.logicalExpr() != null - ? ProcessLogicalExpressionToSql(namedArg.logicalExpr(), null) - : null; - return comparisonText ?? logicalText; + return ResolveNamedArgValue(namedArg); } } } return null; } + // Resolves a named-arg value to SQL, preferring a comparison over a logicalExpr. + private static string? ResolveNamedArgValue(LqlParser.NamedArgContext namedArg) + { + var comparisonText = + namedArg.comparison() != null + ? ProcessComparisonToSql(namedArg.comparison(), null) + : null; + var logicalText = + namedArg.logicalExpr() != null + ? ProcessLogicalExpressionToSql(namedArg.logicalExpr(), null) + : null; + return comparisonText ?? logicalText; + } + /// /// Extracts an identifier from an argument. /// diff --git a/Lql/lql-lsp-rust/crates/lql-analyzer/src/completion.rs b/Lql/lql-lsp-rust/crates/lql-analyzer/src/completion.rs index 31b1afc..145ebc6 100644 --- a/Lql/lql-lsp-rust/crates/lql-analyzer/src/completion.rs +++ b/Lql/lql-lsp-rust/crates/lql-analyzer/src/completion.rs @@ -249,6 +249,27 @@ fn pipeline_completions(prefix: &str) -> Vec { .collect() } +fn build_completions( + entries: Vec<(&str, &str, &str)>, + prefix: &str, + kind: CompletionKind, + with_snippet: bool, + sort_priority: u8, +) -> Vec { + entries + .into_iter() + .filter(|(name, _, _)| name.starts_with(prefix)) + .map(|(name, detail, doc)| CompletionItem { + label: name.into(), + kind, + detail: detail.into(), + documentation: doc.into(), + insert_text: with_snippet.then(|| format!("{name}($0)")), + sort_priority, + }) + .collect() +} + fn aggregate_completions(prefix: &str) -> Vec { let fns = vec![ ("count", "Count rows", "count(*) or count(column)"), @@ -261,17 +282,7 @@ fn aggregate_completions(prefix: &str) -> Vec { ("dense_rank", "Dense rank", "dense_rank() over (...)"), ]; - fns.into_iter() - .filter(|(name, _, _)| name.starts_with(prefix)) - .map(|(name, detail, doc)| CompletionItem { - label: name.into(), - kind: CompletionKind::Function, - detail: detail.into(), - documentation: doc.into(), - insert_text: Some(format!("{name}($0)")), - sort_priority: 2, - }) - .collect() + build_completions(fns, prefix, CompletionKind::Function, true, 2) } fn string_function_completions(prefix: &str) -> Vec { @@ -291,17 +302,7 @@ fn string_function_completions(prefix: &str) -> Vec { ("coalesce", "First non-null", "coalesce(val1, val2, ...)"), ]; - fns.into_iter() - .filter(|(name, _, _)| name.starts_with(prefix)) - .map(|(name, detail, doc)| CompletionItem { - label: name.into(), - kind: CompletionKind::Function, - detail: detail.into(), - documentation: doc.into(), - insert_text: Some(format!("{name}($0)")), - sort_priority: 2, - }) - .collect() + build_completions(fns, prefix, CompletionKind::Function, true, 2) } fn keyword_completions(prefix: &str) -> Vec { @@ -319,17 +320,7 @@ fn keyword_completions(prefix: &str) -> Vec { ("distinct", "Distinct modifier", "Remove duplicates"), ]; - kws.into_iter() - .filter(|(name, _, _)| name.starts_with(prefix)) - .map(|(name, detail, doc)| CompletionItem { - label: name.into(), - kind: CompletionKind::Keyword, - detail: detail.into(), - documentation: doc.into(), - insert_text: None, - sort_priority: 3, - }) - .collect() + build_completions(kws, prefix, CompletionKind::Keyword, false, 3) } fn lambda_completions(prefix: &str) -> Vec { @@ -343,17 +334,7 @@ fn lambda_completions(prefix: &str) -> Vec { ("exists", "EXISTS subquery", "Check subquery has results"), ]; - ops.into_iter() - .filter(|(name, _, _)| name.starts_with(prefix)) - .map(|(name, detail, doc)| CompletionItem { - label: name.into(), - kind: CompletionKind::Keyword, - detail: detail.into(), - documentation: doc.into(), - insert_text: None, - sort_priority: 3, - }) - .collect() + build_completions(ops, prefix, CompletionKind::Keyword, false, 3) } #[cfg(test)] diff --git a/Lql/lql-lsp-rust/crates/lql-analyzer/src/scope.rs b/Lql/lql-lsp-rust/crates/lql-analyzer/src/scope.rs index ff67686..c179cdc 100644 --- a/Lql/lql-lsp-rust/crates/lql-analyzer/src/scope.rs +++ b/Lql/lql-lsp-rust/crates/lql-analyzer/src/scope.rs @@ -192,6 +192,19 @@ fn push_fn_call<'input>( }); } +/// Collect function calls from a single `functionCall` node (its IDENT plus nested argList). +fn collect_fn_calls_from_function_call<'input>( + fc: &lql_parser::FunctionCallContext<'input>, + calls: &mut Vec, +) { + if let Some(ident) = fc.IDENT() { + push_fn_call(&ident, calls); + } + if let Some(inner_args) = fc.argList() { + collect_fn_calls_from_arg_list(&inner_args, calls); + } +} + /// Collect function calls from an expr node. fn collect_fn_calls_from_expr<'input>( expr: &lql_parser::ExprContext<'input>, @@ -223,25 +236,14 @@ fn collect_fn_calls_from_arg_list<'input>( for arg in arg_list.arg_all() { // Direct functionCall rule in arg if let Some(fc) = arg.functionCall() { - if let Some(ident) = fc.IDENT() { - push_fn_call(&ident, calls); - } - // Recurse into functionCall's own argList - if let Some(inner_args) = fc.argList() { - collect_fn_calls_from_arg_list(&inner_args, calls); - } + collect_fn_calls_from_function_call(&fc, calls); } // columnAlias may contain a functionCall if let Some(col_alias) = arg.columnAlias() { use lql_parser::ColumnAliasContextAttrs; if let Some(fc) = col_alias.functionCall() { - if let Some(ident) = fc.IDENT() { - push_fn_call(&ident, calls); - } - if let Some(inner_args) = fc.argList() { - collect_fn_calls_from_arg_list(&inner_args, calls); - } + collect_fn_calls_from_function_call(&fc, calls); } } diff --git a/Lql/lql-lsp-rust/crates/lql-lsp/src/ai.rs b/Lql/lql-lsp-rust/crates/lql-lsp/src/ai.rs index 7a3563f..750eb12 100644 --- a/Lql/lql-lsp-rust/crates/lql-lsp/src/ai.rs +++ b/Lql/lql-lsp-rust/crates/lql-lsp/src/ai.rs @@ -354,6 +354,19 @@ impl AiCompletionProvider for SlowAiProvider { mod tests { use super::*; + fn empty_context() -> AiCompletionContext { + AiCompletionContext { + document_text: String::new(), + line: 0, + column: 0, + line_prefix: String::new(), + word_prefix: String::new(), + file_uri: String::new(), + available_tables: vec![], + schema_description: String::new(), + } + } + // ── AiConfig::from_json ──────────────────────────────────────────── #[test] fn parse_full_config() { @@ -575,16 +588,7 @@ mod tests { #[tokio::test] async fn slow_provider_returns_after_delay() { let provider = SlowAiProvider { delay_ms: 10 }; - let ctx = AiCompletionContext { - document_text: "".to_string(), - line: 0, - column: 0, - line_prefix: "".to_string(), - word_prefix: "".to_string(), - file_uri: "".to_string(), - available_tables: vec![], - schema_description: String::new(), - }; + let ctx = empty_context(); let items = provider.complete(&ctx).await; assert_eq!(items.len(), 1); assert_eq!(items[0].label, "ai_slow_result"); @@ -668,16 +672,7 @@ mod tests { #[tokio::test] async fn test_provider_items_are_snippet_kind() { let provider = TestAiProvider; - let ctx = AiCompletionContext { - document_text: "".to_string(), - line: 0, - column: 0, - line_prefix: "".to_string(), - word_prefix: "".to_string(), - file_uri: "".to_string(), - available_tables: vec![], - schema_description: String::new(), - }; + let ctx = empty_context(); let items = provider.complete(&ctx).await; for item in &items { assert_eq!(item.kind, CompletionKind::Snippet); @@ -759,16 +754,7 @@ mod tests { enabled: true, }; let provider = OllamaProvider::new(&config, "ref".to_string()); - let ctx = AiCompletionContext { - document_text: String::new(), - line: 0, - column: 0, - line_prefix: String::new(), - word_prefix: String::new(), - file_uri: String::new(), - available_tables: vec![], - schema_description: String::new(), - }; + let ctx = empty_context(); let items = provider.complete(&ctx).await; assert!(items.is_empty(), "non-JSON body must yield empty"); } @@ -809,16 +795,7 @@ mod tests { enabled: true, }; let provider = OllamaProvider::new(&config, "ref".to_string()); - let ctx = AiCompletionContext { - document_text: String::new(), - line: 0, - column: 0, - line_prefix: String::new(), - word_prefix: String::new(), - file_uri: String::new(), - available_tables: vec![], - schema_description: String::new(), - }; + let ctx = empty_context(); let items = provider.complete(&ctx).await; assert!(items.is_empty()); } @@ -861,16 +838,7 @@ mod tests { enabled: true, }; let provider = OllamaProvider::new(&config, "ref".to_string()); - let ctx = AiCompletionContext { - document_text: String::new(), - line: 0, - column: 0, - line_prefix: String::new(), - word_prefix: String::new(), - file_uri: String::new(), - available_tables: vec![], - schema_description: String::new(), - }; + let ctx = empty_context(); let items = provider.complete(&ctx).await; assert!(items.iter().any(|i| i.label == "foo"), "should parse foo"); } diff --git a/Lql/lql-lsp-rust/crates/lql-lsp/src/main.rs b/Lql/lql-lsp-rust/crates/lql-lsp/src/main.rs index 1077f49..dd3d13b 100644 --- a/Lql/lql-lsp-rust/crates/lql-lsp/src/main.rs +++ b/Lql/lql-lsp-rust/crates/lql-lsp/src/main.rs @@ -646,6 +646,19 @@ async fn main() { mod tests { use super::*; + async fn open_doc(backend: &LqlBackend, uri: &Url, text: &str) { + backend + .did_open(DidOpenTextDocumentParams { + text_document: TextDocumentItem { + uri: uri.clone(), + language_id: "lql".to_string(), + version: 1, + text: text.to_string(), + }, + }) + .await; + } + // ── format_lql ── #[test] @@ -1155,17 +1168,7 @@ mod tests { let uri = Url::parse("file:///test.lql").unwrap(); // Open a document - service - .inner() - .did_open(DidOpenTextDocumentParams { - text_document: TextDocumentItem { - uri: uri.clone(), - language_id: "lql".to_string(), - version: 1, - text: "users |> ".to_string(), - }, - }) - .await; + open_doc(service.inner(), &uri, "users |> ").await; // Request completions let params = CompletionParams { @@ -1191,17 +1194,7 @@ mod tests { let (service, _socket) = LspService::new(LqlBackend::new); let uri = Url::parse("file:///test.lql").unwrap(); - service - .inner() - .did_open(DidOpenTextDocumentParams { - text_document: TextDocumentItem { - uri: uri.clone(), - language_id: "lql".to_string(), - version: 1, - text: "select".to_string(), - }, - }) - .await; + open_doc(service.inner(), &uri, "select").await; let params = HoverParams { text_document_position_params: TextDocumentPositionParams { @@ -1223,17 +1216,7 @@ mod tests { let (service, _socket) = LspService::new(LqlBackend::new); let uri = Url::parse("file:///test.lql").unwrap(); - service - .inner() - .did_open(DidOpenTextDocumentParams { - text_document: TextDocumentItem { - uri: uri.clone(), - language_id: "lql".to_string(), - version: 1, - text: "let x = users\nlet y = orders".to_string(), - }, - }) - .await; + open_doc(service.inner(), &uri, "let x = users\nlet y = orders").await; let params = DocumentSymbolParams { text_document: TextDocumentIdentifier { uri: uri.clone() }, @@ -1254,17 +1237,7 @@ mod tests { let (service, _socket) = LspService::new(LqlBackend::new); let uri = Url::parse("file:///test.lql").unwrap(); - service - .inner() - .did_open(DidOpenTextDocumentParams { - text_document: TextDocumentItem { - uri: uri.clone(), - language_id: "lql".to_string(), - version: 1, - text: "users".to_string(), - }, - }) - .await; + open_doc(service.inner(), &uri, "users").await; service .inner() @@ -1299,17 +1272,7 @@ mod tests { let (service, _socket) = LspService::new(LqlBackend::new); let uri = Url::parse("file:///test.lql").unwrap(); - service - .inner() - .did_open(DidOpenTextDocumentParams { - text_document: TextDocumentItem { - uri: uri.clone(), - language_id: "lql".to_string(), - version: 1, - text: "users".to_string(), - }, - }) - .await; + open_doc(service.inner(), &uri, "users").await; service .inner() @@ -1335,17 +1298,7 @@ mod tests { let (service, _socket) = LspService::new(LqlBackend::new); let uri = Url::parse("file:///test.lql").unwrap(); - service - .inner() - .did_open(DidOpenTextDocumentParams { - text_document: TextDocumentItem { - uri: uri.clone(), - language_id: "lql".to_string(), - version: 1, - text: "users\n|> select(\nusers.id\n)".to_string(), - }, - }) - .await; + open_doc(service.inner(), &uri, "users\n|> select(\nusers.id\n)").await; let params = DocumentFormattingParams { text_document: TextDocumentIdentifier { uri }, @@ -1368,17 +1321,7 @@ mod tests { let (service, _socket) = LspService::new(LqlBackend::new); let uri = Url::parse("file:///test.lql").unwrap(); - service - .inner() - .did_open(DidOpenTextDocumentParams { - text_document: TextDocumentItem { - uri: uri.clone(), - language_id: "lql".to_string(), - version: 1, - text: "users\n".to_string(), - }, - }) - .await; + open_doc(service.inner(), &uri, "users\n").await; let params = DocumentFormattingParams { text_document: TextDocumentIdentifier { uri }, @@ -1398,17 +1341,7 @@ mod tests { let (service, _socket) = LspService::new(LqlBackend::new); let uri = Url::parse("file:///test.lql").unwrap(); - service - .inner() - .did_open(DidOpenTextDocumentParams { - text_document: TextDocumentItem { - uri: uri.clone(), - language_id: "lql".to_string(), - version: 1, - text: "zzz_unknown".to_string(), - }, - }) - .await; + open_doc(service.inner(), &uri, "zzz_unknown").await; let params = HoverParams { text_document_position_params: TextDocumentPositionParams { @@ -1426,17 +1359,7 @@ mod tests { let (service, _socket) = LspService::new(LqlBackend::new); let uri = Url::parse("file:///test.lql").unwrap(); - service - .inner() - .did_open(DidOpenTextDocumentParams { - text_document: TextDocumentItem { - uri: uri.clone(), - language_id: "lql".to_string(), - version: 1, - text: "users |> ".to_string(), - }, - }) - .await; + open_doc(service.inner(), &uri, "users |> ").await; let params = CompletionParams { text_document_position: TextDocumentPositionParams { @@ -1490,17 +1413,7 @@ mod tests { let (service, _socket) = LspService::new(LqlBackend::new); let uri = Url::parse("file:///test.lql").unwrap(); - service - .inner() - .did_open(DidOpenTextDocumentParams { - text_document: TextDocumentItem { - uri: uri.clone(), - language_id: "lql".to_string(), - version: 1, - text: "let my_query = users\nmy_query |> ".to_string(), - }, - }) - .await; + open_doc(service.inner(), &uri, "let my_query = users\nmy_query |> ").await; let params = CompletionParams { text_document_position: TextDocumentPositionParams { diff --git a/Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresCheckExpressionNormalizer.cs b/Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresCheckExpressionNormalizer.cs index 9fabb10..cb7cc73 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresCheckExpressionNormalizer.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Postgres/PostgresCheckExpressionNormalizer.cs @@ -137,6 +137,19 @@ private static bool IsRedundantCast(Expression.Cast cast) return $"{Render(any.Left)} IN ({string.Join(", ", literals)})"; } + private static string? RenderLiteralValue(Value value) + { + if (value is Value.SingleQuotedString s) + { + return $"'{s.Value.Replace("'", "''", StringComparison.Ordinal)}'"; + } + if (value is Value.Number n) + { + return n.Value; + } + return null; + } + private static string? ExtractStringLiteral(Expression element) { var unwrapped = element; @@ -148,15 +161,7 @@ private static bool IsRedundantCast(Expression.Cast cast) { return null; } - if (lit.Value is Value.SingleQuotedString s) - { - return $"'{s.Value.Replace("'", "''", StringComparison.Ordinal)}'"; - } - if (lit.Value is Value.Number n) - { - return n.Value; - } - return null; + return RenderLiteralValue(lit.Value); } private static string Render(Expression expression) @@ -171,13 +176,10 @@ private static string Render(Expression expression) } if (expression is Expression.LiteralValue lit) { - if (lit.Value is Value.SingleQuotedString s) - { - return $"'{s.Value.Replace("'", "''", StringComparison.Ordinal)}'"; - } - if (lit.Value is Value.Number n) + var rendered = RenderLiteralValue(lit.Value); + if (rendered is not null) { - return n.Value; + return rendered; } } if (expression is Expression.BinaryOp bin) diff --git a/Migration/Nimblesite.DataProvider.Migration.Tests/LqlDefaultsTests.cs b/Migration/Nimblesite.DataProvider.Migration.Tests/LqlDefaultsTests.cs index ef572b0..6bc7d69 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Tests/LqlDefaultsTests.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Tests/LqlDefaultsTests.cs @@ -684,7 +684,7 @@ public void LqlDefaults_Idempotent_BothPlatforms() // ASSERT 2: Table structure is EXACTLY what we defined // PostgreSQL: Verify table exists with correct columns - Assert.True(PostgresTableExists(_pgConnection, "rerunnable", "public")); + Assert.True(SchemaVerifier.PostgresTableExists(_pgConnection, "rerunnable", "public")); var pgColumns = GetPostgresColumns(_pgConnection, "rerunnable", "public"); Assert.Equal(3, pgColumns.Count); // id, active, count - NO EXTRA COLUMNS Assert.Contains("id", pgColumns); @@ -692,7 +692,7 @@ public void LqlDefaults_Idempotent_BothPlatforms() Assert.Contains("count", pgColumns); // SQLite: Verify table exists with correct columns - Assert.True(SqliteTableExists(_sqliteConnection, "rerunnable")); + Assert.True(SchemaVerifier.SqliteTableExists(_sqliteConnection, "rerunnable")); var sqliteColumns = GetSqliteColumns(_sqliteConnection, "rerunnable"); Assert.Equal(3, sqliteColumns.Count); // id, active, count - NO EXTRA COLUMNS Assert.Contains("id", sqliteColumns); @@ -1180,38 +1180,6 @@ private static string GetSqliteTableDdl(SqliteConnection conn, string tableName) return result?.ToString() ?? ""; } - /// - /// Check if a PostgreSQL table exists. - /// - private static bool PostgresTableExists( - NpgsqlConnection conn, - string tableName, - string schema = "public" - ) - { - using var cmd = conn.CreateCommand(); - cmd.CommandText = """ - SELECT EXISTS ( - SELECT 1 FROM information_schema.tables - WHERE table_schema = @schema AND table_name = @table - ) - """; - cmd.Parameters.AddWithValue("@schema", schema); - cmd.Parameters.AddWithValue("@table", tableName); - return (bool)cmd.ExecuteScalar()!; - } - - /// - /// Check if a SQLite table exists. - /// - private static bool SqliteTableExists(SqliteConnection conn, string tableName) - { - using var cmd = conn.CreateCommand(); - cmd.CommandText = "SELECT COUNT(*) FROM sqlite_master WHERE type='table' AND name=@name"; - cmd.Parameters.AddWithValue("@name", tableName); - return (long)cmd.ExecuteScalar()! > 0; - } - /// /// Get list of column names for a PostgreSQL table. /// diff --git a/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresCheckConstraintReplacementIssue57Tests.cs b/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresCheckConstraintReplacementIssue57Tests.cs index 2d9213d..4eed3a6 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresCheckConstraintReplacementIssue57Tests.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresCheckConstraintReplacementIssue57Tests.cs @@ -20,16 +20,22 @@ public async Task Calculate_TableCheckExpressionChanged_RegeneratesConstraintInP try { - Apply( + PostgresTestDb.Apply( connection, - Calculate(Inspect(connection), SchemaWithKindCheck("usage_events_kind_check_v1")) + PostgresTestDb.Calculate( + PostgresTestDb.Inspect(connection, Logger), + SchemaWithKindCheck("usage_events_kind_check_v1"), + Logger + ), + Logger ); Assert.True(LiveExpressionContains(connection, "input_tokens")); Assert.False(LiveExpressionContains(connection, "llm_input_tokens")); - var upgrade = Calculate( - Inspect(connection), - SchemaWithKindCheck("usage_events_kind_check_v2") + var upgrade = PostgresTestDb.Calculate( + PostgresTestDb.Inspect(connection, Logger), + SchemaWithKindCheck("usage_events_kind_check_v2"), + Logger ); Assert.Contains( upgrade, @@ -39,14 +45,15 @@ op is DropCheckConstraintOperation drop ); Assert.Contains(upgrade, op => op is AddCheckConstraintOperation); - Apply(connection, upgrade); + PostgresTestDb.Apply(connection, upgrade, Logger); Assert.True(LiveExpressionContains(connection, "llm_input_tokens")); Assert.False(LiveExpressionContains(connection, "'input_tokens'")); - var replay = Calculate( - Inspect(connection), - SchemaWithKindCheck("usage_events_kind_check_v2") + var replay = PostgresTestDb.Calculate( + PostgresTestDb.Inspect(connection, Logger), + SchemaWithKindCheck("usage_events_kind_check_v2"), + Logger ); Assert.DoesNotContain(replay, op => op is DropCheckConstraintOperation); Assert.DoesNotContain(replay, op => op is AddCheckConstraintOperation); @@ -106,52 +113,6 @@ private static SchemaDefinition SchemaWithKindCheck(string version) }; } - private static SchemaDefinition Inspect(NpgsqlConnection connection) - { - var result = PostgresSchemaInspector.Inspect(connection, SchemaName, Logger); - if (result is SchemaResultOk ok) - { - return ok.Value; - } - Assert.Fail("Expected PostgreSQL schema inspection to succeed."); - return new SchemaDefinition { Name = "failed", Tables = [] }; - } - - private static IReadOnlyList Calculate( - SchemaDefinition current, - SchemaDefinition desired - ) - { - var result = SchemaDiff.Calculate( - current: current, - desired: desired, - allowDestructive: false, - logger: Logger - ); - if (result is OperationsResultOk ok) - { - return ok.Value; - } - Assert.Fail("Expected PostgreSQL schema diff to succeed."); - return []; - } - - private static void Apply( - NpgsqlConnection connection, - IReadOnlyList operations - ) - { - var result = MigrationRunner.Apply( - connection: connection, - operations: operations, - generateDdl: PostgresDdlGenerator.Generate, - options: MigrationOptions.Default, - logger: Logger - ); - var failure = result is MigrationApplyResultError error ? error.Value.ToString() : ""; - Assert.True(result is MigrationApplyResultOk, $"Migration failed: {failure}"); - } - private static bool LiveExpressionContains(NpgsqlConnection connection, string needle) { using var command = connection.CreateCommand(); diff --git a/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresColumnCheckConstraintTests.cs b/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresColumnCheckConstraintTests.cs index 7c5b525..b5deda7 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresColumnCheckConstraintTests.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresColumnCheckConstraintTests.cs @@ -50,7 +50,15 @@ public async Task ApplyYaml_WhenColumnCheckConstraintHasName_CreatesNamedPostgre """ ); - Apply(connection, Calculate(Inspect(connection), schema)); + PostgresTestDb.Apply( + connection, + PostgresTestDb.Calculate( + PostgresTestDb.Inspect(connection, Logger), + schema, + Logger + ), + Logger + ); Assert.Equal( [ @@ -61,14 +69,20 @@ public async Task ApplyYaml_WhenColumnCheckConstraintHasName_CreatesNamedPostgre ConstraintNames(connection) ); - var inspected = Inspect(connection).Tables.Single(t => t.Name == "bundles"); + var inspected = PostgresTestDb + .Inspect(connection, Logger) + .Tables.Single(t => t.Name == "bundles"); var sha256 = inspected.Columns.Single(c => c.Name == "sha256"); var sizeBytes = inspected.Columns.Single(c => c.Name == "size_bytes"); Assert.Equal("bundles_sha256_fmt_chk", sha256.CheckConstraintName); Assert.Equal("bundles_size_bytes_nonneg_chk", sizeBytes.CheckConstraintName); Assert.DoesNotContain( - Calculate(Inspect(connection), schema), + PostgresTestDb.Calculate( + PostgresTestDb.Inspect(connection, Logger), + schema, + Logger + ), operation => operation is AddCheckConstraintOperation ); } @@ -79,50 +93,6 @@ public async Task ApplyYaml_WhenColumnCheckConstraintHasName_CreatesNamedPostgre } } - private static SchemaDefinition Inspect(NpgsqlConnection connection) - { - var result = PostgresSchemaInspector.Inspect(connection, SchemaName, Logger); - if (result is SchemaResultOk ok) - { - return ok.Value; - } - - Assert.Fail("Expected PostgreSQL schema inspection to succeed."); - return Schema.Define("failed").Build(); - } - - private static IReadOnlyList Calculate( - SchemaDefinition current, - SchemaDefinition desired - ) - { - var result = SchemaDiff.Calculate(current, desired, logger: Logger); - if (result is OperationsResultOk ok) - { - return ok.Value; - } - - Assert.Fail("Expected PostgreSQL schema diff to succeed."); - return []; - } - - private static void Apply( - NpgsqlConnection connection, - IReadOnlyList operations - ) - { - var result = MigrationRunner.Apply( - connection, - operations, - PostgresDdlGenerator.Generate, - MigrationOptions.Default, - Logger - ); - var failure = result is MigrationApplyResultError error ? error.Value.ToString() : ""; - - Assert.True(result is MigrationApplyResultOk, $"Migration failed: {failure}"); - } - private static string[] ConstraintNames(NpgsqlConnection connection) { using var command = connection.CreateCommand(); diff --git a/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresDropConstraintBackedIndexTests.cs b/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresDropConstraintBackedIndexTests.cs index 0c0a048..0d58e92 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresDropConstraintBackedIndexTests.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresDropConstraintBackedIndexTests.cs @@ -18,12 +18,25 @@ public async Task DropIndex_WhenIndexBacksUniqueConstraint_DropsConstraint() try { - Apply(connection, Calculate(Inspect(connection), SchemaWithUniqueConstraint())); - var upgrade = Calculate(Inspect(connection), SchemaWithoutUniqueConstraint(), true); + PostgresTestDb.Apply( + connection, + PostgresTestDb.Calculate( + PostgresTestDb.Inspect(connection, Logger), + SchemaWithUniqueConstraint(), + Logger + ), + Logger + ); + var upgrade = PostgresTestDb.Calculate( + PostgresTestDb.Inspect(connection, Logger), + SchemaWithoutUniqueConstraint(), + Logger, + allowDestructive: true + ); Assert.Contains(upgrade, IsConstraintBackedIndexDrop); - Apply(connection, upgrade, MigrationOptions.Destructive); + PostgresTestDb.Apply(connection, upgrade, Logger, MigrationOptions.Destructive); Assert.False(ConstraintExists(connection)); Assert.False(IndexExists(connection)); @@ -47,10 +60,25 @@ public async Task Calculate_WhenUniqueConstraintSchemaConverged_DoesNotDropBacki { var desired = SchemaWithUniqueConstraint(); - Apply(connection, Calculate(Inspect(connection), desired)); + PostgresTestDb.Apply( + connection, + PostgresTestDb.Calculate( + PostgresTestDb.Inspect(connection, Logger), + desired, + Logger + ), + Logger + ); - var inspected = Inspect(connection).Tables.Single(t => t.Name == TableName); - var converged = Calculate(Inspect(connection), desired, true); + var inspected = PostgresTestDb + .Inspect(connection, Logger) + .Tables.Single(t => t.Name == TableName); + var converged = PostgresTestDb.Calculate( + PostgresTestDb.Inspect(connection, Logger), + desired, + Logger, + allowDestructive: true + ); Assert.Contains( inspected.UniqueConstraints, @@ -64,7 +92,7 @@ public async Task Calculate_WhenUniqueConstraintSchemaConverged_DoesNotDropBacki operation => operation is AddUniqueConstraintOperation ); - Apply(connection, converged, MigrationOptions.Destructive); + PostgresTestDb.Apply(connection, converged, Logger, MigrationOptions.Destructive); Assert.True(ConstraintExists(connection)); Assert.True(IndexExists(connection)); @@ -101,52 +129,6 @@ private static SchemaDefinition SchemaWithoutUniqueConstraint() => ) .Build(); - private static SchemaDefinition Inspect(NpgsqlConnection connection) - { - var result = PostgresSchemaInspector.Inspect(connection, SchemaName, Logger); - if (result is SchemaResultOk ok) - { - return ok.Value; - } - - Assert.Fail("Expected PostgreSQL schema inspection to succeed."); - return Schema.Define("failed").Build(); - } - - private static IReadOnlyList Calculate( - SchemaDefinition current, - SchemaDefinition desired, - bool allowDestructive = false - ) - { - var result = SchemaDiff.Calculate(current, desired, allowDestructive, Logger); - if (result is OperationsResultOk ok) - { - return ok.Value; - } - - Assert.Fail("Expected PostgreSQL schema diff to succeed."); - return []; - } - - private static void Apply( - NpgsqlConnection connection, - IReadOnlyList operations, - MigrationOptions? options = null - ) - { - var result = MigrationRunner.Apply( - connection, - operations, - PostgresDdlGenerator.Generate, - options ?? MigrationOptions.Default, - Logger - ); - var failure = result is MigrationApplyResultError error ? error.Value.ToString() : ""; - - Assert.True(result is MigrationApplyResultOk, $"Migration failed: {failure}"); - } - private static bool IsConstraintBackedIndexDrop(SchemaOperation operation) => operation is DropIndexOperation diff --git a/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresGrantRunAsE2ETests.cs b/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresGrantRunAsE2ETests.cs index 1103245..a3dfef5 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresGrantRunAsE2ETests.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresGrantRunAsE2ETests.cs @@ -6,23 +6,23 @@ public sealed class PostgresGrantRunAsE2ETests(PostgresContainerFixture fixture) [Fact] public void GrantRunAs_AppliesNapAuthShapeGrantsThroughSchemaOwner() { - var suffix = Guid.NewGuid().ToString("N")[..8]; - var owner = $"grant_owner_{suffix}"; - var migrate = $"grant_migrate_{suffix}"; - var appUser = $"grant_app_user_{suffix}"; + var (suffix, owner, migrate, appUser) = NewGrantRoles(); var appAdmin = $"grant_app_admin_{suffix}"; var schema = $"auth_{suffix}"; using var connection = fixture.CreateDatabase("grant_run_as"); - Exec(connection, $"CREATE ROLE {Q(owner)} NOLOGIN"); - Exec(connection, $"CREATE ROLE {Q(migrate)} LOGIN PASSWORD 'test'"); - Exec(connection, $"CREATE ROLE {Q(appUser)} NOLOGIN"); - Exec(connection, $"CREATE ROLE {Q(appAdmin)} NOLOGIN"); - Exec(connection, $"GRANT {Q(owner)} TO {Q("test")}"); - Exec(connection, $"GRANT {Q(owner)} TO {Q(migrate)}"); - Exec(connection, $"GRANT CONNECT ON DATABASE {Q(connection.Database)} TO {Q(migrate)}"); - Exec(connection, $"CREATE SCHEMA {Q(schema)} AUTHORIZATION {Q(owner)}"); - Exec( + PostgresTestDb.Exec(connection, $"CREATE ROLE {Q(owner)} NOLOGIN"); + PostgresTestDb.Exec(connection, $"CREATE ROLE {Q(migrate)} LOGIN PASSWORD 'test'"); + PostgresTestDb.Exec(connection, $"CREATE ROLE {Q(appUser)} NOLOGIN"); + PostgresTestDb.Exec(connection, $"CREATE ROLE {Q(appAdmin)} NOLOGIN"); + PostgresTestDb.Exec(connection, $"GRANT {Q(owner)} TO {Q("test")}"); + PostgresTestDb.Exec(connection, $"GRANT {Q(owner)} TO {Q(migrate)}"); + PostgresTestDb.Exec( + connection, + $"GRANT CONNECT ON DATABASE {Q(connection.Database)} TO {Q(migrate)}" + ); + PostgresTestDb.Exec(connection, $"CREATE SCHEMA {Q(schema)} AUTHORIZATION {Q(owner)}"); + PostgresTestDb.Exec( connection, $"SET ROLE {Q(owner)}; CREATE TABLE {Q(schema)}.{Q("users")}(id uuid PRIMARY KEY); RESET ROLE" ); @@ -46,16 +46,16 @@ public void GrantRunAs_AppliesNapAuthShapeGrantsThroughSchemaOwner() [Fact] public void GrantRunAs_MissingRoleMembership_ReturnsClearGrantToMessage() { - var suffix = Guid.NewGuid().ToString("N")[..8]; - var owner = $"grant_owner_{suffix}"; - var migrate = $"grant_migrate_{suffix}"; - var appUser = $"grant_app_user_{suffix}"; + var (_, owner, migrate, appUser) = NewGrantRoles(); using var connection = fixture.CreateDatabase("grant_run_as_missing"); - Exec(connection, $"CREATE ROLE {Q(owner)} NOLOGIN"); - Exec(connection, $"CREATE ROLE {Q(migrate)} LOGIN PASSWORD 'test'"); - Exec(connection, $"CREATE ROLE {Q(appUser)} NOLOGIN"); - Exec(connection, $"GRANT CONNECT ON DATABASE {Q(connection.Database)} TO {Q(migrate)}"); + PostgresTestDb.Exec(connection, $"CREATE ROLE {Q(owner)} NOLOGIN"); + PostgresTestDb.Exec(connection, $"CREATE ROLE {Q(migrate)} LOGIN PASSWORD 'test'"); + PostgresTestDb.Exec(connection, $"CREATE ROLE {Q(appUser)} NOLOGIN"); + PostgresTestDb.Exec( + connection, + $"GRANT CONNECT ON DATABASE {Q(connection.Database)} TO {Q(migrate)}" + ); using var migrateConnection = OpenRoleConnection(connection, migrate); var result = MigrationRunner.Apply( @@ -83,6 +83,17 @@ public void GrantRunAs_MissingRoleMembership_ReturnsClearGrantToMessage() Assert.Contains($"GRANT \"{owner}\" TO \"{migrate}\"", error.Message); } + private static (string Suffix, string Owner, string Migrate, string AppUser) NewGrantRoles() + { + var suffix = Guid.NewGuid().ToString("N")[..8]; + return ( + suffix, + $"grant_owner_{suffix}", + $"grant_migrate_{suffix}", + $"grant_app_user_{suffix}" + ); + } + private static IReadOnlyList NapAuthGrants( string schema, string owner, @@ -168,13 +179,6 @@ private static bool ScalarBool( return command.ExecuteScalar() is true; } - private static void Exec(NpgsqlConnection connection, string sql) - { - using var command = connection.CreateCommand(); - command.CommandText = sql; - command.ExecuteNonQuery(); - } - private static NpgsqlConnection OpenRoleConnection( NpgsqlConnection adminConnection, string role diff --git a/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresLqlOnlyE2ETests.cs b/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresLqlOnlyE2ETests.cs index 5693b69..24ccd77 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresLqlOnlyE2ETests.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresLqlOnlyE2ETests.cs @@ -40,7 +40,7 @@ public async Task DisposeAsync() /// private static void BootstrapRolesAndGucReaders(NpgsqlConnection conn) { - Exec( + PostgresTestDb.Exec( conn, """ DO $$ BEGIN @@ -68,7 +68,7 @@ SELECT NULLIF(current_setting('rls.tenant_id', true), '')::uuid /// membership fns that reference tenant_members. /// private static void BootstrapMembershipFns(NpgsqlConnection conn) => - Exec( + PostgresTestDb.Exec( conn, """ CREATE OR REPLACE FUNCTION is_member(u uuid, t uuid) RETURNS bool @@ -103,7 +103,7 @@ private void ApplyAndGrant(SchemaDefinition desired, params string[] tableNames) foreach (var t in tableNames) { - Exec( + PostgresTestDb.Exec( _connection, $"GRANT USAGE ON SCHEMA public TO lql_user, lql_admin; " + $"GRANT SELECT,INSERT,UPDATE,DELETE ON \"public\".\"{t}\" TO lql_user, lql_admin" @@ -119,9 +119,17 @@ private static void SetSession( Guid? user ) { - Exec(conn, tx, $"SET LOCAL ROLE {role}"); - Exec(conn, tx, $"SET LOCAL rls.tenant_id = '{tenant?.ToString() ?? string.Empty}'"); - Exec(conn, tx, $"SET LOCAL rls.user_id = '{user?.ToString() ?? string.Empty}'"); + PostgresTestDb.Exec(conn, tx, $"SET LOCAL ROLE {role}"); + PostgresTestDb.Exec( + conn, + tx, + $"SET LOCAL rls.tenant_id = '{tenant?.ToString() ?? string.Empty}'" + ); + PostgresTestDb.Exec( + conn, + tx, + $"SET LOCAL rls.user_id = '{user?.ToString() ?? string.Empty}'" + ); } private static SchemaDefinition TenantMembersSchema() => @@ -263,11 +271,11 @@ public void LqlOnly_TenantIsolation_RealCrossUserBlock() var userB = Guid.NewGuid(); // Membership rows. - Exec( + PostgresTestDb.Exec( _connection, $"INSERT INTO tenant_members(id, user_id, tenant_id) VALUES ('{Guid.NewGuid()}', '{userA}', '{tenantA}')" ); - Exec( + PostgresTestDb.Exec( _connection, $"INSERT INTO tenant_members(id, user_id, tenant_id) VALUES ('{Guid.NewGuid()}', '{userB}', '{tenantB}')" ); @@ -277,7 +285,7 @@ public void LqlOnly_TenantIsolation_RealCrossUserBlock() using (var tx = _connection.BeginTransaction()) { SetSession(_connection, tx, "lql_user", tenantA, userA); - Exec( + PostgresTestDb.Exec( _connection, tx, $"INSERT INTO docs_iso(id, tenant_id, title) VALUES ('{docA}', '{tenantA}', 'a')" @@ -290,7 +298,7 @@ public void LqlOnly_TenantIsolation_RealCrossUserBlock() using (var tx = _connection.BeginTransaction()) { SetSession(_connection, tx, "lql_user", tenantB, userB); - Exec( + PostgresTestDb.Exec( _connection, tx, $"INSERT INTO docs_iso(id, tenant_id, title) VALUES ('{docB}', '{tenantB}', 'b')" @@ -351,12 +359,12 @@ public void LqlOnly_Admin_SeesAllTenants() using (var tx = _connection.BeginTransaction()) { SetSession(_connection, tx, "lql_admin", null, null); - Exec( + PostgresTestDb.Exec( _connection, tx, $"INSERT INTO docs_admin(id, tenant_id, title) VALUES ('{Guid.NewGuid()}', '{t1}', 'x')" ); - Exec( + PostgresTestDb.Exec( _connection, tx, $"INSERT INTO docs_admin(id, tenant_id, title) VALUES ('{Guid.NewGuid()}', '{t2}', 'y')" @@ -634,11 +642,11 @@ public void LqlOnly_NapMessagesShape_BareFnCallInLambdaBody_EnforcesIsolation() var tenantB = Guid.NewGuid(); var userA = Guid.NewGuid(); var userB = Guid.NewGuid(); - Exec( + PostgresTestDb.Exec( _connection, $"INSERT INTO tenant_members(id, user_id, tenant_id) VALUES ('{Guid.NewGuid()}', '{userA}', '{tenantA}')" ); - Exec( + PostgresTestDb.Exec( _connection, $"INSERT INTO tenant_members(id, user_id, tenant_id) VALUES ('{Guid.NewGuid()}', '{userB}', '{tenantB}')" ); @@ -646,11 +654,11 @@ public void LqlOnly_NapMessagesShape_BareFnCallInLambdaBody_EnforcesIsolation() // Conversations: convA in tenantA, convB in tenantB. var convA = Guid.NewGuid(); var convB = Guid.NewGuid(); - Exec( + PostgresTestDb.Exec( _connection, $"INSERT INTO conversations(id, tenant_id) VALUES ('{convA}', '{tenantA}')" ); - Exec( + PostgresTestDb.Exec( _connection, $"INSERT INTO conversations(id, tenant_id) VALUES ('{convB}', '{tenantB}')" ); @@ -660,7 +668,7 @@ public void LqlOnly_NapMessagesShape_BareFnCallInLambdaBody_EnforcesIsolation() using (var tx = _connection.BeginTransaction()) { SetSession(_connection, tx, "lql_user", tenantA, userA); - Exec( + PostgresTestDb.Exec( _connection, tx, $"INSERT INTO messages(id, conversation_id, body) VALUES ('{msgA}', '{convA}', 'hi')" @@ -743,19 +751,4 @@ public void LqlOnly_DropPolicy_AllowDestructive_RemovesIt() cmd.CommandText = "SELECT COUNT(*) FROM pg_policies WHERE tablename='docs_drop'"; Assert.Equal(0L, (long)cmd.ExecuteScalar()!); } - - private static void Exec(NpgsqlConnection conn, string sql) - { - using var cmd = conn.CreateCommand(); - cmd.CommandText = sql; - cmd.ExecuteNonQuery(); - } - - private static void Exec(NpgsqlConnection conn, NpgsqlTransaction tx, string sql) - { - using var cmd = conn.CreateCommand(); - cmd.Transaction = tx; - cmd.CommandText = sql; - cmd.ExecuteNonQuery(); - } } diff --git a/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresMigrationTests.cs b/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresMigrationTests.cs index 227bdbe..8351f9a 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresMigrationTests.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresMigrationTests.cs @@ -106,9 +106,7 @@ public void CreateDatabaseFromScratch_MultipleTablesWithForeignKeys_Success() .Build(); // Act - var emptySchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var emptySchema = InspectPublic(); var operations = ( (OperationsResultOk)SchemaDiff.Calculate(emptySchema, schema, logger: _logger) @@ -128,9 +126,7 @@ public void CreateDatabaseFromScratch_MultipleTablesWithForeignKeys_Success() $"Migration failed: {(result as MigrationApplyResultError)?.Value}" ); - var inspected = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var inspected = InspectPublic(); Assert.Contains(inspected.Tables, t => t.Name == "customers"); Assert.Contains(inspected.Tables, t => t.Name == "invoices"); @@ -154,9 +150,7 @@ public void UpgradeExistingDatabase_AddColumn_Success() ) .Build(); - var emptySchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var emptySchema = InspectPublic(); var v1Ops = ( (OperationsResultOk)SchemaDiff.Calculate(emptySchema, v1, logger: _logger) ).Value; @@ -183,9 +177,7 @@ public void UpgradeExistingDatabase_AddColumn_Success() .Build(); // Act - var currentSchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var currentSchema = InspectPublic(); var upgradeOps = ( (OperationsResultOk)SchemaDiff.Calculate(currentSchema, v2, logger: _logger) @@ -206,9 +198,7 @@ public void UpgradeExistingDatabase_AddColumn_Success() // Assert Assert.True(result is MigrationApplyResultOk); - var finalSchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var finalSchema = InspectPublic(); var products = finalSchema.Tables.Single(t => t.Name == "products"); Assert.Equal(4, products.Columns.Count); } @@ -228,9 +218,7 @@ public void UpgradeExistingDatabase_AddTable_Success() ) .Build(); - var emptySchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var emptySchema = InspectPublic(); var v1Ops = ( (OperationsResultOk)SchemaDiff.Calculate(emptySchema, v1, logger: _logger) ).Value; @@ -264,9 +252,7 @@ public void UpgradeExistingDatabase_AddTable_Success() .Build(); // Act - var currentSchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var currentSchema = InspectPublic(); var upgradeOps = ( (OperationsResultOk)SchemaDiff.Calculate(currentSchema, v2, logger: _logger) @@ -286,9 +272,7 @@ public void UpgradeExistingDatabase_AddTable_Success() // Assert Assert.True(result is MigrationApplyResultOk); - var finalSchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var finalSchema = InspectPublic(); Assert.Contains(finalSchema.Tables, t => t.Name == "items"); } @@ -308,9 +292,7 @@ public void UpgradeExistingDatabase_AddIndex_Success() ) .Build(); - var emptySchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var emptySchema = InspectPublic(); var v1Ops = ( (OperationsResultOk)SchemaDiff.Calculate(emptySchema, v1, logger: _logger) ).Value; @@ -337,9 +319,7 @@ public void UpgradeExistingDatabase_AddIndex_Success() .Build(); // Act - var currentSchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var currentSchema = InspectPublic(); var upgradeOps = ( (OperationsResultOk)SchemaDiff.Calculate(currentSchema, v2, logger: _logger) @@ -359,9 +339,7 @@ public void UpgradeExistingDatabase_AddIndex_Success() // Assert Assert.True(result is MigrationApplyResultOk); - var finalSchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var finalSchema = InspectPublic(); var logs = finalSchema.Tables.Single(t => t.Name == "logs"); Assert.Contains(logs.Indexes, i => i.Name == "idx_logs_level"); } @@ -384,9 +362,7 @@ public void Migration_IsIdempotent_NoErrorOnRerun() // Act - Run migration twice for (var i = 0; i < 2; i++) { - var currentSchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var currentSchema = InspectPublic(); var operations = ( (OperationsResultOk)SchemaDiff.Calculate(currentSchema, schema, logger: _logger) @@ -430,9 +406,7 @@ public void CreateTable_PostgresNativeTypes_Success() .Build(); // Act - var emptySchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var emptySchema = InspectPublic(); var operations = ( (OperationsResultOk)SchemaDiff.Calculate(emptySchema, schema, logger: _logger) ).Value; @@ -487,9 +461,7 @@ public void CreateTable_MixedCaseColumnCheckConstraint_PreservesIdentifierCase() ) .Build(); - var current = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var current = InspectPublic(); var operations = ( (OperationsResultOk)SchemaDiff.Calculate(current, schema, logger: _logger) ).Value; @@ -528,9 +500,7 @@ public void ExpressionIndex_CreateWithLowerFunction_Success() .Build(); // Act - var emptySchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var emptySchema = InspectPublic(); var operations = ( (OperationsResultOk)SchemaDiff.Calculate(emptySchema, schema, logger: _logger) ).Value; @@ -584,9 +554,7 @@ public void ExpressionIndex_EnforcesCaseInsensitiveUniqueness() ) .Build(); - var emptySchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var emptySchema = InspectPublic(); var operations = ( (OperationsResultOk)SchemaDiff.Calculate(emptySchema, schema, logger: _logger) ).Value; @@ -650,9 +618,7 @@ public void ExpressionIndex_MultiExpression_CompositeIndexSuccess() .Build(); // Act - var emptySchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var emptySchema = InspectPublic(); var operations = ( (OperationsResultOk)SchemaDiff.Calculate(emptySchema, schema, logger: _logger) ).Value; @@ -723,9 +689,7 @@ public void ExpressionIndex_MultiExpression_AllowsSameNameDifferentSuburb() ) .Build(); - var emptySchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var emptySchema = InspectPublic(); var operations = ( (OperationsResultOk)SchemaDiff.Calculate(emptySchema, schema, logger: _logger) ).Value; @@ -790,9 +754,7 @@ public void ExpressionIndex_Idempotent_NoErrorOnRerun() // but CREATE INDEX IF NOT EXISTS ensures idempotency at the database level for (var i = 0; i < 2; i++) { - var currentSchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var currentSchema = InspectPublic(); var operations = ( (OperationsResultOk)SchemaDiff.Calculate(currentSchema, schema, logger: _logger) @@ -845,9 +807,7 @@ public void UpgradeIndex_ColumnToExpression_RequiresDropAndCreate() .Build(); // Apply v1 - var emptySchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var emptySchema = InspectPublic(); var v1Ops = ( (OperationsResultOk)SchemaDiff.Calculate(emptySchema, v1, logger: _logger) ).Value; @@ -873,9 +833,7 @@ public void UpgradeIndex_ColumnToExpression_RequiresDropAndCreate() .Build(); // Act - Calculate upgrade operations - var currentSchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var currentSchema = InspectPublic(); var upgradeOps = ( (OperationsResultOk) SchemaDiff.Calculate(currentSchema, v2, allowDestructive: true, logger: _logger) @@ -928,9 +886,7 @@ public void UpgradeIndex_ExpressionToColumn_RequiresDropAndCreate() .Build(); // Apply v1 - var emptySchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var emptySchema = InspectPublic(); var v1Ops = ( (OperationsResultOk)SchemaDiff.Calculate(emptySchema, v1, logger: _logger) ).Value; @@ -956,9 +912,7 @@ public void UpgradeIndex_ExpressionToColumn_RequiresDropAndCreate() .Build(); // Act - var currentSchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var currentSchema = InspectPublic(); var upgradeOps = ( (OperationsResultOk) SchemaDiff.Calculate(currentSchema, v2, allowDestructive: true, logger: _logger) @@ -1013,9 +967,7 @@ public void UpgradeIndex_SameNameDifferentType_NotDetectedWithoutDestructive() ) .Build(); - var emptySchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var emptySchema = InspectPublic(); var v1Ops = ( (OperationsResultOk)SchemaDiff.Calculate(emptySchema, v1, logger: _logger) ).Value; @@ -1042,9 +994,7 @@ public void UpgradeIndex_SameNameDifferentType_NotDetectedWithoutDestructive() .Build(); // Act - Calculate without destructive flag - var currentSchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var currentSchema = InspectPublic(); var upgradeOps = ( (OperationsResultOk) SchemaDiff.Calculate(currentSchema, v2, allowDestructive: false, logger: _logger) @@ -1069,9 +1019,7 @@ public void Destructive_DropTable_AllowedWithOption() .Table("public", "dropme", t => t.Column("id", PortableTypes.Uuid, c => c.PrimaryKey())) .Build(); - var emptySchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var emptySchema = InspectPublic(); var v1Ops = ( (OperationsResultOk)SchemaDiff.Calculate(emptySchema, v1, logger: _logger) ).Value; @@ -1094,9 +1042,7 @@ public void Destructive_DropTable_AllowedWithOption() .Build(); // Act - var currentSchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var currentSchema = InspectPublic(); var operations = ( (OperationsResultOk) @@ -1117,9 +1063,7 @@ public void Destructive_DropTable_AllowedWithOption() // Assert Assert.True(result is MigrationApplyResultOk); - var finalSchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var finalSchema = InspectPublic(); Assert.DoesNotContain(finalSchema.Tables, t => t.Name == "dropme"); Assert.Contains(finalSchema.Tables, t => t.Name == "keepers"); } @@ -1148,9 +1092,7 @@ public void LqlDefault_NowFunction_GeneratesCurrentTimestamp() .Build(); // Act - var emptySchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var emptySchema = InspectPublic(); var operations = ( (OperationsResultOk)SchemaDiff.Calculate(emptySchema, schema, logger: _logger) ).Value; @@ -1197,9 +1139,7 @@ public void LqlDefault_GenUuidFunction_GeneratesValidUuid() .Build(); // Act - var emptySchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var emptySchema = InspectPublic(); var operations = ( (OperationsResultOk)SchemaDiff.Calculate(emptySchema, schema, logger: _logger) ).Value; @@ -1265,9 +1205,7 @@ public void LqlDefault_BooleanTrue_GeneratesCorrectValue() .Build(); // Act - var emptySchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var emptySchema = InspectPublic(); var operations = ( (OperationsResultOk)SchemaDiff.Calculate(emptySchema, schema, logger: _logger) ).Value; @@ -1319,9 +1257,7 @@ public void LqlDefault_NumericLiterals_GeneratesCorrectValues() .Build(); // Act - var emptySchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var emptySchema = InspectPublic(); var operations = ( (OperationsResultOk)SchemaDiff.Calculate(emptySchema, schema, logger: _logger) ).Value; @@ -1377,9 +1313,7 @@ public void LqlDefault_StringLiteral_GeneratesCorrectValue() .Build(); // Act - var emptySchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var emptySchema = InspectPublic(); var operations = ( (OperationsResultOk)SchemaDiff.Calculate(emptySchema, schema, logger: _logger) ).Value; @@ -1447,9 +1381,7 @@ public void LqlDefault_AllTypesInOneTable_WorksTogether() .Build(); // Act - var emptySchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var emptySchema = InspectPublic(); var operations = ( (OperationsResultOk)SchemaDiff.Calculate(emptySchema, schema, logger: _logger) ).Value; @@ -1525,9 +1457,7 @@ public void CreateTableWithVectorColumn_MigratesAgainstPgvectorContainer_Success // If the source-side fix is still in progress, this test will // fail loudly on CREATE TABLE, proving the gap. - var emptySchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var emptySchema = InspectPublic(); var operations = ( (OperationsResultOk)SchemaDiff.Calculate(emptySchema, schema, logger: _logger) @@ -1549,9 +1479,7 @@ public void CreateTableWithVectorColumn_MigratesAgainstPgvectorContainer_Success ); // Assert — documents table exists - var inspected = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var inspected = InspectPublic(); Assert.Contains(inspected.Tables, t => t.Name == "documents"); // Assert — pgvector extension is installed in the database @@ -1622,9 +1550,7 @@ public void CreateTableWithVectorColumn_OpenAiLargeDim_Success() ) .Build(); - var emptySchema = ( - (SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger) - ).Value; + var emptySchema = InspectPublic(); var operations = ( (OperationsResultOk)SchemaDiff.Calculate(emptySchema, schema, logger: _logger) ).Value; @@ -1662,4 +1588,7 @@ private void InsertPatientGender(string gender) command.Parameters.AddWithValue("@gender", gender); command.ExecuteNonQuery(); } + + private SchemaDefinition InspectPublic() => + ((SchemaResultOk)PostgresSchemaInspector.Inspect(_connection, "public", _logger)).Value; } diff --git a/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresRlsNapShapeTests.cs b/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresRlsNapShapeTests.cs index da5a08e..7e0fb2e 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresRlsNapShapeTests.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresRlsNapShapeTests.cs @@ -38,7 +38,7 @@ public async Task DisposeAsync() /// private static void BootstrapNapPrelude(NpgsqlConnection conn) { - Exec( + PostgresTestDb.Exec( conn, """ DO $$ BEGIN @@ -154,7 +154,7 @@ private void ApplyAndGrant(SchemaDefinition desired) // own bootstrap; we replicate it here to make the test runnable). foreach (var t in desired.Tables) { - Exec( + PostgresTestDb.Exec( _connection, $"GRANT USAGE ON SCHEMA public TO nap_app_user, nap_app_admin; " + $"GRANT SELECT,INSERT,UPDATE,DELETE ON \"public\".\"{t.Name}\" TO nap_app_user, nap_app_admin" @@ -170,9 +170,17 @@ private static void SetSession( Guid? user ) { - Exec(conn, tx, $"SET LOCAL ROLE {role}"); - Exec(conn, tx, $"SET LOCAL rls.tenant_id = '{tenant?.ToString() ?? string.Empty}'"); - Exec(conn, tx, $"SET LOCAL rls.user_id = '{user?.ToString() ?? string.Empty}'"); + PostgresTestDb.Exec(conn, tx, $"SET LOCAL ROLE {role}"); + PostgresTestDb.Exec( + conn, + tx, + $"SET LOCAL rls.tenant_id = '{tenant?.ToString() ?? string.Empty}'" + ); + PostgresTestDb.Exec( + conn, + tx, + $"SET LOCAL rls.user_id = '{user?.ToString() ?? string.Empty}'" + ); } [Fact] @@ -213,7 +221,7 @@ public void NapShape_TenantIsolation_AppUserBlockedAcrossTenants() using (var tx = _connection.BeginTransaction()) { SetSession(_connection, tx, "nap_app_user", tenantA, userA); - Exec( + PostgresTestDb.Exec( _connection, tx, $"INSERT INTO public.agent_configs(id, tenant_id, title) VALUES ('{configA}', '{tenantA}', 'a')" @@ -226,7 +234,7 @@ public void NapShape_TenantIsolation_AppUserBlockedAcrossTenants() using (var tx = _connection.BeginTransaction()) { SetSession(_connection, tx, "nap_app_user", tenantB, userB); - Exec( + PostgresTestDb.Exec( _connection, tx, $"INSERT INTO public.agent_configs(id, tenant_id, title) VALUES ('{configB}', '{tenantB}', 'b')" @@ -263,12 +271,12 @@ public void NapShape_AdminRole_SeesEverything() using (var tx = _connection.BeginTransaction()) { SetSession(_connection, tx, "nap_app_admin", null, null); - Exec( + PostgresTestDb.Exec( _connection, tx, $"INSERT INTO public.agent_configs(id, tenant_id, title) VALUES ('{Guid.NewGuid()}', '{tenantA}', 'admin1')" ); - Exec( + PostgresTestDb.Exec( _connection, tx, $"INSERT INTO public.agent_configs(id, tenant_id, title) VALUES ('{Guid.NewGuid()}', '{tenantB}', 'admin2')" @@ -518,7 +526,7 @@ public void NapShape_Stress_HundredRowsAcrossTenants_PerUserCountsCorrect() var tenant = tenants[i % tenants.Count]; using var tx = _connection.BeginTransaction(); SetSession(_connection, tx, "nap_app_user", tenant, Guid.NewGuid()); - Exec( + PostgresTestDb.Exec( _connection, tx, $"INSERT INTO public.agent_configs(id, tenant_id, title) VALUES ('{Guid.NewGuid()}', '{tenant}', 'row{i}')" @@ -538,19 +546,4 @@ public void NapShape_Stress_HundredRowsAcrossTenants_PerUserCountsCorrect() Assert.Equal(inserted[tenant], (int)seen); } } - - private static void Exec(NpgsqlConnection conn, string sql) - { - using var cmd = conn.CreateCommand(); - cmd.CommandText = sql; - cmd.ExecuteNonQuery(); - } - - private static void Exec(NpgsqlConnection conn, NpgsqlTransaction tx, string sql) - { - using var cmd = conn.CreateCommand(); - cmd.Transaction = tx; - cmd.CommandText = sql; - cmd.ExecuteNonQuery(); - } } diff --git a/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresTestDb.cs b/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresTestDb.cs index 9a6321a..0063d3c 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresTestDb.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresTestDb.cs @@ -6,6 +6,8 @@ namespace Nimblesite.DataProvider.Migration.Tests; /// internal static class PostgresTestDb { + private const string SchemaName = "public"; + public static void ApplySchema( NpgsqlConnection connection, SchemaDefinition desired, @@ -32,4 +34,72 @@ public static void ApplySchema( var error = apply is MigrationApplyResultError failure ? failure.Value.Message : null; Assert.True(apply is MigrationApplyResultOk, $"Migration failed: {error}"); } + + /// Executes a non-query SQL statement on the connection. + internal static void Exec(NpgsqlConnection conn, string sql) + { + using var command = conn.CreateCommand(); + command.CommandText = sql; + command.ExecuteNonQuery(); + } + + /// Executes a non-query SQL statement within the given transaction. + internal static void Exec(NpgsqlConnection conn, NpgsqlTransaction tx, string sql) + { + using var command = conn.CreateCommand(); + command.Transaction = tx; + command.CommandText = sql; + command.ExecuteNonQuery(); + } + + /// Inspects the live PostgreSQL schema, asserting success. + internal static SchemaDefinition Inspect(NpgsqlConnection connection, ILogger logger) + { + var result = PostgresSchemaInspector.Inspect(connection, SchemaName, logger); + if (result is SchemaResultOk ok) + { + return ok.Value; + } + + Assert.Fail("Expected PostgreSQL schema inspection to succeed."); + return new SchemaDefinition { Name = "failed", Tables = [] }; + } + + /// Calculates the schema diff between current and desired, asserting success. + internal static IReadOnlyList Calculate( + SchemaDefinition current, + SchemaDefinition desired, + ILogger logger, + bool allowDestructive = false + ) + { + var result = SchemaDiff.Calculate(current, desired, allowDestructive, logger); + if (result is OperationsResultOk ok) + { + return ok.Value; + } + + Assert.Fail("Expected PostgreSQL schema diff to succeed."); + return []; + } + + /// Applies the operations against PostgreSQL, asserting the migration succeeded. + internal static void Apply( + NpgsqlConnection connection, + IReadOnlyList operations, + ILogger logger, + MigrationOptions? options = null + ) + { + var result = MigrationRunner.Apply( + connection, + operations, + PostgresDdlGenerator.Generate, + options ?? MigrationOptions.Default, + logger + ); + var failure = result is MigrationApplyResultError error ? error.Value.ToString() : ""; + + Assert.True(result is MigrationApplyResultOk, $"Migration failed: {failure}"); + } } diff --git a/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresUniqueConstraintIssue55E2ETests.cs b/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresUniqueConstraintIssue55E2ETests.cs index 6f5c1cd..4272d62 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresUniqueConstraintIssue55E2ETests.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Tests/PostgresUniqueConstraintIssue55E2ETests.cs @@ -29,13 +29,25 @@ public async Task Calculate_AddingCompositeUniqueOnExistingTableWithFk_EmitsAddU try { - Apply(connection, Calculate(Inspect(connection), SchemaWithoutUnique())); + PostgresTestDb.Apply( + connection, + PostgresTestDb.Calculate( + PostgresTestDb.Inspect(connection, Logger), + SchemaWithoutUnique(), + Logger + ), + Logger + ); - var live = Inspect(connection); + var live = PostgresTestDb.Inspect(connection, Logger); var agentConfigs = live.Tables.Single(t => t.Name == "agent_configs"); Assert.Empty(agentConfigs.UniqueConstraints); - var upgrade = Calculate(Inspect(connection), SchemaWithUnique()); + var upgrade = PostgresTestDb.Calculate( + PostgresTestDb.Inspect(connection, Logger), + SchemaWithUnique(), + Logger + ); Assert.Contains( upgrade, @@ -45,9 +57,11 @@ op is AddUniqueConstraintOperation add && add.UniqueConstraint.Columns.SequenceEqual(["tenant_id", "name"]) ); - Apply(connection, upgrade); + PostgresTestDb.Apply(connection, upgrade, Logger); - var converged = Inspect(connection).Tables.Single(t => t.Name == "agent_configs"); + var converged = PostgresTestDb + .Inspect(connection, Logger) + .Tables.Single(t => t.Name == "agent_configs"); Assert.Contains( converged.UniqueConstraints, c => @@ -55,7 +69,11 @@ op is AddUniqueConstraintOperation add && c.Columns.SequenceEqual(["tenant_id", "name"]) ); - var replay = Calculate(Inspect(connection), SchemaWithUnique()); + var replay = PostgresTestDb.Calculate( + PostgresTestDb.Inspect(connection, Logger), + SchemaWithUnique(), + Logger + ); Assert.DoesNotContain(replay, op => op is AddUniqueConstraintOperation); } finally @@ -136,50 +154,4 @@ private static SchemaDefinition Schema(bool includeUnique) }; return new SchemaDefinition { Name = "issue55", Tables = [tenants, agentConfigs] }; } - - private static SchemaDefinition Inspect(NpgsqlConnection connection) - { - var result = PostgresSchemaInspector.Inspect(connection, SchemaName, Logger); - if (result is SchemaResultOk ok) - { - return ok.Value; - } - Assert.Fail("Expected PostgreSQL schema inspection to succeed."); - return new SchemaDefinition { Name = "failed", Tables = [] }; - } - - private static IReadOnlyList Calculate( - SchemaDefinition current, - SchemaDefinition desired - ) - { - var result = SchemaDiff.Calculate( - current: current, - desired: desired, - allowDestructive: false, - logger: Logger - ); - if (result is OperationsResultOk ok) - { - return ok.Value; - } - Assert.Fail("Expected PostgreSQL schema diff to succeed."); - return []; - } - - private static void Apply( - NpgsqlConnection connection, - IReadOnlyList operations - ) - { - var result = MigrationRunner.Apply( - connection: connection, - operations: operations, - generateDdl: PostgresDdlGenerator.Generate, - options: MigrationOptions.Default, - logger: Logger - ); - var failure = result is MigrationApplyResultError error ? error.Value.ToString() : ""; - Assert.True(result is MigrationApplyResultOk, $"Migration failed: {failure}"); - } } diff --git a/Migration/Nimblesite.DataProvider.Migration.Tests/SchemaDiffCheckConstraintIssue57Tests.cs b/Migration/Nimblesite.DataProvider.Migration.Tests/SchemaDiffCheckConstraintIssue57Tests.cs index 6c74a49..7bdff20 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Tests/SchemaDiffCheckConstraintIssue57Tests.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Tests/SchemaDiffCheckConstraintIssue57Tests.cs @@ -18,26 +18,8 @@ public void Calculate_TableCheckConstraintExpressionChanged_DropsAndReadds() Name = "live", Tables = [ - new TableDefinition + BaseUsageEventsTable() with { - Schema = "public", - Name = "usage_events", - Columns = - [ - new ColumnDefinition - { - Name = "id", - Type = PortableTypes.Uuid, - IsNullable = false, - }, - new ColumnDefinition - { - Name = "kind", - Type = PortableTypes.Text, - IsNullable = false, - }, - ], - PrimaryKey = new PrimaryKeyDefinition { Columns = ["id"] }, CheckConstraints = [ new CheckConstraintDefinition @@ -102,28 +84,17 @@ public void Calculate_ColumnCheckConstraintExpressionChanged_DropsAndReadds() Name = "live", Tables = [ - new TableDefinition + BaseUsageEventsTable() with { - Schema = "public", - Name = "usage_events", Columns = [ - new ColumnDefinition + BaseUsageEventsTable().Columns[0], + BaseUsageEventsTable().Columns[1] with { - Name = "id", - Type = PortableTypes.Uuid, - IsNullable = false, - }, - new ColumnDefinition - { - Name = "kind", - Type = PortableTypes.Text, - IsNullable = false, CheckConstraint = "kind IN ('request','input_tokens','output_tokens','sandbox_seconds')", }, ], - PrimaryKey = new PrimaryKeyDefinition { Columns = ["id"] }, }, ], }; @@ -172,26 +143,8 @@ public void Calculate_CheckConstraintUnchanged_EmitsNoCheckConstraintOps() Name = "stable", Tables = [ - new TableDefinition + BaseUsageEventsTable() with { - Schema = "public", - Name = "usage_events", - Columns = - [ - new ColumnDefinition - { - Name = "id", - Type = PortableTypes.Uuid, - IsNullable = false, - }, - new ColumnDefinition - { - Name = "kind", - Type = PortableTypes.Text, - IsNullable = false, - }, - ], - PrimaryKey = new PrimaryKeyDefinition { Columns = ["id"] }, CheckConstraints = [ new CheckConstraintDefinition @@ -216,4 +169,27 @@ public void Calculate_CheckConstraintUnchanged_EmitsNoCheckConstraintOps() Assert.DoesNotContain(ops, op => op is DropCheckConstraintOperation); Assert.DoesNotContain(ops, op => op is AddCheckConstraintOperation); } + + private static TableDefinition BaseUsageEventsTable() => + new() + { + Schema = "public", + Name = "usage_events", + Columns = + [ + new ColumnDefinition + { + Name = "id", + Type = PortableTypes.Uuid, + IsNullable = false, + }, + new ColumnDefinition + { + Name = "kind", + Type = PortableTypes.Text, + IsNullable = false, + }, + ], + PrimaryKey = new PrimaryKeyDefinition { Columns = ["id"] }, + }; } diff --git a/Migration/Nimblesite.DataProvider.Migration.Tests/SchemaDiffTests.cs b/Migration/Nimblesite.DataProvider.Migration.Tests/SchemaDiffTests.cs index 1234e46..3d7a6d8 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Tests/SchemaDiffTests.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Tests/SchemaDiffTests.cs @@ -65,10 +65,7 @@ public void Calculate_SameSchema_NoOperations() public void Calculate_NewColumn_AddsColumn() { // Arrange - var current = Schema - .Define("Current") - .Table("public", "users", t => t.Column("id", PortableTypes.Uuid, c => c.PrimaryKey())) - .Build(); + var current = UsersIdOnly("Current"); var desired = Schema .Define("Desired") @@ -199,10 +196,7 @@ public void Calculate_RemovedTable_NotDroppedByDefault() ) .Build(); - var desired = Schema - .Define("Desired") - .Table("public", "users", t => t.Column("id", PortableTypes.Uuid, c => c.PrimaryKey())) - .Build(); + var desired = UsersIdOnly("Desired"); // Act var result = SchemaDiff.Calculate(current, desired, allowDestructive: false); @@ -228,10 +222,7 @@ public void Calculate_RemovedTable_DroppedWhenDestructiveAllowed() ) .Build(); - var desired = Schema - .Define("Desired") - .Table("public", "users", t => t.Column("id", PortableTypes.Uuid, c => c.PrimaryKey())) - .Build(); + var desired = UsersIdOnly("Desired"); // Act var result = SchemaDiff.Calculate(current, desired, allowDestructive: true); @@ -261,10 +252,7 @@ public void Calculate_RemovedColumn_NotDroppedByDefault() ) .Build(); - var desired = Schema - .Define("Desired") - .Table("public", "users", t => t.Column("id", PortableTypes.Uuid, c => c.PrimaryKey())) - .Build(); + var desired = UsersIdOnly("Desired"); // Act var result = SchemaDiff.Calculate(current, desired, allowDestructive: false); @@ -291,10 +279,7 @@ public void Calculate_RemovedColumn_DroppedWhenDestructiveAllowed() ) .Build(); - var desired = Schema - .Define("Desired") - .Table("public", "users", t => t.Column("id", PortableTypes.Uuid, c => c.PrimaryKey())) - .Build(); + var desired = UsersIdOnly("Desired"); // Act var result = SchemaDiff.Calculate(current, desired, allowDestructive: true); @@ -695,4 +680,10 @@ private static TableDefinition RlsTable(RlsPolicySetDefinition rls) => PrimaryKey = new PrimaryKeyDefinition { Columns = ["id"] }, RowLevelSecurity = rls, }; + + private static SchemaDefinition UsersIdOnly(string name) => + Schema + .Define(name) + .Table("public", "users", t => t.Column("id", PortableTypes.Uuid, c => c.PrimaryKey())) + .Build(); } diff --git a/Migration/Nimblesite.DataProvider.Migration.Tests/SchemaDiffUniqueConstraintIssue55Tests.cs b/Migration/Nimblesite.DataProvider.Migration.Tests/SchemaDiffUniqueConstraintIssue55Tests.cs index d6e16da..decfd18 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Tests/SchemaDiffUniqueConstraintIssue55Tests.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Tests/SchemaDiffUniqueConstraintIssue55Tests.cs @@ -8,6 +8,28 @@ namespace Nimblesite.DataProvider.Migration.Tests; /// public sealed class SchemaDiffUniqueConstraintIssue55Tests { + private static IReadOnlyList AgentConfigColumns() => + [ + new ColumnDefinition + { + Name = "id", + Type = PortableTypes.Uuid, + IsNullable = false, + }, + new ColumnDefinition + { + Name = "tenant_id", + Type = PortableTypes.Uuid, + IsNullable = false, + }, + new ColumnDefinition + { + Name = "name", + Type = PortableTypes.Text, + IsNullable = false, + }, + ]; + [Fact] public void Calculate_AddingCompositeUniqueConstraintToExistingTable_YieldsAddOperation() { @@ -20,27 +42,7 @@ public void Calculate_AddingCompositeUniqueConstraintToExistingTable_YieldsAddOp { Schema = "public", Name = "agent_configs", - Columns = - [ - new ColumnDefinition - { - Name = "id", - Type = PortableTypes.Uuid, - IsNullable = false, - }, - new ColumnDefinition - { - Name = "tenant_id", - Type = PortableTypes.Uuid, - IsNullable = false, - }, - new ColumnDefinition - { - Name = "name", - Type = PortableTypes.Text, - IsNullable = false, - }, - ], + Columns = AgentConfigColumns(), PrimaryKey = new PrimaryKeyDefinition { Columns = ["id"] }, }, ], @@ -103,27 +105,7 @@ public void Calculate_ReplayAfterUniqueConstraintApplied_YieldsNoUniqueOperation { Schema = "public", Name = "agent_configs", - Columns = - [ - new ColumnDefinition - { - Name = "id", - Type = PortableTypes.Uuid, - IsNullable = false, - }, - new ColumnDefinition - { - Name = "tenant_id", - Type = PortableTypes.Uuid, - IsNullable = false, - }, - new ColumnDefinition - { - Name = "name", - Type = PortableTypes.Text, - IsNullable = false, - }, - ], + Columns = AgentConfigColumns(), PrimaryKey = new PrimaryKeyDefinition { Columns = ["id"] }, UniqueConstraints = [unique], }, diff --git a/Migration/Nimblesite.DataProvider.Migration.Tests/SchemaVerifier.cs b/Migration/Nimblesite.DataProvider.Migration.Tests/SchemaVerifier.cs index ea943d7..179b0c8 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Tests/SchemaVerifier.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Tests/SchemaVerifier.cs @@ -529,7 +529,7 @@ public static void VerifyDefaultsWorkAtRuntime( // HELPER METHODS // ========================================================================= - private static bool PostgresTableExists(NpgsqlConnection conn, string tableName, string schema) + internal static bool PostgresTableExists(NpgsqlConnection conn, string tableName, string schema) { using var cmd = conn.CreateCommand(); cmd.CommandText = """ @@ -584,7 +584,7 @@ SELECT column_name FROM information_schema.columns return columns; } - private static bool SqliteTableExists(SqliteConnection conn, string tableName) + internal static bool SqliteTableExists(SqliteConnection conn, string tableName) { using var cmd = conn.CreateCommand(); cmd.CommandText = "SELECT COUNT(*) FROM sqlite_master WHERE type='table' AND name=@name"; diff --git a/Migration/Nimblesite.DataProvider.Migration.Tests/SqliteMigrationTests.cs b/Migration/Nimblesite.DataProvider.Migration.Tests/SqliteMigrationTests.cs index ed90352..9b20b38 100644 --- a/Migration/Nimblesite.DataProvider.Migration.Tests/SqliteMigrationTests.cs +++ b/Migration/Nimblesite.DataProvider.Migration.Tests/SqliteMigrationTests.cs @@ -31,6 +31,23 @@ private static void CleanupTestDb(SqliteConnection connection, string dbPath) } } + private static void SeedSchema( + SqliteConnection connection, + SchemaDefinition schema, + ILogger logger + ) + { + var current = ((SchemaResultOk)SqliteSchemaInspector.Inspect(connection, logger)).Value; + var ops = ((OperationsResultOk)SchemaDiff.Calculate(current, schema, logger: logger)).Value; + _ = MigrationRunner.Apply( + connection, + ops, + SqliteDdlGenerator.Generate, + MigrationOptions.Default, + logger + ); + } + [Fact] public void CreateDatabaseFromScratch_SingleTable_Success() { @@ -182,19 +199,7 @@ public void UpgradeExistingDatabase_AddColumn_Success() .Build(); // Apply v1 - var emptySchema = ( - (SchemaResultOk)SqliteSchemaInspector.Inspect(connection, _logger) - ).Value; - var v1Ops = ( - (OperationsResultOk)SchemaDiff.Calculate(emptySchema, v1, logger: _logger) - ).Value; - _ = MigrationRunner.Apply( - connection, - v1Ops, - SqliteDdlGenerator.Generate, - MigrationOptions.Default, - _logger - ); + SeedSchema(connection, v1, _logger); // Define v2 with new columns var v2 = Schema @@ -260,19 +265,7 @@ public void UpgradeExistingDatabase_AddTable_Success() .Build(); // Apply v1 - var emptySchema = ( - (SchemaResultOk)SqliteSchemaInspector.Inspect(connection, _logger) - ).Value; - var v1Ops = ( - (OperationsResultOk)SchemaDiff.Calculate(emptySchema, v1, logger: _logger) - ).Value; - _ = MigrationRunner.Apply( - connection, - v1Ops, - SqliteDdlGenerator.Generate, - MigrationOptions.Default, - _logger - ); + SeedSchema(connection, v1, _logger); // v2 adds a new table var v2 = Schema @@ -341,19 +334,7 @@ public void UpgradeExistingDatabase_AddIndex_Success() .Build(); // Apply v1 - var emptySchema = ( - (SchemaResultOk)SqliteSchemaInspector.Inspect(connection, _logger) - ).Value; - var v1Ops = ( - (OperationsResultOk)SchemaDiff.Calculate(emptySchema, v1, logger: _logger) - ).Value; - _ = MigrationRunner.Apply( - connection, - v1Ops, - SqliteDdlGenerator.Generate, - MigrationOptions.Default, - _logger - ); + SeedSchema(connection, v1, _logger); // v2 adds an index var v2 = Schema @@ -541,19 +522,7 @@ public void Destructive_DropTable_BlockedByDefault() .Table("Products", t => t.Column("Id", PortableTypes.Uuid, c => c.PrimaryKey())) .Build(); - var emptySchema = ( - (SchemaResultOk)SqliteSchemaInspector.Inspect(connection, _logger) - ).Value; - var v1Ops = ( - (OperationsResultOk)SchemaDiff.Calculate(emptySchema, v1, logger: _logger) - ).Value; - _ = MigrationRunner.Apply( - connection, - v1Ops, - SqliteDdlGenerator.Generate, - MigrationOptions.Default, - _logger - ); + SeedSchema(connection, v1, _logger); // v2 removes Products table var v2 = Schema @@ -598,19 +567,7 @@ public void Destructive_DropTable_AllowedWithOption() .Table("Products", t => t.Column("Id", PortableTypes.Uuid, c => c.PrimaryKey())) .Build(); - var emptySchema = ( - (SchemaResultOk)SqliteSchemaInspector.Inspect(connection, _logger) - ).Value; - var v1Ops = ( - (OperationsResultOk)SchemaDiff.Calculate(emptySchema, v1, logger: _logger) - ).Value; - _ = MigrationRunner.Apply( - connection, - v1Ops, - SqliteDdlGenerator.Generate, - MigrationOptions.Default, - _logger - ); + SeedSchema(connection, v1, _logger); var v2 = Schema .Define("Test") @@ -674,19 +631,7 @@ public void SchemaInspector_RoundTrip_Matches() .Build(); // Create schema - var emptySchema = ( - (SchemaResultOk)SqliteSchemaInspector.Inspect(connection, _logger) - ).Value; - var operations = ( - (OperationsResultOk)SchemaDiff.Calculate(emptySchema, schema, logger: _logger) - ).Value; - _ = MigrationRunner.Apply( - connection, - operations, - SqliteDdlGenerator.Generate, - MigrationOptions.Default, - _logger - ); + SeedSchema(connection, schema, _logger); // Act - Inspect and compare var inspected = ( @@ -960,19 +905,7 @@ public void ExpressionIndex_EnforcesCaseInsensitiveUniqueness() ) .Build(); - var emptySchema = ( - (SchemaResultOk)SqliteSchemaInspector.Inspect(connection, _logger) - ).Value; - var operations = ( - (OperationsResultOk)SchemaDiff.Calculate(emptySchema, schema, logger: _logger) - ).Value; - _ = MigrationRunner.Apply( - connection, - operations, - SqliteDdlGenerator.Generate, - MigrationOptions.Default, - _logger - ); + SeedSchema(connection, schema, _logger); // Act - Insert first venue using var insertCmd = connection.CreateCommand(); @@ -1179,19 +1112,7 @@ public void UpgradeIndex_ColumnToExpression_RequiresDropAndCreate() .Build(); // Apply v1 - var emptySchema = ( - (SchemaResultOk)SqliteSchemaInspector.Inspect(connection, _logger) - ).Value; - var v1Ops = ( - (OperationsResultOk)SchemaDiff.Calculate(emptySchema, v1, logger: _logger) - ).Value; - _ = MigrationRunner.Apply( - connection, - v1Ops, - SqliteDdlGenerator.Generate, - MigrationOptions.Default, - _logger - ); + SeedSchema(connection, v1, _logger); // v2 changes to expression index (different name since it's semantically different) var v2 = Schema @@ -1263,19 +1184,7 @@ public void UpgradeIndex_ExpressionToColumn_RequiresDropAndCreate() .Build(); // Apply v1 - var emptySchema = ( - (SchemaResultOk)SqliteSchemaInspector.Inspect(connection, _logger) - ).Value; - var v1Ops = ( - (OperationsResultOk)SchemaDiff.Calculate(emptySchema, v1, logger: _logger) - ).Value; - _ = MigrationRunner.Apply( - connection, - v1Ops, - SqliteDdlGenerator.Generate, - MigrationOptions.Default, - _logger - ); + SeedSchema(connection, v1, _logger); // v2 changes back to simple column index (different name) var v2 = Schema diff --git a/Reporting/Nimblesite.Reporting.React/Components/BarChartComponent.cs b/Reporting/Nimblesite.Reporting.React/Components/BarChartComponent.cs index fc950ca..5a9a981 100644 --- a/Reporting/Nimblesite.Reporting.React/Components/BarChartComponent.cs +++ b/Reporting/Nimblesite.Reporting.React/Components/BarChartComponent.cs @@ -45,8 +45,8 @@ public static ReactElement Render( var columnNames = Script.Get(dataSourceResult, "columnNames"); var rows = Script.Get(dataSourceResult, "rows") ?? new object[0]; - var xIndex = FindColumnIndex(columnNames, xField); - var yIndex = FindColumnIndex(columnNames, yField); + var xIndex = ColumnLookup.FindColumnIndex(columnNames, xField); + var yIndex = ColumnLookup.FindColumnIndex(columnNames, yField); var chartClassName = cssClass != null ? "report-chart " + cssClass : "report-chart"; @@ -187,18 +187,6 @@ public static ReactElement Render( ); } - private static int FindColumnIndex(string[] columnNames, string field) - { - if (columnNames == null || field == null) - return -1; - for (var i = 0; i < columnNames.Length; i++) - { - if (columnNames[i] == field) - return i; - } - return -1; - } - private static string TruncateLabel(string label, int maxLen) { if (label == null) diff --git a/Reporting/Nimblesite.Reporting.React/Components/ColumnLookup.cs b/Reporting/Nimblesite.Reporting.React/Components/ColumnLookup.cs new file mode 100644 index 0000000..513396d --- /dev/null +++ b/Reporting/Nimblesite.Reporting.React/Components/ColumnLookup.cs @@ -0,0 +1,23 @@ +namespace Nimblesite.Reporting.React.Components +{ + /// + /// Shared column-name lookup used by report components. + /// + internal static class ColumnLookup + { + /// + /// Returns the index of the named column, or -1 if not found. + /// + internal static int FindColumnIndex(string[] columnNames, string field) + { + if (columnNames == null || field == null) + return -1; + for (var i = 0; i < columnNames.Length; i++) + { + if (columnNames[i] == field) + return i; + } + return -1; + } + } +} diff --git a/Reporting/Nimblesite.Reporting.React/Components/TableComponent.cs b/Reporting/Nimblesite.Reporting.React/Components/TableComponent.cs index aa04511..01ef320 100644 --- a/Reporting/Nimblesite.Reporting.React/Components/TableComponent.cs +++ b/Reporting/Nimblesite.Reporting.React/Components/TableComponent.cs @@ -50,7 +50,7 @@ public static ReactElement Render( for (var c = 0; c < columns.Length; c++) { var field = Script.Get(columns[c], "field"); - var colIndex = FindColumnIndex(allColumnNames, field); + var colIndex = ColumnLookup.FindColumnIndex(allColumnNames, field); var cellValue = colIndex >= 0 ? Script.Write("row[colIndex]") : null; cells[c] = Td( className: "report-table-td", @@ -90,17 +90,5 @@ public static ReactElement Render( } ); } - - private static int FindColumnIndex(string[] columnNames, string field) - { - if (columnNames == null || field == null) - return -1; - for (var i = 0; i < columnNames.Length; i++) - { - if (columnNames[i] == field) - return i; - } - return -1; - } } } diff --git a/Sync/Nimblesite.Sync.Core/LqlExpressionEvaluator.cs b/Sync/Nimblesite.Sync.Core/LqlExpressionEvaluator.cs index 11cafa7..d28f8f2 100644 --- a/Sync/Nimblesite.Sync.Core/LqlExpressionEvaluator.cs +++ b/Sync/Nimblesite.Sync.Core/LqlExpressionEvaluator.cs @@ -261,16 +261,7 @@ private static string ResolveArgument(string arg, JsonElement source) if (source.TryGetProperty(columnName, out var prop)) { - return prop.ValueKind switch - { - JsonValueKind.String => prop.GetString(), - JsonValueKind.Number when prop.TryGetInt64(out var l) => l, - JsonValueKind.Number => prop.GetDouble(), - JsonValueKind.True => true, - JsonValueKind.False => false, - JsonValueKind.Null => null, - _ => prop.GetRawText(), - }; + return ConvertJsonElement(prop); } // Try case-insensitive match @@ -278,22 +269,28 @@ JsonValueKind.Number when prop.TryGetInt64(out var l) => l, { if (property.Name.Equals(columnName, StringComparison.OrdinalIgnoreCase)) { - return property.Value.ValueKind switch - { - JsonValueKind.String => property.Value.GetString(), - JsonValueKind.Number when property.Value.TryGetInt64(out var l) => l, - JsonValueKind.Number => property.Value.GetDouble(), - JsonValueKind.True => true, - JsonValueKind.False => false, - JsonValueKind.Null => null, - _ => property.Value.GetRawText(), - }; + return ConvertJsonElement(property.Value); } } return null; } + /// + /// Converts a JSON element to its CLR value. + /// + private static object? ConvertJsonElement(JsonElement element) => + element.ValueKind switch + { + JsonValueKind.String => element.GetString(), + JsonValueKind.Number when element.TryGetInt64(out var l) => l, + JsonValueKind.Number => element.GetDouble(), + JsonValueKind.True => true, + JsonValueKind.False => false, + JsonValueKind.Null => null, + _ => element.GetRawText(), + }; + /// /// Parses function arguments, handling quoted strings and nested calls. /// diff --git a/Sync/Nimblesite.Sync.Http.Tests/HttpEndpointTests.cs b/Sync/Nimblesite.Sync.Http.Tests/HttpEndpointTests.cs index 3b7daff..cf17c89 100644 --- a/Sync/Nimblesite.Sync.Http.Tests/HttpEndpointTests.cs +++ b/Sync/Nimblesite.Sync.Http.Tests/HttpEndpointTests.cs @@ -412,6 +412,17 @@ Email TEXT TriggerGenerator.CreateTriggers(conn, "Person", NullLogger.Instance); } + private static SyncLogEntry ParseChangeEntry(JsonElement change) => + new SyncLogEntry( + Version: change.GetProperty("version").GetInt64(), + TableName: change.GetProperty("tableName").GetString() ?? "", + PkValue: change.GetProperty("pkValue").GetString() ?? "", + Operation: (SyncOperation)change.GetProperty("operation").GetInt32(), + Payload: change.GetProperty("payload").GetString() ?? "", + Origin: change.GetProperty("origin").GetString() ?? "", + Timestamp: change.GetProperty("timestamp").GetString() ?? "" + ); + public void Dispose() { _client.Dispose(); @@ -530,15 +541,7 @@ public async Task PullChanges_ChangesOnServer_AppearsOnClient() // Apply changes to client (operation is returned as integer enum value) foreach (var change in changesArray.EnumerateArray()) { - var entry = new SyncLogEntry( - Version: change.GetProperty("version").GetInt64(), - TableName: change.GetProperty("tableName").GetString() ?? "", - PkValue: change.GetProperty("pkValue").GetString() ?? "", - Operation: (SyncOperation)change.GetProperty("operation").GetInt32(), - Payload: change.GetProperty("payload").GetString() ?? "", - Origin: change.GetProperty("origin").GetString() ?? "", - Timestamp: change.GetProperty("timestamp").GetString() ?? "" - ); + var entry = ParseChangeEntry(change); SyncSessionManager.EnableSuppression(_clientConn); var result = ChangeApplierSQLite.ApplyChange(_clientConn, entry); @@ -588,15 +591,7 @@ public async Task BidirectionalSync_ChangesBothWays_BothDatabasesHaveBothRecords foreach (var change in pullDoc.RootElement.GetProperty("changes").EnumerateArray()) { - var entry = new SyncLogEntry( - Version: change.GetProperty("version").GetInt64(), - TableName: change.GetProperty("tableName").GetString() ?? "", - PkValue: change.GetProperty("pkValue").GetString() ?? "", - Operation: (SyncOperation)change.GetProperty("operation").GetInt32(), - Payload: change.GetProperty("payload").GetString() ?? "", - Origin: change.GetProperty("origin").GetString() ?? "", - Timestamp: change.GetProperty("timestamp").GetString() ?? "" - ); + var entry = ParseChangeEntry(change); if (entry.Origin != _clientOriginId) { diff --git a/Sync/Nimblesite.Sync.Http.Tests/HttpMappingE2ETests.cs b/Sync/Nimblesite.Sync.Http.Tests/HttpMappingE2ETests.cs index c03fc2b..e738f22 100644 --- a/Sync/Nimblesite.Sync.Http.Tests/HttpMappingE2ETests.cs +++ b/Sync/Nimblesite.Sync.Http.Tests/HttpMappingE2ETests.cs @@ -153,6 +153,20 @@ private static SyncMappingConfig CreateUserToCustomerMapping() => ] ); + /// + /// Seeds SQLite source + Postgres target with fresh origins and returns the User->Customer mapping. + /// + private SyncMappingConfig SetupUserToCustomerScenario() + { + var sourceOrigin = Guid.NewGuid().ToString(); + var targetOrigin = Guid.NewGuid().ToString(); + + SetupSqliteSource(sourceOrigin); + SetupPostgresTarget(targetOrigin); + + return CreateUserToCustomerMapping(); + } + /// /// Merges the PK value into a payload for insert operations. /// PostgresChangeApplier expects PK to be in the payload. @@ -404,13 +418,7 @@ public void MappingEngine_WithSubstringTransform_ExtractsText() public void E2E_SyncWithMapping_TransformsData_AcrossDatabases() { // Arrange - set up databases with DIFFERENT schemas - var sourceOrigin = Guid.NewGuid().ToString(); - var targetOrigin = Guid.NewGuid().ToString(); - - SetupSqliteSource(sourceOrigin); - SetupPostgresTarget(targetOrigin); - - var config = CreateUserToCustomerMapping(); + var config = SetupUserToCustomerScenario(); // Act - insert User in SQLite source using (var cmd = _sqliteConn.CreateCommand()) @@ -483,13 +491,7 @@ INSERT INTO User (Id, FullName, EmailAddress, PasswordHash, SecurityStamp, Creat public void E2E_MultipleRecords_AllTransformedCorrectly() { // Arrange - var sourceOrigin = Guid.NewGuid().ToString(); - var targetOrigin = Guid.NewGuid().ToString(); - - SetupSqliteSource(sourceOrigin); - SetupPostgresTarget(targetOrigin); - - var config = CreateUserToCustomerMapping(); + var config = SetupUserToCustomerScenario(); // Insert multiple users var users = new[] diff --git a/Sync/Nimblesite.Sync.Http/SyncHelpers.cs b/Sync/Nimblesite.Sync.Http/SyncHelpers.cs index 4ac29bb..e76fa64 100644 --- a/Sync/Nimblesite.Sync.Http/SyncHelpers.cs +++ b/Sync/Nimblesite.Sync.Http/SyncHelpers.cs @@ -126,15 +126,7 @@ ILogger logger if (change.Origin == originId) continue; - var entry = new SyncLogEntry( - change.Version, - change.TableName, - change.PkValue, - Enum.Parse(change.Operation, true), - change.Payload, - change.Origin, - change.Timestamp - ); + var entry = ToSyncLogEntry(change); var result = SQLite.ChangeApplierSQLite.ApplyChange(conn, entry); if (result is Outcome.Result.Ok) @@ -168,15 +160,7 @@ ILogger logger if (change.Origin == originId) continue; - var entry = new SyncLogEntry( - change.Version, - change.TableName, - change.PkValue, - Enum.Parse(change.Operation, true), - change.Payload, - change.Origin, - change.Timestamp - ); + var entry = ToSyncLogEntry(change); var result = Postgres.PostgresChangeApplier.ApplyChange(conn, entry, logger); if (result is Outcome.Result.Ok) @@ -227,4 +211,15 @@ private static long GetMaxVersionPostgres(string connectionString, ILogger logge conn.Open(); return Postgres.PostgresSyncLogRepository.GetMaxVersion(conn).Match(ok => ok, _ => 0); } + + private static SyncLogEntry ToSyncLogEntry(SyncLogEntryDto change) => + new( + change.Version, + change.TableName, + change.PkValue, + Enum.Parse(change.Operation, true), + change.Payload, + change.Origin, + change.Timestamp + ); } diff --git a/Sync/Nimblesite.Sync.Integration.Tests/HttpMappingSyncTests.cs b/Sync/Nimblesite.Sync.Integration.Tests/HttpMappingSyncTests.cs index a8f1e88..92bae2e 100644 --- a/Sync/Nimblesite.Sync.Integration.Tests/HttpMappingSyncTests.cs +++ b/Sync/Nimblesite.Sync.Integration.Tests/HttpMappingSyncTests.cs @@ -134,20 +134,7 @@ public void Mapping_TransformsColumnsFromUserToCustomer() new(null, "source", TransformType.Constant, "mobile-app"), }; - var mapping = new TableMapping( - Id: "user-to-customer", - SourceTable: "User", - TargetTable: "customer", - Direction: MappingDirection.Push, - Enabled: true, - PkMapping: new PkMapping("Id", "customer_id"), - ColumnMappings: columnMappings, - ExcludedColumns: ["PasswordHash", "SecurityStamp"], - Filter: null, - SyncTracking: new SyncTrackingConfig() - ); - - var mappingConfig = new SyncMappingConfig("1.0", UnmappedTableBehavior.Strict, [mapping]); + var mappingConfig = UserToCustomerConfig(columnMappings, ["PasswordHash", "SecurityStamp"]); // Act - Insert in source with SOURCE schema columns using (var cmd = source.CreateCommand()) @@ -226,20 +213,7 @@ public void FullSync_WithMapping_TransformsAndApplies() new(null, "source", TransformType.Constant, "sync-test"), }; - var mapping = new TableMapping( - Id: "user-to-customer", - SourceTable: "User", - TargetTable: "customer", - Direction: MappingDirection.Push, - Enabled: true, - PkMapping: new PkMapping("Id", "customer_id"), - ColumnMappings: columnMappings, - ExcludedColumns: ["PasswordHash", "SecurityStamp"], - Filter: null, - SyncTracking: new SyncTrackingConfig() - ); - - var mappingConfig = new SyncMappingConfig("1.0", UnmappedTableBehavior.Strict, [mapping]); + var mappingConfig = UserToCustomerConfig(columnMappings, ["PasswordHash", "SecurityStamp"]); // Insert in source using (var cmd = source.CreateCommand()) @@ -419,20 +393,7 @@ public void UpdateOperation_MapsCorrectly() new("EmailAddress", "email"), }; - var mapping = new TableMapping( - Id: "user-to-customer", - SourceTable: "User", - TargetTable: "customer", - Direction: MappingDirection.Push, - Enabled: true, - PkMapping: new PkMapping("Id", "customer_id"), - ColumnMappings: columnMappings, - ExcludedColumns: [], - Filter: null, - SyncTracking: new SyncTrackingConfig() - ); - - var mappingConfig = new SyncMappingConfig("1.0", UnmappedTableBehavior.Strict, [mapping]); + var mappingConfig = UserToCustomerConfig(columnMappings, []); // Insert using (var cmd = source.CreateCommand()) @@ -491,20 +452,7 @@ public void DeleteOperation_MapsPrimaryKeyCorrectly() var sourceOrigin = Guid.NewGuid().ToString(); using var source = CreateSourceDb(sourceOrigin); - var mapping = new TableMapping( - Id: "user-to-customer", - SourceTable: "User", - TargetTable: "customer", - Direction: MappingDirection.Push, - Enabled: true, - PkMapping: new PkMapping("Id", "customer_id"), - ColumnMappings: [], - ExcludedColumns: [], - Filter: null, - SyncTracking: new SyncTrackingConfig() - ); - - var mappingConfig = new SyncMappingConfig("1.0", UnmappedTableBehavior.Strict, [mapping]); + var mappingConfig = UserToCustomerConfig([], []); // Insert then delete using (var cmd = source.CreateCommand()) @@ -618,20 +566,7 @@ public void NullValues_InPayload_MapsCorrectly() new("EmailAddress", "email"), }; - var mapping = new TableMapping( - Id: "user-to-customer", - SourceTable: "User", - TargetTable: "customer", - Direction: MappingDirection.Push, - Enabled: true, - PkMapping: new PkMapping("Id", "customer_id"), - ColumnMappings: columnMappings, - ExcludedColumns: [], - Filter: null, - SyncTracking: new SyncTrackingConfig() - ); - - var mappingConfig = new SyncMappingConfig("1.0", UnmappedTableBehavior.Strict, [mapping]); + var mappingConfig = UserToCustomerConfig(columnMappings, []); // Insert with NULL email using (var cmd = source.CreateCommand()) @@ -676,20 +611,7 @@ public void UnicodeCharacters_PreservedThroughMapping() new("EmailAddress", "email"), }; - var mapping = new TableMapping( - Id: "user-to-customer", - SourceTable: "User", - TargetTable: "customer", - Direction: MappingDirection.Push, - Enabled: true, - PkMapping: new PkMapping("Id", "customer_id"), - ColumnMappings: columnMappings, - ExcludedColumns: [], - Filter: null, - SyncTracking: new SyncTrackingConfig() - ); - - var mappingConfig = new SyncMappingConfig("1.0", UnmappedTableBehavior.Strict, [mapping]); + var mappingConfig = UserToCustomerConfig(columnMappings, []); // Unicode characters: Japanese, Chinese, Korean, Arabic, Emoji var unicodeNames = new[] @@ -782,20 +704,7 @@ public void SpecialCharacters_InData_HandledCorrectly() new("EmailAddress", "email"), }; - var mapping = new TableMapping( - Id: "user-to-customer", - SourceTable: "User", - TargetTable: "customer", - Direction: MappingDirection.Push, - Enabled: true, - PkMapping: new PkMapping("Id", "customer_id"), - ColumnMappings: columnMappings, - ExcludedColumns: [], - Filter: null, - SyncTracking: new SyncTrackingConfig() - ); - - var mappingConfig = new SyncMappingConfig("1.0", UnmappedTableBehavior.Strict, [mapping]); + var mappingConfig = UserToCustomerConfig(columnMappings, []); // Special chars var specialCases = new[] @@ -867,20 +776,7 @@ public void EmptyString_NotConfusedWithNull() new("EmailAddress", "email"), }; - var mapping = new TableMapping( - Id: "user-to-customer", - SourceTable: "User", - TargetTable: "customer", - Direction: MappingDirection.Push, - Enabled: true, - PkMapping: new PkMapping("Id", "customer_id"), - ColumnMappings: columnMappings, - ExcludedColumns: [], - Filter: null, - SyncTracking: new SyncTrackingConfig() - ); - - var mappingConfig = new SyncMappingConfig("1.0", UnmappedTableBehavior.Strict, [mapping]); + var mappingConfig = UserToCustomerConfig(columnMappings, []); // Insert with empty string (not NULL) using (var cmd = source.CreateCommand()) @@ -923,20 +819,7 @@ public void VeryLongStrings_HandledCorrectly() new("EmailAddress", "email"), }; - var mapping = new TableMapping( - Id: "user-to-customer", - SourceTable: "User", - TargetTable: "customer", - Direction: MappingDirection.Push, - Enabled: true, - PkMapping: new PkMapping("Id", "customer_id"), - ColumnMappings: columnMappings, - ExcludedColumns: [], - Filter: null, - SyncTracking: new SyncTrackingConfig() - ); - - var mappingConfig = new SyncMappingConfig("1.0", UnmappedTableBehavior.Strict, [mapping]); + var mappingConfig = UserToCustomerConfig(columnMappings, []); // Very long string (10000 chars) var longName = new string('A', 10000); @@ -999,20 +882,7 @@ public void ConstantTransform_SpecialValues_Work() new(null, "notes", TransformType.Constant, ""), }; - var mapping = new TableMapping( - Id: "user-to-customer", - SourceTable: "User", - TargetTable: "customer", - Direction: MappingDirection.Push, - Enabled: true, - PkMapping: new PkMapping("Id", "customer_id"), - ColumnMappings: columnMappings, - ExcludedColumns: [], - Filter: null, - SyncTracking: new SyncTrackingConfig() - ); - - var mappingConfig = new SyncMappingConfig("1.0", UnmappedTableBehavior.Strict, [mapping]); + var mappingConfig = UserToCustomerConfig(columnMappings, []); using (var cmd = source.CreateCommand()) { @@ -1051,28 +921,11 @@ public void AllColumnsExcluded_MinimalPayload() var sourceOrigin = Guid.NewGuid().ToString(); using var source = CreateSourceDb(sourceOrigin); - var mapping = new TableMapping( - Id: "user-to-customer", - SourceTable: "User", - TargetTable: "customer", - Direction: MappingDirection.Push, - Enabled: true, - PkMapping: new PkMapping("Id", "customer_id"), - ColumnMappings: [], - ExcludedColumns: - [ - "FullName", - "EmailAddress", - "PasswordHash", - "SecurityStamp", - "CreatedAt", - ], - Filter: null, - SyncTracking: new SyncTrackingConfig() + var mappingConfig = UserToCustomerConfig( + [], + ["FullName", "EmailAddress", "PasswordHash", "SecurityStamp", "CreatedAt"] ); - var mappingConfig = new SyncMappingConfig("1.0", UnmappedTableBehavior.Strict, [mapping]); - using (var cmd = source.CreateCommand()) { cmd.CommandText = @@ -1119,20 +972,7 @@ public void JsonInPayload_PreservedCorrectly() new("EmailAddress", "email"), }; - var mapping = new TableMapping( - Id: "user-to-customer", - SourceTable: "User", - TargetTable: "customer", - Direction: MappingDirection.Push, - Enabled: true, - PkMapping: new PkMapping("Id", "customer_id"), - ColumnMappings: columnMappings, - ExcludedColumns: [], - Filter: null, - SyncTracking: new SyncTrackingConfig() - ); - - var mappingConfig = new SyncMappingConfig("1.0", UnmappedTableBehavior.Strict, [mapping]); + var mappingConfig = UserToCustomerConfig(columnMappings, []); // Name contains JSON-like structure var jsonLikeName = "{\"first\":\"John\",\"last\":\"Doe\"}"; @@ -1180,6 +1020,29 @@ public void JsonInPayload_PreservedCorrectly() #endregion + private static SyncMappingConfig UserToCustomerConfig( + IReadOnlyList columnMappings, + IReadOnlyList excludedColumns + ) => + new( + "1.0", + UnmappedTableBehavior.Strict, + [ + new TableMapping( + Id: "user-to-customer", + SourceTable: "User", + TargetTable: "customer", + Direction: MappingDirection.Push, + Enabled: true, + PkMapping: new PkMapping("Id", "customer_id"), + ColumnMappings: columnMappings, + ExcludedColumns: excludedColumns, + Filter: null, + SyncTracking: new SyncTrackingConfig() + ), + ] + ); + /// /// Helper to apply a mapped entry to PostgreSQL target table. /// Note: Table name is from test fixtures, not user input - safe for test code. diff --git a/Sync/Nimblesite.Sync.Postgres/PostgresChangeApplier.cs b/Sync/Nimblesite.Sync.Postgres/PostgresChangeApplier.cs index 6edd1cd..1770c2e 100644 --- a/Sync/Nimblesite.Sync.Postgres/PostgresChangeApplier.cs +++ b/Sync/Nimblesite.Sync.Postgres/PostgresChangeApplier.cs @@ -137,16 +137,14 @@ ILogger logger return new BoolSyncError(new SyncErrorDatabase("Invalid payload format")); } - var pkJson = JsonSerializer.Deserialize>(entry.PkValue); - if (pkJson is null || pkJson.Count == 0) + var pkParts = TryParsePk(entry, out var pkError); + if (pkParts is not (var pkColumn, var pkColumnLower, var pkValue)) { - return new BoolSyncError(new SyncErrorDatabase("Invalid pk_value format")); + return pkError is { } err + ? err + : new BoolSyncError(new SyncErrorDatabase("Invalid pk_value format")); } - var pkColumn = pkJson.Keys.First(); - var pkColumnLower = pkColumn.ToLowerInvariant(); - var pkValue = GetJsonValue(pkJson[pkColumn]); - var setClauses = new List(); var paramIndex = 0; @@ -190,15 +188,14 @@ private static BoolSyncResult ApplyDelete( ILogger logger ) { - var pkJson = JsonSerializer.Deserialize>(entry.PkValue); - if (pkJson is null || pkJson.Count == 0) + var pkParts = TryParsePk(entry, out var pkError); + if (pkParts is not (_, var pkColumnLower, var pkValue)) { - return new BoolSyncError(new SyncErrorDatabase("Invalid pk_value format")); + return pkError is { } err + ? err + : new BoolSyncError(new SyncErrorDatabase("Invalid pk_value format")); } - var pkColumn = pkJson.Keys.First(); - var pkColumnLower = pkColumn.ToLowerInvariant(); - var pkValue = GetJsonValue(pkJson[pkColumn]); var tableName = entry.TableName.ToLowerInvariant(); using var cmd = connection.CreateCommand(); @@ -210,6 +207,24 @@ ILogger logger return new BoolSyncOk(true); } + // Shared PK-JSON parse+validate prologue for ApplyUpdate/ApplyDelete. + private static (string PkColumn, string PkColumnLower, object PkValue)? TryParsePk( + SyncLogEntry entry, + out BoolSyncResult? error + ) + { + var pkJson = JsonSerializer.Deserialize>(entry.PkValue); + if (pkJson is null || pkJson.Count == 0) + { + error = new BoolSyncError(new SyncErrorDatabase("Invalid pk_value format")); + return null; + } + + var pkColumn = pkJson.Keys.First(); + error = null; + return (pkColumn, pkColumn.ToLowerInvariant(), GetJsonValue(pkJson[pkColumn])); + } + private static object GetJsonValue(JsonElement element) => element.ValueKind switch { diff --git a/Sync/Nimblesite.Sync.SQLite.Tests/SpecComplianceTests.cs b/Sync/Nimblesite.Sync.SQLite.Tests/SpecComplianceTests.cs index 7c3226f..c8e7d9f 100644 --- a/Sync/Nimblesite.Sync.SQLite.Tests/SpecComplianceTests.cs +++ b/Sync/Nimblesite.Sync.SQLite.Tests/SpecComplianceTests.cs @@ -61,9 +61,7 @@ public void Spec_S5_OriginId_Is36CharUuid() public void Spec_S5_OriginId_IncludedInEveryChangeLogEntry() { // Spec S5.5: Origin ID MUST be included in every change log entry - InsertPerson("p1", "Alice", "alice@example.com"); - UpdatePerson("p1", "Alice Updated", "alice2@example.com"); - DeletePerson("p1"); + SeedInsertUpdateDeleteP1(); var changes = FetchChanges(0); Assert.Equal(3, changes.Count); @@ -103,9 +101,7 @@ public void Spec_S6_Timestamps_Iso8601UtcWithMilliseconds() public void Spec_S7_UnifiedChangeLog_AllOperationsInSingleTable() { // Spec S7.1: All changes in single unified table with JSON payloads - InsertPerson("p1", "Alice", "alice@example.com"); - UpdatePerson("p1", "Alice Updated", "alice2@example.com"); - DeletePerson("p1"); + SeedInsertUpdateDeleteP1(); var changes = FetchChanges(0); Assert.Equal(3, changes.Count); @@ -941,6 +937,13 @@ private void DeletePerson(string id) cmd.ExecuteNonQuery(); } + private void SeedInsertUpdateDeleteP1() + { + InsertPerson("p1", "Alice", "alice@example.com"); + UpdatePerson("p1", "Alice Updated", "alice2@example.com"); + DeletePerson("p1"); + } + private List FetchChanges(long fromVersion) { var result = SyncLogRepository.FetchChanges(_db, fromVersion, 1000); diff --git a/Sync/Nimblesite.Sync.SQLite/ChangeApplierSQLite.cs b/Sync/Nimblesite.Sync.SQLite/ChangeApplierSQLite.cs index a17040f..ef3a167 100644 --- a/Sync/Nimblesite.Sync.SQLite/ChangeApplierSQLite.cs +++ b/Sync/Nimblesite.Sync.SQLite/ChangeApplierSQLite.cs @@ -106,15 +106,11 @@ private static BoolSyncResult ApplyUpdate(SqliteConnection connection, SyncLogEn } // Extract PK info - var pkData = JsonSerializer.Deserialize>(entry.PkValue); - if (pkData == null || pkData.Count == 0) + if (ParsePkValue(entry.PkValue) is not (string pkColumn, object pkValue)) { return new BoolSyncError(new SyncErrorDatabase("Invalid pk_value JSON")); } - var pkColumn = pkData.Keys.First(); - var pkValue = JsonElementToValue(pkData[pkColumn]); - // Check for version-based conflict resolution // If record has a Version column, only apply if incoming version is newer if (data.TryGetValue("Version", out var incomingVersionElement)) @@ -159,15 +155,11 @@ private static BoolSyncResult ApplyUpdate(SqliteConnection connection, SyncLogEn )] private static BoolSyncResult ApplyDelete(SqliteConnection connection, SyncLogEntry entry) { - var pkData = JsonSerializer.Deserialize>(entry.PkValue); - if (pkData == null || pkData.Count == 0) + if (ParsePkValue(entry.PkValue) is not (string pkColumn, object pkValue)) { return new BoolSyncError(new SyncErrorDatabase("Invalid pk_value JSON")); } - var pkColumn = pkData.Keys.First(); - var pkValue = JsonElementToValue(pkData[pkColumn]); - using var cmd = connection.CreateCommand(); cmd.CommandText = $"DELETE FROM {entry.TableName} WHERE {pkColumn} = @pk"; cmd.Parameters.AddWithValue("@pk", pkValue); @@ -176,6 +168,18 @@ private static BoolSyncResult ApplyDelete(SqliteConnection connection, SyncLogEn return new BoolSyncOk(true); } + private static (string PkColumn, object PkValue)? ParsePkValue(string pkValue) + { + var pkData = JsonSerializer.Deserialize>(pkValue); + if (pkData == null || pkData.Count == 0) + { + return null; + } + + var pkColumn = pkData.Keys.First(); + return (pkColumn, JsonElementToValue(pkData[pkColumn])); + } + private static object JsonElementToValue(JsonElement element) => element.ValueKind switch { diff --git a/Sync/Nimblesite.Sync.SQLite/SqliteConnectionSyncExtensions.cs b/Sync/Nimblesite.Sync.SQLite/SqliteConnectionSyncExtensions.cs index da7a6ee..b4cde74 100644 --- a/Sync/Nimblesite.Sync.SQLite/SqliteConnectionSyncExtensions.cs +++ b/Sync/Nimblesite.Sync.SQLite/SqliteConnectionSyncExtensions.cs @@ -29,7 +29,7 @@ ORDER BY created_at ASC while (reader.Read()) { - subscriptions.Add(ReadSubscription(reader)); + subscriptions.Add(SubscriptionRepository.ReadSubscription(reader)); } return new SubscriptionListOk(subscriptions); @@ -65,7 +65,7 @@ FROM _sync_subscriptions while (reader.Read()) { - subscriptions.Add(ReadSubscription(reader)); + subscriptions.Add(SubscriptionRepository.ReadSubscription(reader)); } return new SubscriptionListOk(subscriptions); @@ -377,24 +377,4 @@ IEnumerable originIds ); } } - - private static SyncSubscription ReadSubscription(SqliteDataReader reader) => - new( - SubscriptionId: reader.GetString(0), - OriginId: reader.GetString(1), - Type: ParseSubscriptionType(reader.GetString(2)), - TableName: reader.GetString(3), - Filter: reader.IsDBNull(4) ? null : reader.GetString(4), - CreatedAt: reader.GetString(5), - ExpiresAt: reader.IsDBNull(6) ? null : reader.GetString(6) - ); - - private static SubscriptionType ParseSubscriptionType(string type) => - type.ToLowerInvariant() switch - { - "record" => SubscriptionType.Record, - "table" => SubscriptionType.Table, - "query" => SubscriptionType.Query, - _ => SubscriptionType.Table, - }; } diff --git a/Sync/Nimblesite.Sync.SQLite/SubscriptionRepository.cs b/Sync/Nimblesite.Sync.SQLite/SubscriptionRepository.cs index e8be957..aaa0bb4 100644 --- a/Sync/Nimblesite.Sync.SQLite/SubscriptionRepository.cs +++ b/Sync/Nimblesite.Sync.SQLite/SubscriptionRepository.cs @@ -256,7 +256,7 @@ private static SubscriptionListResult QueryList( } } - private static SyncSubscription ReadSubscription(SqliteDataReader reader) => + internal static SyncSubscription ReadSubscription(SqliteDataReader reader) => new( SubscriptionId: reader.GetString(0), OriginId: reader.GetString(1), @@ -267,7 +267,7 @@ private static SyncSubscription ReadSubscription(SqliteDataReader reader) => ExpiresAt: reader.IsDBNull(6) ? null : reader.GetString(6) ); - private static SubscriptionType ParseSubscriptionType(string type) => + internal static SubscriptionType ParseSubscriptionType(string type) => type.ToLowerInvariant() switch { "record" => SubscriptionType.Record, diff --git a/Sync/Nimblesite.Sync.Tests/BatchManagerTests.cs b/Sync/Nimblesite.Sync.Tests/BatchManagerTests.cs index 4d9e731..b4f9c6a 100644 --- a/Sync/Nimblesite.Sync.Tests/BatchManagerTests.cs +++ b/Sync/Nimblesite.Sync.Tests/BatchManagerTests.cs @@ -1,5 +1,4 @@ using Microsoft.Extensions.Logging.Abstractions; -using Outcome; namespace Nimblesite.Sync.Tests; @@ -18,7 +17,7 @@ public void FetchBatch_EmptyLog_ReturnsEmptyBatch() _logger ); - var batch = AssertSuccess(result); + var batch = TestDb.AssertSuccess(result); Assert.Empty(batch.Changes); Assert.Equal(0, batch.FromVersion); Assert.Equal(0, batch.ToVersion); @@ -52,7 +51,7 @@ public void FetchBatch_WithChanges_ReturnsBatch() _logger ); - var batch = AssertSuccess(result); + var batch = TestDb.AssertSuccess(result); Assert.Equal(2, batch.Changes.Count); Assert.Equal(0, batch.FromVersion); Assert.Equal(2, batch.ToVersion); @@ -81,7 +80,7 @@ public void FetchBatch_ExceedsBatchSize_HasMoreTrue() _logger ); - var batch = AssertSuccess(result); + var batch = TestDb.AssertSuccess(result); Assert.Equal(3, batch.Changes.Count); Assert.True(batch.HasMore); Assert.Equal(3, batch.ToVersion); @@ -109,7 +108,7 @@ public void FetchBatch_FromVersion_SkipsOlderEntries() _logger ); - var batch = AssertSuccess(result); + var batch = TestDb.AssertSuccess(result); Assert.Equal(2, batch.Changes.Count); Assert.Equal(4, batch.Changes[0].Version); Assert.Equal(5, batch.Changes[1].Version); @@ -148,17 +147,11 @@ public void ProcessAllBatches_ProcessesMultipleBatches() _logger ); - var totalApplied = AssertSuccess(result); + var totalApplied = TestDb.AssertSuccess(result); Assert.Equal(10, totalApplied); Assert.Equal(4, appliedBatches.Count); Assert.Equal(10, lastVersion); } - private static T AssertSuccess(Result result) - { - Assert.IsType.Ok>(result); - return ((Result.Ok)result).Value; - } - public void Dispose() => _db.Dispose(); } diff --git a/Sync/Nimblesite.Sync.Tests/ChangeApplierTests.cs b/Sync/Nimblesite.Sync.Tests/ChangeApplierTests.cs index 3fbb404..7275721 100644 --- a/Sync/Nimblesite.Sync.Tests/ChangeApplierTests.cs +++ b/Sync/Nimblesite.Sync.Tests/ChangeApplierTests.cs @@ -1,6 +1,5 @@ using Microsoft.Data.Sqlite; using Microsoft.Extensions.Logging.Abstractions; -using Outcome; namespace Nimblesite.Sync.Tests; @@ -53,7 +52,7 @@ public void ApplyBatch_SkipsOwnOriginChanges() Logger ); - var applyResult = AssertSuccess(result); + var applyResult = TestDb.AssertSuccess(result); Assert.Single(appliedEntries); Assert.Equal("other-origin", appliedEntries[0].Origin); Assert.Equal(1, applyResult.AppliedCount); @@ -111,7 +110,7 @@ public void ApplyBatch_DefersAndRetriesFkViolations() Logger ); - var applyResult = AssertSuccess(result); + var applyResult = TestDb.AssertSuccess(result); Assert.Equal(2, applyResult.AppliedCount); Assert.Equal(2, attemptCounts[1]); // Child tried twice Assert.Equal(1, attemptCounts[2]); // Parent tried once @@ -184,7 +183,7 @@ public void ApplyBatch_WithRealSqlite_HandlesInsertUpdateDelete() var result = ChangeApplier.ApplyBatch(batch, "my-origin", 3, ApplyToDb, Logger); - var applyResult = AssertSuccess(result); + var applyResult = TestDb.AssertSuccess(result); Assert.Equal(2, applyResult.AppliedCount); // Verify final state @@ -270,11 +269,5 @@ private BoolSyncResult ApplyToDb(SyncLogEntry entry) } } - private static T AssertSuccess(Result result) - { - Assert.IsType.Ok>(result); - return ((Result.Ok)result).Value; - } - public void Dispose() => _db.Dispose(); } diff --git a/Sync/Nimblesite.Sync.Tests/LqlExpressionEvaluatorTests.cs b/Sync/Nimblesite.Sync.Tests/LqlExpressionEvaluatorTests.cs index a2da08a..030ce5b 100644 --- a/Sync/Nimblesite.Sync.Tests/LqlExpressionEvaluatorTests.cs +++ b/Sync/Nimblesite.Sync.Tests/LqlExpressionEvaluatorTests.cs @@ -177,7 +177,7 @@ public void MappingEngine_LqlTransform_AppliesUppercase() }; var mapping = CreateMappingWithColumns("user-mapping", "User", "Customer", columnMappings); - var config = new SyncMappingConfig("1.0", UnmappedTableBehavior.Strict, [mapping]); + var config = CreateStrictConfig(mapping); var entry = CreateEntry("User", """{"Id":"u1"}""", """{"Id":"u1","Name":"alice"}"""); var result = MappingEngine.ApplyMapping(entry, config, MappingDirection.Push, _logger); @@ -204,7 +204,7 @@ public void MappingEngine_LqlTransform_ConcatNamesForDifferentSchema() }; var mapping = CreateMappingWithColumns("user-mapping", "User", "Customer", columnMappings); - var config = new SyncMappingConfig("1.0", UnmappedTableBehavior.Strict, [mapping]); + var config = CreateStrictConfig(mapping); var entry = CreateEntry( "User", """{"Id":"u1"}""", @@ -230,7 +230,7 @@ public void MappingEngine_LqlTransform_ExtractDomainFromEmail() }; var mapping = CreateMappingWithColumns("user-mapping", "User", "Customer", columnMappings); - var config = new SyncMappingConfig("1.0", UnmappedTableBehavior.Strict, [mapping]); + var config = CreateStrictConfig(mapping); var entry = CreateEntry( "User", """{"Id":"u1"}""", @@ -260,7 +260,7 @@ public void MappingEngine_LqlTransform_NormalizeAndFormat() }; var mapping = CreateMappingWithColumns("user-mapping", "User", "Customer", columnMappings); - var config = new SyncMappingConfig("1.0", UnmappedTableBehavior.Strict, [mapping]); + var config = CreateStrictConfig(mapping); var entry = CreateEntry( "User", """{"Id":"u1"}""", @@ -292,7 +292,7 @@ public void MappingEngine_LqlTransform_FormatDatesForDifferentSystem() }; var mapping = CreateMappingWithColumns("user-mapping", "User", "Customer", columnMappings); - var config = new SyncMappingConfig("1.0", UnmappedTableBehavior.Strict, [mapping]); + var config = CreateStrictConfig(mapping); var entry = CreateEntry( "User", """{"Id":"u1"}""", @@ -325,7 +325,7 @@ public void MappingEngine_LqlTransform_MultipleTransformsInSameMapping() }; var mapping = CreateMappingWithColumns("user-mapping", "User", "Customer", columnMappings); - var config = new SyncMappingConfig("1.0", UnmappedTableBehavior.Strict, [mapping]); + var config = CreateStrictConfig(mapping); var entry = CreateEntry( "User", """{"Id":"u1"}""", @@ -353,7 +353,7 @@ public void MappingEngine_LqlTransform_WithConstantsAndLql() }; var mapping = CreateMappingWithColumns("user-mapping", "User", "Customer", columnMappings); - var config = new SyncMappingConfig("1.0", UnmappedTableBehavior.Strict, [mapping]); + var config = CreateStrictConfig(mapping); var entry = CreateEntry("User", """{"Id":"u1"}""", """{"Id":"u1","Name":"alice"}"""); var result = MappingEngine.ApplyMapping(entry, config, MappingDirection.Push, _logger); @@ -407,7 +407,7 @@ public void E2E_MobileToServerSchemaTransform() "ServerUser", columnMappings ); - var config = new SyncMappingConfig("1.0", UnmappedTableBehavior.Strict, [mapping]); + var config = CreateStrictConfig(mapping); var mobileEntry = CreateEntry( "MobileUser", @@ -469,7 +469,7 @@ public void E2E_LegacyCRMToModernERPTransform() "ModernCustomer", columnMappings ); - var config = new SyncMappingConfig("1.0", UnmappedTableBehavior.Strict, [mapping]); + var config = CreateStrictConfig(mapping); var legacyEntry = CreateEntry( "LegacyCustomer", @@ -521,7 +521,7 @@ public void E2E_OrderToAuditLogTransform() "AuditLog", columnMappings ); - var config = new SyncMappingConfig("1.0", UnmappedTableBehavior.Strict, [mapping]); + var config = CreateStrictConfig(mapping); var orderEntry = CreateEntry( "Order", @@ -619,5 +619,8 @@ private static SyncLogEntry CreateEntry(string table, string pk, string? payload Timestamp: "2024-01-01T00:00:00Z" ); + private static SyncMappingConfig CreateStrictConfig(TableMapping mapping) => + new("1.0", UnmappedTableBehavior.Strict, [mapping]); + #endregion } diff --git a/Sync/Nimblesite.Sync.Tests/SyncCoordinatorTests.cs b/Sync/Nimblesite.Sync.Tests/SyncCoordinatorTests.cs index 98255bb..f5e9133 100644 --- a/Sync/Nimblesite.Sync.Tests/SyncCoordinatorTests.cs +++ b/Sync/Nimblesite.Sync.Tests/SyncCoordinatorTests.cs @@ -762,29 +762,21 @@ INSERT INTO _sync_log (table_name, pk_value, operation, payload, origin, timesta cmd.ExecuteNonQuery(); } - private static string? GetPersonName(SqliteConnection db, string id) + private static string? GetName(SqliteConnection db, string table, string id) { using var cmd = db.CreateCommand(); - cmd.CommandText = "SELECT Name FROM Person WHERE Id = $id"; + cmd.CommandText = $"SELECT Name FROM {table} WHERE Id = $id"; cmd.Parameters.AddWithValue("$id", id); return cmd.ExecuteScalar() as string; } - private static string? GetParentName(SqliteConnection db, string id) - { - using var cmd = db.CreateCommand(); - cmd.CommandText = "SELECT Name FROM Parent WHERE Id = $id"; - cmd.Parameters.AddWithValue("$id", id); - return cmd.ExecuteScalar() as string; - } + private static string? GetPersonName(SqliteConnection db, string id) => + GetName(db, "Person", id); - private static string? GetChildName(SqliteConnection db, string id) - { - using var cmd = db.CreateCommand(); - cmd.CommandText = "SELECT Name FROM Child WHERE Id = $id"; - cmd.Parameters.AddWithValue("$id", id); - return cmd.ExecuteScalar() as string; - } + private static string? GetParentName(SqliteConnection db, string id) => + GetName(db, "Parent", id); + + private static string? GetChildName(SqliteConnection db, string id) => GetName(db, "Child", id); private static void SetLastSyncVersion(SqliteConnection db, long version) { diff --git a/Sync/Nimblesite.Sync.Tests/SyncStateTests.cs b/Sync/Nimblesite.Sync.Tests/SyncStateTests.cs index bd87af9..d841efe 100644 --- a/Sync/Nimblesite.Sync.Tests/SyncStateTests.cs +++ b/Sync/Nimblesite.Sync.Tests/SyncStateTests.cs @@ -255,10 +255,7 @@ public void BatchApplyResult_CreatesCorrectRecord() [Fact] public void SyncBatch_CreatesCorrectRecord() { - var changes = new List - { - new(1, "T", "PK", SyncOperation.Insert, "{}", "O", "TS"), - }; + var changes = SingleInsertChange(); var batch = new SyncBatch(changes, 0, 1, false); Assert.Single(batch.Changes); @@ -270,10 +267,7 @@ public void SyncBatch_CreatesCorrectRecord() [Fact] public void SyncBatch_HasMore_True() { - var changes = new List - { - new(1, "T", "PK", SyncOperation.Insert, "{}", "O", "TS"), - }; + var changes = SingleInsertChange(); var batch = new SyncBatch(changes, 0, 1, true); Assert.True(batch.HasMore); @@ -282,10 +276,7 @@ public void SyncBatch_HasMore_True() [Fact] public void SyncBatch_WithHash() { - var changes = new List - { - new(1, "T", "PK", SyncOperation.Insert, "{}", "O", "TS"), - }; + var changes = SingleInsertChange(); var batch = new SyncBatch(changes, 0, 1, false, "abc123hash"); Assert.Equal("abc123hash", batch.Hash); @@ -298,4 +289,7 @@ public void ConflictStrategy_HasCorrectValues() Assert.Equal(ConflictStrategy.ServerWins, (ConflictStrategy)1); Assert.Equal(ConflictStrategy.ClientWins, (ConflictStrategy)2); } + + private static IReadOnlyList SingleInsertChange() => + new List { new(1, "T", "PK", SyncOperation.Insert, "{}", "O", "TS") }; } diff --git a/Sync/Nimblesite.Sync.Tests/TestDb.cs b/Sync/Nimblesite.Sync.Tests/TestDb.cs index 525bfac..fa20bc0 100644 --- a/Sync/Nimblesite.Sync.Tests/TestDb.cs +++ b/Sync/Nimblesite.Sync.Tests/TestDb.cs @@ -1,4 +1,5 @@ using Microsoft.Data.Sqlite; +using Outcome; namespace Nimblesite.Sync.Tests; @@ -136,6 +137,12 @@ private static SyncOperation ParseOperation(string op) => _ => throw new ArgumentException($"Unknown operation: {op}"), }; + internal static T AssertSuccess(Result result) + { + Assert.IsType.Ok>(result); + return ((Result.Ok)result).Value; + } + public void Dispose() { Connection.Dispose();