From 075f5db22dd2f6eb4f8706f1af3858f91e72fdaf Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Wed, 20 Mar 2024 10:19:02 +1100 Subject: [PATCH] refactor: remove old method for loading versions collection (#669) * refactor: remove old method for loading versions collection * chore: fix argument --- ...n_pattern_for_eager_loading_collections.md | 23 +++++++++++++++++ .../api/decorators/base_decorator.rb | 11 +++++++- .../api/decorators/version_decorator.rb | 14 +++++++++++ .../api/resources/branch_versions.rb | 2 +- lib/pact_broker/api/resources/versions.rb | 2 +- lib/pact_broker/versions/repository.rb | 22 ++-------------- lib/pact_broker/versions/service.rb | 8 ++---- spec/features/get_branch_versions_spec.rb | 2 +- .../pact_broker/versions/repository_spec.rb | 25 ------------------- 9 files changed, 54 insertions(+), 55 deletions(-) create mode 100644 docs/developer/design_pattern_for_eager_loading_collections.md diff --git a/docs/developer/design_pattern_for_eager_loading_collections.md b/docs/developer/design_pattern_for_eager_loading_collections.md new file mode 100644 index 000000000..ebc759490 --- /dev/null +++ b/docs/developer/design_pattern_for_eager_loading_collections.md @@ -0,0 +1,23 @@ +# Design pattern for eager loading collections + +For collection resources (eg. `/versions` ), associations included in the items (eg. branch versions) must be eager loaded for performance reasons. + +The responsiblities of each class used to render a collection resource are as follows: + +* collection decorator (eg. `VersionsDecorator`) - delegate each item in the collection to be rendered by the decorator for the individual item, render pagination links +* item decorator (eg. `VersionDecorator`) - render the JSON for each item +* resource (eg. `PactBroker::Api::Resources::Versions`) - coordinate between, and delegate to, the service and the decorator +* service (eg. `PactBroker::Versions::Service`) - just delegate to repository, as there is no business logic required +* repository (eg. `PactBroker::Versions::Repository`) - load the domain objects from the database + +If the associations for a model are not eager loaded, then each individual association will be lazy loaded when the decorator for the item calls the association method to render it. This results in at least ` * ` calls to the database, and potentially more if any of the associations have their own associations that are required to render the item. This can cause significant performance issues. + +To efficiently render a collection resource, associations must be eager loaded when the collection items are loaded from the database in the repository. Since the repository method for loading the collection may be used in multiple places, and the eager loaded associations required for each of those places may be different (some may not require any associations to be eager loaded), we do not want to hard code the repository to load a fixed set of associations. The list of associations to eager load is therefore passed in to the repository finder method as an argument `eager_load_associations`. + +The decorator is the class that knows what associations are going to be called on the model to render the JSON, so following the design guideline of "put things together that change together", the best place for the declaration of "what associations should be eager loaded for this decorator" is in the decorator itself. The `PactBroker::Api::Decorators::BaseDecorator` has a default implementation of this method called `eager_load_associations` which attempts to automatically identify the required associations, but this can be overridden when necessary. + +We can therefore add the following responsiblities to our previous list: + +* item decorator - return a list of all the associations (including nested associations) that should be eager loaded in order to render its item +* repository - eager load the associations that have been passed into it +* resource - pass in the eager load associations to the repository from the decorator diff --git a/lib/pact_broker/api/decorators/base_decorator.rb b/lib/pact_broker/api/decorators/base_decorator.rb index 96bc5892c..b14524cc8 100644 --- a/lib/pact_broker/api/decorators/base_decorator.rb +++ b/lib/pact_broker/api/decorators/base_decorator.rb @@ -34,7 +34,16 @@ def self.property(name, options={}, &block) end end - # Returns the names of the model associations to eager load for use with this decorator + # Returns the names of the model associations to eager load for use with this decorator. + # The default implementation attempts to do an "auto detect" of the associations. + # For single item decorators, it attempts to identify the attributes that are themselves models. + # For collection decorators, it delegates to the eager_load_associations + # method of the single item decorator used to decorate the collection. + # + # The "auto detect" logic can only go so far. It cannot identify when a child object needs its own + # child object(s) to render the attribute. + # This method should be overridden when the "auto detect" logic cannot identify the correct associations + # to load. eg VersionDecorator # @return [Array] def self.eager_load_associations if is_collection_resource? diff --git a/lib/pact_broker/api/decorators/version_decorator.rb b/lib/pact_broker/api/decorators/version_decorator.rb index 875d95884..7b94dbc59 100644 --- a/lib/pact_broker/api/decorators/version_decorator.rb +++ b/lib/pact_broker/api/decorators/version_decorator.rb @@ -17,6 +17,20 @@ class VersionDecorator < BaseDecorator include Timestamps + # Returns the list of associations that must be eager loaded to efficiently render a version + # when this decorator is used in a collection (eg. VersionsDecorator) + # The associations that need to be eager loaded for the VersionDecorator + # are hand coded + # @return + def self.eager_load_associations + [ + :pacticipant, + :pact_publications, + { branch_versions: [:version, :branch_head, { branch: :pacticipant }] }, + { tags: :head_tag } + ] + end + link :self do | options | { title: "Version", diff --git a/lib/pact_broker/api/resources/branch_versions.rb b/lib/pact_broker/api/resources/branch_versions.rb index 6bb1b10ea..0a25d9d7a 100644 --- a/lib/pact_broker/api/resources/branch_versions.rb +++ b/lib/pact_broker/api/resources/branch_versions.rb @@ -31,7 +31,7 @@ def to_json end def versions - @versions ||= version_service.find_pacticipant_versions_in_reverse_order(pacticipant_name, { branch_name: identifier_from_path[:branch_name] }, pagination_options) + @versions ||= version_service.find_pacticipant_versions_in_reverse_order(pacticipant_name, { branch_name: identifier_from_path[:branch_name] }, pagination_options, decorator_class(:versions_decorator).eager_load_associations) end def policy_name diff --git a/lib/pact_broker/api/resources/versions.rb b/lib/pact_broker/api/resources/versions.rb index e03b2d76d..4e91ca18a 100644 --- a/lib/pact_broker/api/resources/versions.rb +++ b/lib/pact_broker/api/resources/versions.rb @@ -31,7 +31,7 @@ def to_json end def versions - @versions ||= version_service.find_all_pacticipant_versions_in_reverse_order(pacticipant_name, pagination_options) + @versions ||= version_service.find_pacticipant_versions_in_reverse_order(pacticipant_name, {}, pagination_options, decorator_class(:versions_decorator).eager_load_associations) end def policy_name diff --git a/lib/pact_broker/versions/repository.rb b/lib/pact_broker/versions/repository.rb index 28091d084..4ae4c25ed 100644 --- a/lib/pact_broker/versions/repository.rb +++ b/lib/pact_broker/versions/repository.rb @@ -57,29 +57,11 @@ def find_by_pacticipant_name_and_number pacticipant_name, number .single_record end - # The eager loaded relations are hardcoded here to support the PactBroker::Api::Decorators::VersionDecorator - # Newer "find all" implementations for other models pass the relations to eager load in - # from the decorator via the resource. - def find_all_pacticipant_versions_in_reverse_order name, pagination_options = {} - pacticipant = pacticipant_repository.find_by_name!(name) - query = PactBroker::Domain::Version - .where(pacticipant: pacticipant) - .eager(:pacticipant) - .eager(branch_versions: [:version, :branch_head, { branch: :pacticipant }]) - .eager(tags: :head_tag) - .eager(:pact_publications) - .reverse_order(:order) - query.all_with_pagination_options(pagination_options) - end - - def find_pacticipant_versions_in_reverse_order(pacticipant_name, options = {}, pagination_options = {}) + def find_pacticipant_versions_in_reverse_order(pacticipant_name, options = {}, pagination_options = {}, eager_load_associations = []) pacticipant = pacticipant_repository.find_by_name!(pacticipant_name) query = PactBroker::Domain::Version .where(pacticipant: pacticipant) - .eager(:pacticipant) - .eager(branch_versions: [:version, :branch_head, { branch: :pacticipant }]) - .eager(tags: :head_tag) - .eager(:pact_publications) + .eager(*eager_load_associations) .reverse_order(:order) if options[:branch_name] diff --git a/lib/pact_broker/versions/service.rb b/lib/pact_broker/versions/service.rb index 439133509..2f5d876c2 100644 --- a/lib/pact_broker/versions/service.rb +++ b/lib/pact_broker/versions/service.rb @@ -26,12 +26,8 @@ def self.find_latest_by_pacticipant_name_and_branch_name(pacticipant_name, branc version_repository.find_latest_by_pacticipant_name_and_branch_name(pacticipant_name, branch_name) end - def self.find_all_pacticipant_versions_in_reverse_order(name, pagination_options = {}) - version_repository.find_all_pacticipant_versions_in_reverse_order(name, pagination_options) - end - - def self.find_pacticipant_versions_in_reverse_order(pacticipant_name, options, pagination_options = {}) - version_repository.find_pacticipant_versions_in_reverse_order(pacticipant_name, options, pagination_options) + def self.find_pacticipant_versions_in_reverse_order(pacticipant_name, options, pagination_options = {}, eager_load_associations = []) + version_repository.find_pacticipant_versions_in_reverse_order(pacticipant_name, options, pagination_options, eager_load_associations) end def self.create_or_overwrite(pacticipant_name, version_number, version) diff --git a/spec/features/get_branch_versions_spec.rb b/spec/features/get_branch_versions_spec.rb index 9884cab86..dadb3eb18 100644 --- a/spec/features/get_branch_versions_spec.rb +++ b/spec/features/get_branch_versions_spec.rb @@ -1,4 +1,4 @@ -describe "Get a branch version" do +describe "Get versions for branch" do before do td.create_consumer("Foo") .create_consumer_version("1", branch: "main") diff --git a/spec/lib/pact_broker/versions/repository_spec.rb b/spec/lib/pact_broker/versions/repository_spec.rb index 356a664d6..3df5ecebb 100644 --- a/spec/lib/pact_broker/versions/repository_spec.rb +++ b/spec/lib/pact_broker/versions/repository_spec.rb @@ -24,31 +24,6 @@ module Versions end end - describe "#find_all_pacticipant_versions_in_reverse_order" do - before do - td - .create_consumer("Foo") - .create_consumer_version("1.2.3") - .create_consumer_version("4.5.6") - .create_consumer("Bar") - .create_consumer_version("8.9.0") - end - - subject { Repository.new.find_all_pacticipant_versions_in_reverse_order "Foo" } - - it "returns all the application versions for the given consumer" do - expect(subject.collect(&:number)).to eq ["4.5.6", "1.2.3"] - end - - context "with pagination options" do - subject { Repository.new.find_all_pacticipant_versions_in_reverse_order "Foo", page_number: 1, page_size: 1 } - - it "paginates the query" do - expect(subject.collect(&:number)).to eq ["4.5.6"] - end - end - end - describe "#find_pacticipant_versions_in_reverse_order" do before do td