Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

BatchLoader::GraphQL.for doesn't delegate missing methods as BatchLoader.for does #86

Open
stanhu opened this issue Feb 15, 2023 · 0 comments

Comments

@stanhu
Copy link
Contributor

stanhu commented Feb 15, 2023

With the deprecation of BatchLoader.for in favor of BatchLoader::GraphQL.for in #62, we noticed there was a change in the API. We have a test that looks something like:

require 'spec_helper'
require 'batch-loader'

RSpec.describe Gitlab::Utils::BatchLoader do
  let(:stubbed_loader) do
    double( # rubocop:disable RSpec/VerifiedDoubles
      'Loader',
      load_lazy_method: [],
    )
  end

  let(:test_module) do
    Module.new do
      def self.lazy_method(id)
        BatchLoader.for(id).batch(key: :my_batch_name) do |ids, loader|
          stubbed_loader.load_lazy_method(ids)

          ids.each { |id| loader.call(id, id) }
        end
      end
  end

  before do
    BatchLoader::Executor.clear_current
    allow(test_module).to receive(:stubbed_loader).and_return(stubbed_loader)
  end

  describe '.clear_key' do
    it 'clears batched items which match the specified batch key' do
      test_module.lazy_method(1)

      described_class.clear_key(:my_batch_name)

      test_module.lazy_method(4).to_i
   end
end

When we changed BatchLoader::for to use BatchLoader::GraphQL.for, this failed:

  1) Gitlab::Utils::BatchLoader.clear_key clears batched items which match the specified batch key
     Failure/Error: test_module.lazy_method(4).to_i
     
     NoMethodError:
       undefined method `to_i' for #<BatchLoader::GraphQL:0x00007f2dd33f6d50 @batch_loader=#<BatchLoader:0x545480>>
       Did you mean?  to_s
     # ./spec/lib/gitlab/utils/batch_loader_spec.rb:x:in `block (3 levels) in <main>'

This happens because BatchLoader has a method_missing that auto-syncs:

__sync!.public_send(method_name, *args, &block)

Is the expectation now that we must call sync to evaluate a lazy method? Or should BatchLoader::GraphQL also implement its own method_missing like the following?

def method_missing(method_name, *args, &block)
  sync.public_send(method_name, *args, &block)
end
@stanhu stanhu changed the title BatchLoader::GraphQL.for BatchLoader::GraphQL.for doesn't delegate missing methods as BatchLoader.for does Feb 15, 2023
@stanhu stanhu changed the title BatchLoader::GraphQL.for doesn't delegate missing methods as BatchLoader.for does BatchLoader::GraphQL.for doesn't delegate missing methods as BatchLoader.for does Feb 15, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant