Merge pull request #1360 from alphagov/fix-engine

Refactor handling of non-managed resources

Dockerfile

@@ -15,7 +15,6 @@ RUN bootsnap precompile --gemfile .
 # current build stage.
 ENV SECRET_KEY_BASE=none
 ENV GOVUK_NOTIFY_API_KEY=none
-ENV DISCOVERY_ENGINE_DATASTORE=none
 ENV DISCOVERY_ENGINE_DEFAULT_COLLECTION_NAME=none
 RUN rails assets:precompile && rm -fr log
 

app/clients/discovery_engine/control_client.rb

@@ -5,8 +5,8 @@ class ControlClient
     def create(control)
       discovery_engine_client.create_control(
         control: control.to_discovery_engine_control,
-        control_id: control.discovery_engine_id,
-        parent: control.parent,
+        control_id: control.remote_resource_id,
+        parent: control.parent.name,
       )
     rescue Google::Cloud::Error => e
       set_record_errors(control, e)

app/models/concerns/discovery_engine_nameable.rb

@@ -0,0 +1,31 @@
+# Enhances models with a `#name` method returning their fully qualified Google Cloud Platform
+# resource name (like a path).
+#
+# For example, for a `Control`, this would be:
+#   projects/{project}/locations/{location}/collections/{collection_id}/engines/
+#   {engine_id}/controls/{control_id}
+#
+# Requires the including class to implement `#remote_resource_id`, and optionally `#parent` if the
+# parent resource is not the default collection.
+module DiscoveryEngineNameable
+  # The name (fully qualified path) of this Discovery Engine resource on GCP
+  def name
+    [parent_name, resource_path_fragment, remote_resource_id].join("/")
+  end
+
+private
+
+  def resource_path_fragment
+    # For example: `DataStore` -> `dataStores`
+    self.class.name.downcase_first.pluralize
+  end
+
+  def parent_name
+    if respond_to?(:parent)
+      parent.name
+    else
+      # The default collection is the parent of all root-level resources
+      Rails.configuration.discovery_engine_default_collection_name
+    end
+  end
+end

app/models/control.rb

@@ -10,6 +10,7 @@
 # see
 # https://cloud.google.com/ruby/docs/reference/google-cloud-discovery_engine-v1/latest/Google-Cloud-DiscoveryEngine-V1-Control
 class Control < ApplicationRecord
+  include DiscoveryEngineNameable
   include RemoteSynchronizable
   remote_synchronize with: DiscoveryEngine::ControlClient
 
@@ -30,18 +31,13 @@ def to_discovery_engine_control
     }
   end
 
-  # The fully qualified name of the control on Discovery Engine (like a path)
-  def name
-    [parent, "controls", discovery_engine_id].join("/")
-  end
-
-  # The parent of the control on Discovery Engine (always the engine)
+  # The parent of the control on Discovery Engine (always the default engine)
   def parent
-    Rails.configuration.discovery_engine_engine
+    Engine.default
   end
 
   # The ID of the resource on Discovery Engine
-  def discovery_engine_id
+  def remote_resource_id
     "search-admin-#{id}"
   end
 end

app/models/control/boost_action.rb

@@ -23,7 +23,7 @@ def to_discovery_engine_control_action
       boost_action: {
         filter: filter_expression,
         boost: boost_factor,
-        data_store: Rails.configuration.discovery_engine_datastore,
+        data_store: DataStore.default.name,
       },
     }
   end

app/models/control/filter_action.rb

@@ -16,7 +16,7 @@ def to_discovery_engine_control_action
     {
       filter_action: {
         filter: filter_expression,
-        data_store: Rails.configuration.discovery_engine_datastore,
+        data_store: DataStore.default.name,
       },
     }
   end

app/models/data_store.rb

@@ -0,0 +1,28 @@
+# Represents a data store on Discovery Engine.
+#
+# A data store contains the indexed documents that can be searched through an engine.
+#
+# Our architecture currently only has a single data store, so we do not need the ability to manage
+# data stores through Search Admin.
+#
+# see https://cloud.google.com/ruby/docs/reference/google-cloud-discovery_engine-v1/latest/Google-Cloud-DiscoveryEngine-V1-DataStore
+class DataStore
+  include DiscoveryEngineNameable
+
+  # The ID of the default datastore created through Terraform in `govuk_infrastructure`
+  DEFAULT_DATA_STORE_ID = "govuk_content".freeze
+
+  attr_reader :remote_resource_id
+
+  def self.default
+    new(DEFAULT_DATA_STORE_ID)
+  end
+
+  def initialize(remote_resource_id)
+    @remote_resource_id = remote_resource_id
+  end
+
+  def ==(other)
+    remote_resource_id == other.remote_resource_id
+  end
+end

app/models/engine.rb

@@ -0,0 +1,30 @@
+# Represents an engine on Discovery Engine.
+#
+# An engine (called "app" in the Google Cloud UI) is an abstraction over the data stores that
+# contain our searchable documents, and is used for querying. It is the parent resource of several
+# other resources such as controls and serving configs.
+#
+# Our architecture currently only has a single engine, so we do not need the ability to manage
+# engines through Search Admin.
+#
+# see https://cloud.google.com/ruby/docs/reference/google-cloud-discovery_engine-v1/latest/Google-Cloud-DiscoveryEngine-V1-Engine
+class Engine
+  include DiscoveryEngineNameable
+
+  # The ID of the default engine created through Terraform in `govuk-infrastructure`
+  DEFAULT_ENGINE_ID = "govuk".freeze
+
+  attr_reader :remote_resource_id
+
+  def self.default
+    new(DEFAULT_ENGINE_ID)
+  end
+
+  def initialize(remote_resource_id)
+    @remote_resource_id = remote_resource_id
+  end
+
+  def ==(other)
+    remote_resource_id == other.remote_resource_id
+  end
+end

