From bdbf11d2b61cc5b46838938db4c1577082c0234b Mon Sep 17 00:00:00 2001 From: Zeke Gabrielse Date: Sat, 9 Mar 2024 02:37:32 -0600 Subject: [PATCH] add query matcher for multi-query unions and preloading --- Gemfile | 1 - Gemfile.lock | 8 - spec/lib/union_of_spec.rb | 210 +++++++++++++++++++------ spec/models/user_spec.rb | 2 +- spec/support/matchers/query_matcher.rb | 78 +++++++++ 5 files changed, 242 insertions(+), 57 deletions(-) create mode 100644 spec/support/matchers/query_matcher.rb diff --git a/Gemfile b/Gemfile index f037e8fdb2..2c0d6cacf1 100644 --- a/Gemfile +++ b/Gemfile @@ -125,7 +125,6 @@ group :test do gem 'rspec-rails', '~> 6.0.3' gem 'rspec-expectations', '~> 3.12.1' gem 'anbt-sql-formatter' - gem 'db-query-matchers' gem 'factory_bot_rails', '~> 6.2' gem 'database_cleaner', '~> 2.0' gem 'webmock', '~> 3.14.0' diff --git a/Gemfile.lock b/Gemfile.lock index 4143a47809..d5c5359372 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -192,9 +192,6 @@ GEM database_cleaner-core (~> 2.0.0) database_cleaner-core (2.0.1) date (3.3.3) - db-query-matchers (0.12.0) - activesupport (>= 4.0, < 7.2) - rspec (>= 3.0) diff-lcs (1.5.0) dotenv (2.7.6) dotenv-rails (2.7.6) @@ -408,10 +405,6 @@ GEM rack (>= 1.4) rexml (3.2.5) rotp (6.2.0) - rspec (3.12.0) - rspec-core (~> 3.12.0) - rspec-expectations (~> 3.12.0) - rspec-mocks (~> 3.12.0) rspec-core (3.12.2) rspec-support (~> 3.12.0) rspec-expectations (3.12.1) @@ -531,7 +524,6 @@ DEPENDENCIES cucumber-rails (~> 2.5) cuke_modeler (~> 3.19) database_cleaner (~> 2.0) - db-query-matchers dotenv-rails ed25519 elif (~> 0.1.0) diff --git a/spec/lib/union_of_spec.rb b/spec/lib/union_of_spec.rb index 6a19972419..84d09948d2 100644 --- a/spec/lib/union_of_spec.rb +++ b/spec/lib/union_of_spec.rb @@ -338,21 +338,14 @@ end end - # TODO(ezekg) Need to match on multiple queries since this is done in 2 - # parts, for performance reasons. - skip 'should produce a union query' do + it 'should produce a union query' do user = create(:user, account:) - license = create(:license, owner: user) + license = create(:license, account:, owner: user) machine = create(:machine, license:) - expect(user.machines.to_sql).to match_sql <<~SQL.squish - SELECT - "machines".* - FROM - "machines" - INNER JOIN "licenses" ON "machines"."license_id" = "licenses"."id" - WHERE - "licenses"."id" IN ( + expect { user.machines }.to( + match_queries(count: 2) do |queries| + expect(queries.first).to match_sql <<~SQL.squish SELECT "licenses"."id" FROM @@ -363,7 +356,7 @@ FROM "licenses" WHERE - "licenses"."user_id" = '#{record.id}' + "licenses"."user_id" = '#{user.id}' ) UNION ( @@ -373,13 +366,24 @@ "licenses" INNER JOIN "license_users" ON "licenses"."id" = "license_users"."license_id" WHERE - "license_users"."user_id" = '#{record.id}' + "license_users"."user_id" = '#{user.id}' ) ) "licenses" - ) - ORDER BY - "machines"."created_at" ASC - SQL + SQL + + expect(queries.second).to match_sql <<~SQL.squish + SELECT + "machines".* + FROM + "machines" + INNER JOIN "licenses" ON "machines"."license_id" = "licenses"."id" + WHERE + "licenses"."id" IN ('#{license.id}') + ORDER BY + "machines"."created_at" ASC + SQL + end + ) end it 'should produce a deep union join' do @@ -413,22 +417,15 @@ SQL end - # TODO(ezekg) Need to match on multiple queries here - skip 'should produce a deep union query' do + it 'should produce a deep union query' do user = create(:user, account:) - license = create(:license, owner: user) + license = create(:license, account:, owner: user) machine = create(:machine, license:) component = create(:component, machine:) - expect(record.components.to_sql).to match_sql <<~SQL.squish - SELECT - "machine_components".* - FROM - "machine_components" - INNER JOIN "machines" ON "machine_components"."machine_id" = "machines"."id" - INNER JOIN "licenses" ON "machines"."license_id" = "licenses"."id" - WHERE - "licenses"."id" IN ( + expect { user.components }.to( + match_queries(count: 2) do |queries| + expect(queries.first).to match_sql <<~SQL.squish SELECT "licenses"."id" FROM @@ -439,7 +436,7 @@ FROM "licenses" WHERE - "licenses"."user_id" = '#{record.id}' + "licenses"."user_id" = '#{user.id}' ) UNION ( @@ -448,14 +445,26 @@ FROM "licenses" INNER JOIN "license_users" ON "licenses"."id" = "license_users"."license_id" - WHERE - "license_users"."user_id" = '#{record.id}' + WHERE + "license_users"."user_id" = '#{user.id}' ) ) "licenses" - ) - ORDER BY - "machine_components"."created_at" ASC - SQL + SQL + + expect(queries.second).to match_sql <<~SQL.squish + SELECT + "machine_components".* + FROM + "machine_components" + INNER JOIN "machines" ON "machine_components"."machine_id" = "machines"."id" + INNER JOIN "licenses" ON "machines"."license_id" = "licenses"."id" + WHERE + "licenses"."id" IN ('#{license.id}') + ORDER BY + "machine_components"."created_at" ASC + SQL + end + ) end it 'should produce a deeper union join' do @@ -868,7 +877,7 @@ expect(license.association(:owner).loaded?).to be false expect(license.association(:licensees).loaded?).to be false - expect { license.users }.to_not make_database_queries + expect { license.users }.to match_queries(count: 0) expect(license.users.sort_by(&:id)).to eq license.reload.users.sort_by(&:id) end end @@ -943,7 +952,7 @@ expect(user.association(:machines).loaded?).to be true expect(user.association(:licenses).loaded?).to be false - expect { user.machines }.to_not make_database_queries + expect { user.machines }.to match_queries(count: 0) expect(user.machines.sort_by(&:id)).to eq user.reload.machines.sort_by(&:id) end end @@ -951,16 +960,63 @@ it 'should support preloading a union' do licenses = License.preload(:users) - # FIXME(ezekg) How can I test the actual SQL used for preloading? - expect { licenses.to_a }.to make_database_queries(count: 4) - .and not_raise_error + expect { licenses }.to( + match_queries(count: 4) do |queries| + license_ids = licenses.ids.uniq + owner_ids = licenses.map(&:owner_id).compact.uniq + user_ids = licenses.flat_map(&:licensee_ids).uniq + + expect(queries.first).to match_sql <<~SQL.squish + SELECT "licenses".* FROM "licenses" ORDER BY "licenses"."created_at" ASC + SQL + + expect(queries.second).to match_sql <<~SQL.squish + SELECT + "license_users".* + FROM + "license_users" + WHERE + "license_users"."license_id" IN ( + #{license_ids.map { "'#{_1}'" }.join(', ')} + ) + ORDER BY + "license_users"."created_at" ASC + SQL + + expect(queries.third).to match_sql <<~SQL.squish + SELECT + "users".* + FROM + "users" + WHERE + "users"."id" IN ( + #{owner_ids.map { "'#{_1}'" }.join(', ')} + ) + ORDER BY + "users"."created_at" ASC + SQL + + expect(queries.fourth).to match_sql <<~SQL.squish + SELECT + "users".* + FROM + "users" + WHERE + "users"."id" IN ( + #{user_ids.map { "'#{_1}'" }.join(', ')} + ) + ORDER BY + "users"."created_at" ASC + SQL + end + ) licenses.each do |license| expect(license.association(:users).loaded?).to be true expect(license.association(:owner).loaded?).to be true expect(license.association(:licensees).loaded?).to be true - expect { license.users }.to_not make_database_queries + expect { license.users }.to match_queries(count: 0) expect(license.users.sort_by(&:id)).to eq license.reload.users.sort_by(&:id) end end @@ -968,15 +1024,75 @@ it 'should support preloading a through union' do users = User.preload(:machines) - # FIXME(ezekg) How can I test the actual SQL used for preloading? - expect { users.to_a }.to make_database_queries(count: 5) - .and not_raise_error + expect { users }.to( + match_queries(count: 5) do |queries| + user_ids = users.ids.uniq + user_license_ids = users.flat_map(&:user_license_ids).reverse.uniq.reverse # order is significant + license_ids = users.flat_map(&:license_ids).uniq + + expect(queries.first).to match_sql <<~SQL.squish + SELECT "users" . * FROM "users" ORDER BY "users"."created_at" ASC + SQL + + expect(queries.second).to match_sql <<~SQL.squish + SELECT + "licenses".* + FROM + "licenses" + WHERE + "licenses"."user_id" IN ( + #{user_ids.map { "'#{_1}'" }.join(', ')} + ) + ORDER BY + "licenses"."created_at" ASC + SQL + + expect(queries.third).to match_sql <<~SQL.squish + SELECT + "license_users".* + FROM + "license_users" + WHERE + "license_users"."user_id" IN ( + #{user_ids.map { "'#{_1}'" }.join(', ')} + ) + ORDER BY + "license_users"."created_at" ASC + SQL + + expect(queries.fourth).to match_sql <<~SQL.squish + SELECT + "licenses".* + FROM + "licenses" + WHERE + "licenses"."id" IN ( + #{user_license_ids.map { "'#{_1}'" }.join(', ')} + ) + ORDER BY + "licenses"."created_at" ASC + SQL + + expect(queries.fifth).to match_sql <<~SQL.squish + SELECT + "machines".* + FROM + "machines" + WHERE + "machines"."license_id" IN ( + #{license_ids.map { "'#{_1}'" }.join(', ')} + ) + ORDER BY + "machines"."created_at" ASC + SQL + end + ) users.each do |user| expect(user.association(:machines).loaded?).to be true expect(user.association(:licenses).loaded?).to be true - expect { user.machines }.to_not make_database_queries + expect { user.machines }.to match_queries(count: 0) expect(user.machines.sort_by(&:id)).to eq user.reload.machines.sort_by(&:id) end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 0bf742de16..8ec71fdba9 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -292,7 +292,7 @@ it 'should preload user statuses' do statuses = nil - expect { statuses = User.preload(:any_active_licenses).collect(&:status) }.to make_database_queries(count: 4) + expect { statuses = User.preload(:any_active_licenses).collect(&:status) }.to match_queries(count: 4) expect(statuses).to eq User.all.collect(&:status) end diff --git a/spec/support/matchers/query_matcher.rb b/spec/support/matchers/query_matcher.rb new file mode 100644 index 0000000000..e93dd8c345 --- /dev/null +++ b/spec/support/matchers/query_matcher.rb @@ -0,0 +1,78 @@ +# frozen_string_literal: true + +# FIXME(ezekg) There doesn't seem to be an elegant way of making an RSpec +# matcher that accepts a block, e.g.: +# +# expect { ... }.to match_queries { ... } +# +# From what I gathered, this is the best way... +def match_queries(...) = QueryMatcher.new(...) +def match_query(...) = match_queries(...) + +class QueryMatcher + def initialize(count: 0, &block) + @count = count + @block = block + end + + def supports_block_expectations? = true + def supports_value_expectations? = true + + def matches?(block) + @queries = QueryLogger.log(&block) + + if @block + @block.call(@queries) + end + + @queries.size == @count + end + + def failure_message + "expected to match #{@count} queries but got #{@queries.size}" + end + + def failure_message_when_negated + "expected to not match #{@count} queries" + end + + private + + class QueryLogger + IGNORED_STATEMENTS = %w[CACHE SCHEMA] + IGNORED_QUERIES = %r{^(?:ROLLBACK|BEGIN|COMMIT|SAVEPOINT|RELEASE)} + IGNORED_COMMENTS = %r{ + /\*(\w+='\w+',?)+\*/ # query log tags + }x + + def initialize + @queries = [] + end + + def self.log(&) = new.log(&) + def log(&block) + ActiveSupport::Notifications.subscribed( + logger_proc, + 'sql.active_record', + &proc { + result = block.call + result.load if result in ActiveRecord::Relation # autoload relations + } + ) + + @queries + end + + private + + def logger_proc = proc(&method(:logger)) + def logger(event) + unless IGNORED_STATEMENTS.include?(event.payload[:name]) || IGNORED_QUERIES.match(event.payload[:sql]) + query = event.payload[:sql].gsub(IGNORED_COMMENTS, '') + .squish + + @queries << query + end + end + end +end