Upgrade to work on Rails 6.1.3

[codeodor]

• [x] Run tests against Rails 6.1.0.rc1
• [x] Get my app booting
• [x] Fix a few CPK tests but 100+ remain broken

Figured I'd share that in case anyone else is looking to start working on this too.

Other things to do:

• [x] Get running through a few smoke tests in my app
• [x] Get tests passing in CPK
• [x] Get tests passing in my app
• [x] Review # CPK code to see if those methods still exist or need to change based on new Rails
• [x] Review https://github.com/composite-primary-keys/composite_primary_keys/pull/504 to see if it is still an issue after the other updates being made
• [x] Get the Travis CI build passing
• [x] Fix deprecation warnings
• [x] I have an error in my app when doing includes(...).references(...) where Rails adds in its mapping: `students`.`id,district_id` AS t0_r0. In my case I could easily fix to do joins instead, but it worries me that there is somewhere in here we are not correctly giving rails the column name.
• [x] double check #find_each works. In my Rails 6.0 app, it stops processing after the first batch.

[codeodor]

Ok, this is getting too deep for me this evening. Currently stuck where I think visit_CompositePrimaryKeys_CompositeKeys is not getting called, because I've traced a failure to ActiveRecord::ConnectionAdapters::DatabaseStatements#to_sql_and_binds in Rails 6.1, where converts to SQL and later when it tries to execute, tells me [:keycol1, :keycol2] is not a column.

[codeodor]

Only 1 failure and 24 more errors remain on the mysql tests. Lots are related though, so that's good.

[codeodor]

I got all the tests passing on mysql, postgresql, and sqlite3 on my machine. Gonna try to figure out why Travis is erroring out.

[codeodor]

My app, however, still has tons of failures in the tests, but I am not sure if they are related to Rails changes directly or CPK. My hunch is Rails.

[codeodor]

It would be super if there is a way to change travis configs and re-try the build without making a commit. May have to look into that at some point.

[codeodor]

I guess pgsql 13 works but 12 doesn't on Travis right now?

¯\_(ツ)_/¯

[codeodor]

Reviewing my app tests, there is at least one thing in there definitely related to CPK, as it is an error that "col1,col2" column does not exist -- like it's doing #to_s on the array.

There are also a lot of errors where attributes are missing, which I think are probably related to CPK.

Then I'm getting a bunch of stack level too deep errors, which I think is related to a change in Rails that would be unique to my app, without regard to CPK.

[codeodor]

Note on the deprecation warnings in 591167b which resulted in a failure: I am not sure what method to use in a replacement on the model, yet. I will have to research that.

@cfis to answer your question about the version of MySQL I was running to receive the failures in that commit I reverted: I am testing against MySQL 8.0.18 on my dev environment. Not the latest, but not old enough that I think there should be an issue with compatibility?

My app does run on an older version though.

[cfis]

Ok, I was incorrect on MySQL. I thought I had tested version 8 but have not. Note the database doesn't even build due to the table name "groups"

I still think we keep this commit - its a better solution for MariaDB, Postgresql, Oracle and Sqlite. We can then special case MySQL. Also note that code included an implementation of using a ANY where clause instead of IN. That didn't work across all databases, but perhaps MySQL can use it. Or we could try to use a JOIN instead of a WHERE clause.

[codeodor]

I will see what I can do to special-case it so each version of the code will run on the appropriate databases.

[ysbaddaden]

For what it's worth, this branch works perfectly in my app, which uses a legacy MySQL 5.6 database.

[codeodor]

I am sorry I haven't had much time to move it forward to be ready for release. I had to work on a couple of other things, but I should be able to resume soon.

[cfis]

No worries. I will fix the SqlServer breakage on head. Then create a new branch for ar_6.0.x so master can then be updated to work with ar_6.1.x and your updates.

[cfis]

Ok done. MySQL fixed as well as SqlServer! So now I get a clean 100% test run for Sqlite, Postgresql, MySQL, MariaDB, SQLServer and Oracle.

