Do not store mapping field in Configuration #3398
Conversation
| Assert.That(mapping, Is.Not.Null, "Unable to find mapping object"); | ||
| return mapping; | ||
| } | ||
| private static IMapping GetMapping(Configuration cfg) => (IMapping) cfg.BuildSessionFactory(); |
There was a problem hiding this comment.
The true equivalent for the original code in the new way would be GetMapping(Configuration cfg) => cfg.BuildMapping(), but Dialect would throw an InvalidOperationException. However, in this case it does not matter and we can use fully resolved session factory instead.
There was a problem hiding this comment.
Now new public "BuildMapping" can be used.
There was a problem hiding this comment.
I was thinking about it and don't like it. cfg.BuildMapping just should not exist as it provides an incomplete version of mapping, that can create a lot of problems.
There was a problem hiding this comment.
nit: I don't understand your concern. In my understanding IMapping was specifically designed to be safely used in "uncompiled" state. See interface description:
nhibernate-core/src/NHibernate/Engine/IMapping.cs
Lines 6 to 7 in a0bc61f
So as long you use interface methods and don't do some funny castings you should be safe.
You agreed to keep method public - so it would be good to have it in tests too.
There was a problem hiding this comment.
I don't think the IMapping was designed. It was probably appeared incidentally from some other bad design decision. Problem with the interface that it is not clear when it is safe to use compiled or raw mappings. For example, SingleTableEntityPersister accepts both IMapping and ISessionFactoryImplementor. Can it use only ISessionFactoryImplementor?
This is really not clear.
Also, Hibernate has removed this interface and redesigned the mapping compilation.
This comment was marked as resolved.
This comment was marked as resolved.
…into refactor-cfg
hazzik
changed the title
…into refactor-cfg # Conflicts: # src/NHibernate/Cfg/Configuration.cs
| Assert.That(mapping, Is.Not.Null, "Unable to find mapping object"); | ||
| return mapping; | ||
| } | ||
| private static IMapping GetMapping(Configuration cfg) => (IMapping) cfg.BuildSessionFactory(); |
There was a problem hiding this comment.
Now new public "BuildMapping" can be used.
Fixes #3397.
(Actually patched with minimal changes in 5.3x with #3399, then fixed more robustly here.)