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

fix: Use more permissive argument passthrough for insert_all and upsert_all #275

Closed
wants to merge 1 commit into from

Conversation

abachman
Copy link

@abachman abachman commented Nov 15, 2023

fixes #274

Rails 7.0 introduced new keyword arguments to ActiveRecord::Base.upsert_all and ActiveRecord::Base.insert_all. The same methods without _all call through to the _all versions of the methods, passing along the new keyword arguments.

For example, in Rails 7.0.8 activerecord/lib/active_record/persistence.rb we see:

def upsert(attributes, on_duplicate: :update, returning: nil, unique_by: nil, record_timestamps: nil)
  upsert_all([ attributes ], on_duplicate: on_duplicate, returning: returning, unique_by: unique_by, record_timestamps: record_timestamps)
end

When this library's override of upsert_all is called with the on_duplicate: and record_timestamps: keyword arguments, an error is raised:

ArgumentError: unknown keywords: :on_duplicate, :record_timestamps

We can see this in the test suite by adding a test to acceptance/cases/models/insert_all_test.rb like:

def test_upsert
  Author.create id: 1, name: "David"
  authors = Author.all.order(:name)
  assert_equal 1, authors.length
  assert_equal "David", authors[0].name

  value = { id: 1, name: "Alice" }

  Author.upsert(value)

  authors = Author.all.order(:name)

  assert_equal 1, authors.length
  assert_equal "Alice", authors[0].name
end

and setting AR_VERSION=7.0.8 in the environment before running the acceptance suite.

Changes

This PR changes the method signatures for insert_all! and upsert_all to use keyword splats (**kwargs) instead of specific, individual keywords.

@abachman abachman requested review from olavloite and a team as code owners November 15, 2023 22:08
Copy link

google-cla bot commented Nov 15, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link

conventional-commit-lint-gcf bot commented Nov 15, 2023

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/ruby-spanner-activerecord API. label Nov 15, 2023
@olavloite
Copy link
Collaborator

@abachman Thanks for looking into this and sorry for the slow turnaround time. Would you mind signing the CLA. This link contains more information on how to do that.

If you would prefer not to do that, then I can create a separate RP with a similar change.

@abachman
Copy link
Author

abachman commented Dec 1, 2023

Hi! 👋🏻

I thought I had done that?

image

Possible complication: I signed as an individual contributor, but using my corporate Google account. Is it possible there's conflict between my emails on this GitHub account (personal + work) and Google (work only) accounts?

@abachman
Copy link
Author

abachman commented Dec 1, 2023

I also have no personal objections or license claims that prevent a duplicate PR with the same effective changes. Particularly if there's a better / more idiomatic way you'd like to implement this.

@olavloite
Copy link
Collaborator

Possible complication: I signed as an individual contributor, but using my corporate Google account. Is it possible there's conflict between my emails on this GitHub account (personal + work) and Google (work only) accounts?

Yes, probably. It seems that GitHub only knows your gmail + Recurly email: https://github.com/googleapis/ruby-spanner-activerecord/pull/275/checks?check_run_id=18724134666

If you signed the CLA using an @google.com account, then this check won't be able to make the link.

I don't have a better way to implement this, I just wanted to make sure that you don't feel like I'm stealing your credits here.

olavloite added a commit that referenced this pull request Dec 6, 2023
Credits to @abachman for creating #275. This PR is created separately to
work around issues with the CLA check in the original PR.

Fixes #274
olavloite added a commit that referenced this pull request Dec 7, 2023
Credits to @abachman for creating #275. This PR is created separately to
work around issues with the CLA check in the original PR.

Fixes #274
olavloite added a commit that referenced this pull request Dec 12, 2023
)

* fix: more permissive arg passthrough for insert_all and upsert_all

Credits to @abachman for creating #275. This PR is created separately to
work around issues with the CLA check in the original PR.

Fixes #274

* fix: get attrib value in 7.x
@olavloite
Copy link
Collaborator

Closing this as #283 has been merged and released. Thanks for your contribution!

@olavloite olavloite closed this Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/ruby-spanner-activerecord API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ArgumentError on upsert due to this library's upsert_all method, which conflicts with Rails'
2 participants