-
-
Notifications
You must be signed in to change notification settings - Fork 194
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
All code to support non-comparable columns #941
base: master
Are you sure you want to change the base?
Conversation
The problem I see here is that all tests passed because the Adventureworks generated database has no You can change that by specifying that one column from one table (that is filtered) has a Like for example if (this.ProviderType == ProviderType.Sql)
entity.Property(e => e.Comment).HasColumnType("ntext"); That way, the Once you have modified the database generation, you will see that errors are showing up ;) |
I forgot to mention that I had found some bugs that I'm working out. I tried using DMS on a 15 yr old database that was designed by people who had no idea what they were doing. It has everything ugly in it: Text, NText, Image columns, Tables with no primary keys, column names with special characters and very little referential integrity. It is a dumpster-fire of a database, but it's been great for testing and finding edge cases. Once I work threw all the issues, I will make a commit with the fixes for everything I have found. |
…into FixNonComaparableColumnsFixed
…dSinner/Dotmim.Sync into FixNonComaparableColumnsFixed
I think this is ready for merging. There are still a few failing tests (below) that I do not think are related to anything in this PR. Still failing (Net 6.0) tests: |
I've merged in all recent changes and re-ran all the tests. There is now only one failing test (Net 6.0): |
Hello team, first I would like to thank all the maintainers for a great library. however, I am facing the issue of ntext type not being supported. I believe this issue has to do with it. Is this to be released soon? Is there anything that I can help with to move this fix to Nuget packages? |
This PR adds the support of these types, but with a lot of complexicity. I believe we should encourage people to migrate to I think it's a good example, as a provider, but should not be add to the core source of |
Much more importantly, this PR adds support for XML columns which are a first class SQL object that will continue being used well into the future. ntext, text and image support is just there because it takes no extra effort to support them if you are already supporting XML columns. Also, given that ntext, text and image support haven't been a best practice since 2005 when NVARCHAR(MAX), VARCHAR(MAX) and VARBINARY(MAX) came out yet they still exist in the wild, it is a good idea to support them. Databases often move at glacial paces and still contain stuff designed pre-2005 because there hasn't been a business case to update them. *** (Personal request) Since this is critical code for what I'm using this for, if this doesn't get merged in, it will make my future pull requests of other features that I have added extremely difficult if I need to keep all of these changes out of future pull requests. I enjoy giving back to this project, and I'd love if you kept that as an easy option for me by keep our source trees similar. |
Until it is merged, you are welcome to use the source code from the source of this pull request |
This replaces pull requests #938 and #923.
Fixes issue: #910