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

Fixes issues with tables that contain non-comparable columns #923

Merged
merged 8 commits into from
Jan 18, 2023

Conversation

SvdSinner
Copy link
Contributor

Fixes issues with tables that contain non-comparable columns (XML, TXT, NTEXT)

@SvdSinner
Copy link
Contributor Author

I made these changes in a style to mimic existing code. Feel welcome to tweak it, reformat it, etc. in any way you would like

@SvdSinner
Copy link
Contributor Author

I'm not sure what to do with the failed MySql & MariaDB Tests. This PR doesn't touch any of that functionality. If I need to do something to fix these tests, please let me know.

@Mimetis
Copy link
Owner

Mimetis commented Jan 6, 2023

Hey @SvdSinner

I've reviewed your PR, and I think you've found a pretty interesting solution.
Indeed, there is some failed tests, but I'll fixes them, no worry

I've reviewed the SQL generated by the GetChanges stored proc and I remember now, why we have a distinct in the query.
It's only used when we have a filter associated with the query.

If there is no filter, there is no benefits to add a distinct to the Select clause (as the tracking table has a 1-1 relationship with its parent table)

I'm currently working on refactoring tests and some providers routine, but once it's done, I will work on your PR, to fix everything.

Thanks for this great solution to improve the support of old text and ntext fields! (That's being said, please... don't use them :D )

@SvdSinner
Copy link
Contributor Author

I realized there were some bugs in the ChangeTracking project that this caused. There is now a new commit that fixes the 3 typos that caused the bugs.

@Mimetis
Copy link
Owner

Mimetis commented Jan 16, 2023

Hey @SvdSinner

Sorry to come back to you so late.
I was struggling with the refactor of all the tests and the integration of the Postgres provider.

I've made some adjustments in the GetChanges stored procedures generation for SQL Server.
Can you update your PR to resolve the conflicts and be sure your adjustments for "Xml", "Text", "NText" are generated only when filter != null ?

Once it's done, I will make the rights adjustments to integrate the corresponding's tests and be sure it's working fine with all the other providers.

Thanks a ton for your contribution!

@Mimetis
Copy link
Owner

Mimetis commented Jan 18, 2023

Thanks !
I will make the relevant tests before the next release

@Mimetis
Copy link
Owner

Mimetis commented Jan 18, 2023

I'm sorry @SvdSinner, I had to revert back your PR (see #936 ) as all tests are failing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants