Commit 8a8784f1 authored by Dylan Griffith's avatar Dylan Griffith

Fix Elasticsearch sorting incorrectly from DB

Before this our search results were getting mixed up within a page. The
[`Records#records`](
https://github.com/elastic/elasticsearch-rails/blob/v6.1.0/elasticsearch-model/lib/elasticsearch/model/adapters/active_record.rb#L21
)
method is meant to sort again the query results based on the original
Elasticsearch ordering after loading from the DB, but this was not
working correctly in our code due to us monkey patching the
elasticsearch-model Gem and changing the value of
[`_id`](
https://gitlab.com/gitlab-org/gitlab/blob/8214da073b9692ffb75095390bc2fa5d862b5c05/ee/lib/gem_extensions/elasticsearch/model/response/records.rb#L9
)
for Elasticsearch records.

As such we needed to dig ourselves further into the monkey patch hole.
This MR replaces a single line inside `Records#records` method, which,
unfortunately, required us to replace the entire lengthy method.

https://gitlab.com/gitlab-org/gitlab/-/issues/229982
parent 39c5d919
......@@ -13,6 +13,7 @@ Gitlab.ee do
Elasticsearch::Model::Adapter::Multiple::Records.prepend GemExtensions::Elasticsearch::Model::Adapter::Multiple::Records
Elasticsearch::Model::Indexing::InstanceMethods.prepend GemExtensions::Elasticsearch::Model::Indexing::InstanceMethods
Elasticsearch::Model::Adapter::ActiveRecord::Importing.prepend GemExtensions::Elasticsearch::Model::Adapter::ActiveRecord::Importing
Elasticsearch::Model::Adapter::ActiveRecord::Records.prepend GemExtensions::Elasticsearch::Model::Adapter::ActiveRecord::Records
Elasticsearch::Model::Client::InstanceMethods.prepend GemExtensions::Elasticsearch::Model::Client
Elasticsearch::Model::Client::ClassMethods.prepend GemExtensions::Elasticsearch::Model::Client
Elasticsearch::Model::ClassMethods.prepend GemExtensions::Elasticsearch::Model::Client
......
---
title: Fix Elasticsearch sorting incorrectly from DB
merge_request: 37665
author:
type: fixed
# frozen_string_literal: true
module GemExtensions
module Elasticsearch
module Model
module Adapter
module ActiveRecord
# rubocop:disable all
module Records
# Original method
# https://github.com/elastic/elasticsearch-rails/blob/v6.1.0/elasticsearch-model/lib/elasticsearch/model/adapters/active_record.rb#L21
def records
sql_records = klass.where(klass.primary_key => ids)
sql_records = sql_records.includes(self.options[:includes]) if self.options[:includes]
# Re-order records based on the order from Elasticsearch hits
# by redefining `to_a`, unless the user has called `order()`
#
sql_records.instance_exec(response.response['hits']['hits']) do |hits|
ar_records_method_name = :to_a
ar_records_method_name = :records if defined?(::ActiveRecord) && ::ActiveRecord::VERSION::MAJOR >= 5
define_singleton_method(ar_records_method_name) do
if defined?(::ActiveRecord) && ::ActiveRecord::VERSION::MAJOR >= 4
self.load
else
self.__send__(:exec_queries)
end
if !self.order_values.present?
# BEGIN_MONKEY_PATCH
#
# Monkey patch sorting because we monkey patch ids and the
# previous code uses `hit['_id']` which does not match our
# record.id. We instead need `hit['_source']['id']`.
@records.sort_by { |record| hits.index { |hit| hit['_source']['id'].to_s == record.id.to_s } }
# END_MONKEY_PATCH
else
@records
end
end if self
end
sql_records
end
end
# rubocop:enable all
end
end
end
end
end
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Elasticsearch::Model::Adapter::ActiveRecord::Records, :elastic do
describe '#records' do
let(:user) { create(:user) }
let(:search_options) { { options: { current_user: user, project_ids: :any } } }
before do
stub_ee_application_setting(elasticsearch_indexing: true)
@middle_relevant = create(
:issue,
title: 'Sorting could improve', # Some keywords in title
description: 'I think you could make it better'
)
@least_relevant = create(
:issue,
title: 'I love GitLab', # No keywords in title
description: 'There is so much to love! For example, you could not possibly make sorting any better'
)
@most_relevant = create(
:issue,
title: 'Make sorting better', # All keywords in title
description: 'This issue is here to make the sorting better'
)
ensure_elasticsearch_index!
end
it 'returns results in the same sorted order as they come back from Elasticsearch' do
expect(Issue.elastic_search('make sorting better', search_options).records.to_a).to eq([
@most_relevant,
@middle_relevant,
@least_relevant
])
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment