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

Wrap mapping usages in StaticDialectMappingWrapper in Configuration class #3399

Merged
merged 2 commits into from
Aug 12, 2023

Conversation

bahusoid
Copy link
Member

@bahusoid bahusoid commented Aug 6, 2023

Fixes #3397

This is version of fix with minimal changes and impact specific for released branches. (as hazzik works on more general fix for master) I think it's worth fixing at least in 5.4 (as seems reported issue is 5.0 regression)

@fredericDelaporte
Copy link
Member

fredericDelaporte commented Aug 6, 2023

It could target 5.2.x in my opinion. According to NuGet stats, it still has some significant usage.

5.1.x is not so far below, but targeting it would require to back-port #2011 to v5.1.x.

It should at least target 5.3.x.

Aggregated by minor versions, the NuGet download stats for last six weeks are:

  • 5.4.x: 209 724
  • 5.3.x: 588 152
  • 5.2.x: 187 245
  • 5.1.x: 88 131
  • 5.0.x: 7 772
  • 4.1.x: 39 710
  • 4.0.x: 111 330
  • 3.4.x: 11 002
  • 3.3.x: 81 307
  • 3.2.x: 15 791
  • 3.1.x: 7 655
  • 3.0.x: 2 985
  • 2.1.2: 5 118

@@ -941,11 +945,12 @@ public string[] GenerateSchemaCreationScript(Dialect.Dialect dialect)

var script = new List<string>();

var staticDialectMapping = new StaticDialectMappingWrapper(mapping, dialect);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You would potentially need to do same as in BuildSessionFactory if you want to leave mapping field in place: replace mapping field with wrapped instance and back when operation finished, otherwise something can access the unwrapped mapping field and then have different dialect than was configured.

Even if there are no places that would be affected now - this could be in the future.

This is the reason I've decided to remove mapping field altogether.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a fix of previous versions, it is good enough for me.

@bahusoid bahusoid changed the base branch from 5.4.x to 5.2.x August 7, 2023 07:29
@bahusoid bahusoid closed this Aug 7, 2023
@bahusoid bahusoid reopened this Aug 7, 2023
@bahusoid
Copy link
Member Author

bahusoid commented Aug 7, 2023

It could target 5.2.x in my opinion.

Ok. Done

@hazzik
Copy link
Member

hazzik commented Aug 7, 2023

IMO, 5.2 is too old. The issue is too minor and affects only schema generation which usually happens only on a startup of the application.

@bahusoid
Copy link
Member Author

bahusoid commented Aug 8, 2023

5.2 also requires fixing the following errors:

error NU5125: The 'licenseUrl' element will be deprecated. Consider using the 'license' element instead.
error NU5048: The 'PackageIconUrl'/'iconUrl' element is deprecated. Consider using the 'PackageIcon'/'icon' element instead. Learn more at https://aka.ms/deprecateIconUrl [D:\BuildAgent\work\30546188361a242\src\NHibernate\NHibernate.csproj]

So 5.3. then?

@fredericDelaporte
Copy link
Member

Ok then, 5.3.x

@fredericDelaporte fredericDelaporte added this to the 5.3.19 milestone Aug 8, 2023
@fredericDelaporte fredericDelaporte changed the base branch from 5.2.x to 5.3.x August 8, 2023 18:16
@bahusoid bahusoid merged commit 737e2c8 into nhibernate:5.3.x Aug 12, 2023
17 of 20 checks passed
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.

None yet

3 participants