toptal / chewy

High-level Elasticsearch Ruby framework based on the official elasticsearch-ruby client
MIT License
1.88k stars 366 forks source link

Fix a redundant crutch call when :update_fields does not contain related field #863

Closed skcc321 closed 1 year ago

skcc321 commented 1 year ago

what is the issue?

this one https://github.com/toptal/chewy/issues/864

I have an index definition with some crutches inside like this:

class UsersIndex < Chewy::Index
  index_scope User....

  crutch :driver_role do |collection|
      ::User.includes(employees: :group)
        .where(id: collection.map(&:id))
        .group(:id, "groups.kind", "groups.role")
        .pluck(
          :id,
          Arel.sql(
            "jsonb_build_object(
            'driver_role', CASE
                             WHEN (groups.kind = 'carrier' AND groups.role ='ca_driver') THEN TRUE
                             ELSE FALSE
                           END
            )"
          )
        ).to_h
  end

  field :first_name
  field :last_name
  field :driver_role, type: :boolean, value: ->(user, crutch) { crutch.driver_role.dig(user.id, "driver_role") }
  ...
end

when I call

UsersIndex.reset [1], update_fields: [:first_name, :last_name]

it always triggers :driver_role crutch execution even if I don't want to reindex that one by skipping the related field using :update_fields looks like the reason is this line of code https://github.com/toptal/chewy/blob/master/lib/chewy/index/import/bulk_builder.rb#L52 where we don't pass @fields option.


skcc321 commented 1 year ago

@konalegi I'm not sure you are the right person (I see you contributed recently as a Toptal org member). Could you please start the CI, please?

konalegi commented 1 year ago

Thanks for the improvement 🚀 Started CI

skcc321 commented 1 year ago

Thanks for the improvement 🚀 Started CI

One more time, please. just realized that parent existence can cause extra calls as well after the fix. so I cache data_for result in case of parent existence now.

skcc321 commented 1 year ago

@konalegi any objections to getting it merged ( & released)?

konalegi commented 1 year ago

Sorry, I haven't time to review it properly, I'll do it in 1-2 days and most likely release it. One thing that I see, please provide a changelog entry, so that will be easier to generate a release (example)

konalegi commented 1 year ago

@skcc321 sorry for the delay, the person who knows that part very well is now on vacation, so we have to wait until next week to review and release.

konalegi commented 1 year ago

@skcc321 released