Skip to content

SQLiteDialect: Correct GUID to string conversion when BinaryGuid=False (regression) (fixes #2110) #2111

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

Merged
merged 4 commits into from
Aug 24, 2019

Conversation

oskarb
Copy link
Member

@oskarb oskarb commented Apr 9, 2019

Attached are test cases for #2110 (and #2109), and a proposed fix for #2110.

The suggested fix teaches SQLiteDialect to detect the effective value of the BinaryGuid from the connection string and apply different expressions for the strguid function based on that. This fixes the first failure mentioned below.

In NH 5.2, without the fix, the result of the test cases are:

GH2110.ByCodeFixture(False).DbExplicitIdGuidToStringIsSameAsDotNet
Fails, because the expression given in the dialect for strguid is not valid with BinaryGuid=False. This is the regression caused by #1151. Fixed by this PR.

GH2110.ByCodeFixture(False).DbImplicitIdGuidToStringIsSameAsDotNet
Success, but only because NHibernate forgets to apply the strguid expression, opting for a straightforward cast(entity0_.Id as char) instead.

GH2110.ByCodeFixture(True).DbExplicitIdGuidToStringIsSameAsDotNet
Success. This is the case that was fixed by #1151.

GH2110.ByCodeFixture(True).DbImplicitIdGuidToStringIsSameAsDotNet
Fails (still), since the fix for #1151 is not applied here because NH doesn't apply the strguid method. Reported as #2109. Since it still fails, this case is ignored for now.

@hazzik
Copy link
Member

hazzik commented Apr 10, 2019

Ok. This will go to 5.2.6.

@hazzik hazzik added this to the 5.2.6 milestone Apr 10, 2019
@fredericDelaporte

This comment has been minimized.

@oskarb oskarb changed the base branch from master to 5.2.x April 14, 2019 10:47
@oskarb
Copy link
Member Author

oskarb commented Apr 14, 2019

Rebased, and PR retargeted.

I also figured it would be better to expand the existing test cases for the original issue NH-3426 instead of effectively duplicating.

@oskarb
Copy link
Member Author

oskarb commented Apr 22, 2019

It seems the test cases SelectGuidToStringImplicit and WhereGuidToStringImplicit that I added to showcase GH-2109 (affecting SQLite with BinaryGuid=true) also expose same problem on Firebird, Mysql and Oracle (any dialect where converting guid to string cannot be done using a regular cast in SQL).

@oskarb
Copy link
Member Author

oskarb commented Apr 22, 2019

I've pushed an updated version with the following changes:

@oskarb oskarb force-pushed the GH-2110 branch 2 times, most recently from 1974c8e to acedecb Compare April 22, 2019 18:41
@hazzik
Copy link
Member

hazzik commented Apr 29, 2019

Actually, I got into thinking... this applies not only to SQLite, but also to MySQL. The MySQL provider from Oracle has OldGuids=true/false connection string option. This kinda minor, so I would not bother with it now.

@fredericDelaporte

This comment has been minimized.

@oskarb
Copy link
Member Author

oskarb commented May 27, 2019

This is intended to fix a regression I discovered, related to sqlite. Though I suppose there would be regressions for the other database where strguid was introduced/changed at the same time too.

I discovered this while working with unit tests that create the database on each run. Thinking about it, it's actually quite likely that I added BinaryGuid=false just to work around the problem with string conversions that has now been fixed in NH so maybe I don't need it anymore.

However, if someone have used BinaryGuid=false on a file-backed database that they can't easily just discard and recreate, they will be in more trouble.

Copy link
Member

@fredericDelaporte fredericDelaporte left a comment

Choose a reason for hiding this comment

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

Looking into it again, I think this patch should be merged in its current state.

The MySql case is not so similar, because it is somewhat already handled by having two dialects: MySql5Dialect, working with binary(16) guids (so expecting OldGuids set as true), and MySql55Dialect, working with char(36) guids (so expecting OldGuids unset or false). And adapting the MySql dialect behavior with guids according to the OldGuids option would also require to override the guid type registration. So it seems better to handle that in another PR, not necessarily on 5.2.x.

@fredericDelaporte

This comment has been minimized.

@fredericDelaporte fredericDelaporte merged commit d636a40 into nhibernate:5.2.x Aug 24, 2019
@bahusoid

This comment has been minimized.

@fredericDelaporte fredericDelaporte mentioned this pull request Sep 8, 2019
Copy link
Member

@fredericDelaporte fredericDelaporte left a comment

Choose a reason for hiding this comment

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

I just realize now the nhibernate-configuration.xsd has not been updated, so the new setting is anyway not usable in config file, because it will fail the validation...

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