Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a statically resolved dialect when building the session factory #2011

Merged
merged 2 commits into from
Feb 28, 2019

Conversation

fredericDelaporte
Copy link
Member

@fredericDelaporte fredericDelaporte commented Feb 17, 2019

This change is more a trivial optimization: when building the session factory, the dialect has no reason to change, and so we can use the same dialect instance from the start of the process to its end.
#1951, merged in v5.3, was already doing most of the work, by supplying to the built session factory an IMapping resolving the dialect at instantiation rather than dynamically.

This PR cherry-pick #1951 into 5.2.x, and furthermore switch Configuration.mapping member to the mapping having a static dialect, for the duration of the build.

So if anything does use the mapping.Dialect in steps prior to factory instantiation, it will now use that static one instead of instantiating it.

Well, that does probably not actually optimize anything in most cases as far as I know.

But it allows to restore a hack used by NHibernate.Spatial, which relies on having its dialect instantiated early in the factory building process. Without that hack, Spatial users cannot upgrade to NHibernate 5.2.x.
See nhibernate/NHibernate.Spatial#104.

That is why I issue this PR, targeting the 5.2.x branch.

maca88 and others added 2 commits February 17, 2019 13:41
Back-ported from 5.3

(cherry picked from commit cbc2527)
This by the way allows NHibernate.Spatial dialect hack to work again,
see nhibernate/NHibernate.Spatial#104
@hazzik
Copy link
Member

hazzik commented Feb 17, 2019

I don't think this will help spatial, the error happens long before BuildSessionFactory call.

@hazzik
Copy link
Member

hazzik commented Feb 17, 2019

Ok, I've checked the stack trace. It should work.

@fredericDelaporte
Copy link
Member Author

fredericDelaporte commented Feb 17, 2019

I had checked with a locally generated NuGet package with those changes, and an upgraded Spatial.

Still, @peetw, can you also check this change solves nhibernate/NHibernate.Spatial#104, just to be sure? Releasing a patch takes some time, so it is better to double check.

A release build is ongoing and should be available soon, with a Nuget package in its artifacts, for testing, if that helps.

@fredericDelaporte fredericDelaporte added this to the 5.2.4 milestone Feb 17, 2019
@hazzik
Copy link
Member

hazzik commented Feb 19, 2019

Could you also include the release notes in the PR?

@peetw
Copy link

peetw commented Feb 19, 2019

Thank you both for looking into this - much appreciated! I was away for the weekend so apologies for not replying sooner. I should have some time today to test whether the PR fixes the issue.

@fredericDelaporte
Copy link
Member Author

Could you also include the release notes in the PR?

I have not put them directly due to not having definitely decided what to do for #1983 (but most likely I will just add a possible breaking change notice), and also due to the suspected regression on not-found discussed in off-topic comments of #1995. If there is actually a regression there, it would then be better to fix it in 5.2.4 too.

@peetw
Copy link

peetw commented Feb 19, 2019

@fredericDelaporte Using the NuGet package from the build artifacts, I have confirmed that this PR fixes the issue reported in nhibernate/NHibernate.Spatial#104 - thanks!

@fredericDelaporte
Copy link
Member Author

Release commit added, adding by the way a lacking possible breaking change.

@hazzik
Copy link
Member

hazzik commented Feb 24, 2019

The issue type has to be bug or task. Let’s call it bug.

@fredericDelaporte
Copy link
Member Author

I have adjusted the releasenotes file accordingly.

@fredericDelaporte
Copy link
Member Author

Release commit dropped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants