pat / thinking-sphinx

Sphinx/Manticore plugin for ActiveRecord/Rails
http://freelancing-gods.com/thinking-sphinx
MIT License
1.63k stars 469 forks source link

Fixes for Rails 7.1 #1252

Closed jdelStrother closed 2 months ago

jdelStrother commented 10 months ago

This was going to just be a tiny fix for a LogSubscriber deprecation in Rails 7.1 (https://github.com/pat/thinking-sphinx/commit/cde7554aaf5169ea633015b40992204c53953e2d) but I ran into a few other issues getting the test-suite running.

atomical commented 9 months ago

@jdelStrother What errors do you get without this fix?

jdelStrother commented 9 months ago

Fix kwarg expectations with new rspec fixes, eg,

  1) ThinkingSphinx.count passes through the given query and options
     Failure/Error:
       expect(ThinkingSphinx::Search).to receive(:new).with('foo', :bar => :baz).
         and_return(search)

     ArgumentError:
       wrong number of arguments (given 1, expected 0)
     # ./.devenv/bundle/ruby/3.1.0/gems/activesupport-7.1.0/lib/active_support/core_ext/object/with.rb:24:in `with'
     # ./spec/thinking_sphinx_spec.rb:19:in `block (3 levels) in <top (required)>'
     # ./.devenv/bundle/ruby/3.1.0/gems/rspec-retry-0.5.7/lib/rspec/retry.rb:115:in `block in run'
     # ./.devenv/bundle/ruby/3.1.0/gems/rspec-retry-0.5.7/lib/rspec/retry.rb:104:in `loop'
     # ./.devenv/bundle/ruby/3.1.0/gems/rspec-retry-0.5.7/lib/rspec/retry.rb:104:in `run'
     # ./.devenv/bundle/ruby/3.1.0/gems/rspec-retry-0.5.7/lib/rspec_ext/rspec_ext.rb:12:in `run_with_retry'
     # ./.devenv/bundle/ruby/3.1.0/gems/rspec-retry-0.5.7/lib/rspec/retry.rb:33:in `block (2 levels) in setup'

Fix FilterReflection with Rails 7.1 fixes, eg,

  5) specifying SQL for index definitions concatenates references that have column
     Failure/Error: ReflectionGenerator.new(reflection, name, class_name).call

     NoMethodError:
       undefined method `new' for nil:NilClass

           ReflectionGenerator.new(reflection, name, class_name).call
                              ^^^^
     # ./lib/thinking_sphinx/active_record/filter_reflection.rb:16:in `call'
     # ./lib/thinking_sphinx/active_record/polymorpher.rb:31:in `clone_with'
     # ./lib/thinking_sphinx/active_record/polymorpher.rb:21:in `block in append_reflections'
     # ./lib/thinking_sphinx/active_record/polymorpher.rb:18:in `each'
     # ./lib/thinking_sphinx/active_record/polymorpher.rb:18:in `append_reflections'
     # ./lib/thinking_sphinx/active_record/polymorpher.rb:9:in `morph!'
     # ./lib/thinking_sphinx/active_record/sql_source.rb:160:in `each'
     # ./lib/thinking_sphinx/active_record/sql_source.rb:160:in `prepare_for_render'
     # ./lib/thinking_sphinx/active_record/sql_source.rb:83:in `render'
     # ./.devenv/bundle/ruby/3.1.0/gems/riddle-2.4.3/lib/riddle/configuration/index.rb:30:in `block in render'
     # ./.devenv/bundle/ruby/3.1.0/gems/riddle-2.4.3/lib/riddle/configuration/index.rb:30:in `collect'
     # ./.devenv/bundle/ruby/3.1.0/gems/riddle-2.4.3/lib/riddle/configuration/index.rb:30:in `render'
     # ./lib/thinking_sphinx/core/index.rb:74:in `render'
     # ./spec/acceptance/specifying_sql_spec.rb:139:in `block (2 levels) in <top (required)>'
     # ./.devenv/bundle/ruby/3.1.0/gems/rspec-retry-0.5.7/lib/rspec/retry.rb:115:in `block in run'
     # ./.devenv/bundle/ruby/3.1.0/gems/rspec-retry-0.5.7/lib/rspec/retry.rb:104:in `loop'
     # ./.devenv/bundle/ruby/3.1.0/gems/rspec-retry-0.5.7/lib/rspec/retry.rb:104:in `run'
     # ./.devenv/bundle/ruby/3.1.0/gems/rspec-retry-0.5.7/lib/rspec_ext/rspec_ext.rb:12:in `run_with_retry'
     # ./.devenv/bundle/ruby/3.1.0/gems/rspec-retry-0.5.7/lib/rspec/retry.rb:33:in `block (2 levels) in setup'

Fix a LogSubscriber deprecation in Rails 7.1 prevents this deprecation warning:

DEPRECATION WARNING: Bolding log text with a positional boolean is deprecated and will be removed in Rails 7.2. Use an option hash instead (eg. `color("my text", :red, bold: true)`).
rpheath commented 2 months ago

Hey @jdelStrother I'd love that fix for the LogSubscriber, it's definitely cluttering up the test output in my application. Looks like this PR stalled out on momentum, any chance of finalizing it and/or isolating that LogSubscriber fix so at least that gets released? Thanks!

jdelStrother commented 2 months ago

It doesn't introduce any new test failures (all the test failures are on ruby 2.7 or sphinx, which are already failing on the main branch), so the branch is kind of 'finished' from that point of view. If @pat's interested in merging new stuff I'll take a look at fixing the remaining test failures, though I wonder if our energy would be better spent just removing the ruby 2.7 & sphinx tests. Certainly from my POV - I'm on an M1 mac, and both those dependencies are pretty problematic on aarch64 last time I checked.

rpheath commented 2 months ago

Thanks @jdelStrother — any thoughts @pat?

jdelStrother commented 2 months ago

I've fixed the existing failures in https://github.com/pat/thinking-sphinx/pull/1263, maybe we can start from there

pat commented 2 months ago

Would love to get this merged in - @jdelStrother are you able to rebase? (I can sort it out otherwise)

jdelStrother commented 2 months ago

Hmm, some timeouts in the tests. I’m surprised they’re that slow - want to try re-running them, or I can raise the timeout? (Do we still want to be running tests against rails 5.0 & 5.1 ?)

pat commented 2 months ago

Re-running them did the trick - it's rare that a full run passes in a single go.

rpheath commented 2 months ago

@pat great to see this one merged! Curious, what's the workflow for getting a new gem published? Thanks again!

pat commented 1 month ago

@rpheath it's really dependant on my spare time for OSS, which I feel is much less these days :(

Still, trying to get a release out today. I'm just reviewing open PRs, merging what I feel is worthwhile, seeing if there's any open issues I can also offer fixes for, and then it's just a matter of waiting for CI to catch up, writing up the changelog, and running rake release. 🤞🏻

rpheath commented 1 month ago

@pat hey no worries! I get it man, completely. I've been a long-time user of this code, it's incredible that you're still willing to put in the time to help all of us out. Thank you!