Rails 7.1 compatibility #601
Conversation
rails/rails#48241 changed the method signature for `sql_for_insert` from three to four arguments. This caused some applications to fail with: `ArgumentError: wrong number of arguments (given 4, expected 3)` in `lib/composite_primary_keys/connection_adapters/postgresql/database_statements.rb:5:in `sql_for_insert'`
| new_id = self.class._insert_record( | ||
| attributes_with_values(attribute_names) | ||
| attributes_with_values(attribute_names), | ||
| returning_columns_for_insert |
There was a problem hiding this comment.
This seems like a low-effort "fix" from my side.
For reference, upstream changed this from
new_id = self.class._insert_record(
attributes_with_values(attribute_names)
)
self.id ||= new_id if @primary_keyto
returning_columns = self.class._returning_columns_for_insert
returning_values = self.class._insert_record(
attributes_with_values(attribute_names),
returning_columns
)
returning_columns.zip(returning_values).each do |column, value|
_write_attribute(column, value) if !_read_attribute(column)
end if returning_values
|
Thanks for the PR. Looks like we also need to implement the other changes though including using returning parameter? |
|
If I understand the versioning right we can "break" things in a new major version since every version is just designed to work with one version of ActiveRecord, right? So a potential 15.x release doesn't have to be compatible with ActiveRecord 7.0 |
|
Yes, that is correct. Are there big differences between 7.0 and 7.1? Note the tests failed with this change. |
|
Hello, I'm seeing the following in Rails 7.1 beta notes:
What does this mean for this gem? Does Rails 7.1 obsolete this gem? |
|
Good question - don't know. Hopefully. |
|
I am not sure about performance of |
|
More about composite keys support here: https://guides.rubyonrails.org/active_record_composite_primary_keys.html |
|
It seems from #608 there isn't any reason to update CPK for Rails 7.1. Thoughts? |
|
I think it might make sense to support 7.1 so you have an easier upgrade path. So that you can upgrade to Rails 7.1 without also having to migrate to the new built-in composite primary keys at the same time. |
|
Maybe, but it sounded from the other issue the upgrade was pretty easy. But I haven't tried it myself. |
|
When I get around to it, I'll see if I can upgrade my app by changing CPK to just add the code: Which should theoretically work? |
|
And the :foreign_key => [a,b] to :query_constraints it looked.
On 2023-10-11 13:41, Sammy Larbi ***@***.***> wrote:
When I get around to it, I'll see if I can upgrade my app by changing CPK to just add the code:
def self.primary_keys = (array)
self.primary_key = array
end
Which should theoretically work?
—
Reply to this email directly, view it on GitHub (#601 (comment)), or unsubscribe (https://github.com/notifications/unsubscribe-auth/AAAERRRQDJEZPQZVYPCGG53X63765ANCNFSM6AAAAAA2OWQUDM).
You are receiving this because you commented.Message ID: ***@***.***>
[ { ***@***.***": "http://schema.org", ***@***.***": "EmailMessage", "potentialAction": { ***@***.***": "ViewAction", "target": "#601 (comment)", "url": "#601 (comment)", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { ***@***.***": "Organization", "name": "GitHub", "url": "https://github.com" } } ]
|
|
FYI: I have opened a bug report on rails regarding composite primary keys: |
|
Hello @marcoroth any plans to continue this implementation? |
|
@marcocarvalho I migrated to the built-in Rails solution in the meantime. I also realized that my approach is kinda flawed and somehow only just worked to make my tests pass. I think this would need another more serious take to make it work with Rails 7.1. |
|
Yes, I think better to just port to the official Rails support. |
Hey there 👋🏼
First of all, thanks for the long-running support and continued maintenance of the
composite_primary_keysgem! 🙏🏼While following Rails Edge in one of my apps I noticed that the pull request rails/rails#48241 changed the method signature for
ConnectionAdapters#sql_for_insertfrom three to four arguments andPersitance#_insert_recordfrom one to two arguments.This caused applications using
composite_primary_keysfail to boot and crash with:Full Backtrace
And
Full Backtrace
This pull requests updates the methods signature for
sql_for_insertincomposite_primary_keysso it could take either 3 or 4 arguments and the call to_insert_recordfrom one to two inside_create_record.I'm also aware that Rails 7.1 is going to get some kind of support for composite primary keys, but I guess it would still make sense for
composite_primary_keysto be Rails 7.1+ compatible to provide a smoother upgrade/migration path.I'm not sure if this is the proper and correct way to fix this. At least it's making my apps test pass and lets the app boot again. So I think this pull request is at least a starting point for making
composite_primary_keysRails 7.1 compatible.