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

Bug: Property mapping fails with Turkish locale #502

Closed
mertkokusen opened this issue Aug 6, 2020 · 13 comments
Closed

Bug: Property mapping fails with Turkish locale #502

mertkokusen opened this issue Aug 6, 2020 · 13 comments
Assignees
Labels
bug Something isn't working deployed Feature or bug is deployed at the current release fixed The bug, issue, incident has been fixed. todo Things to be done in the future

Comments

@mertkokusen
Copy link
Contributor

Thanks for this library, it really has everything I was missing with other micro ORMs.

Property names that contain uppercase letter "I" causes property mapping to fail on some machines because of culture (ex. on a machine with turkish windows). using ToLowerInvariant instead of ToLower in resolvers solves this issue.

@mikependon mikependon self-assigned this Aug 6, 2020
@mikependon
Copy link
Owner

Thanks @mertkokusen. I would assume your stacktrace is different from the root cause you mentioned. Can you paste the exception here? Or, a very small steps to replicate it would be very great. Thanks!

@mertkokusen
Copy link
Contributor Author

mertkokusen commented Aug 6, 2020

Unfortunately, there is no exception, just the properties that contain letter I are not filled with values from the DB.

I cloned RepoDB and ran tests with RepoDb.Sqlite solution (in in-memory mode). Initially QueryTests were failing I replaced all ToLower calls with ToLowerInvariant and all tests in QueryTests are passing. I have experienced this problem with other libraries in the last couple of years because in Turkish uppercase i is İ and lowercase I is ı, which causes lots of headaches.

Running tests on a machine with windows installed in turkish should reproduce the issue.

@mikependon
Copy link
Owner

@mertkokusen - thank you for the clarity. Will do cover this on my next release.

@mikependon mikependon added bug Something isn't working todo Things to be done in the future labels Aug 6, 2020
@mikependon mikependon changed the title Property mapping fails with turkish locale Bug: Property mapping fails with Turkish locale Aug 6, 2020
@mertkokusen
Copy link
Contributor Author

@mikependon thanks

@mikependon
Copy link
Owner

@mertkokusen - would you be able to issue a PR in relation to this? It is much better actually, specially you already cloned it to your machine and had made the tests passed there. I will do cover all the other needed tests. Thanks in advance! Please do let me know if you can make it.

@mertkokusen
Copy link
Contributor Author

@mikependon sure I will make a PR :)

@mertkokusen
Copy link
Contributor Author

@mikependon made the PR

@mikependon
Copy link
Owner

Big Thanks. I am in a middle of refactoring the core compiler of RepoDb. I will handle the case of FunctionFactory.cs, would it be possible to exclude it from your PR? Lastly, can you also handle the string.Equals()?

@mertkokusen
Copy link
Contributor Author

@mikependon Sure I will exclude FunctionFactory.cs (the one in RepoDb.Core solution). But excluding this file results in some tests failing. Is it ok like this?
About string.Equals(), all calls to it uses StringComparison.OrdinalIgnoreCase and I think it should be fine this way but if you have something else in mind I would like to help

@mikependon
Copy link
Owner

Thanks. I know that the StringComparisson.OrdinalIgnoreCase is the fastest, but was not it to address the locale problem, you should impose the StringComparisson.InvariantCultureIgnoreCase? It should not be a performance problem at all, we only use the comparissons before the actual compilation.

For the failing tests, I guess it is OK. It will only fail in your side due to locale, but once I committed the changes in the FunctionFactory class, then everything will work as you expect.

Hope this helps.

@mertkokusen
Copy link
Contributor Author

mertkokusen commented Aug 6, 2020

StringComparisson.InvariantCultureIgnoreCasemakes additional linguistic decisions like character expantions (ex. ss being considered equal to ß). I think in our situation the way to go is to keep using OrdinalIgnoreCase which makes a byte by byte comparison.

I took the info from here
String comparison best practices

mikependon added a commit that referenced this issue Aug 6, 2020
Use the ToLowerInvariant instead of ToLower. Fixes the #502
@mikependon
Copy link
Owner

mikependon commented Aug 6, 2020

That is correct, that makes the string comparisson slower. But would not it unresolving the special chars automatically? In anyway, for as long your PR is working your local machine, then that is enough. I will make sure to handle the core compiler changes. Thanks for the PR

@mikependon mikependon added the fixed The bug, issue, incident has been fixed. label Aug 6, 2020
@mikependon mikependon added the deployed Feature or bug is deployed at the current release label Aug 29, 2020
@mikependon
Copy link
Owner

The fix is now available at RepoDb v1.12.0-beta4 and RepoDb.SqlServer v1.1.0-beta2.

> Install-Package RepoDb -version 1.12.0-beta4
> Install-Package RepoDb.SqlServer -version 1.1.0-beta2

We are closing this ticket now. Kindly let us know if you still have questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working deployed Feature or bug is deployed at the current release fixed The bug, issue, incident has been fixed. todo Things to be done in the future
Projects
None yet
Development

No branches or pull requests

2 participants