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

Add compatibility with Ruby 3 #992

Merged
merged 4 commits into from
Sep 7, 2021

Conversation

indirect
Copy link
Contributor

Since Ruby 3 changed the way keyword arguments work, Rails now only accepts keyword arguments for find_in_batches. Unfortunately, the method_missing implementation on Elasticsearch::Model::Proxy doesn't proxy keyword arguments, so Rails now explodes. The error is triggered by calling Model.import, and looks something like this:

ArgumentError: wrong number of arguments (given 1, expected 0)
bundle/gems/activerecord-6.1.3.1/lib/active_record/relation/batches.rb:128:in 'find_in_batches'

The fix is to forward not just positional arguments, but also keyword arguments. This PR adds a test (failing before the patch), and a patch to forward keyword arguments.

Fixes #989, #977.

@cla-checker-service
Copy link

cla-checker-service bot commented Apr 26, 2021

💚 CLA has been signed

@indirect
Copy link
Contributor Author

I have now read and signed the contributor agreement.

@indirect
Copy link
Contributor Author

I've updated the implementation to work on Ruby 2.4 through 3.0, based on this ruby-core blog post, which should keep this change backwards-compatible.

momocus added a commit to momocus/sakazuki that referenced this pull request Jul 3, 2021
momocus added a commit to momocus/sakazuki that referenced this pull request Jul 3, 2021
momocus added a commit to momocus/sakazuki that referenced this pull request Jul 3, 2021
momocus added a commit to momocus/sakazuki that referenced this pull request Jul 3, 2021
momocus added a commit to momocus/sakazuki that referenced this pull request Jul 3, 2021
@picandocodigo
Copy link
Member

Hi @indirect,
Thank you very much for this contribution! I'm working on a new release and I'd like to include this change. However, could you please restore this line? We need to keep the license header on every source code file.

Thanks!

when the proxy fails to forward keywoard arguments, the error for
methods that only accept keyword arguments looks like this:

ArgumentError:
  wrong number of arguments (given 1, expected 0)
this resolves a bug with Rails 6.1 on Ruby 3, where keyword arguments are not forwarded and this causes an ArgumentError
since Ruby 1.9.2, about 12 years ago, you're supposed to define respond_to_missing? if you also define method_missing. there's more explanation in this blog post by a ruby committer in 2010:

http://blog.marc-andre.ca/2010/11/15/methodmissing-politely/
@indirect
Copy link
Contributor Author

Whoops, I must have accidentally deleted the first line when I opened the file and not noticed. Fixed.

@therrick
Copy link

therrick commented Sep 2, 2021

Any outlook on merging this?

@picandocodigo picandocodigo merged commit d12d812 into elastic:master Sep 7, 2021
@tiendo1011
Copy link

@picandocodigo can you release a new version that includes this fix?
The newest one (7.2.0) is released before this fix is merged.
Thank you

@sebfie
Copy link

sebfie commented Feb 22, 2022

I also need this fix ! Please can you release a new version?

@jonnynux
Copy link

I upvote for releasing

@picandocodigo
Copy link
Member

Version 7.2.1 has been released with these changes.

@mojobiri
Copy link

mojobiri commented May 27, 2022

Would it be possible to add this patch to the 6.x version? We are using Elasticsearch 6 and would love to have Ruby 3 support in 6.1.2 too. Right now we have to monkeypatch.

@dersnek
Copy link

dersnek commented Jan 20, 2024

Would it be possible to add this patch to the 6.x version? We are using Elasticsearch 6 and would love to have Ruby 3 support in 6.1.2 too. Right now we have to monkeypatch.

#1053

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.

Rails Application Template 03-expert.rb - ArgumentError: wrong number of arguments (given 1, expected 0)
8 participants