v3(services): make offering fetcher like plan fetcher

We wrote the service offering fetcher before the service plan fetcher,
and learnt some lessons while doing it which we applied to the service
plan fetcher. This change applies those lessons to the service offering
fetcher, pulling common code into a new base class.

[#171435755](https://www.pivotaltracker.com/story/show/171435755)

Author: blgm

app/controllers/v3/service_offerings_controller.rb

@@ -21,14 +21,14 @@ def index
     invalid_param!(message.errors.full_messages) unless message.valid?
 
     dataset = if !current_user
-                ServiceOfferingListFetcher.fetch_public(message)
-              elsif permission_queryer.can_read_globally?
                 ServiceOfferingListFetcher.fetch(message)
+              elsif permission_queryer.can_read_globally?
+                ServiceOfferingListFetcher.fetch(message, omniscient: true)
               else
-                ServiceOfferingListFetcher.fetch_visible(
+                ServiceOfferingListFetcher.fetch(
                   message,
-                  permission_queryer.readable_org_guids,
-                  permission_queryer.readable_space_scoped_space_guids,
+                  readable_org_guids: permission_queryer.readable_org_guids,
+                  readable_space_guids: permission_queryer.readable_space_scoped_space_guids,
                 )
               end
 

app/fetchers/base_service_list_fetcher.rb

@@ -0,0 +1,79 @@
+require 'set'
+require 'fetchers/base_list_fetcher'
+require 'fetchers/label_selector_query_generator'
+
+module VCAP::CloudController
+  class BaseServiceListFetcher < BaseListFetcher
+    class << self
+      private
+
+      def select_readable(dataset, omniscient: false, readable_space_guids: [], readable_org_guids: [])
+        if readable_org_guids.any?
+          dataset = dataset.where do
+            (Sequel[:service_plans][:public] =~ true) |
+              (Sequel[:plan_orgs][:guid] =~ readable_org_guids) |
+              (Sequel[:broker_spaces][:guid] =~ readable_space_guids)
+          end
+        elsif !omniscient
+          dataset = dataset.where { Sequel[:service_plans][:public] =~ true }
+        end
+
+        dataset
+      end
+
+      def filter(message, dataset, klass)
+        if message.requested?(:service_broker_guids)
+          dataset = dataset.where { Sequel[:service_brokers][:guid] =~ message.service_broker_guids }
+        end
+
+        if message.requested?(:service_broker_names)
+          dataset = dataset.where { Sequel[:service_brokers][:name] =~ message.service_broker_names }
+        end
+
+        super(message, dataset, klass)
+      end
+
+      def filter_orgs(dataset, organization_guids)
+        dataset.where do
+          (Sequel[:service_plans][:public] =~ true) |
+            (Sequel[:plan_orgs][:guid] =~ organization_guids) |
+            (Sequel[:broker_orgs][:guid] =~ organization_guids)
+        end
+      end
+
+      def filter_spaces(dataset, filtered_space_guids:, readable_space_guids:, omniscient:)
+        space_guids = authorized_space_guids(
+          space_guids: filtered_space_guids,
+          readable_space_guids: readable_space_guids,
+          omniscient: omniscient,
+        )
+
+        dataset.where do
+          (Sequel[:service_plans][:public] =~ true) |
+            (Sequel[:plan_spaces][:guid] =~ space_guids) |
+            (Sequel[:broker_spaces][:guid] =~ space_guids)
+        end
+      end
+
+      def authorized_space_guids(space_guids: [], readable_space_guids: [], omniscient: false)
+        return space_guids if omniscient
+
+        (Set.new(readable_space_guids) & Set.new(space_guids)).to_a
+      end
+
+      def visibility_filter?(message)
+        [:space_guids, :organization_guids].any? { |filter| message.requested?(filter) }
+      end
+
+      def join_all_parent_tables(dataset)
+        dataset.
+          join(:service_brokers, id: Sequel[:services][:service_broker_id]).
+          left_join(Sequel[:spaces].as(:broker_spaces), id: Sequel[:service_brokers][:space_id]).
+          left_join(Sequel[:organizations].as(:broker_orgs), id: Sequel[:broker_spaces][:organization_id]).
+          left_join(:service_plan_visibilities, service_plan_id: Sequel[:service_plans][:id]).
+          left_join(Sequel[:organizations].as(:plan_orgs), id: Sequel[:service_plan_visibilities][:organization_id]).
+          left_join(Sequel[:spaces].as(:plan_spaces), organization_id: Sequel[:plan_orgs][:id])
+      end
+    end
+  end
+end

app/fetchers/service_offering_list_fetcher.rb

@@ -1,95 +1,64 @@
-require 'fetchers/base_list_fetcher'
-require 'fetchers/label_selector_query_generator'
+require 'fetchers/base_service_list_fetcher'
 
 module VCAP::CloudController
-  class ServiceOfferingListFetcher < BaseListFetcher
+  class ServiceOfferingListFetcher < BaseServiceListFetcher
     class << self
-      def fetch(message)
-        dataset = Service.dataset.
-                  join(:service_brokers, id: Sequel[:services][:service_broker_id]).
-                  left_join(:service_plans, service_id: Sequel[:services][:id]).
-                  left_join(:spaces, id: Sequel[:service_brokers][:space_id]).
-                  left_join(:organizations, id: Sequel[:spaces][:organization_id]).
-                  group(Sequel[:services][:id]).
-                  select_all(:services)
+      def fetch(message, omniscient: false, readable_space_guids: [], readable_org_guids: [])
+        dataset = Service.dataset
 
-        dataset = filter(message, dataset)
+        dataset = join_tables(dataset, message, omniscient)
 
-        return dataset unless message.requested?(:organization_guids)
+        dataset = select_readable(
+          dataset,
+          omniscient: omniscient,
+          readable_org_guids: readable_org_guids,
+          readable_space_guids: readable_space_guids,
+        )
 
-        dataset_with_visibilities(dataset, message)
-      end
+        if message.requested?(:space_guids)
+          dataset = filter_spaces(
+            dataset,
+            filtered_space_guids: message.space_guids,
+            readable_space_guids: readable_space_guids,
+            omniscient: omniscient,
+          )
+        end
 
-      def fetch_public(message)
-        dataset = Service.dataset.
-                  join(:service_plans, service_id: Sequel[:services][:id]).
-                  join(:service_brokers, id: Sequel[:services][:service_broker_id]).
-                  left_join(:spaces, id: Sequel[:service_brokers][:space_id]).
-                  left_join(:organizations, id: Sequel[:spaces][:organization_id]).
-                  where { Sequel[:service_plans][:public] =~ true }.
-                  group(Sequel[:services][:id]).
-                  select_all(:services)
-
-        filter(message, dataset)
-      end
+        dataset = filter_orgs(dataset, message.organization_guids) if message.requested?(:organization_guids)
 
-      def fetch_visible(message, org_guids, space_guids)
-        dataset = Service.dataset.
-                  left_join(:service_plans, service_id: Sequel[:services][:id]).
-                  join(:service_brokers, id: Sequel[:services][:service_broker_id]).
-                  left_join(:spaces, id: Sequel[:service_brokers][:space_id]).
-                  left_join(:service_plan_visibilities, service_plan_id: Sequel[:service_plans][:id]).
-                  left_join(:organizations, id: Sequel[:service_plan_visibilities][:organization_id]).
-                  where do
-          (Sequel[:organizations][:guid] =~ org_guids) |
-            (Sequel[:service_plans][:public] =~ true) |
-            (Sequel[:spaces][:guid] =~ space_guids)
-        end.
-                  group(Sequel[:services][:id]).
-                  select_all(:services)
-
-        filter(message, dataset)
-      end
+        dataset = filter(message, dataset)
 
-      private
+        dataset.
+          select_all(:services).
+          distinct
+      end
 
-      def dataset_with_visibilities(dataset, message)
-        dataset_with_visibilities = Service.dataset.
-                                    join(:service_brokers, id: Sequel[:services][:service_broker_id]).
-                                    join(:service_plans, service_id: Sequel[:services][:id]).
-                                    left_join(:spaces, id: Sequel[:service_brokers][:space_id]).
-                                    join(:service_plan_visibilities, service_plan_id: Sequel[:service_plans][:id]).
-                                    join(:organizations, id: Sequel[:service_plan_visibilities][:organization_id]).
-                                    select_all(:services)
+      def join_tables(dataset, message, omniscient)
+        need_all_parent_tables = !omniscient || visibility_filter?(message)
 
-        dataset_with_visibilities = filter(message, dataset_with_visibilities)
+        filter_properties = [
+          :service_broker_guids,
+          :service_broker_names,
+        ]
 
-        dataset.union(dataset_with_visibilities, alias: :services)
-      end
+        need_broker_tables = filter_properties.any? { |filter| message.requested?(filter) }
 
-      def filter(message, dataset)
-        if message.requested?(:available)
-          dataset = dataset.where(Sequel[:services][:active] =~ string_to_boolean(message.available))
+        if need_all_parent_tables
+          dataset = join_all_parent_tables(dataset.left_join(:service_plans, service_id: Sequel[:services][:id]))
+        elsif need_broker_tables
+          dataset = dataset.join(:service_brokers, id: Sequel[:services][:service_broker_id])
         end
 
-        if message.requested?(:service_broker_guids)
-          dataset = dataset.where(Sequel[:service_brokers][:guid] =~ message.service_broker_guids)
-        end
-
-        if message.requested?(:service_broker_names)
-          dataset = dataset.where(Sequel[:service_brokers][:name] =~ message.service_broker_names)
-        end
+        dataset
+      end
 
+      def filter(message, dataset)
         if message.requested?(:names)
           dataset = dataset.where(Sequel[:services][:label] =~ message.names)
         end
 
-        if message.requested?(:space_guids)
-          dataset = dataset.where((Sequel[:spaces][:guid] =~ message.space_guids) | (Sequel[:service_plans][:public] =~ true))
-        end
-
-        if message.requested?(:organization_guids)
-          dataset = dataset.where((Sequel[:organizations][:guid] =~ message.organization_guids) | (Sequel[:service_plans][:public] =~ true))
+        if message.requested?(:available)
+          dataset = dataset.where { Sequel[:services][:active] =~ message.available? }
         end
 
         if message.requested?(:label_selector)
@@ -103,10 +72,6 @@ def filter(message, dataset)
 
         super(message, dataset, Service)
       end
-
-      def string_to_boolean(value)
-        value == 'true'
-      end
     end
   end
 end

app/fetchers/service_plan_list_fetcher.rb

@@ -1,16 +1,14 @@
-require 'set'
-require 'fetchers/base_list_fetcher'
-require 'fetchers/label_selector_query_generator'
+require 'fetchers/base_service_list_fetcher'
 
 module VCAP::CloudController
-  class ServicePlanListFetcher < BaseListFetcher
+  class ServicePlanListFetcher < BaseServiceListFetcher
     class << self
       def fetch(message, omniscient: false, readable_space_guids: [], readable_org_guids: [])
         dataset = ServicePlan.dataset
 
         dataset = join_tables(dataset, message, omniscient)
 
-        dataset = select_readable_plans(
+        dataset = select_readable(
           dataset,
           omniscient: omniscient,
           readable_org_guids: readable_org_guids,
@@ -28,7 +26,7 @@ def fetch(message, omniscient: false, readable_space_guids: [], readable_org_gui
 
         dataset = filter_orgs(dataset, message.organization_guids) if message.requested?(:organization_guids)
 
-        dataset = filter(dataset, message)
+        dataset = filter(message, dataset)
 
         dataset.
           select_all(:service_plans).
@@ -38,7 +36,7 @@ def fetch(message, omniscient: false, readable_space_guids: [], readable_org_gui
       private
 
       def join_tables(dataset, message, omniscient)
-        need_all_parent_tables = !omniscient || [:space_guids, :organization_guids].any? { |filter| message.requested?(filter) }
+        need_all_parent_tables = !omniscient || visibility_filter?(message)
         filter_properties = [
           :service_broker_guids,
           :service_broker_names,
@@ -50,15 +48,7 @@ def join_tables(dataset, message, omniscient)
         need_broker_and_offering_tables = filter_properties.any? { |filter| message.requested?(filter) }
 
         if need_all_parent_tables
-          dataset = dataset.
-                    join(:services, id: Sequel[:service_plans][:service_id]).
-                    join(:service_brokers, id: Sequel[:services][:service_broker_id]).
-                    left_join(Sequel[:spaces].as(:broker_spaces), id: Sequel[:service_brokers][:space_id]).
-                    left_join(Sequel[:organizations].as(:broker_orgs), id: Sequel[:broker_spaces][:organization_id]).
-                    left_join(:service_plan_visibilities, service_plan_id: Sequel[:service_plans][:id]).
-                    left_join(Sequel[:organizations].as(:plan_orgs), id: Sequel[:service_plan_visibilities][:organization_id]).
-                    left_join(Sequel[:spaces].as(:plan_spaces), organization_id: Sequel[:plan_orgs][:id])
-
+          dataset = join_all_parent_tables(dataset.join(:services, id: Sequel[:service_plans][:service_id]))
         elsif need_broker_and_offering_tables
           dataset = dataset.
                     join(:services, id: Sequel[:service_plans][:service_id]).
@@ -72,57 +62,13 @@ def join_tables(dataset, message, omniscient)
         dataset
       end
 
-      def select_readable_plans(dataset, omniscient: false, readable_space_guids: [], readable_org_guids: [])
-        if readable_org_guids.any?
-          dataset = dataset.where do
-            (Sequel[:service_plans][:public] =~ true) |
-              (Sequel[:plan_orgs][:guid] =~ readable_org_guids) |
-              (Sequel[:broker_spaces][:guid] =~ readable_space_guids)
-          end
-        elsif !omniscient
-          dataset = dataset.where { Sequel[:service_plans][:public] =~ true }
-        end
-
-        dataset
-      end
-
-      def filter_orgs(dataset, organization_guids)
-        dataset.where do
-          (Sequel[:service_plans][:public] =~ true) |
-            (Sequel[:plan_orgs][:guid] =~ organization_guids) |
-            (Sequel[:broker_orgs][:guid] =~ organization_guids)
-        end
-      end
-
-      def filter_spaces(dataset, filtered_space_guids:, readable_space_guids:, omniscient:)
-        space_guids = authorized_space_guids(
-          space_guids: filtered_space_guids,
-          readable_space_guids: readable_space_guids,
-          omniscient: omniscient,
-        )
-
-        dataset.where do
-          (Sequel[:service_plans][:public] =~ true) |
-            (Sequel[:plan_spaces][:guid] =~ space_guids) |
-            (Sequel[:broker_spaces][:guid] =~ space_guids)
-        end
-      end
-
-      def filter(dataset, message)
-        if message.requested?(:names)
-          dataset = dataset.where { Sequel[:service_plans][:name] =~ message.names }
-        end
-
+      def filter(message, dataset)
         if message.requested?(:available)
           dataset = dataset.where { Sequel[:service_plans][:active] =~ message.available? }
         end
 
-        if message.requested?(:service_broker_guids)
-          dataset = dataset.where { Sequel[:service_brokers][:guid] =~ message.service_broker_guids }
-        end
-
-        if message.requested?(:service_broker_names)
-          dataset = dataset.where { Sequel[:service_brokers][:name] =~ message.service_broker_names }
+        if message.requested?(:names)
+          dataset = dataset.where { Sequel[:service_plans][:name] =~ message.names }
         end
 
         if message.requested?(:service_offering_guids)
@@ -152,12 +98,6 @@ def filter(dataset, message)
 
         super(message, dataset, ServicePlan)
       end
-
-      def authorized_space_guids(space_guids: [], readable_space_guids: [], omniscient: false)
-        return space_guids if omniscient
-
-        (Set.new(readable_space_guids) & Set.new(space_guids)).to_a
-      end
     end
   end
 end

app/messages/service_offerings_list_message.rb

@@ -46,5 +46,9 @@ def pagination_options
         end
       end
     end
+
+    def available?
+      requested?(:available) && available == 'true'
+    end
   end
 end