Use Type as PropertyPart CustomType instead of only name #347
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Use the constructor of TypeReference with Type argument instead of the constructor with string argument in the overloads of CustomType in PropertyPart where an explicit Type is passed. - This fixes the problem that Fluent NHibernate may map a wrong type for a property although an explicit type was passed to CustomType. This problem can happen if there are two types with names which only differ in upper / lower case. Such similar type names can e.g. be caused by obfuscation tools.
- The production code of the CustomType overloads of PropertyPart was changed to use a different constructor of TypeReference when an explicit Type was given. This caused two tests to fail although the code still should work correctly. - Change failing tests to expect the new behaviour.
|
Will review tomorrow |
|
TeamCity verified this for 4.0, but I still need to figure out if this is 3.5 compatible |
|
Any news on this? |
|
WIll take a look this week. Got lots of stuff on my plate ATM
2017-04-25 15:45 GMT+03:00 visubesy <[email protected]>:
… Any news on this?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#347 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AALnfLAh6yMbtD4sEYYvfhseuOY10RdLks5rzerbgaJpZM4JE-U3>
.
--
Yours faithfully,
Gleb
|
govorovvs
added a commit
to govorovvs/fluent-nhibernate
that referenced
this pull request
Nov 16, 2017
|
@chester89 or @hazzik: Could you please merge this fix I provided on July 2016? |
|
Could you please merge the pull request? The bugfix of Fluent NHibernate works nicely with one of our products since 2016. It would be nice to have the fix in an official version of Fluent NHibernate. Or do newer versions of Fluent NHibernate support case-sensitive class names? Such class names are caused by obfuscation tools. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, Fluent NHibernate uses only the case-insensitive name of the type in PropertyPart.CustomType() because it uses the constructor TypeReference(string) instead of TypeReference(Type). This leads to problems if there are two types in the same namespace which only differ in upper case or lower case, e.g. MyProject.Abc and MyProject.AbC. CustomType() may result into a wrong mapping which uses AbC instead of Abc (or vice versa). The result depends on the order of the types in the assembly.
My commits fix that problem.
It looks like that the other classes of Fluent NHibernate already use the correct constructor of TypeReference.
I don't know if there are further places in the Fluent NHibernate code which use only case-insensitive names although a concrete Type was passed.
I didn't change the constructor TypeReference(string) to work case-sensitively because someone could depend on its case-insensitive behaviour.
Could you please pull my changes?