app/views/controls/show.html.erb

@@ -39,6 +39,10 @@
       field: t_model_attr(:display_name),
       value: @control.display_name,
     },
+    {
+      field: t_model_attr(:name),
+      value: tag.code(@control.name),
+    }
   ]
 } %>
 

config/application.rb

@@ -47,7 +47,6 @@ class Application < Rails::Application
     config.active_record.default_column_serializer = YAML
 
     # Google Discovery Engine configuration
-    config.discovery_engine_datastore = ENV.fetch("DISCOVERY_ENGINE_DATASTORE")
     config.discovery_engine_default_collection_name = ENV.fetch("DISCOVERY_ENGINE_DEFAULT_COLLECTION_NAME")
   end
 end

config/environments/test.rb

@@ -59,7 +59,5 @@
   config.action_controller.raise_on_missing_callback_actions = true
 
   # Google Discovery Engine configuration
-  config.discovery_engine_datastore = "[datastore]"
-  config.discovery_engine_engine = "[engine]"
-  config.discovery_engine_serving_config = "[serving_config]"
+  config.discovery_engine_default_collection_name = "[collection]"
 end

config/locales/en.yml

@@ -78,6 +78,7 @@ en:
       control:
         display_name: Name
         action: Action
+        name: Google Cloud identifier
       control/boost_action:
         boost_factor: Boost factor
         filter_expression: Filter expression

spec/clients/discovery_engine/control_client_spec.rb

@@ -20,7 +20,7 @@
       expect(discovery_engine_client).to receive(:create_control).with(
         control: control.to_discovery_engine_control,
         control_id: "search-admin-42",
-        parent: "[engine]",
+        parent: Engine.default.name,
       )
 
       subject.create(control) # rubocop:disable Rails/SaveBang (not an ActiveRecord model)
@@ -80,7 +80,7 @@
   describe "#delete" do
     it "deletes the control on Discovery Engine" do
       expect(discovery_engine_client).to receive(:delete_control).with(
-        name: "[engine]/controls/search-admin-42",
+        name: control.name,
       )
 
       subject.delete(control)

spec/models/control/boost_action_spec.rb

@@ -59,7 +59,7 @@
         boost_action: {
           filter: "foo = 1",
           boost: 0.13,
-          data_store: "[datastore]",
+          data_store: DataStore.default.name,
         },
       })
     end

spec/models/control/filter_action_spec.rb

@@ -23,7 +23,7 @@
       expect(filter.to_discovery_engine_control_action).to eq({
         filter_action: {
           filter: "foo = 1",
-          data_store: "[datastore]",
+          data_store: DataStore.default.name,
         },
       })
     end

spec/models/control_spec.rb

@@ -34,28 +34,28 @@
 
     let(:action) { build(:control_boost_action) }
 
-    describe "#discovery_engine_id" do
+    describe "#remote_resource_id" do
       it "builds an ID from the control's database ID" do
-        expect(control.discovery_engine_id).to eq("search-admin-42")
+        expect(control.remote_resource_id).to eq("search-admin-42")
       end
     end
 
     describe "#parent" do
       it "is the configured engine" do
-        expect(control.parent).to eq("[engine]")
+        expect(control.parent).to eq(Engine.default)
       end
     end
 
     describe "#name" do
       it "returns the fully qualified name of the control" do
-        expect(control.name).to eq("[engine]/controls/search-admin-42")
+        expect(control.name).to eq("#{Engine.default.name}/controls/search-admin-42")
       end
     end
 
     describe "#to_discovery_engine_control" do
       it "returns a representation of the control for Discovery Engine" do
         expect(control.to_discovery_engine_control).to include(
-          name: "[engine]/controls/search-admin-42",
+          name: "#{Engine.default.name}/controls/search-admin-42",
           display_name: "My boost control",
           # We don't care what's in the action (that's tested elsewhere), but we do care that the
           # key is present

spec/models/data_store_spec.rb

@@ -0,0 +1,15 @@
+RSpec.describe DataStore do
+  subject(:data_store) { described_class.new("my-data-store") }
+
+  describe ".default" do
+    it "returns the default data store" do
+      expect(described_class.default).to eq(described_class.new("govuk_content"))
+    end
+  end
+
+  describe "#name" do
+    it "returns the fully qualified name of the data store" do
+      expect(subject.name).to eq("[collection]/dataStores/my-data-store")
+    end
+  end
+end

spec/models/engine_spec.rb

@@ -0,0 +1,15 @@
+RSpec.describe Engine do
+  subject(:engine) { described_class.new("my-engine") }
+
+  describe ".default" do
+    it "returns the default engine" do
+      expect(described_class.default).to eq(described_class.new("govuk"))
+    end
+  end
+
+  describe "#name" do
+    it "returns the fully qualified name of the engine" do
+      expect(subject.name).to eq("[collection]/engines/my-engine")
+    end
+  end
+end