diff --git a/adminapp/src/shared/react/useDebugEffect.jsx b/adminapp/src/shared/react/useDebugEffect.jsx new file mode 100644 index 000000000..ed37ddbeb --- /dev/null +++ b/adminapp/src/shared/react/useDebugEffect.jsx @@ -0,0 +1,22 @@ +import React from "react"; + +/** + * Just a `React.useEffect(cb, [])` that does not fire unless in development mode. + * @param cb + * @param {Array=} deps Dependency list. If not given, use empty list. + * @param {boolean=} once If true, fire just once, even in strict mode. + */ +export default function useDebugEffect(cb, { deps, once } = {}) { + const calledRef = React.useRef(false); + React.useEffect(() => { + if (process.env.NODE_ENV !== "development") { + return; + } + if (once && calledRef.current) { + return; + } + cb(); + calledRef.current = true; + // eslint-disable-next-line react-hooks/exhaustive-deps + }, deps || []); +} diff --git a/db/migrations/111_combined_notes.rb b/db/migrations/111_combined_notes.rb new file mode 100644 index 000000000..dc2144e65 --- /dev/null +++ b/db/migrations/111_combined_notes.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +Sequel.migration do + up do + create_view :support_notes_combined_view, + from(:support_notes). + left_join(:support_notes_members, {note_id: :id}). + select( + Sequel[:support_notes].*, + :member_id, + Sequel[nil].as(:verification_id), + ). + union( + from(:support_notes). + left_join(:support_notes_organization_membership_verifications, {note_id: :id}). + left_join(:organization_membership_verifications, {id: :verification_id}). + left_join(:organization_memberships, {id: :membership_id}). + select( + Sequel[:support_notes].*, + :member_id, + :verification_id, + ), + ).order(Sequel.desc(Sequel.function(:coalesce, :edited_at, :created_at)), :id) + end + down do + drop_view :support_notes_combined_view + end +end diff --git a/lib/sequel/plugins/efficient_each.rb b/lib/sequel/plugins/efficient_each.rb new file mode 100644 index 000000000..4bca33ab5 --- /dev/null +++ b/lib/sequel/plugins/efficient_each.rb @@ -0,0 +1,85 @@ +# frozen_string_literal: true + +module Sequel::Plugins::EfficientEach + class UnknownAssociation < ArgumentError; end + + DEFAULT_OPTIONS = { + page_size: 100, + }.freeze + + class << self + def configure(model, opts=DEFAULT_OPTIONS) + opts = DEFAULT_OPTIONS.merge(opts) + model.efficient_each_page_size = opts[:page_size] + end + end + + module ClassMethods + attr_accessor :efficient_each_page_size + + def inherited(subclass) + super + [:efficient_each_page_size].each do |m| + subclass.send("#{m}=", self.send(m)) + end + end + end + + module DatasetMethods + # Call a block for each row in a dataset. + # This is the same as paged_each or use_cursor.each, except that for each page, + # rows are re-fetched using self.where(primary_key => [pks]).all to enable eager loading. + # + # @param page_size [Integer] Size of each page. Smaller uses less memory. + # @param order [Symbol] Column to order by. Default to primary key. + # @param yield_page [true,false] If true, yield the page to the block, rather than individual rows. + # Helpful when bulk processing. + # + # (Note that paged_each does not do eager loading, which makes enumerating model associations very slow) + def each_cursor_page(page_size: nil, order: nil, yield_page: false, &block) + raise LocalJumpError unless block + raise "dataset requires a use_cursor method, class may need `extension(:pagination)`" unless + self.respond_to?(:use_cursor) + model = self.model + page_size ||= model.efficient_each_page_size + pk = model.primary_key + order ||= pk + current_chunk_pks = [] + order = [order] unless order.respond_to?(:to_ary) + self.naked.select(pk).order(*order).use_cursor(rows_per_fetch: page_size, hold: true).each do |row| + current_chunk_pks << row[pk] + next if current_chunk_pks.length < page_size + page = model.where(pk => current_chunk_pks).order(*order).all + current_chunk_pks.clear + yield_page ? yield(page) : page.each(&block) + end + remainder = model.where(pk => current_chunk_pks).order(*order).all + yield_page && !remainder.empty? ? yield(remainder) : remainder.each(&block) + end + end + + module InstanceMethods + def efficient_each(association_name, &) + return enum_for(:efficient_each, association_name) unless block_given? + + assoc = self.class.association_reflection(association_name) + raise UnknownAssociation, "#{self.class.name} has no association :#{association_name}" if + assoc.nil? + loaded = self.associations[association_name] + unless loaded.nil? + loaded.each(&) + return nil + end + dataset = self.send(assoc.fetch(:dataset_method)) + pagecount = 0 + prev_page = [] + dataset.each_cursor_page(yield_page: true) do |page| + pagecount += 1 + prev_page = page + page.each(&) + end + self.associations[association_name] = prev_page if pagecount < 2 + return nil + end + end +end diff --git a/lib/sequel/plugins/large_association_warning.rb b/lib/sequel/plugins/large_association_warning.rb new file mode 100644 index 000000000..5bb092b42 --- /dev/null +++ b/lib/sequel/plugins/large_association_warning.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +module Sequel::Plugins::LargeAssociationWarning + DEFAULT_CALLBACK = lambda do |m, assoc, array| + Appydays::Loggable[m].warn( + "large_assocation_loaded", + model_pk: m.primary_key, + model_type: m.class.name, + model_association: assoc, + model_association_size: array.size, + ) + end + + DEFAULT_OPTIONS = { + threshold: 100, + callback: DEFAULT_CALLBACK, + }.freeze + + class << self + attr_reader :warned_associations + + def configure(model, opts=DEFAULT_OPTIONS) + opts = DEFAULT_OPTIONS.merge(opts) + model.large_association_warning_threshold = opts[:threshold] + model.large_association_warning_callback = opts[:callback] + @warned_associations = Set.new + end + end + + module ClassMethods + attr_accessor :large_association_warning_threshold, :large_association_warning_callback + + def inherited(subclass) + super + [:large_association_warning_threshold, :large_association_warning_callback].each do |m| + subclass.send("#{m}=", self.send(m)) + end + end + end + + module InstanceMethods + def load_associated_objects(opts, dynamic_opts={}) + results = super + if results.is_a?(Array) && results.size > model.large_association_warning_threshold + assoc = opts.fetch(:name) + warn_key = [self.class, assoc] + unless Sequel::Plugins::LargeAssociationWarning.warned_associations.include?(warn_key) + Sequel::Plugins::LargeAssociationWarning.warned_associations.add(warn_key) + model.large_association_warning_callback[self, assoc, results] + end + end + return results + end + end +end diff --git a/lib/suma/analytics/model.rb b/lib/suma/analytics/model.rb index b03969c28..57d5dd6c8 100644 --- a/lib/suma/analytics/model.rb +++ b/lib/suma/analytics/model.rb @@ -29,8 +29,10 @@ class RowMismatch < StandardError; end max_connections: self.max_connections, pool_timeout: self.pool_timeout, } - db = Sequel.connect(self.uri, options) - self.db = db + if self.guard_db_reconnect?(self.uri, options) + db = Sequel.connect(self.uri, options) + self.db = db + end end end diff --git a/lib/suma/member.rb b/lib/suma/member.rb index 7eab91712..eb9c379e2 100644 --- a/lib/suma/member.rb +++ b/lib/suma/member.rb @@ -125,6 +125,18 @@ def initialize(reason) join_table: :support_notes_members, left_key: :member_id, order: order_desc + many_to_many :combined_notes, + class: "Suma::Support::Note", + eager_loader: (lambda do |eo| + eo[:rows].each { |p| p.associations[:combined_notes] = [] } + ds = self.db[:support_notes_combined_view].where(member_id: eo[:id_map].keys) + ds.all.each do |note| + member = eo[:id_map][note[:member_id]].first + member.associations[:combined_notes] << note + end + end) do |_ds| + Suma::Support::Note.for_member(self) + end one_to_many :eligibility_assignments, class: "Suma::Eligibility::Assignment", order: order_desc one_to_many :expanded_eligibility_assignments, @@ -276,15 +288,6 @@ def default_payment_instrument return self.public_payment_instruments.find { |pi| pi.status == :ok } end - def combined_notes - ds = Suma::Support::Note.combine_datasets( - Sequel[members: self], - Sequel[organization_membership_verifications: Suma::Organization::Membership::Verification. - where(membership: self.organization_memberships_dataset)], - ) - return ds.all - end - # @return [Suma::Member::StripeAttributes] def stripe return @stripe ||= Suma::Member::StripeAttributes.new(self) diff --git a/lib/suma/organization/membership/verification.rb b/lib/suma/organization/membership/verification.rb index 78ef03e13..66cf06dfc 100644 --- a/lib/suma/organization/membership/verification.rb +++ b/lib/suma/organization/membership/verification.rb @@ -44,6 +44,31 @@ class Suma::Organization::Membership::Verification < Suma::Postgres::Model(:orga join_table: :support_notes_organization_membership_verifications, left_key: :verification_id, order: order_desc + many_to_many :combined_notes, + class: "Suma::Support::Note", + eager_loader: (lambda do |eo| + eo[:rows].each { |p| p.associations[:combined_notes] = [] } + verifications_for_member_ids = {} + verifications_by_ids = {} + eo[:rows].each do |v| + verifications_for_member_ids[v.membership.member.id] ||= [] + verifications_for_member_ids[v.membership.member.id] << v + verifications_by_ids[v.id] = v + end + ds = self.db[:support_notes_combined_view].where(Sequel[member_id: verifications_for_member_ids.keys]) + ds.all.each do |note| + if (source_id = note[:verification_id]) + verifications_by_ids[source_id].associations[:combined_notes] << note + else + verifications_for_member_ids[note[:member_id]].each do |v| + v.associations[:combined_notes] << note + end + end + end + end) do |_ds| + Suma::Support::Note.for_verification(self) + end + many_to_one :owner, class: "Suma::Member" many_to_one :front_partner_conversation, @@ -340,8 +365,6 @@ def find_duplicates = DuplicateFinder.lookup_matches(self) # Duplicates are stored sorted so we can use the 0th item. def duplicate_risk = self.find_duplicates.first&.max_risk - def combined_notes = Suma::Support::Note.combine_instances(self.notes, self.membership.member.notes) - def rel_admin_link = "/membership-verification/#{self.id}" def hybrid_search_fields diff --git a/lib/suma/payment/card.rb b/lib/suma/payment/card.rb index 26d673fbf..1d16ae8e1 100644 --- a/lib/suma/payment/card.rb +++ b/lib/suma/payment/card.rb @@ -17,6 +17,15 @@ class Suma::Payment::Card < Suma::Postgres::Model(:payment_cards) one_to_many :originated_funding_stripe_card_strategies, key: :originating_card_id, class: "Suma::Payment::FundingTransaction::StripeCardStrategy" + many_through_many :originated_funding_transactions, + [ + [:payment_funding_transaction_stripe_card_strategies, :originating_card_id, :id], + ], + class: "Suma::Payment::FundingTransaction", + left_primary_key: :id, + right_primary_key: :stripe_card_strategy_id, + read_only: true, + order: [:created_at, :id] dataset_module do def usable_for_funding = self.unexpired_as_of(Time.now) @@ -63,9 +72,6 @@ def refetch_remote_data @stripe_data = nil end - # Could move this to an association later - def originated_funding_transactions = self.originated_funding_stripe_card_strategies.map(&:funding_transaction) - def _external_links_self return [ self._external_link( diff --git a/lib/suma/payment/ledger.rb b/lib/suma/payment/ledger.rb index 535a179d2..31abefa41 100644 --- a/lib/suma/payment/ledger.rb +++ b/lib/suma/payment/ledger.rb @@ -230,7 +230,7 @@ def category_used_to_purchase(has_vnd_svc_categories) def find_unbalanced_counterparty_ledgers(include_all: false) platform_account = Suma::Payment::Account.lookup_platform_account totals_by_ledger = {} - self.combined_book_transactions.each do |bx| + self.efficient_each(:combined_book_transactions).each do |bx| if bx.originating_ledger === self counterparty = bx.receiving_ledger amount = bx.amount * -1 diff --git a/lib/suma/postgres/model.rb b/lib/suma/postgres/model.rb index 5c38735ed..1eee8ca9f 100644 --- a/lib/suma/postgres/model.rb +++ b/lib/suma/postgres/model.rb @@ -35,6 +35,8 @@ class Suma::Postgres::Model setting :extension_schema, "public" + setting :large_association_warning_threshold, 500 + # The number of (Float) seconds that should be considered "slow" for a # single query; queries that take longer than this amount of time will be logged # at `warn` level. @@ -58,16 +60,36 @@ class Suma::Postgres::Model pool_timeout: self.pool_timeout, log_warn_duration: self.slow_query_seconds, } - db = Sequel.connect(self.uri, options) - db.extension(:pagination) - db.extension(:pg_json) - db.extension(:pg_inet) - db.extension(:pg_array) - db.extension(:pg_streaming) - db.extension(:pg_range) - db.extension(:pg_interval) - db.extension(:pretty_table) - self.db = db + if self.guard_db_reconnect?(self.uri, options) + db = Sequel.connect(self.uri, options) + db.extension(:pagination) + db.extension(:pg_json) + db.extension(:pg_inet) + db.extension(:pg_array) + db.extension(:pg_streaming) + db.extension(:pg_range) + db.extension(:pg_interval) + db.extension(:pretty_table) + self.db = db + end + + plugin :large_association_warning, + threshold: self.large_association_warning_threshold, + callback: lambda { |m, assoc, array| + Sentry.capture_message("Large association loaded") do |scope| + scope.set_extras( + model_pk: m.pk, + model_type: m.class.name, + model_association: assoc, + model_association_size: array.size, + ) + end + Sequel::Plugins::LargeAssociationWarning::DEFAULT_CALLBACK[m, assoc, array] + } + plugin :efficient_each, + # Use a page size of the large warning threshold, + # as this is what we consider a reasonable page size. + page_size: self.large_association_warning_threshold end end diff --git a/lib/suma/postgres/model_utilities.rb b/lib/suma/postgres/model_utilities.rb index ac6d7d897..ec8c347ea 100644 --- a/lib/suma/postgres/model_utilities.rb +++ b/lib/suma/postgres/model_utilities.rb @@ -47,6 +47,17 @@ def update_connection_appname end end + def guard_db_reconnect?(uri, options) + if self.instance_variable_get(:@db).nil? + @original_options = [self.uri, options] + return true + end + raise Suma::InvalidPrecondition, "cannot modify DB at runtime" unless Suma.test? + raise Suma::InvalidPrecondition, "cannot modify DB with new parameters" unless + @original_options == [uri, options] + return false + end + # Some model classes just map Sequel models onto readonly connections. # These models should override this method and return true. def read_only? = false @@ -361,57 +372,6 @@ def reduce_expr(op_symbol, operands, method: :where) return self.send(method, full_op) end - # Call a block for each row in a dataset. - # This is the same as paged_each or use_cursor.each, except that for each page, - # rows are re-fetched using self.where(primary_key => [pks]).all to enable eager loading. - # - # @param page_size [Integer] Size of each page. Smaller uses less memory. - # @param order [Symbol] Column to order by. Default to primary key. - # @param yield_page [true,false] If true, yield the page to the block, rather than individual rows. - # Helpful when bulk processing. - # - # (Note that paged_each does not do eager loading, which makes enumerating model associations very slow) - def each_cursor_page(page_size: 500, order: nil, yield_page: false, &block) - raise LocalJumpError unless block - raise "dataset requires a use_cursor method, class may need `extension(:pagination)`" unless - self.respond_to?(:use_cursor) - model = self.model - pk = model.primary_key - order ||= pk - current_chunk_pks = [] - order = [order] unless order.respond_to?(:to_ary) - self.naked.select(pk).order(*order).use_cursor(rows_per_fetch: page_size, hold: true).each do |row| - current_chunk_pks << row[pk] - next if current_chunk_pks.length < page_size - page = model.where(pk => current_chunk_pks).order(*order).all - current_chunk_pks.clear - yield_page ? yield(page) : page.each(&block) - end - remainder = model.where(pk => current_chunk_pks).order(*order).all - yield_page ? yield(remainder) : remainder.each(&block) - end - - # See each_cursor_page, but takes an additional action on each chunk of returned rows. - # The action is called with pages of return values from the block when a page is is reached. - # Each call to action should return nil, a result, or an array of results (nil results are ignored). - # - # The most common case is for ETL: process one dataset, map it in a block to return new row values, - # and multi_insert it into a different table. - def each_cursor_page_action(action:, page_size: 500, order: :id) - raise LocalJumpError unless block_given? - returned_rows_chunk = [] - self.each_cursor_page(page_size:, order:) do |instance| - new_row = yield(instance) - next if action.nil? || new_row.nil? - new_row.respond_to?(:to_ary) ? returned_rows_chunk.concat(new_row) : returned_rows_chunk.push(new_row) - if returned_rows_chunk.length >= page_size - action.call(returned_rows_chunk) - returned_rows_chunk.clear - end - end - action&.call(returned_rows_chunk) - end - # Reselect is shorthandle for "ds.select(Sequel[ds.model.table_name][Sequel.lit("*")])". # This is useful after a join that is used in the query, but we only want to return the original model. def reselect diff --git a/lib/suma/spec_helpers/postgres.rb b/lib/suma/spec_helpers/postgres.rb index 9509c15a1..84426debb 100644 --- a/lib/suma/spec_helpers/postgres.rb +++ b/lib/suma/spec_helpers/postgres.rb @@ -23,7 +23,7 @@ module Suma::SpecHelpers::Postgres SPECDIR = BASEDIR + "spec" DATADIR = SPECDIR + "data" - SNIFF_LEAKY_TESTS = false + SNIFF_LEAKY_TESTS = ENV["SNIFF_LEAKY_TESTS"] == "true" Suma::Postgres.register_model("suma/postgres/testing_pixie") SequelHybridSearch.indexing_mode = :off @@ -47,24 +47,15 @@ def self.included(context) Suma::SpecHelpers::Postgres.wrap_example_in_transactions(example) else example.run + truncate_all if example.metadata[:db] == :no_transaction end - has_leaked = SNIFF_LEAKY_TESTS && ( - !Suma::Member.empty? || - !Suma::TranslatedText.empty? - ) - if has_leaked - puts "Database is not cleaned up, failing for diagnosis." - puts "Check this or the spec that ran before: #{example.metadata[:full_description]}" - exit - end + SNIFF_LEAKY_TESTS && Suma::SpecHelpers::Postgres.check_for_leaky_tests end context.after(:each) do |example| Suma::Postgres.do_not_defer_events = false if example.metadata[:do_not_defer_events] Suma::Postgres.unsafe_skip_transaction_check = false if example.metadata[:no_transaction_check] SequelHybridSearch.embedding_generator = nil if example.metadata[:hybrid_search] - - truncate_all if example.metadata[:db] == :no_transaction end super @@ -82,6 +73,28 @@ def self.wrap_example_in_transactions(example) wrapped_proc.call end + def self.check_for_leaky_tests + raise "why is DB still in a transaction?" if Suma::Member.db.in_transaction? + ok_tables = [:schema_info, :uploaded_file_blobs, :roles] + bad_tables = {} + Suma::Postgres::Model.descendants.reject(&:anonymous?).each do |cls| + tbl = cls.table_name + next if ok_tables.include?(tbl) + next unless cls.db.table_exists?(tbl) + rows = cls.dataset.naked.all + next if rows.empty? + bad_tables[tbl] = rows + end + return if bad_tables.empty? + puts "Database is not cleaned up, failing for diagnosis." + puts "Check the last spec that ran" + bad_tables.each do |tbl, rows| + puts tbl + rows.each { |r| puts r } + end + exit + end + singleton_attr_accessor :current_test_model_uid self.current_test_model_uid = 0 diff --git a/lib/suma/support/note.rb b/lib/suma/support/note.rb index ab390370d..48116ab37 100644 --- a/lib/suma/support/note.rb +++ b/lib/suma/support/note.rb @@ -19,18 +19,21 @@ class Suma::Support::Note < Suma::Postgres::Model(:support_notes) left_key: :note_id, right_key: :member_id - class << self - def combine_datasets(*exprs) - ds = self.reduce_expr(:|, exprs) + dataset_module do + def for_verification(verification) + criteria = Sequel[verification_id: verification.id] | + Sequel[member_id: verification.membership.member_id, verification_id: nil] + ds = self.db[:support_notes_combined_view].where(criteria) + ds = self.where(id: ds.select(:id)) ds = ds.order(Sequel.desc(Sequel.function(:coalesce, :edited_at, :created_at)), :id) return ds end - def combine_instances(*arrays) - notes = [].concat(*arrays) - notes.sort_by! { |n| [n.authored_at, -n.id] } - notes.reverse! - return notes + def for_member(member) + ds = self.db[:support_notes_combined_view].where(member_id: member.id) + ds = self.where(id: ds.select(:id)) + ds = ds.order(Sequel.desc(Sequel.function(:coalesce, :edited_at, :created_at)), :id) + return ds end end @@ -48,13 +51,13 @@ def content_md c = self.content start_of_string_regex = %r{^https?://\S+} c = c.gsub(start_of_string_regex) do |url| - # Since this is the start of the string, we can just format as markdown. + # Since this is the start of the string, we can just format as Markdown. "[#{url}](#{url})" end in_string_regex = %r{\shttps?://\S+} c = c.gsub(in_string_regex) do |match| - # The leading character is a space; remove it from the url and prepend it before the markdown link. + # The leading character is a space; remove it from the url and prepend it before the Markdown link. # We do not have matchdata available so cannot use capture groups. url = match[1..] "#{match[0]}[#{url}](#{url})" diff --git a/lib/suma/vendor/service_rate.rb b/lib/suma/vendor/service_rate.rb index e724a1108..62a265c23 100644 --- a/lib/suma/vendor/service_rate.rb +++ b/lib/suma/vendor/service_rate.rb @@ -9,6 +9,7 @@ class Suma::Vendor::ServiceRate < Suma::Postgres::Model(:vendor_service_rates) plugin :money_fields, :unit_amount, :surcharge many_to_one :undiscounted_rate, key: :undiscounted_rate_id, class: "Suma::Vendor::ServiceRate" + one_to_many :discounted_rates, key: :undiscounted_rate_id, class: "Suma::Vendor::ServiceRate" one_to_many :program_pricings, class: "Suma::Program::Pricing", diff --git a/spec/sequel/plugins/efficient_each_spec.rb b/spec/sequel/plugins/efficient_each_spec.rb new file mode 100644 index 000000000..3f597708c --- /dev/null +++ b/spec/sequel/plugins/efficient_each_spec.rb @@ -0,0 +1,135 @@ +# frozen_string_literal: true + +require "sequel/plugins/efficient_each" + +RSpec.describe Sequel::Plugins::EfficientEach, :db do + before(:all) do + @db = Sequel.connect(ENV.fetch("DATABASE_URL")) + @db.drop_table?(:efeach) + @db.create_table(:efeach) do + primary_key :id + text :name + foreign_key :parent_id, :efeach + end + end + after(:all) do + @db.disconnect + end + + let(:cls) do + Class.new(Sequel::Model(:efeach)) do + plugin :efficient_each, page_size: 2 + many_to_one :parent, class: self + one_to_many :children, class: self, key: :parent_id + end + end + + it "errors for an unknown association" do + n = cls.create + expect { n.efficient_each(:foo).first }.to raise_error(described_class::UnknownAssociation) + end + + it "copies configuration to subclasses" do + sub = Class.new(cls) + + expect(cls.efficient_each_page_size).to eq(2) + expect(sub.efficient_each_page_size).to eq(2) + end + + it "can work with a block or return an enumerator" do + parent = cls.create + ch = cls.create(parent:) + expect(parent.children).to contain_exactly(ch) + + expect(parent.efficient_each(:children).to_a).to contain_exactly(ch) + + calls = [] + parent.efficient_each(:children) { |r| calls << r } + expect(calls).to contain_exactly(ch) + end + + describe "with a loaded association" do + let(:parent) { cls.create } + let!(:children) { Array.new(3) { cls.create(parent:) } } + before(:each) do + parent.associations[:children] = children + end + + it "yields each item in the dataset" do + got = parent.efficient_each(:children).to_a + expect(got).to eq(children) + end + end + + describe "without a loaded association" do + let(:parent) { cls.create } + let!(:children) { Array.new(3) { cls.create(parent:) } } + before(:each) do + parent.refresh + end + + it "streams pages from the dataset" do + got = parent.efficient_each(:children).to_a + expect(got).to contain_exactly(be === children[0], be === children[1], be === children[2]) + end + + it "stores the association if there is only one page" do + children[2].destroy + got = parent.efficient_each(:children).to_a + expect(got).to contain_exactly(be === children[0], be === children[1]) + expect(parent.associations).to include(children: have_length(2)) + end + + it "handles an empty association array" do + children.each(&:destroy) + got = parent.efficient_each(:children).to_a + expect(got).to be_empty + expect(parent.associations).to include(children: []) + end + + it "does not store the association if there is more than one page" do + got = parent.efficient_each(:children).to_a + expect(got).to contain_exactly(be === children[0], be === children[1], be === children[2]) + expect(parent.associations).to be_empty + end + end + + describe "dataset method each_cursor_page" do + names = ["a", "b", "c", "d"] + let(:ds) { cls.dataset } + + before(:each) do + names.each { |n| cls.create(name: n) } + end + + it "yields each item to the block" do + result = [] + cls.dataset.each_cursor_page { |r| result << r.name } + expect(result).to eq(names) + end + + it "can order by a column" do + result = [] + cls.dataset.each_cursor_page(order: Sequel.desc(:name)) { |r| result << r.name } + expect(result).to eq(names.reverse) + end + + it "can order by multiple columns" do + result = [] + cls.dataset.each_cursor_page(order: [Sequel.desc(:name), :id]) { |r| result << r.name } + expect(result).to eq(names.reverse) + end + + it "can yield the full page rather than a row" do + result = [] + cls.dataset.each_cursor_page(yield_page: true) { |page| result << page.map(&:name) } + expect(result).to eq([["a", "b"], ["c", "d"]]) + end + + it "can use an override page size" do + result = [] + cls.dataset.each_cursor_page(yield_page: true, page_size: 4) { |page| result << page.map(&:name) } + expect(result).to eq([["a", "b", "c", "d"]]) + end + end +end diff --git a/spec/sequel/plugins/large_association_warning_spec.rb b/spec/sequel/plugins/large_association_warning_spec.rb new file mode 100644 index 000000000..53117d568 --- /dev/null +++ b/spec/sequel/plugins/large_association_warning_spec.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +require "sequel/plugins/large_association_warning" + +RSpec.describe Sequel::Plugins::LargeAssociationWarning, :db do + before(:all) do + @db = Sequel.connect(ENV.fetch("DATABASE_URL")) + @db.drop_table?(:largeassocwarn) + @db.create_table(:largeassocwarn) do + primary_key :id + foreign_key :parent_id, :largeassocwarn + end + end + after(:all) do + @db.disconnect + end + + it "can use its default callback" do + cls = Class.new(Sequel::Model(:largeassocwarn)) do + plugin :large_association_warning, threshold: 2 + many_to_one :parent, class: self + one_to_many :children, class: self, key: :parent_id + end + + parent = cls.create + Array.new(3) { cls.create(parent:) } + parent.refresh.children + end + + it "warns about large associations" do + calls = [] + cls = Class.new(Sequel::Model(:largeassocwarn)) do + plugin :large_association_warning, threshold: 5, callback: ->(*args) { calls << args } + many_to_one :parent, class: self + one_to_many :children, class: self, key: :parent_id + end + + parent = cls.create + Array.new(5) { cls.create(parent:) } + parent.refresh.children + expect(calls).to be_empty + cls.create(parent:) + expect(calls).to be_empty + parent.refresh.children + expect(calls).to contain_exactly([parent, :children, have_length(6)]) + # Do not re-warn + parent.refresh.children + expect(calls).to contain_exactly([parent, :children, have_length(6)]) + end + + it "copies configuration to subclasses" do + base = Class.new(Sequel::Model(:largeassocwarn)) do + plugin :large_association_warning, threshold: 5 + end + sub = Class.new(base) + + expect(base.large_association_warning_threshold).to eq(5) + expect(sub.large_association_warning_threshold).to eq(5) + end +end diff --git a/spec/suma/member_spec.rb b/spec/suma/member_spec.rb index f1a8a9e2b..0d7fe5b49 100644 --- a/spec/suma/member_spec.rb +++ b/spec/suma/member_spec.rb @@ -373,7 +373,7 @@ def skip_verification?(c, list=nil) end end - describe "#combine_notes" do + describe "#combined_notes" do it "selects and sorts all notes from related resources" do m = Suma::Fixtures.member.create v_fac = Suma::Fixtures.organization_membership_verification.member(m) diff --git a/spec/suma/payment/card_spec.rb b/spec/suma/payment/card_spec.rb index 23fa0db3f..ec4c83342 100644 --- a/spec/suma/payment/card_spec.rb +++ b/spec/suma/payment/card_spec.rb @@ -7,6 +7,16 @@ it_behaves_like "a payment instrument" + describe "associations" do + it "knows originated funding transactions" do + card = Suma::Fixtures.card.create + strategy = Suma::Payment::FundingTransaction::StripeCardStrategy.create(originating_card: card) + xaction = Suma::Fixtures.funding_transaction(strategy:).create + + expect(card.originated_funding_transactions).to have_same_ids_as(xaction) + end + end + it "knows when it is usable for funding and payouts" do c = Suma::Fixtures.card.create expect(c).to be_usable_for_funding diff --git a/spec/suma/postgres/model_spec.rb b/spec/suma/postgres/model_spec.rb index 67fe613e2..ac140de67 100755 --- a/spec/suma/postgres/model_spec.rb +++ b/spec/suma/postgres/model_spec.rb @@ -22,6 +22,24 @@ module SumaTestModels; end expect(subclass.db).to eq(described_class.db) end + describe "reconfiguring/replacing the database" do + it "allows modification of non-DB configuration", db: false do + subclass = create_model(:conn_setter) + expect(subclass.db).to equal(described_class.db) + + described_class.reset_configuration(large_association_warning_threshold: 1) + + expect do + described_class.reset_configuration(pool_timeout: 1) + end.to raise_error(Suma::InvalidPrecondition) + end + + it "errors if not in test env" do + stub_const("Suma::RACK_ENV", "development") + expect { described_class.reset_configuration }.to raise_error(Suma::InvalidPrecondition) + end + end + it "registers a topological dependency for associations" do subclass = create_model(:allergies) other_class = create_model(:food_preferences) @@ -510,74 +528,6 @@ def inspect = "MyCls" end end - describe "each_cursor_page" do - names = ["a", "b", "c", "d"] - cls = Suma::Postgres::TestingPixie - let(:ds) { cls.dataset } - - before(:each) do - names.each { |n| cls.create(name: n) } - end - - it "chunks pages and calls each item in the block" do - result = [] - cls.each_cursor_page(page_size: 2) { |r| result << r.name } - expect(result).to eq(names) - end - - it "can order by a column" do - result = [] - cls.each_cursor_page(page_size: 2, order: Sequel.desc(:name)) { |r| result << r.name } - expect(result).to eq(names.reverse) - end - - it "can order by multiple columns" do - result = [] - cls.each_cursor_page(page_size: 2, order: [Sequel.desc(:name), :id]) { |r| result << r.name } - expect(result).to eq(names.reverse) - end - - it "can yield the full page rather than a row" do - result = [] - cls.each_cursor_page(page_size: 3, yield_page: true) { |page| page.map(&:name).each { |n| result << n } } - expect(result).to eq(names) - end - - it "can perform an action on the returned values of each chunk" do - clean_ds = ds.exclude(Sequel.like(:name, "%prime")) # Avoid re-selecting the stuff we just inserted - clean_ds.each_cursor_page_action(page_size: 3, action: ds.method(:multi_insert)) do |tp| - {name: tp.name + "prime"} - end - expect(ds.order(:id).all.map(&:name)).to eq( - ["a", "b", "c", "d", "aprime", "bprime", "cprime", "dprime"], - ) - end - - it "can handle multiple return rows" do - action_calls = 0 - action = lambda { |v| - action_calls += 1 - ds.multi_insert(v) - } - cls.each_cursor_page_action(page_size: 3, action:) do |tp| - tp.name == "a" ? Array.new(10) { |i| {name: "a#{i}"} } : nil - end - expect(ds.order(:id).all.map(&:name)).to eq( - ["a", "b", "c", "d", "a0", "a1", "a2", "a3", "a4", "a5", "a6", "a7", "a8", "a9"], - ) - expect(action_calls).to eq(2) - end - - it "ignores nil results returned from the block" do - cls.each_cursor_page_action(page_size: 1, action: ds.method(:multi_insert)) do |tp| - tp.name >= "c" ? nil : {name: tp.name + "prime"} - end - expect(ds.order(:id).all.map(&:name)).to eq( - ["a", "b", "c", "d", "aprime", "bprime"], - ) - end - end - describe "one_to_many" do Suma::Postgres::Model.descendants.reject(&:anonymous?).each do |host_class| describe host_class.name do @@ -621,4 +571,16 @@ def inspect = "MyCls" end end end + + describe "large association plugin", reset_configuration: described_class, db: false do + it "warns to sentry" do + described_class.reset_configuration(large_association_warning_threshold: 3) + expect(Sentry).to receive(:capture_message).with("Large association loaded") + described_class.db.transaction(rollback: :always) do + vendor = Suma::Fixtures.vendor.create + Array.new(4) { Suma::Fixtures.vendor_service(vendor:).create } + vendor.refresh.services + end + end + end end diff --git a/spec/suma/support/note_spec.rb b/spec/suma/support/note_spec.rb index 47eb75161..fb24dfcd3 100644 --- a/spec/suma/support/note_spec.rb +++ b/spec/suma/support/note_spec.rb @@ -18,6 +18,63 @@ expect(ver.notes).to have_same_ids_as(note) end + describe "combined_notes association" do + it "combines and sorts verification and member notes" do + content = "hi" + v = Suma::Fixtures.organization_membership_verification.create + m = v.membership.member + vn1 = v.add_note(content:, edited_at: 4.hours.ago) + vn2 = v.add_note(content:, created_at: 2.hours.ago) + vn3 = v.add_note(content:, created_at: 3.hours.ago) + mn1 = m.add_note(content:, created_at: 5.hours.ago) + mn2 = m.add_note(content:, edited_at: 1.hours.ago) + + other_vn = Suma::Fixtures.organization_membership_verification.create.add_note(content:) + + expect(v.combined_notes).to have_same_ids_as(mn2, vn2, vn3, vn1, mn1).ordered + eagered_v = refetch_for_eager(v) + expect(eagered_v.combined_notes).to have_same_ids_as(mn2, vn2, vn3, vn1, mn1).ordered + + expect(m.combined_notes).to have_same_ids_as(mn2, vn2, vn3, vn1, mn1).ordered + eagered_m = refetch_for_eager(m) + expect(eagered_m.combined_notes).to have_same_ids_as(mn2, vn2, vn3, vn1, mn1).ordered + end + + it "handles notes on members shared between eager loaded instances" do + m1 = Suma::Fixtures.member.create + m2 = Suma::Fixtures.member.create + m1v1 = Suma::Fixtures.organization_membership_verification.member(m1).create + m1v2 = Suma::Fixtures.organization_membership_verification.member(m1).create + m2v1 = Suma::Fixtures.organization_membership_verification.member(m2).create + m1note = m1.add_note(content: "m1note") + m2note = m2.add_note(content: "m2note") + m1v1note = m1v1.add_note(content: "m1v1note") + m1v2note = m1v2.add_note(content: "m1v2note") + m2v1note = m2v1.add_note(content: "m2v1note") + + expect(m1v1.combined_notes).to have_same_ids_as(m1note, m1v1note) + expect(m1v2.combined_notes).to have_same_ids_as(m1note, m1v2note) + expect(m1.combined_notes).to have_same_ids_as(m1note, m1v1note, m1v2note) + expect(m2v1.combined_notes).to have_same_ids_as(m2note, m2v1note) + expect(m2.combined_notes).to have_same_ids_as(m2note, m2v1note) + + eagered_verifications = Suma::Organization::Membership::Verification.dataset.all + m1v1 = eagered_verifications.find { |v| v === m1v1 } + m1v2 = eagered_verifications.find { |v| v === m1v2 } + m2v1 = eagered_verifications.find { |v| v === m2v1 } + + eagered_members = Suma::Member.all + m1 = eagered_members.find { |m| m === m1 } + m2 = eagered_members.find { |m| m === m2 } + + expect(m1v1.combined_notes).to have_same_ids_as(m1note, m1v1note) + expect(m1v2.combined_notes).to have_same_ids_as(m1note, m1v2note) + expect(m1.combined_notes).to have_same_ids_as(m1note, m1v1note, m1v2note) + expect(m2v1.combined_notes).to have_same_ids_as(m2note, m2v1note) + expect(m2.combined_notes).to have_same_ids_as(m2note, m2v1note) + end + end + describe "rendering" do it "renders markdown to html" do m = Suma::Fixtures.member.create