Paymentsense / Dapper.SimpleSave

Saving of new and updated data using Dapper
MIT License
31 stars 12 forks source link

Attempt to INSERT object with nested reference data children fails with InvalidOperationException #29

Open bartread opened 9 years ago

bartread commented 9 years ago

In our case we've got types referencing eachother like this: UserDto > PhoneNumberDto > CountryDto > CurrencyCodeDto.

PhoneNumberDto isn't reference data, but is treated as such because the relevant properties on UserDto are marked as reference data, like so:

[Column("OfficeNumberKey")]
[OneToOne]
[ForeignKeyReference(typeof(PhoneNumberDto)]
[ReferenceData]
public PhoneNumberDto OfficeNumber { get; set; }

CountryDto and CurrencyCodeDto are both reference data and marked as such at the type level with [ReferenceData] attribute decorations.

The problem occurs when you try to INSERT a UserDto. Now SimpleSave 1.0.65 correctly skips the PhoneNumberDto but, of course, it's continued to diff all the way down beyond that, so generates an UpdateOperation, which gets caught later as invalid, and leads to the following error:

System.InvalidOperationException : You cannot perform UPDATEs on a reference data table with no updateable foreign keys. Attempt was made to UPDATE table [gen].[COUNTRY_ENUM], column CurrencyCodeKey.
  at Dapper.SimpleSave.Impl.CommandBuilder.ValidateUpdateOperation(UpdateOperation update)
  at Dapper.SimpleSave.Impl.CommandBuilder.Coalesce(IEnumerable`1 operations)
  at Dapper.SimpleSave.Impl.TransactionBuilder.BuildUpdateScripts(T oldObject, T newObject, Boolean softDelete)
  at Dapper.SimpleSave.SimpleSaveExtensions.UpdateInternal(IDbConnection connection, IEnumerable`1 oldAndNewObjects, Boolean softDelete, IDbTransaction transaction)
  at Dapper.SimpleSave.SimpleSaveExtensions.UpdateAll(IDbConnection connection, IEnumerable`1 oldAndNewObjects, IDbTransaction transaction)
  at Dapper.SimpleSave.SimpleSaveExtensions.Update(IDbConnection connection, T oldObject, T newObject, IDbTransaction transaction)
  at PS.Mothership.Domain.UserManagement.UserRepositoryV2.SaveUserInternal(IDbConnection connection, UserDto oldUser, UserDto newUser, IUserInputFilter[] filters) in UserRepositoryV2.cs: line 183
  at PS.Mothership.Domain.UserManagement.UserRepositoryV2.<>c__DisplayClassa.<SaveUser>b__8(IDbConnection connection) in UserRepositoryV2.cs: line 167
  at PS.Mothership.Repository.Implementations.BaseCustomQueryDapperRepository.Execute(Func`2 work) in BaseCustomQueryDapperRepository.cs: line 21
  at PS.Mothership.Domain.UserManagement.UserRepositoryV2.SaveUser(UserDto oldVersionOfUser, UserDto newVersionOfUser, IUserInputFilter[] filters) in UserRepositoryV2.cs: line 167
  at PS.Mothership.Domain.UserManagement.HrUserUpdateService.Update(LegacyHrUpdateUserDto hrUser) in HrUserUpdateService.cs: line 105
  at PS.Mothership.IntegrationTest.HumanResources.HrIntegrationUserTests.hr_can_create_and_update_user() in HrIntegrationUserTests.cs: line 109

It's good that the problem is caught and the error thrown, but the fact remains that operations for tables that are children of the initial reference data table should not be generated.

This actually needs to be fixed at the diff level, before we start generating operations because, by that point, the hierarchy information isn't available in an easy to digest form.

bartread commented 9 years ago

This is really weird. I've spent a bit of time trying to reproduce this, even going to the extent of copying the exact object definitions that fail in PS and duplicating them in the test case, and it fails, but for a different reason. Rather than in the script generation, which is where I'd expect it to break, it's failing (predictably) when it tries to execute the SQL because we don't have a real, open connection to a database.

I'm quite certain I know how to fix it but I don't want to go ahead and do that, ideally, without a test in place that demonstrates it's now fixed.

bartread commented 9 years ago

This appears to be fixed but it would still be nice to have a test that reproduces it and protects against regression.