From 45ec626b16e24ceb3a3671803d89fa0af25c1393 Mon Sep 17 00:00:00 2001 From: Christian Sutter Date: Thu, 6 Feb 2025 11:53:39 +0000 Subject: [PATCH 1/6] Add `Engine` and `DataStore` models These are plain Ruby objects representing the concepts of engine and data store on Discovery Engine. Our architecture only has a single one of each, and that is unlikely to change in the medium term, so these are not persisted in the database but rather just have a single default instance available as `.default`. --- app/models/data_store.rb | 35 ++++++++++++++++++++++++++++++++ app/models/engine.rb | 37 ++++++++++++++++++++++++++++++++++ spec/models/data_store_spec.rb | 15 ++++++++++++++ spec/models/engine_spec.rb | 15 ++++++++++++++ 4 files changed, 102 insertions(+) create mode 100644 app/models/data_store.rb create mode 100644 app/models/engine.rb create mode 100644 spec/models/data_store_spec.rb create mode 100644 spec/models/engine_spec.rb diff --git a/app/models/data_store.rb b/app/models/data_store.rb new file mode 100644 index 00000000..fcf10282 --- /dev/null +++ b/app/models/data_store.rb @@ -0,0 +1,35 @@ +# 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 + # The ID of the default datastore created through Terraform in `govuk_infrastructure` + DEFAULT_DATA_STORE_ID = "govuk_content".freeze + + attr_reader :discovery_engine_id + + def self.default + new(DEFAULT_DATA_STORE_ID) + end + + def initialize(discovery_engine_id) + @discovery_engine_id = discovery_engine_id + end + + # The fully qualified name of the data store on Discovery Engine (like a path) + def name + [ + Rails.configuration.discovery_engine_default_collection_name, + "dataStores", + discovery_engine_id + ].join("/") + end + + def ==(other) + discovery_engine_id == other.discovery_engine_id + end +end diff --git a/app/models/engine.rb b/app/models/engine.rb new file mode 100644 index 00000000..dbbb06e1 --- /dev/null +++ b/app/models/engine.rb @@ -0,0 +1,37 @@ +# 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 + # The ID of the default engine created through Terraform in `govuk-infrastructure` + DEFAULT_ENGINE_ID = "govuk".freeze + + attr_reader :discovery_engine_id + + def self.default + new(DEFAULT_ENGINE_ID) + end + + def initialize(discovery_engine_id) + @discovery_engine_id = discovery_engine_id + end + + # The fully qualified name of the engine on Discovery Engine (like a path) + def name + [ + Rails.configuration.discovery_engine_default_collection_name, + "engines", + discovery_engine_id + ].join("/") + end + + def ==(other) + discovery_engine_id == other.discovery_engine_id + end +end diff --git a/spec/models/data_store_spec.rb b/spec/models/data_store_spec.rb new file mode 100644 index 00000000..237e2822 --- /dev/null +++ b/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 diff --git a/spec/models/engine_spec.rb b/spec/models/engine_spec.rb new file mode 100644 index 00000000..c7300760 --- /dev/null +++ b/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 From bd53ebf8d6a032050b3ce35f5af021fa38b51a40 Mon Sep 17 00:00:00 2001 From: Christian Sutter Date: Thu, 6 Feb 2025 12:03:45 +0000 Subject: [PATCH 2/6] Use new `Engine` and `DataStore` models in `Control` This changes `Control` and its actions to use the new models instead of getting their engine and data store names straight from the Rails config. --- app/clients/discovery_engine/control_client.rb | 2 +- app/models/control.rb | 6 +++--- app/models/control/boost_action.rb | 2 +- app/models/control/filter_action.rb | 2 +- spec/clients/discovery_engine/control_client_spec.rb | 4 ++-- spec/models/control/boost_action_spec.rb | 2 +- spec/models/control/filter_action_spec.rb | 2 +- spec/models/control_spec.rb | 6 +++--- 8 files changed, 13 insertions(+), 13 deletions(-) diff --git a/app/clients/discovery_engine/control_client.rb b/app/clients/discovery_engine/control_client.rb index 3568824b..56a1f741 100644 --- a/app/clients/discovery_engine/control_client.rb +++ b/app/clients/discovery_engine/control_client.rb @@ -6,7 +6,7 @@ def create(control) discovery_engine_client.create_control( control: control.to_discovery_engine_control, control_id: control.discovery_engine_id, - parent: control.parent, + parent: control.parent.name, ) rescue Google::Cloud::Error => e set_record_errors(control, e) diff --git a/app/models/control.rb b/app/models/control.rb index 37207c02..cc7e3b7d 100644 --- a/app/models/control.rb +++ b/app/models/control.rb @@ -32,12 +32,12 @@ def to_discovery_engine_control # The fully qualified name of the control on Discovery Engine (like a path) def name - [parent, "controls", discovery_engine_id].join("/") + [parent.name, "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 diff --git a/app/models/control/boost_action.rb b/app/models/control/boost_action.rb index 10ef2ada..4ae5b800 100644 --- a/app/models/control/boost_action.rb +++ b/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 diff --git a/app/models/control/filter_action.rb b/app/models/control/filter_action.rb index 4956f70d..8d2da335 100644 --- a/app/models/control/filter_action.rb +++ b/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 diff --git a/spec/clients/discovery_engine/control_client_spec.rb b/spec/clients/discovery_engine/control_client_spec.rb index 51e22b88..6126fa2f 100644 --- a/spec/clients/discovery_engine/control_client_spec.rb +++ b/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) diff --git a/spec/models/control/boost_action_spec.rb b/spec/models/control/boost_action_spec.rb index 18036efc..ec44f5f3 100644 --- a/spec/models/control/boost_action_spec.rb +++ b/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 diff --git a/spec/models/control/filter_action_spec.rb b/spec/models/control/filter_action_spec.rb index 941dd508..261bd846 100644 --- a/spec/models/control/filter_action_spec.rb +++ b/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 diff --git a/spec/models/control_spec.rb b/spec/models/control_spec.rb index 1632484b..68a5c0ca 100644 --- a/spec/models/control_spec.rb +++ b/spec/models/control_spec.rb @@ -42,20 +42,20 @@ 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 From 8ec3318a8103c7fe6dc6b5bbc9aa999175de2e88 Mon Sep 17 00:00:00 2001 From: Christian Sutter Date: Thu, 6 Feb 2025 12:04:53 +0000 Subject: [PATCH 3/6] Remove superfluous Discovery Engine configuration This removes the unnecessary configuration of a data store, and also removes the erroneous test environment configuration for engine that was left in as part of a previous PR. --- Dockerfile | 1 - config/application.rb | 1 - config/environments/test.rb | 4 +--- 3 files changed, 1 insertion(+), 5 deletions(-) diff --git a/Dockerfile b/Dockerfile index 249311c9..132c5afa 100644 --- a/Dockerfile +++ b/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 diff --git a/config/application.rb b/config/application.rb index 86c3aea1..19dfef91 100644 --- a/config/application.rb +++ b/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 diff --git a/config/environments/test.rb b/config/environments/test.rb index 6c68aa70..7c6a7739 100644 --- a/config/environments/test.rb +++ b/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 From 2884990b3efd87ae8f7af1180a90e28340c63764 Mon Sep 17 00:00:00 2001 From: Christian Sutter Date: Thu, 6 Feb 2025 12:22:04 +0000 Subject: [PATCH 4/6] Refactor common GCP naming into mixin This refactors the repetitive name generation into a `DiscoveryEngineNameable` mixin. --- .../concerns/discovery_engine_nameable.rb | 31 +++++++++++++++++++ app/models/control.rb | 6 +--- app/models/data_store.rb | 11 ++----- app/models/engine.rb | 11 ++----- 4 files changed, 36 insertions(+), 23 deletions(-) create mode 100644 app/models/concerns/discovery_engine_nameable.rb diff --git a/app/models/concerns/discovery_engine_nameable.rb b/app/models/concerns/discovery_engine_nameable.rb new file mode 100644 index 00000000..f583e174 --- /dev/null +++ b/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 `#discovery_engine_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, discovery_engine_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 diff --git a/app/models/control.rb b/app/models/control.rb index cc7e3b7d..bbaed379 100644 --- a/app/models/control.rb +++ b/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,11 +31,6 @@ def to_discovery_engine_control } end - # The fully qualified name of the control on Discovery Engine (like a path) - def name - [parent.name, "controls", discovery_engine_id].join("/") - end - # The parent of the control on Discovery Engine (always the default engine) def parent Engine.default diff --git a/app/models/data_store.rb b/app/models/data_store.rb index fcf10282..8094abaf 100644 --- a/app/models/data_store.rb +++ b/app/models/data_store.rb @@ -7,6 +7,8 @@ # # 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 @@ -20,15 +22,6 @@ def initialize(discovery_engine_id) @discovery_engine_id = discovery_engine_id end - # The fully qualified name of the data store on Discovery Engine (like a path) - def name - [ - Rails.configuration.discovery_engine_default_collection_name, - "dataStores", - discovery_engine_id - ].join("/") - end - def ==(other) discovery_engine_id == other.discovery_engine_id end diff --git a/app/models/engine.rb b/app/models/engine.rb index dbbb06e1..a44d19ee 100644 --- a/app/models/engine.rb +++ b/app/models/engine.rb @@ -9,6 +9,8 @@ # # 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 @@ -22,15 +24,6 @@ def initialize(discovery_engine_id) @discovery_engine_id = discovery_engine_id end - # The fully qualified name of the engine on Discovery Engine (like a path) - def name - [ - Rails.configuration.discovery_engine_default_collection_name, - "engines", - discovery_engine_id - ].join("/") - end - def ==(other) discovery_engine_id == other.discovery_engine_id end From f9e19e7811f526536d076566de2f252e4f125cfe Mon Sep 17 00:00:00 2001 From: Christian Sutter Date: Thu, 6 Feb 2025 12:30:30 +0000 Subject: [PATCH 5/6] Add control's GCP name (path) to UI This could come in helpful for debugging. --- app/views/controls/show.html.erb | 4 ++++ config/locales/en.yml | 1 + 2 files changed, 5 insertions(+) diff --git a/app/views/controls/show.html.erb b/app/views/controls/show.html.erb index a85f90f3..146c558c 100644 --- a/app/views/controls/show.html.erb +++ b/app/views/controls/show.html.erb @@ -37,6 +37,10 @@ field: t_model_attr(:display_name), value: @control.display_name, }, + { + field: t_model_attr(:name), + value: tag.code(@control.name), + } ] } %> diff --git a/config/locales/en.yml b/config/locales/en.yml index d7539450..24265ba3 100644 --- a/config/locales/en.yml +++ b/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 From 5cd43b063656ba75a1a3f71f034d67e1442aa532 Mon Sep 17 00:00:00 2001 From: Christian Sutter Date: Thu, 6 Feb 2025 16:30:28 +0000 Subject: [PATCH 6/6] Rename `#discovery_engine_id` to `#remote_resource_id` This is a more sensible name that doesn't cause confusion around "engines". --- app/clients/discovery_engine/control_client.rb | 2 +- app/models/concerns/discovery_engine_nameable.rb | 4 ++-- app/models/control.rb | 2 +- app/models/data_store.rb | 8 ++++---- app/models/engine.rb | 8 ++++---- spec/models/control_spec.rb | 4 ++-- 6 files changed, 14 insertions(+), 14 deletions(-) diff --git a/app/clients/discovery_engine/control_client.rb b/app/clients/discovery_engine/control_client.rb index 56a1f741..d5df3556 100644 --- a/app/clients/discovery_engine/control_client.rb +++ b/app/clients/discovery_engine/control_client.rb @@ -5,7 +5,7 @@ class ControlClient def create(control) discovery_engine_client.create_control( control: control.to_discovery_engine_control, - control_id: control.discovery_engine_id, + control_id: control.remote_resource_id, parent: control.parent.name, ) rescue Google::Cloud::Error => e diff --git a/app/models/concerns/discovery_engine_nameable.rb b/app/models/concerns/discovery_engine_nameable.rb index f583e174..dcd9c3b6 100644 --- a/app/models/concerns/discovery_engine_nameable.rb +++ b/app/models/concerns/discovery_engine_nameable.rb @@ -5,12 +5,12 @@ # projects/{project}/locations/{location}/collections/{collection_id}/engines/ # {engine_id}/controls/{control_id} # -# Requires the including class to implement `#discovery_engine_id`, and optionally `#parent` if the +# 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, discovery_engine_id].join("/") + [parent_name, resource_path_fragment, remote_resource_id].join("/") end private diff --git a/app/models/control.rb b/app/models/control.rb index bbaed379..a443ed0b 100644 --- a/app/models/control.rb +++ b/app/models/control.rb @@ -37,7 +37,7 @@ def parent end # The ID of the resource on Discovery Engine - def discovery_engine_id + def remote_resource_id "search-admin-#{id}" end end diff --git a/app/models/data_store.rb b/app/models/data_store.rb index 8094abaf..45146058 100644 --- a/app/models/data_store.rb +++ b/app/models/data_store.rb @@ -12,17 +12,17 @@ class DataStore # The ID of the default datastore created through Terraform in `govuk_infrastructure` DEFAULT_DATA_STORE_ID = "govuk_content".freeze - attr_reader :discovery_engine_id + attr_reader :remote_resource_id def self.default new(DEFAULT_DATA_STORE_ID) end - def initialize(discovery_engine_id) - @discovery_engine_id = discovery_engine_id + def initialize(remote_resource_id) + @remote_resource_id = remote_resource_id end def ==(other) - discovery_engine_id == other.discovery_engine_id + remote_resource_id == other.remote_resource_id end end diff --git a/app/models/engine.rb b/app/models/engine.rb index a44d19ee..0cec4cc5 100644 --- a/app/models/engine.rb +++ b/app/models/engine.rb @@ -14,17 +14,17 @@ class Engine # The ID of the default engine created through Terraform in `govuk-infrastructure` DEFAULT_ENGINE_ID = "govuk".freeze - attr_reader :discovery_engine_id + attr_reader :remote_resource_id def self.default new(DEFAULT_ENGINE_ID) end - def initialize(discovery_engine_id) - @discovery_engine_id = discovery_engine_id + def initialize(remote_resource_id) + @remote_resource_id = remote_resource_id end def ==(other) - discovery_engine_id == other.discovery_engine_id + remote_resource_id == other.remote_resource_id end end diff --git a/spec/models/control_spec.rb b/spec/models/control_spec.rb index 68a5c0ca..b870c764 100644 --- a/spec/models/control_spec.rb +++ b/spec/models/control_spec.rb @@ -34,9 +34,9 @@ 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