elastic / elasticsearch-rails

Elasticsearch integrations for ActiveModel/Record and Ruby on Rails
Apache License 2.0
3.07k stars 793 forks source link

Satisfy all tests on 8.x branch #1075

Open sinisterchipmunk opened 5 months ago

sinisterchipmunk commented 5 months ago

I found this conversation ( https://github.com/elastic/elasticsearch-rails/pull/1056 ) which led me to the unreleased 8.x branch, but the tests are currently failing.

This pull request makes as few changes as possible in order to make all unit tests pass when running rake test:all from the project root.

The first commit makes some changes that I found necessary in order to connect to Elasticsearch 8.13.2 running in Docker on my local machine. It...

The second commit actually fixes the tests. There was only one logic change needed.

I hope that with passing tests we may see a formal 8.x release soon. I am not clear on what else needs to be done but am available to contribute more if needed.

sinisterchipmunk commented 5 months ago

Actually, the problem was more subtle. When the tests were failing, changing from include to extend did make the tests pass, but broke in a real Rails app.

It turns out the call to class_eval was triggering different behavior in test than it was in a real Rails app. In test, class_eval was called on an instance of ClassMethodsProxy, which overrides method_missing and passes the call to class_eval into the inheriting model. Thus calling include as the original code did (before this PR) was including the adapter's methods into the inheriting model, polluting its namespace, while not defining them in the ClassMethodsProxy as intended. The resulting methods in the inheriting model were instance methods, not class methods, and so method_missing couldn't find them when __transform was called at run-time. Changing include to extend made method_missing proxy into the methods, so the tests passed. However, the behavior was still incorrect, because the model's namespace was unintentionally polluted.

In the real application, on the other hand, ActiveSupport defines Kernel#class_eval, so that all objects have this method. But its implementation causes a method to be defined on the singleton class of an instance of an object. So when class_eval was called, this didn't go into ClassMethodsProxy#method_missing at all, but rather called the ActiveSupport implementation. This defined the methods in the right place (the singleton of the ClassMethodsProxy instance), but then extend made them class methods, not instance methods as they needed to be, and broke the real Rails app.

So the solution was to revert the previous commit, changing extend back to include, and then simply ensure ActiveSupport is fully loaded in test with require "active_support/all". This causes the test environment to behave the same as the real Rails app.

Although this was technically all that was needed, I decided to also add a test case to check that the adapter's __transform method isn't erroneously placed into the inheriting model's namespace, since that test might have made this easier to debug, and verifies the correct behavior of ClassMethodsProxy in the first place.

Ironically, this also means no actual business logic change was needed at all, as the test failures were entirely due to the test environment.

picandocodigo commented 5 months ago

@sinisterchipmunk this is great work, thank you very much. Great find on the class_eval code, and thanks for adding a test. I've incorporated these changes into 8.x and I'll work on a pre-release of elasticsearch-rails 8.0.0 soon. Thanks!