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

Get all tests passing on MySQL and PostgreSQL on Rails 6.0.0 #496

Merged
merged 6 commits into from Sep 4, 2019

Conversation

codeodor
Copy link
Collaborator

@codeodor codeodor commented Sep 2, 2019

Fixes #495 and includes changes from #492

@codeodor
Copy link
Collaborator Author

codeodor commented Sep 2, 2019

Can someone check the tests for the other supported databases?

@codeodor
Copy link
Collaborator Author

codeodor commented Sep 2, 2019

CI is failing but appears due to a config issue: says it can't connect to MySQL through the socket.

@codeodor
Copy link
Collaborator Author

codeodor commented Sep 2, 2019

Still getting some errors in my app's test suite. Not sure if they're related to CPK or not, yet. Will report back when I find out.

@codeodor
Copy link
Collaborator Author

codeodor commented Sep 2, 2019

One error is related to a count query. Prior upgrade, it would select count from (select distinct table.id1, table.id2 ... )... but now it is dropping the table qualifier, which makes the id1 and id2 columns ambiguous (due to joining other tables which have those as columns too).

@codeodor
Copy link
Collaborator Author

codeodor commented Sep 2, 2019

The major error in my tests is due to includes/references now coming after joins in the generated query. I don't know if I have a good way to tell in my app if that's due to something in CPK or something in Rails that changed.

@codeodor
Copy link
Collaborator Author

codeodor commented Sep 2, 2019

I have not looked into the one related to the count yet, but given that it is a count on a table with CPK and the column names do not include the table name, I think it probably is an issue with CPK.

The one related to includes I think is due to rails/rails#36805 and is probably not related to CPK. I can reproduce another similar error on another project that does not use CPK -- but instead of rearranging the order, it just removes the includes part of the query.

This is important because any joins to tables with the same columns would produce ambiguous column names
@codeodor codeodor mentioned this pull request Sep 3, 2019
@codeodor
Copy link
Collaborator Author

codeodor commented Sep 3, 2019

@cfis I think this might be ready for a review, if you'd just like to double check me.

The remaining issue in my app's test suite is that includes/references gets moved and further joins reference columns that aren't part of the query yet, but I am starting to think that is an issue with Rails itself, and I've been working around it by just calling left_joins(:relationship_name) and that seems to work.

@codeodor codeodor requested a review from cfis September 3, 2019 15:32
@cfis
Copy link
Contributor

cfis commented Sep 4, 2019

Looks ok to me. Doesn't look like CPK overrides any of the methods changed in rails/rails#36805.

Would it be possible to add a test case for the includes/references issue?

@cfis cfis merged commit a38fede into composite-primary-keys:master Sep 4, 2019
@codeodor
Copy link
Collaborator Author

codeodor commented Sep 4, 2019

I hope to get around to it. I still need to see if I can get it to work correctly in Rails without CPK. I thought I did, but when I ran what I thought was the same scenario in another Rails project, it actually just removed the includes/references altogether, so that's what makes me think it would be Rails at fault rather than CPK. Still need to do that research.

@codeodor
Copy link
Collaborator Author

codeodor commented Sep 4, 2019

@cfis I confirmed the includes/references + join problem is an issue in Rails with this script, and reported to Rails: rails/rails#37133

@cfis
Copy link
Contributor

cfis commented Sep 9, 2019

Got it - thanks for reporting.

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.

ArgumentError: wrong number of arguments (given 1, expected 2) on 12.0.0.rc5
3 participants