From c66041e6a4c94cb0e6eadb8eab27c05afb0fed15 Mon Sep 17 00:00:00 2001 From: Joost Boomkamp Date: Tue, 16 Oct 2018 14:05:51 +0200 Subject: [PATCH 1/7] Update AbstractEntityPersister.cs I wasted a morning figuring this out. A more explicit exception would have helped me tremendously. see https://stackoverflow.com/questions/10689054/fluent-nhibernate-indexoutofrange/52834741#52834741 --- src/NHibernate/Persister/Entity/AbstractEntityPersister.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/NHibernate/Persister/Entity/AbstractEntityPersister.cs b/src/NHibernate/Persister/Entity/AbstractEntityPersister.cs index aba30452de9..dd717bea66d 100644 --- a/src/NHibernate/Persister/Entity/AbstractEntityPersister.cs +++ b/src/NHibernate/Persister/Entity/AbstractEntityPersister.cs @@ -2456,6 +2456,10 @@ protected int Dehydrate(object id, object[] fields, object rowId, bool[] include PropertyTypes[i].NullSafeSet(statement, fields[i], index, includeColumns[i], session); index += ArrayHelper.CountTrue(includeColumns[i]); //TODO: this is kinda slow... } + catch (ArgumentOutOfRangeException arex) + { + throw new PropertyValueException("Column count does not match property count (Duplicate column mapping?) for", EntityName, entityMetamodel.PropertyNames[i], ex); + } catch (Exception ex) { throw new PropertyValueException("Error dehydrating property value for", EntityName, entityMetamodel.PropertyNames[i], ex); From b6f0c89b1e5fc0fffa52c8e9067a288ade2720b4 Mon Sep 17 00:00:00 2001 From: Joost Boomkamp Date: Tue, 16 Oct 2018 17:11:50 +0200 Subject: [PATCH 2/7] I wasted a morning figuring this out. A more explicit exception would have helped me tremendously. see https://stackoverflow.com/questions/10689054/fluent-nhibernate-indexoutofrange/52834741#52834741 --- .../Fixture.cs | 125 ++++++++++++++++++ .../Entity/AbstractEntityPersister.cs | 4 +- 2 files changed, 126 insertions(+), 3 deletions(-) create mode 100644 src/NHibernate.Test/Async/NHSpecificTest/MultipleFieldsMappedToOneColumn/Fixture.cs diff --git a/src/NHibernate.Test/Async/NHSpecificTest/MultipleFieldsMappedToOneColumn/Fixture.cs b/src/NHibernate.Test/Async/NHSpecificTest/MultipleFieldsMappedToOneColumn/Fixture.cs new file mode 100644 index 00000000000..68cfa5f6c1b --- /dev/null +++ b/src/NHibernate.Test/Async/NHSpecificTest/MultipleFieldsMappedToOneColumn/Fixture.cs @@ -0,0 +1,125 @@ +using System; +using NHibernate.Cfg.MappingSchema; +using NHibernate.Mapping.ByCode; +using NUnit.Framework; + +namespace NHibernate.Test.Async.NHSpecificTest.MultipleFieldsMappedToOneColumn +{ + public class BadlyMappedEntity + { + public virtual long Id { get; set; } + public virtual long FirstValue { get; set; } + public virtual long SecondValue {get; set; } + } + + public class CorrectlyMappedEntity + { + public virtual long Id { get; set; } + public virtual long FirstValue { get; set; } + public virtual long SecondValue {get; set; } + } + + [TestFixture] + public class FixtureAsync : TestCaseMappingByCode + { + protected override HbmMapping GetMappings() + { + var mapper = new ModelMapper(); + mapper.Class(ca => + { + ca.Abstract(true); + ca.Id(x => x.Id, map => + { + map.Column("BadlyMappedEntityId"); + map.Generator(Generators.Native); + }); + ca.Property(x => x.FirstValue, map => map.Column("SameColumn")); + ca.Property(x => x.SecondValue, map => map.Column("SameColumn")); + // SecondValue is mapped badly, this gives the AbstractEntityMapper more entries in the fields array + // than there are in the includeColumns array; this causes the index to fall out of bounds. + }); + + mapper.Class(ca => + { + ca.Abstract(true); + ca.Id(x => x.Id, map => + { + map.Column("BadlyMappedEntityId"); + map.Generator(Generators.Native); + }); + ca.Property(x => x.FirstValue, map => map.Column("SameColumn")); + ca.Property(x => x.SecondValue, map => map.Column("OtherColumn")); + // SecondValue is mapped correctly, should not break + }); + + return mapper.CompileMappingFor(new[] { typeof(BadlyMappedEntity), typeof(CorrectlyMappedEntity) }); + } + + [Test] + protected void ShouldThrowAREXForBadlyMappedEntity() + { + // arrange + ArgumentOutOfRangeException result = null; + var e = new BadlyMappedEntity + { + FirstValue = 1, + SecondValue = 2 + }; + + // act + try + { + using (var session = OpenSession()) + using (var transaction = session.BeginTransaction()) + { + + session.Save(e); + transaction.Commit(); + } + } + catch (ArgumentOutOfRangeException arex) + { + result = arex; + } + + // assert + Assert.That(result != null); + Assert.That(result.Message.Contains("(Duplicate column mapping?)")); + } + + [Test] + protected void ShouldNotThrow() + { + // arrange + ArgumentOutOfRangeException result = null; + var e = new BadlyMappedEntity + { + FirstValue = 1, + SecondValue = 2 + }; + + // act + try + { + using (var session = OpenSession()) + using (var transaction = session.BeginTransaction()) + { + + session.Save(e); + transaction.Commit(); + } + } + catch (ArgumentOutOfRangeException arex) + { + result = arex; + } + + // assert + Assert.That(result == null); + Assert.That(e.Id > 0); + + // cleanup after Test + OpenSession().Delete(e); + } + } +} diff --git a/src/NHibernate/Persister/Entity/AbstractEntityPersister.cs b/src/NHibernate/Persister/Entity/AbstractEntityPersister.cs index dd717bea66d..f7c6ac7ba30 100644 --- a/src/NHibernate/Persister/Entity/AbstractEntityPersister.cs +++ b/src/NHibernate/Persister/Entity/AbstractEntityPersister.cs @@ -4,7 +4,6 @@ using System.Data; using System.Data.Common; using System.Text; - using NHibernate.AdoNet; using NHibernate.Cache; using NHibernate.Cache.Entry; @@ -20,7 +19,6 @@ using NHibernate.Metadata; using NHibernate.Properties; using NHibernate.SqlCommand; -using NHibernate.Tuple; using NHibernate.Tuple.Entity; using NHibernate.Type; using NHibernate.Util; @@ -2458,7 +2456,7 @@ protected int Dehydrate(object id, object[] fields, object rowId, bool[] include } catch (ArgumentOutOfRangeException arex) { - throw new PropertyValueException("Column count does not match property count (Duplicate column mapping?) for", EntityName, entityMetamodel.PropertyNames[i], ex); + throw new PropertyValueException("Column count does not match property count (Duplicate column mapping?) for", EntityName, entityMetamodel.PropertyNames[i], arex); } catch (Exception ex) { From 914eaa206ada87ed9d878342678c53381b67e9c2 Mon Sep 17 00:00:00 2001 From: Joost Boomkamp Date: Tue, 16 Oct 2018 17:24:30 +0200 Subject: [PATCH 3/7] public tests --- .../NHSpecificTest/MultipleFieldsMappedToOneColumn/Fixture.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/NHibernate.Test/Async/NHSpecificTest/MultipleFieldsMappedToOneColumn/Fixture.cs b/src/NHibernate.Test/Async/NHSpecificTest/MultipleFieldsMappedToOneColumn/Fixture.cs index 68cfa5f6c1b..2a393176700 100644 --- a/src/NHibernate.Test/Async/NHSpecificTest/MultipleFieldsMappedToOneColumn/Fixture.cs +++ b/src/NHibernate.Test/Async/NHSpecificTest/MultipleFieldsMappedToOneColumn/Fixture.cs @@ -56,7 +56,7 @@ protected override HbmMapping GetMappings() } [Test] - protected void ShouldThrowAREXForBadlyMappedEntity() + public void ShouldThrowAREXForBadlyMappedEntity() { // arrange ArgumentOutOfRangeException result = null; @@ -88,7 +88,7 @@ protected void ShouldThrowAREXForBadlyMappedEntity() } [Test] - protected void ShouldNotThrow() + public void ShouldNotThrow() { // arrange ArgumentOutOfRangeException result = null; From d477e0be63fd27a528d683b24f2f90a617fb887b Mon Sep 17 00:00:00 2001 From: Joost Boomkamp Date: Wed, 17 Oct 2018 11:39:12 +0200 Subject: [PATCH 4/7] moved unit tests according to contribution rules --- .../GH1875}/Fixture.cs | 39 +++++++++---------- 1 file changed, 19 insertions(+), 20 deletions(-) rename src/NHibernate.Test/{Async/NHSpecificTest/MultipleFieldsMappedToOneColumn => NHSpecificTest/GH1875}/Fixture.cs (80%) diff --git a/src/NHibernate.Test/Async/NHSpecificTest/MultipleFieldsMappedToOneColumn/Fixture.cs b/src/NHibernate.Test/NHSpecificTest/GH1875/Fixture.cs similarity index 80% rename from src/NHibernate.Test/Async/NHSpecificTest/MultipleFieldsMappedToOneColumn/Fixture.cs rename to src/NHibernate.Test/NHSpecificTest/GH1875/Fixture.cs index 2a393176700..7253bf7f520 100644 --- a/src/NHibernate.Test/Async/NHSpecificTest/MultipleFieldsMappedToOneColumn/Fixture.cs +++ b/src/NHibernate.Test/NHSpecificTest/GH1875/Fixture.cs @@ -1,9 +1,10 @@ using System; using NHibernate.Cfg.MappingSchema; using NHibernate.Mapping.ByCode; +using NSubstitute.ExceptionExtensions; using NUnit.Framework; -namespace NHibernate.Test.Async.NHSpecificTest.MultipleFieldsMappedToOneColumn +namespace NHibernate.Test.NHSpecificTest.GH1875 { public class BadlyMappedEntity { @@ -20,7 +21,7 @@ public class CorrectlyMappedEntity } [TestFixture] - public class FixtureAsync : TestCaseMappingByCode + public class Fixture : TestCaseMappingByCode { protected override HbmMapping GetMappings() { @@ -59,32 +60,30 @@ protected override HbmMapping GetMappings() public void ShouldThrowAREXForBadlyMappedEntity() { // arrange - ArgumentOutOfRangeException result = null; - var e = new BadlyMappedEntity + var bad = new BadlyMappedEntity { FirstValue = 1, SecondValue = 2 }; + // Bobbytables1337 - // act - try - { - using (var session = OpenSession()) - using (var transaction = session.BeginTransaction()) - { - - session.Save(e); - transaction.Commit(); - } - } - catch (ArgumentOutOfRangeException arex) + // assert + Assert.That( + () => SaveEntity(bad), + Throws.TypeOf().And.Message.Contains("(Duplicate column mapping?)")); + } + + private bool SaveEntity(T entity) + { + using (var session = OpenSession()) + using (var transaction = session.BeginTransaction()) { - result = arex; + + session.Save(entity); + transaction.Commit(); } - // assert - Assert.That(result != null); - Assert.That(result.Message.Contains("(Duplicate column mapping?)")); + return true; } [Test] From 4b6a66c98a2190c8b7a595974238e3ab4c8d8146 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Delaporte?= <12201973+fredericdelaporte@users.noreply.github.com> Date: Mon, 29 Oct 2018 18:08:42 +0100 Subject: [PATCH 5/7] Clean the test and generate async --- .../Async/NHSpecificTest/GH1875/Fixture.cs | 76 ++++++++++++ .../NHSpecificTest/GH1875/Fixture.cs | 109 +++++------------- .../Entity/AbstractEntityPersister.cs | 6 +- 3 files changed, 108 insertions(+), 83 deletions(-) create mode 100644 src/NHibernate.Test/Async/NHSpecificTest/GH1875/Fixture.cs diff --git a/src/NHibernate.Test/Async/NHSpecificTest/GH1875/Fixture.cs b/src/NHibernate.Test/Async/NHSpecificTest/GH1875/Fixture.cs new file mode 100644 index 00000000000..3af9edd5aab --- /dev/null +++ b/src/NHibernate.Test/Async/NHSpecificTest/GH1875/Fixture.cs @@ -0,0 +1,76 @@ +//------------------------------------------------------------------------------ +// +// This code was generated by AsyncGenerator. +// +// Changes to this file may cause incorrect behavior and will be lost if +// the code is regenerated. +// +//------------------------------------------------------------------------------ + + +using System; +using NHibernate.Cfg.MappingSchema; +using NHibernate.Mapping.ByCode; +using NUnit.Framework; + +namespace NHibernate.Test.NHSpecificTest.GH1875 +{ + using System.Threading.Tasks; + + [TestFixture] + public class FixtureAsync : TestCaseMappingByCode + { + protected override HbmMapping GetMappings() + { + var mapper = new ModelMapper(); + mapper.Class( + ca => + { + ca.Abstract(true); + ca.Id( + x => x.Id, + map => + { + map.Column("BadlyMappedEntityId"); + map.Generator(Generators.GuidComb); + }); + ca.Property(x => x.FirstValue, map => map.Column("SameColumn")); + // SecondValue is mapped with same name as another column, this gives the AbstractEntityMapper + // more entries in the fields array than there are in the includeColumns array; this causes the + // index to fall out of bounds. + ca.Property(x => x.SecondValue, map => map.Column("SameColumn")); + }); + + return mapper.CompileMappingFor(new[] { typeof(BadlyMappedEntity) }); + } + + [Test] + public async Task ShouldThrowSoundErrorForBadlyMappedEntityAsync() + { + var bad = new BadlyMappedEntity + { + FirstValue = 1, + SecondValue = 2 + }; + + using (var session = OpenSession()) + using (var transaction = session.BeginTransaction()) + { + await (session.SaveAsync(bad)); + Assert.That( + transaction.Commit, + Throws.TypeOf().And.Message.Contains("(Duplicate column mapping?)")); + } + } + + protected override void OnTearDown() + { + using (var session = OpenSession()) + using (var transaction = session.BeginTransaction()) + { + session.CreateQuery("delete from System.Object").ExecuteUpdate(); + transaction.Commit(); + } + } + } +} diff --git a/src/NHibernate.Test/NHSpecificTest/GH1875/Fixture.cs b/src/NHibernate.Test/NHSpecificTest/GH1875/Fixture.cs index 7253bf7f520..07485a0c3fa 100644 --- a/src/NHibernate.Test/NHSpecificTest/GH1875/Fixture.cs +++ b/src/NHibernate.Test/NHSpecificTest/GH1875/Fixture.cs @@ -1,23 +1,15 @@ using System; using NHibernate.Cfg.MappingSchema; using NHibernate.Mapping.ByCode; -using NSubstitute.ExceptionExtensions; using NUnit.Framework; namespace NHibernate.Test.NHSpecificTest.GH1875 { public class BadlyMappedEntity { - public virtual long Id { get; set; } + public virtual Guid Id { get; set; } public virtual long FirstValue { get; set; } - public virtual long SecondValue {get; set; } - } - - public class CorrectlyMappedEntity - { - public virtual long Id { get; set; } - public virtual long FirstValue { get; set; } - public virtual long SecondValue {get; set; } + public virtual long SecondValue { get; set; } } [TestFixture] @@ -26,99 +18,54 @@ public class Fixture : TestCaseMappingByCode protected override HbmMapping GetMappings() { var mapper = new ModelMapper(); - mapper.Class(ca => - { - ca.Abstract(true); - ca.Id(x => x.Id, map => + mapper.Class( + ca => { - map.Column("BadlyMappedEntityId"); - map.Generator(Generators.Native); + ca.Abstract(true); + ca.Id( + x => x.Id, + map => + { + map.Column("BadlyMappedEntityId"); + map.Generator(Generators.GuidComb); + }); + ca.Property(x => x.FirstValue, map => map.Column("SameColumn")); + // SecondValue is mapped with same name as another column, this gives the AbstractEntityMapper + // more entries in the fields array than there are in the includeColumns array; this causes the + // index to fall out of bounds. + ca.Property(x => x.SecondValue, map => map.Column("SameColumn")); }); - ca.Property(x => x.FirstValue, map => map.Column("SameColumn")); - ca.Property(x => x.SecondValue, map => map.Column("SameColumn")); - // SecondValue is mapped badly, this gives the AbstractEntityMapper more entries in the fields array - // than there are in the includeColumns array; this causes the index to fall out of bounds. - }); - mapper.Class(ca => - { - ca.Abstract(true); - ca.Id(x => x.Id, map => - { - map.Column("BadlyMappedEntityId"); - map.Generator(Generators.Native); - }); - ca.Property(x => x.FirstValue, map => map.Column("SameColumn")); - ca.Property(x => x.SecondValue, map => map.Column("OtherColumn")); - // SecondValue is mapped correctly, should not break - }); - - return mapper.CompileMappingFor(new[] { typeof(BadlyMappedEntity), typeof(CorrectlyMappedEntity) }); + return mapper.CompileMappingFor(new[] { typeof(BadlyMappedEntity) }); } [Test] - public void ShouldThrowAREXForBadlyMappedEntity() + public void ShouldThrowSoundErrorForBadlyMappedEntity() { - // arrange var bad = new BadlyMappedEntity { FirstValue = 1, SecondValue = 2 }; - // Bobbytables1337 - - // assert - Assert.That( - () => SaveEntity(bad), - Throws.TypeOf().And.Message.Contains("(Duplicate column mapping?)")); - } - private bool SaveEntity(T entity) - { using (var session = OpenSession()) using (var transaction = session.BeginTransaction()) { - - session.Save(entity); - transaction.Commit(); + session.Save(bad); + Assert.That( + transaction.Commit, + Throws.TypeOf().And.Message.Contains("(Duplicate column mapping?)")); } - - return true; } - [Test] - public void ShouldNotThrow() + protected override void OnTearDown() { - // arrange - ArgumentOutOfRangeException result = null; - var e = new BadlyMappedEntity - { - FirstValue = 1, - SecondValue = 2 - }; - - // act - try - { - using (var session = OpenSession()) - using (var transaction = session.BeginTransaction()) - { - - session.Save(e); - transaction.Commit(); - } - } - catch (ArgumentOutOfRangeException arex) + using (var session = OpenSession()) + using (var transaction = session.BeginTransaction()) { - result = arex; + session.CreateQuery("delete from System.Object").ExecuteUpdate(); + transaction.Commit(); } - - // assert - Assert.That(result == null); - Assert.That(e.Id > 0); - - // cleanup after Test - OpenSession().Delete(e); } } } diff --git a/src/NHibernate/Async/Persister/Entity/AbstractEntityPersister.cs b/src/NHibernate/Async/Persister/Entity/AbstractEntityPersister.cs index bae2ed62a83..1f9de5c2886 100644 --- a/src/NHibernate/Async/Persister/Entity/AbstractEntityPersister.cs +++ b/src/NHibernate/Async/Persister/Entity/AbstractEntityPersister.cs @@ -14,7 +14,6 @@ using System.Data; using System.Data.Common; using System.Text; - using NHibernate.AdoNet; using NHibernate.Cache; using NHibernate.Cache.Entry; @@ -30,7 +29,6 @@ using NHibernate.Metadata; using NHibernate.Properties; using NHibernate.SqlCommand; -using NHibernate.Tuple; using NHibernate.Tuple.Entity; using NHibernate.Type; using NHibernate.Util; @@ -291,6 +289,10 @@ protected async Task DehydrateAsync(object id, object[] fields, object rowI index += ArrayHelper.CountTrue(includeColumns[i]); //TODO: this is kinda slow... } catch (OperationCanceledException) { throw; } + catch (ArgumentOutOfRangeException arex) + { + throw new PropertyValueException("Column count does not match property count (Duplicate column mapping?) for", EntityName, entityMetamodel.PropertyNames[i], arex); + } catch (Exception ex) { throw new PropertyValueException("Error dehydrating property value for", EntityName, entityMetamodel.PropertyNames[i], ex); From 9a410ac3efca1122d7fe7ff3164256cfb609d3ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Delaporte?= <12201973+fredericdelaporte@users.noreply.github.com> Date: Tue, 30 Oct 2018 19:38:45 +0100 Subject: [PATCH 6/7] Fix the SQL builder overriding parameterized columns silently --- .../Persister/Entity/AbstractEntityPersister.cs | 4 ---- .../Persister/Entity/AbstractEntityPersister.cs | 4 ---- src/NHibernate/SqlCommand/SqlInsertBuilder.cs | 15 ++++++++++++--- src/NHibernate/SqlCommand/SqlUpdateBuilder.cs | 17 +++++++++++++---- 4 files changed, 25 insertions(+), 15 deletions(-) diff --git a/src/NHibernate/Async/Persister/Entity/AbstractEntityPersister.cs b/src/NHibernate/Async/Persister/Entity/AbstractEntityPersister.cs index 1f9de5c2886..eac27c8468a 100644 --- a/src/NHibernate/Async/Persister/Entity/AbstractEntityPersister.cs +++ b/src/NHibernate/Async/Persister/Entity/AbstractEntityPersister.cs @@ -289,10 +289,6 @@ protected async Task DehydrateAsync(object id, object[] fields, object rowI index += ArrayHelper.CountTrue(includeColumns[i]); //TODO: this is kinda slow... } catch (OperationCanceledException) { throw; } - catch (ArgumentOutOfRangeException arex) - { - throw new PropertyValueException("Column count does not match property count (Duplicate column mapping?) for", EntityName, entityMetamodel.PropertyNames[i], arex); - } catch (Exception ex) { throw new PropertyValueException("Error dehydrating property value for", EntityName, entityMetamodel.PropertyNames[i], ex); diff --git a/src/NHibernate/Persister/Entity/AbstractEntityPersister.cs b/src/NHibernate/Persister/Entity/AbstractEntityPersister.cs index f7c6ac7ba30..f8e428dd438 100644 --- a/src/NHibernate/Persister/Entity/AbstractEntityPersister.cs +++ b/src/NHibernate/Persister/Entity/AbstractEntityPersister.cs @@ -2454,10 +2454,6 @@ protected int Dehydrate(object id, object[] fields, object rowId, bool[] include PropertyTypes[i].NullSafeSet(statement, fields[i], index, includeColumns[i], session); index += ArrayHelper.CountTrue(includeColumns[i]); //TODO: this is kinda slow... } - catch (ArgumentOutOfRangeException arex) - { - throw new PropertyValueException("Column count does not match property count (Duplicate column mapping?) for", EntityName, entityMetamodel.PropertyNames[i], arex); - } catch (Exception ex) { throw new PropertyValueException("Error dehydrating property value for", EntityName, entityMetamodel.PropertyNames[i], ex); diff --git a/src/NHibernate/SqlCommand/SqlInsertBuilder.cs b/src/NHibernate/SqlCommand/SqlInsertBuilder.cs index 8fdd229b6cc..39cea643d79 100644 --- a/src/NHibernate/SqlCommand/SqlInsertBuilder.cs +++ b/src/NHibernate/SqlCommand/SqlInsertBuilder.cs @@ -55,7 +55,7 @@ public virtual SqlInsertBuilder AddColumn(string columnName, IType propertyType) SqlType[] sqlTypes = propertyType.SqlTypes(factory); if (sqlTypes.Length > 1) throw new AssertionFailure("Adding one column for a composed IType."); - columns[columnName] = sqlTypes[0]; + AddColumnWithValueOrType(columnName, sqlTypes[0]); return this; } @@ -80,7 +80,7 @@ public SqlInsertBuilder AddColumn(string columnName, object val, ILiteralType li /// The SqlInsertBuilder. public SqlInsertBuilder AddColumn(string columnName, string val) { - columns[columnName] = val; + AddColumnWithValueOrType(columnName, val); return this; } @@ -94,13 +94,22 @@ public SqlInsertBuilder AddColumns(string[] columnNames, bool[] insertable, ITyp { if (i >= sqlTypes.Length) throw new AssertionFailure("Different columns and it's IType."); - columns[columnNames[i]] = sqlTypes[i]; + AddColumnWithValueOrType(columnNames[i], sqlTypes[i]); } } return this; } + private void AddColumnWithValueOrType(string columnName, object valueOrType) + { + // As the column may be associated with a parameter, we must be sure it has been newly added, and + // fail otherwise. So we use Add instead of the indexer access. Otherwise, a parameter count + // mismatch may occur and will cause various failures, depending on the used data provider. + // See #1875. + columns.Add(columnName, valueOrType); + } + public virtual SqlInsertBuilder AddIdentityColumn(string columnName) { string value = Dialect.IdentityInsertString; diff --git a/src/NHibernate/SqlCommand/SqlUpdateBuilder.cs b/src/NHibernate/SqlCommand/SqlUpdateBuilder.cs index d51aed776bd..50b8c2cd651 100644 --- a/src/NHibernate/SqlCommand/SqlUpdateBuilder.cs +++ b/src/NHibernate/SqlCommand/SqlUpdateBuilder.cs @@ -60,7 +60,7 @@ public SqlUpdateBuilder AddColumn(string columnName, object val, ILiteralType li /// The SqlUpdateBuilder. public SqlUpdateBuilder AddColumn(string columnName, string val) { - columns[columnName] = val; + AddColumnWithValueOrType(columnName, val); return this; } @@ -73,7 +73,7 @@ public SqlUpdateBuilder AddColumn(string columnName, string val) public SqlUpdateBuilder AddColumns(string[] columnsName, string val) { foreach (string columnName in columnsName) - columns[columnName] = val; + AddColumnWithValueOrType(columnName, val); return this; } @@ -83,7 +83,7 @@ public virtual SqlUpdateBuilder AddColumn(string columnName, IType propertyType) SqlType[] sqlTypes = propertyType.SqlTypes(Mapping); if (sqlTypes.Length > 1) throw new AssertionFailure("Adding one column for a composed IType."); - columns[columnName] = sqlTypes[0]; + AddColumnWithValueOrType(columnName, sqlTypes[0]); return this; } @@ -114,13 +114,22 @@ public SqlUpdateBuilder AddColumns(string[] columnNames, bool[] updateable, ITyp { if (i >= sqlTypes.Length) throw new AssertionFailure("Different columns and it's IType."); - columns[columnNames[i]] = sqlTypes[i]; + AddColumnWithValueOrType(columnNames[i], sqlTypes[i]); } } return this; } + private void AddColumnWithValueOrType(string columnName, object valueOrType) + { + // As the column may be associated with a parameter, we must be sure it has been newly added, and + // fail otherwise. So we use Add instead of the indexer access. Otherwise, a parameter count + // mismatch may occur and will cause various failures, depending on the used data provider. + // See #1875. + columns.Add(columnName, valueOrType); + } + public SqlUpdateBuilder AppendAssignmentFragment(SqlString fragment) { // SqlString is immutable From 60c68d31f0eeb21dbae42ba362dace7b836788f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Delaporte?= <12201973+fredericdelaporte@users.noreply.github.com> Date: Fri, 2 Nov 2018 16:41:50 +0100 Subject: [PATCH 7/7] Throw a better exception in case of duplicated columns --- .../Async/NHSpecificTest/GH1875/Fixture.cs | 76 ------------------- .../NHSpecificTest/GH1875/Fixture.cs | 32 +++----- .../Entity/AbstractEntityPersister.cs | 54 +++++++++++-- src/NHibernate/SqlCommand/SqlInsertBuilder.cs | 9 ++- src/NHibernate/SqlCommand/SqlUpdateBuilder.cs | 9 ++- 5 files changed, 69 insertions(+), 111 deletions(-) delete mode 100644 src/NHibernate.Test/Async/NHSpecificTest/GH1875/Fixture.cs diff --git a/src/NHibernate.Test/Async/NHSpecificTest/GH1875/Fixture.cs b/src/NHibernate.Test/Async/NHSpecificTest/GH1875/Fixture.cs deleted file mode 100644 index 3af9edd5aab..00000000000 --- a/src/NHibernate.Test/Async/NHSpecificTest/GH1875/Fixture.cs +++ /dev/null @@ -1,76 +0,0 @@ -//------------------------------------------------------------------------------ -// -// This code was generated by AsyncGenerator. -// -// Changes to this file may cause incorrect behavior and will be lost if -// the code is regenerated. -// -//------------------------------------------------------------------------------ - - -using System; -using NHibernate.Cfg.MappingSchema; -using NHibernate.Mapping.ByCode; -using NUnit.Framework; - -namespace NHibernate.Test.NHSpecificTest.GH1875 -{ - using System.Threading.Tasks; - - [TestFixture] - public class FixtureAsync : TestCaseMappingByCode - { - protected override HbmMapping GetMappings() - { - var mapper = new ModelMapper(); - mapper.Class( - ca => - { - ca.Abstract(true); - ca.Id( - x => x.Id, - map => - { - map.Column("BadlyMappedEntityId"); - map.Generator(Generators.GuidComb); - }); - ca.Property(x => x.FirstValue, map => map.Column("SameColumn")); - // SecondValue is mapped with same name as another column, this gives the AbstractEntityMapper - // more entries in the fields array than there are in the includeColumns array; this causes the - // index to fall out of bounds. - ca.Property(x => x.SecondValue, map => map.Column("SameColumn")); - }); - - return mapper.CompileMappingFor(new[] { typeof(BadlyMappedEntity) }); - } - - [Test] - public async Task ShouldThrowSoundErrorForBadlyMappedEntityAsync() - { - var bad = new BadlyMappedEntity - { - FirstValue = 1, - SecondValue = 2 - }; - - using (var session = OpenSession()) - using (var transaction = session.BeginTransaction()) - { - await (session.SaveAsync(bad)); - Assert.That( - transaction.Commit, - Throws.TypeOf().And.Message.Contains("(Duplicate column mapping?)")); - } - } - - protected override void OnTearDown() - { - using (var session = OpenSession()) - using (var transaction = session.BeginTransaction()) - { - session.CreateQuery("delete from System.Object").ExecuteUpdate(); - transaction.Commit(); - } - } - } -} diff --git a/src/NHibernate.Test/NHSpecificTest/GH1875/Fixture.cs b/src/NHibernate.Test/NHSpecificTest/GH1875/Fixture.cs index 07485a0c3fa..cc5d8d282e5 100644 --- a/src/NHibernate.Test/NHSpecificTest/GH1875/Fixture.cs +++ b/src/NHibernate.Test/NHSpecificTest/GH1875/Fixture.cs @@ -13,9 +13,9 @@ public class BadlyMappedEntity } [TestFixture] - public class Fixture : TestCaseMappingByCode + public class Fixture { - protected override HbmMapping GetMappings() + protected HbmMapping GetMappings() { var mapper = new ModelMapper(); mapper.Class( @@ -42,29 +42,21 @@ protected override HbmMapping GetMappings() [Test] public void ShouldThrowSoundErrorForBadlyMappedEntity() { - var bad = new BadlyMappedEntity - { - FirstValue = 1, - SecondValue = 2 - }; + var mappings = GetMappings(); + var cfg = TestConfigurationHelper.GetDefaultConfiguration(); + cfg.AddMapping(mappings); - using (var session = OpenSession()) - using (var transaction = session.BeginTransaction()) + ISessionFactory factory = null; + try { - session.Save(bad); Assert.That( - transaction.Commit, - Throws.TypeOf().And.Message.Contains("(Duplicate column mapping?)")); + () => factory = cfg.BuildSessionFactory(), + Throws.TypeOf().And.Message.Contains("BadlyMappedEntity").And.InnerException + .Message.Contains("SameColumn")); } - } - - protected override void OnTearDown() - { - using (var session = OpenSession()) - using (var transaction = session.BeginTransaction()) + finally { - session.CreateQuery("delete from System.Object").ExecuteUpdate(); - transaction.Commit(); + factory?.Dispose(); } } } diff --git a/src/NHibernate/Persister/Entity/AbstractEntityPersister.cs b/src/NHibernate/Persister/Entity/AbstractEntityPersister.cs index f8e428dd438..a17a70970b4 100644 --- a/src/NHibernate/Persister/Entity/AbstractEntityPersister.cs +++ b/src/NHibernate/Persister/Entity/AbstractEntityPersister.cs @@ -2257,7 +2257,17 @@ protected internal SqlCommandInfo GenerateUpdateString(bool[] includeProperty, i if (includeProperty[i] && IsPropertyOfTable(i, j)) { // this is a property of the table, which we are updating - updateBuilder.AddColumns(GetPropertyColumnNames(i), propertyColumnUpdateable[i], PropertyTypes[i]); + try + { + updateBuilder.AddColumns(GetPropertyColumnNames(i), propertyColumnUpdateable[i], PropertyTypes[i]); + } + catch (ArgumentException arex) + { + throw new MappingException( + $"Unable to build the update statement for class {entityMetamodel.Name}: " + + $"a failure occured when adding the property {PropertyNames[i]}", + arex); + } hasColumns = hasColumns || GetPropertyColumnSpan(i) > 0; } } @@ -2346,24 +2356,54 @@ protected virtual SqlCommandInfo GenerateInsertString(bool identityInsert, bool[ if (includeProperty[i] && IsPropertyOfTable(i, j)) { // this property belongs on the table and is to be inserted - builder.AddColumns(GetPropertyColumnNames(i), propertyColumnInsertable[i], PropertyTypes[i]); + try + { + builder.AddColumns(GetPropertyColumnNames(i), propertyColumnInsertable[i], PropertyTypes[i]); + } + catch (ArgumentException arex) + { + throw new MappingException( + $"Unable to build the insert statement for class {entityMetamodel.Name}: " + + $"a failure occured when adding the property {PropertyNames[i]}", + arex); + } } } // add the discriminator if (j == 0) { - AddDiscriminatorToInsert(builder); + try + { + AddDiscriminatorToInsert(builder); + } + catch (ArgumentException arex) + { + throw new MappingException( + $"Unable to build the insert statement for class {entityMetamodel.Name}: " + + "a failure occured when adding the discriminator", + arex); + } } // add the primary key - if (j == 0 && identityInsert) + try { - builder.AddIdentityColumn(GetKeyColumns(0)[0]); + if (j == 0 && identityInsert) + { + builder.AddIdentityColumn(GetKeyColumns(0)[0]); + } + else + { + builder.AddColumns(GetKeyColumns(j), null, GetIdentifierType(j)); + } } - else + catch (ArgumentException arex) { - builder.AddColumns(GetKeyColumns(j), null, GetIdentifierType(j)); + throw new MappingException( + $"Unable to build the insert statement for class {entityMetamodel.Name}: " + + "a failure occured when adding the Id of the class", + arex); } if (Factory.Settings.IsCommentsEnabled) diff --git a/src/NHibernate/SqlCommand/SqlInsertBuilder.cs b/src/NHibernate/SqlCommand/SqlInsertBuilder.cs index 39cea643d79..e2d1a8a3f4b 100644 --- a/src/NHibernate/SqlCommand/SqlInsertBuilder.cs +++ b/src/NHibernate/SqlCommand/SqlInsertBuilder.cs @@ -1,3 +1,4 @@ +using System; using System.Collections.Generic; using NHibernate.Engine; @@ -103,10 +104,10 @@ public SqlInsertBuilder AddColumns(string[] columnNames, bool[] insertable, ITyp private void AddColumnWithValueOrType(string columnName, object valueOrType) { - // As the column may be associated with a parameter, we must be sure it has been newly added, and - // fail otherwise. So we use Add instead of the indexer access. Otherwise, a parameter count - // mismatch may occur and will cause various failures, depending on the used data provider. - // See #1875. + if (columns.ContainsKey(columnName)) + throw new ArgumentException( + $"The column '{columnName}' has already been added in this SQL builder", + nameof(columnName)); columns.Add(columnName, valueOrType); } diff --git a/src/NHibernate/SqlCommand/SqlUpdateBuilder.cs b/src/NHibernate/SqlCommand/SqlUpdateBuilder.cs index 50b8c2cd651..4b9fd670918 100644 --- a/src/NHibernate/SqlCommand/SqlUpdateBuilder.cs +++ b/src/NHibernate/SqlCommand/SqlUpdateBuilder.cs @@ -1,3 +1,4 @@ +using System; using System.Collections.Generic; using System.Linq; using NHibernate.Engine; @@ -123,10 +124,10 @@ public SqlUpdateBuilder AddColumns(string[] columnNames, bool[] updateable, ITyp private void AddColumnWithValueOrType(string columnName, object valueOrType) { - // As the column may be associated with a parameter, we must be sure it has been newly added, and - // fail otherwise. So we use Add instead of the indexer access. Otherwise, a parameter count - // mismatch may occur and will cause various failures, depending on the used data provider. - // See #1875. + if (columns.ContainsKey(columnName)) + throw new ArgumentException( + $"The column '{columnName}' has already been added in this SQL builder", + nameof(columnName)); columns.Add(columnName, valueOrType); }