Skip to content

Commit

Permalink
Controls: Add RemoteSynchronizable concern
Browse files Browse the repository at this point in the history
This adds a `RemoteSynchronizable` model concern used by `Control` (and
other models in the future) to persist themselves to DiscoveryEngine via
the `DiscoveryEngine::Control` client.

This concern uses ActiveRecord lifecycle callbacks to create/update/
delete a corresponding Control resource in Discovery Engine whenever its
local version changes.

Normally we would avoid using ActiveRecord callbacks to make network
calls to remote APIs, but as the core purpose of Search Admin is to
provide an interface to manage these resources on their various remote
APIs, this is part of its core domain.
  • Loading branch information
csutter committed Feb 3, 2025
1 parent 5686e3b commit 455af93
Show file tree
Hide file tree
Showing 8 changed files with 190 additions and 1 deletion.
1 change: 1 addition & 0 deletions app/clients/discovery_engine/control_client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ module DiscoveryEngine
class ControlClient
# Creates a corresponding resource for this control on Discovery Engine.
def create(control)
raise Google::Cloud::Error, "OOPS"
discovery_engine_client.create_control(

Check failure on line 7 in app/clients/discovery_engine/control_client.rb

View workflow job for this annotation

GitHub Actions / Lint Ruby / Run RuboCop

Lint/UnreachableCode: Unreachable code detected.
control: control.to_discovery_engine_control,
control_id: control.discovery_engine_id,
Expand Down
68 changes: 68 additions & 0 deletions app/models/concerns/remote_synchronizable.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
# Enhances a model with lifecycle callbacks to synchronise it with a remote resource using a client
# class (conventionally located in `app/clients/`).
#
# Example:
# ```ruby
# class Foo < ApplicationRecord
# include RemoteSynchronizable
# remote_synchronize with: BarApi::FooClient
# end
# ```
#
# If the remote operation fails, the record will not be created/updated/destroyed, and will be
# marked invalid.
module RemoteSynchronizable
extend ActiveSupport::Concern

included do
# Client class to use for synchronisation
class_attribute :client_class

# Create and update the remote resource using the client during ActiveRecord lifecycle events.
#
# Normally we would avoid using ActiveRecord callbacks to make network calls, but as the core
# purpose of Search Admin is to provide an interface to manage resources on various remote APIs,
# this is part of its core domain.
before_create :create_remote, unless: :skip_remote_synchronization_on_create
before_update :update_remote
before_destroy :destroy_remote

# Skips the creation of the remote synchronisation on create.
#
# This allows to create new instances of a record without a remote counterpart, for example
# when importing existing remote resources, or as part of test setup (see `spec/factories.rb`).
attr_accessor :skip_remote_synchronization_on_create
end

class_methods do
# Set the class to be used for synchronisation for this model. It must allow initialisation with
# a record, and respond to `#create`, `#update` and `#delete`.
def remote_synchronize(with:)
self.client_class = with
end
end

private

def create_remote
client.create(self) # rubocop:disable Rails/SaveBang (not an ActiveRecord model)
rescue ClientError
raise ActiveRecord::RecordInvalid, self
end

def update_remote
client.update(self) # rubocop:disable Rails/SaveBang (not an ActiveRecord model)
rescue ClientError
raise ActiveRecord::RecordInvalid, self
end

def destroy_remote
client.delete(self)
rescue ClientError
throw :abort
end

def client
@client ||= client_class.new
end
end
3 changes: 3 additions & 0 deletions app/models/control.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@
# see
# https://cloud.google.com/ruby/docs/reference/google-cloud-discovery_engine-v1/latest/Google-Cloud-DiscoveryEngine-V1-Control
class Control < ApplicationRecord
include RemoteSynchronizable
remote_synchronize with: DiscoveryEngine::ControlClient

delegated_type :action, types: %w[BoostAction FilterAction], dependent: :destroy
accepts_nested_attributes_for :action

Expand Down
7 changes: 7 additions & 0 deletions config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,13 @@ en:
keywords: Keywords
comment: Comments

errors:
models:
control:
remote_error: |
We couldn't save this control on Vertex AI Search because of an unexpected error.
This error has been logged. Please try again later.
controls:
index:
page_title: Controls
Expand Down
19 changes: 18 additions & 1 deletion spec/factories.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,22 @@
FactoryBot.define do
factory :control do
# Unlike fixtures, record instances created through FactoryBot go through the full Rails callback
# lifecycle on creation. That is often desirable, but not in the case of our models using the
# `RemoteSynchronizable` concern to create counterparts on remote APIs automatically whenever you
# create a new record.
#
# This trait sets the `skip_remote_synchronization_on_create` flag to true on records created
# through factories that include it, so that this behaviour is skipped.
#
# If you _do_ want to specifically test remote synchronization on a record, you can override this
# manually when you build your model using `FactoryBot.build`:
# ```ruby
# build(:my_model, skip_remote_synchronization_on_create: false)
# ```
trait :remote_synchronizable do
skip_remote_synchronization_on_create { true }
end

factory :control, traits: %i[remote_synchronizable] do
display_name { "Control" }

action factory: :control_boost_action
Expand Down
2 changes: 2 additions & 0 deletions spec/models/control_spec.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
RSpec.describe Control, type: :model do
it_behaves_like "RemoteSynchronizable", DiscoveryEngine::ControlClient

describe "validations" do
subject(:control) { build(:control) }

Expand Down
83 changes: 83 additions & 0 deletions spec/support/shared_examples/concerns/remote_synchronizable.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
RSpec.shared_examples "RemoteSynchronizable" do |client_class|
let(:client) do
instance_double(client_class, create: true, update: true, delete: true)
end
let(:factory) { described_class.model_name.param_key }

before do
allow(client_class).to receive(:new).and_return(client)
end

describe "when creating a new record" do
subject(:record) { build(factory, skip_remote_synchronization_on_create: false) }

it "creates the remote resource using the client" do
record.save!

expect(client).to have_received(:create).with(record)
end

context "when the remote resource creation fails" do
let(:error) { ClientError.new("Uh oh") }

before do
allow(client).to receive(:create).and_raise(error)

record.save # rubocop:disable Rails/SaveBang (we're checking record state)
end

it "stops the record from being created" do
expect(record).not_to be_persisted
end
end
end

describe "when updating an existing record" do
subject(:record) { create(factory) }

it "updates the remote resource" do
record.save!

expect(client).to have_received(:update).with(record)
end

context "when the remote resource update fails" do
let(:error) { ClientError.new("Uh oh") }

before do
allow(client).to receive(:update).and_raise(error)

record.updated_at = Time.current # change something so we can check it won't save
record.save # rubocop:disable Rails/SaveBang (we're checking record state)
end

it "stops the record from being persisted" do
expect(record).to be_changed
end
end
end

describe "when destroying an existing record" do
subject(:record) { create(factory) }

it "deletes the remote resource" do
record.destroy!

expect(client).to have_received(:delete).with(record)
end

context "when the remote resource deletion fails with an internal error" do
let(:error) { ClientError.new("Uh oh") }

before do
allow(client).to receive(:delete).and_raise(error)

record.destroy # rubocop:disable Rails/SaveBang (we're checking record state)
end

it "stops the record from being destroyed" do
expect(described_class.exists?(record.id)).to be(true)
end
end
end
end
8 changes: 8 additions & 0 deletions spec/system/controls_spec.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,12 @@
RSpec.describe "Controls", type: :system do
let(:control) do
instance_double(DiscoveryEngine::ControlClient, create: true, update: true, delete: true)
end

before do
allow(DiscoveryEngine::ControlClient).to receive(:new).and_return(control)
end

scenario "Viewing controls" do
given_several_controls_exist

Expand Down

0 comments on commit 455af93

Please sign in to comment.