Do need to update to github actions...I'll make a separate ticket for that.

New gem version (12.0.4) pushed. So all yours now for 6.1 support....

[codeodor]

Not excited that local tests work and CI doesn't. :(

[codeodor]

Regarding this item:

I have an error in my app when doing includes(...).references(...) where Rails adds in its mapping: students.id,district_id AS t0_r0. In my case I could easily fix to do joins instead, but it worries me that there is somewhere in here we are not correctly giving rails the column name.

I have changed my mind. It seems like such a random edge case, particularly because I meant to use joins in the first place (it's a query where I'm trying to group by so it doesn't even make sense to try to instantiate the objects), that I am just going to do that. If someone else encounters it, we could look into it then. I did look some more but couldn't find the source of the issue.

[codeodor]

Regarding issue #504, it seems to no longer take 28+ minutes for my query to run, but it still takes 30 seconds vs. the non-subquery route. And, worse, it doesn't actually update any of the records.

[codeodor]

@cfis if you want to add an extra set of eyes on "Review # CPK code to see if those methods still exist or need to change based on new Rails" it may be worthwhile.

Other than that, I still have a couple tests failing in my app, but I'm not sure yet whether they are related to CPK or not.

I hope to check next week.

[codeodor]

@cfis actually, I had a bit more time to review my own app's failing tests and they are not due to CPK. It was a bad merge I did and ripped out some code that was needed. 😳

Anyway, if you've had a chance to review or try this out on your own app as well, and it's working, then I think it's ready for release.

Only thing I don't like is my one caveat above, but I just don't feel like it's worth the time to find it when the code that triggers it wasn't the code I intended to write. (And using my "simplified" version, I could not get to trigger in the CPK tests)

[codeodor]

Ugh... mariadb has an error. 😭

[codeodor]

Actually, I noticed it only failed on Ruby 3, and it runs the same query as all the other ones that passed for MySQL and MariaDB:

SELECT COUNT(count_column) FROM (SELECT DISTINCT `reference_codes`.`reference_type_id`, `reference_codes`.`reference_code` AS count_column FROM `reference_codes` LEFT OUTER JOIN `reference_types` ON `reference_types`.`reference_type_id` = `reference_codes`.`reference_type_id`) subquery_for_count

So I am thinking maybe there was just something wrong with the CI and I'm going to re-run it and hope it passes.

[codeodor]

And :boom:

[cfis]

So I'm not actually working with Rails anymore, thus haven't dived into the Rails 6.1 upgrade. I can see if I can test against some Rails app I have if you want.

I would like to make sure the recent changes in master are included in the 6.1 branch. Maybe you can rebase against master first? And then create a branch for Rails 6.0 and make master support Rails 6.1?

[codeodor]

@cfis ok, I think it's mostly prepared and ready now. However, that CI error on the count is bugging me, so let me try a couple of things to see. Really, I think I added it to be sure the SQL generated was not bombing, so the count itself is not really that important. But it is bugging me that we see those random failures.

[codeodor]

I think I figured it out. The fixtures were not declared, so most of the time it was catching it from having loaded in another test, but sometimes it would have run before the other test ran, so that's when it would fail.

OK then @cfis I think this is ready. In the past I'd rather have been running it in production first, but that's gonna be at least another week from me, if not more, and I don't want to hold it up if others are having success running it as-is and all my tests are passing, at least.

[cfis]

I can try and give it a quick spin on a Rails app I have - but I first would have to upgrade it. That might take me a day or two. But I agree, let's get your changes out this week.

[LyricL-Gitster]

@codeodor Thanks for working on this update! I've been following this PR for a bit and have in the meantime been using your fork on my new app. It's been working great, but recently you reverted the commit we were using and pushed an update that broke us on Rails 6.1.1 with this output:

rake aborted!
ArgumentError: wrong number of arguments (given 2, expected 1)
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/relation/where_clause.rb:137:in `equalities'
/usr/local/bundle/bundler/gems/composite_primary_keys-9035a54587d6/lib/composite_primary_keys/relation/where_clause.rb:5:in `to_h'
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/relation.rb:682:in `where_values_hash'
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/relation.rb:686:in `scope_for_create'
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/scoping.rb:18:in `scope_attributes'
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/scoping.rb:38:in `populate_with_current_scope_attributes'
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/scoping.rb:44:in `initialize_internals_callback'
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/core.rb:476:in `initialize'
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/inheritance.rb:72:in `new'
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/inheritance.rb:72:in `new'
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/relation.rb:798:in `_new'
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/relation.rb:71:in `block in new'
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/relation.rb:406:in `block in scoping'
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/relation.rb:811:in `_scoping'
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/relation.rb:406:in `scoping'
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/relation.rb:71:in `new'
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/relation.rb:226:in `find_or_initialize_by'
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/querying.rb:22:in `find_or_initialize_by'
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/internal_metadata.rb:33:in `[]='
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/migration.rb:1310:in `record_environment'
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/migration.rb:1303:in `migrate_without_lock'
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/migration.rb:1251:in `block in migrate'
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/migration.rb:1401:in `block in with_advisory_lock'
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/migration.rb:1416:in `block in with_advisory_lock_connection'
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/connection_adapters/abstract/connection_pool.rb:462:in `with_connection'
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/migration.rb:1416:in `with_advisory_lock_connection'
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/migration.rb:1397:in `with_advisory_lock'
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/migration.rb:1251:in `migrate'
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/migration.rb:1086:in `up'
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/migration.rb:1061:in `migrate'
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/tasks/database_tasks.rb:237:in `migrate'
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/railties/databases.rake:92:in `block (3 levels) in <main>'
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/railties/databases.rake:90:in `each'
/usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/railties/databases.rake:90:in `block (2 levels) in <main>'
/usr/local/bin/bundle:23:in `load'
/usr/local/bin/bundle:23:in `<main>'
Tasks: TOP => db:migrate
(See full trace by running task with --trace)

Upgrading to Rails 6.1.3 seems to have fixed it in our subsequent builds so far, but I thought I'd drop this here in case it highlighted anything interesting from your end.

Again, thanks!

[codeodor]

Thanks! Yes, this was a change from 6.1.0 to 6.1.3 in Rails as far as I could tell. Not sure I want to get into maintaining different method signatures for a minor point release in Rails since this hasn't been released yet. Glad you were able to upgrade to 6.1.3. I was kind of surprised they changed that method in between, and apologize for not noting it for anyone who got caught by it.

On Tue, Mar 2, 2021 at 9:33 AM Lyric notifications@github.com wrote:

@codeodor https://github.com/codeodor Thanks for working on this update! I've been following this PR for a bit and have in the meantime been using your fork on my new app. It's been working great, but recently you reverted the commit we were using and pushed an update that broke us on Rails 6.1.1 with this output:

rake aborted! ArgumentError: wrong number of arguments (given 2, expected 1) /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/relation/where_clause.rb:137:in equalities' /usr/local/bundle/bundler/gems/composite_primary_keys-9035a54587d6/lib/composite_primary_keys/relation/where_clause.rb:5:into_h' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/relation.rb:682:in where_values_hash' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/relation.rb:686:inscope_for_create' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/scoping.rb:18:in scope_attributes' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/scoping.rb:38:inpopulate_with_current_scope_attributes' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/scoping.rb:44:in initialize_internals_callback' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/core.rb:476:ininitialize' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/inheritance.rb:72:in new' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/inheritance.rb:72:innew' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/relation.rb:798:in _new' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/relation.rb:71:inblock in new' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/relation.rb:406:in block in scoping' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/relation.rb:811:in_scoping' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/relation.rb:406:in scoping' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/relation.rb:71:innew' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/relation.rb:226:in find_or_initialize_by' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/querying.rb:22:infind_or_initialize_by' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/internal_metadata.rb:33:in []=' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/migration.rb:1310:inrecord_environment' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/migration.rb:1303:in migrate_without_lock' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/migration.rb:1251:inblock in migrate' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/migration.rb:1401:in block in with_advisory_lock' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/migration.rb:1416:inblock in with_advisory_lock_connection' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/connection_adapters/abstract/connection_pool.rb:462:in with_connection' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/migration.rb:1416:inwith_advisory_lock_connection' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/migration.rb:1397:in with_advisory_lock' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/migration.rb:1251:inmigrate' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/migration.rb:1086:in up' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/migration.rb:1061:inmigrate' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/tasks/database_tasks.rb:237:in migrate' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/railties/databases.rake:92:inblock (3 levels) in

' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/railties/databases.rake:90:in each' /usr/local/bundle/gems/activerecord-6.1.1/lib/active_record/railties/databases.rake:90:inblock (2 levels) in

' /usr/local/bin/bundle:23:in load' /usr/local/bin/bundle:23:in

' Tasks: TOP => db:migrate (See full trace by running task with --trace)

Upgrading to Rails 6.1.3 seems to have fixed it in our subsequent builds so far, but I thought I'd drop this here in case it highlighted anything interesting from your end.

Again, thanks!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/composite-primary-keys/composite_primary_keys/pull/531#issuecomment-788995159, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAFXNZEDLYAT4VKTVWGSRLTBUALRANCNFSM4UBQAXJQ .

[cfis]

Ok, I was able to spin up the app I have which makes heavy use of CPK (and what I have used over the years to validate new CPK versions).

Sorry to report, but in my opinion these changes are not ready yet. I see hundreds of failures in the test suite. Not all are CPK, but some clearly are. Things like "undefined method composite?" and the tell tale query errors with "key1,key2" mashed together in a string. I'll see if I can find some time to pinpoint the issues.

So I'd say branch cpk 12 at this point, and then rebase your changes on top of master and merge them in. So people who are willing to run against git master can have something that at least partially works. Then we can work out the issues before an official release.

[codeodor]

Thanks @cfis, this will improve it. Particularly if you can find a way to translate those failures into tests. I tried for a couple hours on mine (where I went from includes.references... to join instead), but I couldn't make it fail in a test case.

I already created a 12.0 branch off the main branch and rebased this one against it, so we're good to go there. I am hesitant to merge this into master though, knowing you are seeing a bunch of failures in your app.

[cfis]

That works - I just needed a branch where I can push some changes to - 12.0 sounds good. Hopefully this is just a couple of things that need fixing - one change can sometimes fix a whole lot of failures. I'll see if I can get some time this weekend to see what's happening.

I agree on the tests. I think the test suite isn't bad but it does seem to miss stuff on major upgrades. I"ll try to make a test for each fix if possible to make life easier in the future.

Anyway - thanks for pushing this forward!

[codeodor]

Feel free to fork my repo/branch in this PR and add a PR there too, if you want. However you'd like to do it!

[cfis]

Ok, the nil.composite? bug comes from:

ActiveRecord::PredicateBuilder::AssociationQueryValue#ids

It fails to account for the fact that value is often nil. Note this was not a method we previously overrode so I just commented it out and then a bunch of tests started passing again.

A minor thing:

AssociationReflection:#foreign_key had some debug code that I removed:

raise "cpk called #{@foreign_key}" if @foreign_key == "[:student_id, :district_id]"

These are also new overrides that have been added. At a quick glance, those seemed like good additions (did ActiveRecord just add those methods in 6.1?)

[codeodor]

Good catch. Sorry I left that in there. To be honest I'm not sure if those were added in 6.1 or if they just changed in such a way as they showed up as needing to be overridden!

[cfis]

Ok, once I fixed that method everythings seem good. I now see 28 failures in my test suite, which do not look CPK related. So I sent ahead and merged your changes to master.

Nice work!

[codeodor]

Thanks!

[cfis]

FYI, the way I triggered this code is via an association this is nil (thus the value is nil) when creating an model. It is a test of creating an object with an invalid associated object. I'll see if I can write up a test.