NH-3667 & NH-3102 Dictionary mapping-by-code improvements

[rjperes]

https://nhibernate.jira.com/browse/NH-3667
https://nhibernate.jira.com/browse/NH-3102

[hazzik]

«Stop, little pot»

[rjperes]

Needs something else... Like this!

[rjperes]

Fixed

[rjperes]

Hooray!

[oskarb]

For readability, can we get that split on several lines please? Use a nested if-statement if it helps readability.

[rjperes]

Alex, you are a hard man to please! :-)

[hazzik]

Probably I'm stupid, but I don't understand that condition:(

[rjperes]

You're not stupid! :-)
The problem was: ModelMapper was considering a dictionary to always be a many-to-many. That, however, is not the case in two situations: when we have values of elements or components instead of entities.
If the collection is not generic, we can't really say anything about it, so we go with the original behavior. When we have entities as values, we also go for the original behavior. In any other case, we know that we do not have a many-to-many.

[hazzik]

I'm asking only about args.Length < 2 || args.Length > 1

[rjperes]

If we have a generic collection with a single generic parameter (eg, IList) or if we have one with two (eg, IDictionary<TKey, TValue>)...

[AlexeiScherbakov]

(args.Length < 2 || args.Length > 1) is true always

0 - true ( args.Length < 2 is true)
1 - true ( args.Length < 2 is true)
2 - true (args.Length > 1 is true)
And so on

[rjperes]

Alexei:
It's a new technique called LNDP - Late Night Driven Programming!
Of course you and Alex are right. The point is: if we have a generic collection with a single generic parameter, check that it is an entity. If the collection has two, check the second. I will update the pull request. What's more important is, this fixes some reported problems.

[rjperes]

I cleaned up the code. Can you try rebuilding?

[oskarb]

I cannot tell - is the change made in the correct place or is it the implementation of modelInspector.IsManyToMany() that should really change? There is such a large number of classes there, with very little documentation, so I must admit I'm confused.

[rjperes]

The two errors from ExplicitColumnNameIsAlwaysMapped are caused because class Bar is not mapped. The test would also fail if we were to open a session factory - see for yourself.

[rjperes]

The problem is that whenever we call IMapKeyRelation.ManyToMany to register an entity key the property is registered as a many to many in class AbstractExplicitlyDeclaredModel. So method ModelMapper.DetermineCollectionElementRelationType assumes that the collection value is an entity, but sometimes it is an element or a component. My solution fixes this, but there are other possibilities. I think this is an important issue.

[hazzik]

Hi Ricardo, could you please squash all this stuff? Or alternatively - could you add me to your fork?

[rjperes]

Alex: what do you mean?

[hazzik]

What question is confusing you?

[rjperes]

Well, the "add me to your fork" is easy, but what do you mean by "squash all this stuff"?

[hazzik]

Something like that: http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html http://davidwalsh.name/squash-commits-git http://makandracards.com/makandra/527-squash-several-git-commits-into-a-single-commit

[rjperes]

Right, I added you, because I'm on an ipad far away from my machine. Will only be back by sunday.

[hazzik]

Ok, thanks

[oskarb]

So this is ready to go in then?

To be honest it looks a bit like a hack to me, but the test case seems solid and the code is localized so I don't think it's too much of a problem. The related DetermineMapKeyRelationType() does contain the comment "Perhaps we have to change IModelInspector with IsDictionaryKeyManyToMany(member), IsDictionaryKeyComponent(member) and so on", which might be a hint that really MapKeyManyToManyCustomizer should do something slightly different than just explicitDeclarationsHolder.AddAsManyToManyRelation(propertyPath.LocalMember);.

I havent't studied the code close enough to tell really... so this patch LGTM.