From 099d17c0058c59a1d656322646913189987b88fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20=C3=96berg?= Date: Tue, 1 Sep 2026 13:40:53 +0200 Subject: [PATCH] Fix MutateOrNew required-key updates Validate supplied primary-key arguments against the immutable row's canonical identity without recording key assignments. Preserve missing-row construction and required non-key updates, with generator and provider-compliance regressions for scalar, composite, GUID, typed, and binary keys. --- .../ScalarConverterGeneratorTests.cs | 80 +++++++++++ .../Generator/GeneratorFileFactory.cs | 36 ++++- .../State/EmployeesMutationTests.cs | 129 ++++++++++++++++++ .../Core/GeneratorFileFactoryTests.cs | 29 ++++ 4 files changed, 269 insertions(+), 5 deletions(-) diff --git a/src/DataLinq.Generators.Tests/ScalarConverterGeneratorTests.cs b/src/DataLinq.Generators.Tests/ScalarConverterGeneratorTests.cs index 0cd6d795..2fe6d696 100644 --- a/src/DataLinq.Generators.Tests/ScalarConverterGeneratorTests.cs +++ b/src/DataLinq.Generators.Tests/ScalarConverterGeneratorTests.cs @@ -71,6 +71,86 @@ await Assert.That(code).Contains( "global::DataLinq.Instances.KeyFactory.CreateKeyFromModelValues([tenant, id], [DataLinqColumn_Tenant, DataLinqColumn_Id])"); await Assert.That(code).Contains( "public static ScalarRow? Get(int tenant, CustomerId id, IDataSourceAccess dataSource)"); + await Assert.That(code).Contains( + "KeyFactory.CreateKeyFromModelValue(tenant, ScalarRow.DataLinqColumn_Tenant).Equals("); + await Assert.That(code).Contains( + "DataLinqKey.FromValue(model.PrimaryKeys().GetValue(0))"); + await Assert.That(code).Contains( + "KeyFactory.CreateKeyFromModelValue(id, ScalarRow.DataLinqColumn_Id).Equals("); + await Assert.That(code).Contains( + "DataLinqKey.FromValue(model.PrimaryKeys().GetValue(1))"); + await Assert.That(code).DoesNotContain("mutable.Tenant = tenant;"); + await Assert.That(code).DoesNotContain("mutable.Id = id;"); + } + + [Test] + public async Task MutateOrNew_UsesCanonicalComparisonForGuidTypedAndBinaryKeyComponents() + { + var source = Parse( + """ + using DataLinq; + using DataLinq.Attributes; + using DataLinq.Instances; + using DataLinq.Interfaces; + using DataLinq.Mutation; + + namespace ScalarGenerator; + + public readonly record struct CustomerId(int Value); + + public sealed class CustomerIdConverter : DataLinqScalarConverter + { + public override int ToProvider(CustomerId value, in ScalarConversionContext context) => value.Value; + public override CustomerId FromProvider(int value, in ScalarConversionContext context) => new(value); + } + + [Database("mixed_keys")] + public partial class MixedKeyDb(DataSourceAccess dataSource) : IDatabaseModel + { + public DbRead Rows { get; } = new(dataSource); + } + + [Table("mixed_key_rows")] + public abstract partial class MixedKeyRow(IRowData rowData, IDataSourceAccess dataSource) + : Immutable(rowData, dataSource), ITableModel + { + [PrimaryKey] + [Type(DatabaseType.SQLite, "TEXT")] + [GuidStorage(GuidStorageFormat.Text36)] + [Column("guid_key")] + public abstract System.Guid GuidKey { get; } + + [PrimaryKey] + [Type(DatabaseType.SQLite, "INTEGER")] + [ScalarConverter(typeof(CustomerIdConverter))] + [Column("typed_key")] + public abstract CustomerId TypedKey { get; } + + [PrimaryKey] + [Type(DatabaseType.SQLite, "BLOB")] + [Column("binary_key")] + public abstract byte[] BinaryKey { get; } + + [Type(DatabaseType.SQLite, "TEXT")] + [Column("name")] + public abstract string Name { get; } + } + """); + + var (outputCompilation, diagnostics, generatedTrees) = RunGeneratorAgainstRuntimeWithDiagnostics([source]); + var code = string.Join(Environment.NewLine, generatedTrees.Select(static tree => tree.ToString())); + + await AssertNoErrors(outputCompilation, diagnostics); + await Assert.That(code).Contains( + "KeyFactory.CreateKeyFromModelValue(guidKey, MixedKeyRow.DataLinqColumn_GuidKey).Equals("); + await Assert.That(code).Contains( + "KeyFactory.CreateKeyFromModelValue(typedKey, MixedKeyRow.DataLinqColumn_TypedKey).Equals("); + await Assert.That(code).Contains( + "KeyFactory.CreateKeyFromModelValue(binaryKey, MixedKeyRow.DataLinqColumn_BinaryKey).Equals("); + await Assert.That(code).Contains("mutable.Name = name;"); + await Assert.That(code).DoesNotContain("mutable.GuidKey = guidKey;"); + await Assert.That(code).DoesNotContain("mutable.TypedKey = typedKey;"); + await Assert.That(code).DoesNotContain("mutable.BinaryKey = binaryKey;"); } [Test] diff --git a/src/DataLinq.SharedCore/Factories/Generator/GeneratorFileFactory.cs b/src/DataLinq.SharedCore/Factories/Generator/GeneratorFileFactory.cs index 6e0b3778..ae1badfa 100644 --- a/src/DataLinq.SharedCore/Factories/Generator/GeneratorFileFactory.cs +++ b/src/DataLinq.SharedCore/Factories/Generator/GeneratorFileFactory.cs @@ -1433,12 +1433,38 @@ private IEnumerable ExtensionMethodsFileContents(ModelDefinition model, { var constructorParams = requiredProps.Select(GetConstructorParam).ToJoinedString(", "); var constructorArgs = requiredProps.Select(v => v.Column.ValueProperty.PropertyName.ToCamelCase()).ToJoinedString(", "); - - yield return $"{namespaceTab}{tab}public static Mutable{model.CsType.Name} MutateOrNew(this {model.CsType.Name}{GetUseNullableReferenceTypes()} model, {constructorParams}) => model is null ? new Mutable{model.CsType.Name}({constructorArgs}) : model.Mutate(x =>"; + var requiredPrimaryKeys = model.Table.PrimaryKeyColumns + .Select((column, index) => (Property: column.ValueProperty, Index: index)) + .Where(primaryKey => requiredProps.Contains(primaryKey.Property)) + .ToList(); + var requiredNonPrimaryKeys = requiredProps + .Where(property => !property.Column.PrimaryKey) + .ToList(); + + yield return $"{namespaceTab}{tab}public static Mutable{model.CsType.Name} MutateOrNew(this {model.CsType.Name}{GetUseNullableReferenceTypes()} model, {constructorParams})"; yield return $"{namespaceTab}{tab}{{"; - foreach (var v in requiredProps) - yield return $"{namespaceTab}{tab}{tab}x.{v.PropertyName} = {v.Column.ValueProperty.PropertyName.ToCamelCase()};"; - yield return $"{namespaceTab}{tab}}});"; + yield return $"{namespaceTab}{tab}{tab}if (model is null)"; + yield return $"{namespaceTab}{tab}{tab}{tab}return new Mutable{model.CsType.Name}({constructorArgs});"; + + foreach (var primaryKey in requiredPrimaryKeys) + { + var property = primaryKey.Property; + var parameterName = property.PropertyName.ToCamelCase(); + var columnHandle = GetGeneratedColumnHandleName(property); + var message = $"Primary-key argument '{parameterName}' does not match the existing {model.CsType.Name} row's authoritative key."; + + yield return ""; + yield return $"{namespaceTab}{tab}{tab}if (!global::DataLinq.Instances.KeyFactory.CreateKeyFromModelValue({parameterName}, {model.CsType.Name}.{columnHandle}).Equals("; + yield return $"{namespaceTab}{tab}{tab}{tab}global::DataLinq.Instances.DataLinqKey.FromValue(model.PrimaryKeys().GetValue({primaryKey.Index.ToString(CultureInfo.InvariantCulture)}))))"; + yield return $"{namespaceTab}{tab}{tab}{tab}throw new global::System.ArgumentException({FormatStringLiteral(message)}, nameof({parameterName}));"; + } + + yield return ""; + yield return $"{namespaceTab}{tab}{tab}var mutable = new Mutable{model.CsType.Name}(model);"; + foreach (var property in requiredNonPrimaryKeys) + yield return $"{namespaceTab}{tab}{tab}mutable.{property.PropertyName} = {property.PropertyName.ToCamelCase()};"; + yield return $"{namespaceTab}{tab}{tab}return mutable;"; + yield return $"{namespaceTab}{tab}}}"; yield return $"{namespaceTab}{tab}public static Mutable{model.CsType.Name} MutateOrNew(this {model.CsType.Name}{GetUseNullableReferenceTypes()} model, {constructorParams}, Action changes) => model.MutateOrNew({constructorArgs}).Mutate(changes);"; } else diff --git a/src/DataLinq.Tests.Compliance/State/EmployeesMutationTests.cs b/src/DataLinq.Tests.Compliance/State/EmployeesMutationTests.cs index 0f585dcb..6e7c2e6f 100644 --- a/src/DataLinq.Tests.Compliance/State/EmployeesMutationTests.cs +++ b/src/DataLinq.Tests.Compliance/State/EmployeesMutationTests.cs @@ -54,6 +54,120 @@ public async Task Mutation_MutateOrNewOnMissingModel_ReturnsNewMutableEmployee(T await Assert.That(employee.IsNew()).IsTrue(); } + [Test] + [Property(TestProviderAffinity.PropertyName, TestProviderAffinity.EveryProvider)] + [MethodDataSource(typeof(TestProviderDataSources), nameof(TestProviderDataSources.ActiveProviders))] + public async Task Mutation_MutateOrNewOnExistingRequiredKey_UpdatesWithoutTrackingPrimaryKey(TestProviderDescriptor provider) + { + using var databaseScope = EmployeesTestDatabase.CreateIsolated( + provider, + nameof(Mutation_MutateOrNewOnExistingRequiredKey_UpdatesWithoutTrackingPrimaryKey), + EmployeesFixtureProfile.TinySeeded); + + var database = databaseScope.Database; + var department = database.Query().Departments.OrderBy(row => row.DeptNo).First(); + var updatedName = $"Updated {department.DeptNo}"; + + var mutable = department.MutateOrNew( + deptNo: department.DeptNo, + name: updatedName); + + await Assert.That(mutable.GetChanges().Any(change => change.Key.PrimaryKey)).IsFalse(); + + var saved = mutable.Save(database); + + await Assert.That(saved.DeptNo).IsEqualTo(department.DeptNo); + await Assert.That(saved.Name).IsEqualTo(updatedName); + } + + [Test] + [Property(TestProviderAffinity.PropertyName, TestProviderAffinity.EveryProvider)] + [MethodDataSource(typeof(TestProviderDataSources), nameof(TestProviderDataSources.ActiveProviders))] + public async Task Mutation_MutateOrNewOnExistingRequiredKey_RejectsDifferentPrimaryKeyImmediately(TestProviderDescriptor provider) + { + using var databaseScope = EmployeesTestDatabase.CreateIsolated( + provider, + nameof(Mutation_MutateOrNewOnExistingRequiredKey_RejectsDifferentPrimaryKeyImmediately), + EmployeesFixtureProfile.TinySeeded); + + var department = databaseScope.Database.Query().Departments + .OrderBy(row => row.DeptNo) + .First(); + + var exception = Capture(() => department.MutateOrNew( + deptNo: "z112", + name: department.Name)); + + await Assert.That(exception.ParamName).IsEqualTo("deptNo"); + await Assert.That(exception.Message).Contains("authoritative key"); + } + + [Test] + [Property(TestProviderAffinity.PropertyName, TestProviderAffinity.EveryProvider)] + [MethodDataSource(typeof(TestProviderDataSources), nameof(TestProviderDataSources.ActiveProviders))] + public async Task Mutation_MutateOrNewOnMissingRequiredKey_InsertsSuppliedPrimaryKey(TestProviderDescriptor provider) + { + using var databaseScope = EmployeesTestDatabase.CreateIsolated( + provider, + nameof(Mutation_MutateOrNewOnMissingRequiredKey_InsertsSuppliedPrimaryKey), + EmployeesFixtureProfile.TinySeeded); + + const string departmentNumber = "z112"; + const string departmentName = "Issue 112"; + var database = databaseScope.Database; + var missing = database.Query().Departments + .FirstOrDefault(row => row.DeptNo == departmentNumber); + + var mutable = missing.MutateOrNew( + deptNo: departmentNumber, + name: departmentName); + var saved = mutable.Save(database); + + await Assert.That(saved.DeptNo).IsEqualTo(departmentNumber); + await Assert.That(saved.Name).IsEqualTo(departmentName); + } + + [Test] + [Property(TestProviderAffinity.PropertyName, TestProviderAffinity.EveryProvider)] + [MethodDataSource(typeof(TestProviderDataSources), nameof(TestProviderDataSources.ActiveProviders))] + public async Task Mutation_MutateOrNewOnExistingCompositeKey_ValidatesAllKeyComponents(TestProviderDescriptor provider) + { + using var databaseScope = EmployeesTestDatabase.CreateIsolated( + provider, + nameof(Mutation_MutateOrNewOnExistingCompositeKey_ValidatesAllKeyComponents), + EmployeesFixtureProfile.TinySeeded); + + var database = databaseScope.Database; + var assignment = database.Query().DepartmentEmployees + .OrderBy(row => row.dept_no) + .ThenBy(row => row.emp_no) + .First(); + var updatedToDate = assignment.to_date.AddDays(-1); + + var mutable = assignment.MutateOrNew( + deptNo: assignment.dept_no, + empNo: assignment.emp_no, + fromDate: assignment.from_date, + toDate: updatedToDate); + + await Assert.That(mutable.GetChanges().Any(change => change.Key.PrimaryKey)).IsFalse(); + + var saved = mutable.Save(database); + + await Assert.That(saved.dept_no).IsEqualTo(assignment.dept_no); + await Assert.That(saved.emp_no).IsEqualTo(assignment.emp_no); + await Assert.That(saved.to_date).IsEqualTo(updatedToDate); + + var exception = Capture(() => assignment.MutateOrNew( + deptNo: assignment.dept_no, + empNo: assignment.emp_no + 1, + fromDate: assignment.from_date, + toDate: assignment.to_date)); + + await Assert.That(exception.ParamName).IsEqualTo("empNo"); + await Assert.That(exception.Message).Contains("authoritative key"); + } + [Test] [Property(TestProviderAffinity.PropertyName, TestProviderAffinity.EveryProvider)] [MethodDataSource(typeof(TestProviderDataSources), nameof(TestProviderDataSources.ActiveProviders))] @@ -158,4 +272,19 @@ private static async Task AssertThrows(Action action) await Assert.That(threw).IsTrue(); } + + private static TException Capture(Action action) + where TException : Exception + { + try + { + action(); + } + catch (TException exception) + { + return exception; + } + + throw new InvalidOperationException($"Expected {typeof(TException).Name}."); + } } diff --git a/src/DataLinq.Tests.Unit/Core/GeneratorFileFactoryTests.cs b/src/DataLinq.Tests.Unit/Core/GeneratorFileFactoryTests.cs index 322207d4..e6af2097 100644 --- a/src/DataLinq.Tests.Unit/Core/GeneratorFileFactoryTests.cs +++ b/src/DataLinq.Tests.Unit/Core/GeneratorFileFactoryTests.cs @@ -144,6 +144,28 @@ public async Task CreateModelFiles_ExplicitNullableOptOut_CompositePrimaryKey_Do await Assert.That(generatedFile.contents).DoesNotContain("out global::DataLinq.Instances.IImmutableInstance? row)"); } + [Test] + public async Task CreateModelFiles_MutateOrNew_ValidatesRequiredPrimaryKeysWithoutAssigningThem() + { + var database = CreateDatabaseWithCompositePrimaryKey(); + + var generatedFile = new GeneratorFileFactory(new GeneratorFileFactoryOptions()) + .CreateModelFiles(database) + .Single(file => file.path == "CompositeModel.cs"); + + await Assert.That(generatedFile.contents).Contains( + "KeyFactory.CreateKeyFromModelValue(firstId, CompositeModel.DataLinqColumn_FirstId).Equals("); + await Assert.That(generatedFile.contents).Contains( + "DataLinqKey.FromValue(model.PrimaryKeys().GetValue(0))"); + await Assert.That(generatedFile.contents).Contains( + "KeyFactory.CreateKeyFromModelValue(secondId, CompositeModel.DataLinqColumn_SecondId).Equals("); + await Assert.That(generatedFile.contents).Contains( + "DataLinqKey.FromValue(model.PrimaryKeys().GetValue(1))"); + await Assert.That(generatedFile.contents).Contains("mutable.Name = name;"); + await Assert.That(generatedFile.contents).DoesNotContain("mutable.FirstId = firstId;"); + await Assert.That(generatedFile.contents).DoesNotContain("mutable.SecondId = secondId;"); + } + [Test] public async Task CreateModelFiles_Model_EmitsIndexedGeneratedAccess() { @@ -581,6 +603,13 @@ private static DatabaseDefinition CreateDatabaseWithCompositePrimaryKey() { PrimaryKey = true, DbTypes = [new DatabaseColumnType(DatabaseType.MySQL, "int")] + }), + new MetadataValuePropertyDraft( + "Name", + new CsTypeDeclaration(typeof(string)), + new MetadataColumnDraft("name") + { + DbTypes = [new DatabaseColumnType(DatabaseType.MySQL, "varchar", 100)] }) ] },