refactor: remove old method for loading versions collection (#669)

* refactor: remove old method for loading versions collection

* chore: fix argument

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 `<number of items in the collection> * <number of associations to render>` 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

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<Symbol>]
         def self.eager_load_associations
           if is_collection_resource?

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 <Array>
+        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",

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

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

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]

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)

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")

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