can't set more than one Keycolumn in SubclassMap<> anymore

[firo222]

this issue is not present in FNH 1.2 but with current Head

public class TestMap : SubclassMap<Test>
{
    public TestMap()
    {
        KeyColumn("col1");
        KeyColumn("col2");

        // key has both columns here
    }
}

var provider = (IIndeterminateSubclassMappingProvider)new TestMap();
var count = provider.GetSubclassMapping(SubclassType.JoinedSubclass).Key.Columns.Count();
// count == 1
// Columns.First().Name == "col1"

i tracked it down to FNH.Util.DeepCopy() which is broken for HashSet<>, Dictionary<,>, and LinkedList<> see

http://stackoverflow.com/q/4193868/671619

[firo222]

failing test

    [Test]
    public void Test()
    {
        var set = new HashSet<ColumnMapping>(new ColumnMappingComparer());

        set.Add(new ColumnMapping("foo"));
        set.Add(new ColumnMapping("bar"));
        Assert.Equal(2, set.Count);

        Assert.Equal(2, set.DeepClone().Count); // fails here with 2 != 1
    }

[Shihayazad]

The fix was being reverted, was something wrong with it? I have a similar issue: In a SubclassMap, I can't use CompositeUserTypes because of this (DeepCopy is not able to deserialize several ColumnMappings). The fix worked for me, and for now it does not seem to have any side effects. But of course, I do not have an overall overview, so any indication why this fix was reverted would be helpful.

[firo222]

the changes just fix the root cause of it, the deserialization of the inner Dictionary. It should be safe. no idea why it was reverted, no info to me yet.

[jagregory]

Unfortunately, TeamCity doesn't contain enough builds to actually verify this - but when I merged in these changes tests were failing (I foolishly used Githubs auto-merge, which of course made the build break when the tests failed).

[Shihayazad]

So, could we give this changeset another try? I ran all test provided in FluentNHibernate.Testing and they were all green (after applying firo222's changes to my local rep). Are there any other tests that should be checked?

[Shihayazad]

Any update on this? As far as I can tell, this is also affecting https://github.com/jagregory/fluent-nhibernate/issues/140 So, even if this would make tests fail, wouldn't it be good to see which ones are failing so that we can fix the potential issues introduced with this change?

[chester89]

@Shihayazad I agree - I'll try to merge these changes and see what's going on there

[chester89]

After I applied deep copy changes, 1 MSpec test failed - "when subclass has a has-many to another entity, should only use one column in the target entity's key'. that being said, I haven't applied second commit concerning ColumnMapping and null references

[chester89]

it seems I was a bit off yesterday and didn't notice that the failed test indicates exactly the problem shown above. Should we change it? Because I think we should - the test clearly expresses the requirement that has changed since it was written

[Shihayazad]

Thanks for picking this up! And yeah, sounds like the test needs to be updated

[chester89]

Not at all. By the way, what's the main use-case on this - key that consists of multiple columns (composite)?

[Shihayazad]

I don't know the use case of the initial issue, but my issue is that I can't use Composite UserTypes with multiple columns in subclass mappings, because the DeepCopy is not able to deserialize several ColumnMappings

Regarding the test I might have misunderstood something. What did you mean with "Should we change it?"

[chester89]

I meant that we can either change it or throw it away, the second option is less preferrable, of course

[robscottnh]

So, I've just run into this as well. Is a fix for this in the works?

[chester89]

Yes, but I can't say when it will be posted.

[chester89]

Although, If I can figure out how to write a good test for that situation, it may as well be done before the end of the month

[robscottnh]

I'd be happy to provide (most of )a test case for you. It's actually fairly simple scenario to set up. I'm just not sure what parts to assert against in your tests..., but I could provide the mappings for you. As an aside I've been able to make things work with .hbm files, so I have a workaround, but I'd really like to get the three hbm files out of my solution :)

[chester89]

giving it a second look - I was completely wrong about a failing test. it doesn't cover this particular use-case (having more than one key column), but it still fails. anyone volunteers to debug? =)

[chester89]

@jagregory may be you have an idea - why AutoPersistenceModel generates two identical columns when mapping one-to-many from a derived class (derived extends base with a collection of other entity)? config is here https://gist.github.com/3948140

[chester89]

and the most weird thing is that when I write the resulting sql script to a file, it's perfectly valid. so the trouble is in object model

[chester89]

oh no. two possibilities - either I'm crazy or this is just magic. the failing test included MySql configuration and output of a sql script. when I commented that part - test became green. need someone to double check if I'm sane here. pull request will be up tomorrow

[chester89]

also, I think the spec for this particular scenario would be nice. if I'm not lazy tomorrow, will definitely do

[chester89]

so, I believe I got it working as intended. the below test passes: [Test] public void Can_Have_Multiple_key_Columns() { var provider = (IIndeterminateSubclassMappingProvider)new TestMap(); provider.GetSubclassMapping(SubclassType.JoinedSubclass).Key.Columns.Count().ShouldEqual(2); }

public class Test { public int Id { get; set; } }

public class TestMap : SubclassMap { public TestMap() { KeyColumn("col1"); KeyColumn("col2"); } }