Introduce Railway Oriented Programming (ROP) to RD Domain (Part 1)

- See ee/lib/remote_development/README.md for context
- Adds Railway Oriented Programming (ROP) to
  Remote Development Workspace Update module
- Add generic Result type (not domain specific)
  to support ROP

app/services/service_response.rb

@@ -56,6 +56,10 @@ def to_h
       reason: reason)
   end
 
+  def deconstruct_keys(keys)
+    to_h.slice(*keys)
+  end
+
   def success?
     status == :success
   end

ee/app/graphql/resolvers/remote_development/workspaces_resolver.rb

@@ -23,7 +23,6 @@ class WorkspacesResolver < ::Resolvers::BaseResolver
 
       def resolve(**args)
         unless ::Feature.enabled?(:remote_development_feature_flag)
-          # noinspection RubyMismatchedArgumentType
           raise_resource_not_available_error! "'remote_development_feature_flag' feature flag is disabled"
         end
 

ee/app/services/remote_development/agent_config/update_service.rb

@@ -3,6 +3,12 @@
 module RemoteDevelopment
   module AgentConfig
     class UpdateService
+      # NOTE: This constructor intentionally does not follow all of the conventions from
+      #       https://docs.gitlab.com/ee/development/reusing_abstractions.html#service-classes
+      #       suggesting that the dependencies be passed via the constructor.
+      #
+      #       See "Stateless Service layer classes" in ee/lib/remote_development/README.md for more details.
+
       # @param [Clusters::Agent] agent
       # @param [Hash] config
       # @return [Hash, FalseClass]

ee/app/services/remote_development/service_response_factory.rb

+# frozen_string_literal: true
+
+module RemoteDevelopment
+  module ServiceResponseFactory
+    # @param [Hash] response_hash
+    # @return [ServiceResponse]
+    def create_service_response(response_hash)
+      # NOTE: We are not using the ServiceResponse class directly in the Domain Logic layer, but instead we
+      # have the Domain Logic layer return a hash with the necessary entries to create a ServiceResponse object.
+      # This is because:
+      #
+      # 1. It makes the specs for the classes on the outer edge of the Domain Logic layer more concise
+      #    and straightforward if they can assert on plain hash return values rather than unpacking ServiceResponse
+      #    objects.
+      # 2. We can use this as a centralized place to do some type-checking of the values to be contained in
+      #    the ServiceResponse (this could be added to ServiceResponse in the future if we choose, but it is
+      #    currently dependent upon the experimental rightward assignment feature).
+      # 3. This would technically be a circular dependency, since the ServiceResponse class is part of the
+      #    Service layer, but the Service layer calls the Domain Logic layer.
+      #
+      # We may change this in the future as we evolve the abstractions around the Service layer,
+      # but for now we are keeping them strictly decoupled.
+      #
+      # See ee/lib/remote_development/README.md for more context.
+
+      validate_response_hash(response_hash)
+      ServiceResponse.new(**response_hash)
+    end
+
+    private
+
+    # @param [Hash] response_hash
+    # @return [void]
+    # @raise [RuntimeError]
+    def validate_response_hash(response_hash)
+      # Explicitly assign nil to all valid values, so we can type-check the values using rightward assignment,
+      #    which requires that nil values must be explicitly set.
+      hash = { status: nil, payload: nil, message: nil, reason: nil }.merge(response_hash)
+
+      # Type-check response using rightward assignment
+      hash => {
+        status: Symbol => status,
+        payload: (Hash | NilClass) => payload,
+        message: (String | NilClass) => message,
+        reason: (Symbol | NilClass)=> reason,
+      }
+
+      raise "Invalid 'status:' value for response: #{status}" unless [:success, :error].include?(status)
+
+      # NOTE: These rules are more strict than the ones in ServiceResponse, but we want to enforce this pattern of
+      #       usage within the Remote Development domain.
+      if status == :success
+        raise "'reason:' cannot specified if 'status:' is :success" if reason
+
+        raise "'message:' cannot specified if 'status:' is :success" if message
+
+        raise "'payload:' must specified if 'status:' is :success" if payload.nil?
+      else
+        raise "'reason:' must be specified if 'status:' is :error" if reason.nil?
+
+        raise "'message:' must be specified if 'status:' is :error" if message.nil?
+
+        raise "'payload:' cannot be specified if 'status:' is :error" if payload
+      end
+
+      nil
+    end
+  end
+end

ee/app/services/remote_development/workspaces/create_service.rb

@@ -7,19 +7,10 @@ class CreateService
 
       # NOTE: This constructor intentionally does not follow all of the conventions from
       #       https://docs.gitlab.com/ee/development/reusing_abstractions.html#service-classes
-      #       suggesting that the dependencies be passed via the constructor. This is because
-      #       the RemoteDevelopment feature architecture follows a more pure-functional style,
-      #       by avoiding instance variables and instance state and preferring to pass data
-      #       directly in method calls rather than via constructor. We also don't use any of the
-      #       provided superclasses like BaseContainerService or its descendants, because all of the
-      #       domain logic is isolated and decoupled to the architectural tier below this,
-      #       i.e. in the `*Processor` classes, and therefore these superclasses provide nothing
-      #       of use. However, we do still conform to the convention of passing the current_user
-      #       in the constructor, since this convention is related to security, and worth following
-      #       the existing patterns and principle of least surprise.
+      #       suggesting that the dependencies be passed via the constructor.
       #
-      #       See https://gitlab.com/gitlab-org/remote-development/gitlab-remote-development-docs/-/blob/main/doc/remote-development-feature-architectural-standards.md
-      #       for more discussion on this topic.
+      #       See "Stateless Service layer classes" in ee/lib/remote_development/README.md for more details.
+
       # @param [User] current_user
       # @return [void]
       def initialize(current_user:)

ee/app/services/remote_development/workspaces/reconcile_service.rb

@@ -3,20 +3,11 @@
 module RemoteDevelopment
   module Workspaces
     class ReconcileService
-      # NOTE: This class intentionally does not follow the constructor conventions from
+      # NOTE: This constructor intentionally does not follow all of the conventions from
       #       https://docs.gitlab.com/ee/development/reusing_abstractions.html#service-classes
-      #       suggesting that the dependencies be passed via the constructor. This is because
-      #       the RemoteDevelopment feature architecture follows a more pure-functional style,
-      #       directly in method calls rather than via constructor. We also don't use any of the
-      #       provided superclasses like BaseContainerService or its descendants, because all of the
-      #       domain logic is isolated and decoupled to the architectural tier below this,
-      #       i.e. in the `*Processor` classes, and therefore these superclasses provide nothing
-      #       of use. In this case we also do not even pass the `current_user:` parameter, because this
-      #       service is called from GA4K kas from an internal kubernetes endpoint, and thus there
-      #       is no current_user in context. Therefore we have no need for a constructor at all.
+      #       suggesting that the dependencies be passed via the constructor.
       #
-      #       See https://gitlab.com/gitlab-org/remote-development/gitlab-remote-development-docs/-/blob/main/doc/remote-development-feature-architectural-standards.md
-      #       for more discussion on this topic.
+      #       See "Stateless Service layer classes" in ee/lib/remote_development/README.md for more details.
 
       # @param [Clusters::Agent] agent
       # @param [Hash] params

ee/app/services/remote_development/workspaces/update_service.rb

@@ -3,23 +3,15 @@
 module RemoteDevelopment
   module Workspaces
     class UpdateService
+      include ServiceResponseFactory
       attr_reader :current_user
 
       # NOTE: This constructor intentionally does not follow all of the conventions from
       #       https://docs.gitlab.com/ee/development/reusing_abstractions.html#service-classes
-      #       suggesting that the dependencies be passed via the constructor. This is because
-      #       the RemoteDevelopment feature architecture follows a more pure-functional style,
-      #       by avoiding instance variables and instance state and preferring to pass data
-      #       directly in method calls rather than via constructor. We also don't use any of the
-      #       provided superclasses like BaseContainerService or its descendants, because all of the
-      #       domain logic is isolated and decoupled to the architectural tier below this,
-      #       i.e. in the `*Processor` classes, and therefore these superclasses provide nothing
-      #       of use. However, we do still conform to the convention of passing the current_user
-      #       in the constructor, since this convention is related to security, and worth following
-      #       the existing patterns and principle of least surprise.
+      #       suggesting that the dependencies be passed via the constructor.
       #
-      #       See https://gitlab.com/gitlab-org/remote-development/gitlab-remote-development-docs/-/blob/main/doc/remote-development-feature-architectural-standards.md
-      #       for more discussion on this topic.
+      #       See "Stateless Service layer classes" in ee/lib/remote_development/README.md for more details.
+
       # @param [User] current_user
       # @return [void]
       def initialize(current_user:)
@@ -30,22 +22,12 @@ def initialize(current_user:)
       # @param [Hash] params
       # @return [ServiceResponse]
       def execute(workspace:, params:)
-        return ServiceResponse.error(message: 'Unauthorized', reason: :unauthorized) unless authorized?(workspace)
-
-        payload, error = RemoteDevelopment::Workspaces::Update::UpdateProcessor.new.process(
-          workspace: workspace,
-          params: params
-        )
+        response_hash = Update::Main.main(workspace: workspace, current_user: current_user, params: params)
 
-        return ServiceResponse.error(message: error.message, reason: error.reason) if error
+        # Type-check payload using rightward assignment
+        response_hash[:payload] => { workspace: Workspace } if response_hash[:payload]
 
-        ServiceResponse.success(payload: payload)
-      end
-
-      # @param [RemoteDevelopment::Workspace] workspace
-      # @return [TrueClass, FalseClass]
-      def authorized?(workspace)
-        current_user&.can?(:update_workspace, workspace)
+        create_service_response(response_hash)
       end
     end
   end

ee/lib/remote_development/message.rb

+# frozen_string_literal: true
+
+module RemoteDevelopment
+  # A message's context can be a hash containing any object that is relevant to the message. It will be
+  # used to provide context when the final Result from the chain is pattern matched
+  # on the message type and returned to the user.
+  # The context is required to be a hash so that it can be destructured and type-checked with
+  # rightward assignment.
+  class Message
+    attr_reader :context
+
+    # @param [Hash] context
+    # @return [Message]
+    # raise [ArgumentError] if context is not a Hash
+    def initialize(context = {})
+      raise ArgumentError, 'context must be a Hash' unless context.is_a?(Hash)
+
+      @context = context
+    end
+
+    # @param [RemoteDevelopment::Message] other
+    # @return [TrueClass, FalseClass]
+    def ==(other)
+      self.class == other.class && context == other.context
+    end
+  end
+end

ee/lib/remote_development/message_support.rb

+# frozen_string_literal: true
+
+require 'active_model/errors'
+
+module RemoteDevelopment
+  module MessageSupport
+    # @param [RemoteDevelopment::Message] message
+    # @param [Symbol] reason
+    # @return [Hash]
+    def generate_error_response_from_message(message:, reason:)
+      details_string =
+        case message.context
+        in {}
+          nil
+        in { details: String => error_details }
+          error_details
+        in { errors: ActiveModel::Errors => errors }
+          errors.full_messages.join(', ')
+        else
+          raise "Unexpected message context, add a case to pattern match it and convert it to a String."
+        end
+      # NOTE: Safe navigation operator is used here to prevent a type error, because Module#name is a 'String | nil'
+      message_string = message.class.name&.demodulize&.underscore&.humanize
+      error_message = details_string ? "#{message_string}: #{details_string}" : message_string
+      { status: :error, message: error_message, reason: reason }
+    end
+  end
+end

ee/lib/remote_development/messages.rb

+# frozen_string_literal: true
+
+module RemoteDevelopment
+  # This module contains all messages for the Remote Development domain, both errors and domain events.
+  # Note that we intentionally have not DRY'd up the declaration of the subclasses with loops and
+  # metaprogramming, because we want the types to be easily indexable and navigable within IDEs.
+  module Messages
+    #---------------------------------------------------------------
+    # Errors - message name should describe the reason for the error
+    #---------------------------------------------------------------
+
+    # Auth errors
+    Unauthorized = Class.new(Message)
+
+    # Workspace errors
+    WorkspaceUpdateFailed = Class.new(Message)
+
+    #---------------------------------------------------------
+    # Domain Events - message name should describe the outcome
+    #---------------------------------------------------------
+    WorkspaceUpdateSuccessful = Class.new(Message)
+  end
+end

ee/lib/remote_development/unmatched_result_error.rb

+# frozen_string_literal: true
+
+module RemoteDevelopment
+  class UnmatchedResultError < RuntimeError
+    # @param [Result] result
+    # @return [void]
+    def initialize(result:)
+      msg = "Failed to pattern match #{result.ok? ? "'ok'" : "'err'"} Result " \
+            "containing message of type: #{(result.ok? ? result.unwrap : result.unwrap_err).class.name}"
+
+      super(msg)
+    end
+  end
+end

ee/lib/remote_development/workspaces/update/authorizer.rb

+# frozen_string_literal: true
+
+module RemoteDevelopment
+  module Workspaces
+    module Update
+      class Authorizer
+        include Messages
+
+        # @param [Hash] value
+        # @return [Result]
+        def self.authorize(value)
+          value => { workspace: RemoteDevelopment::Workspace => workspace, current_user: User => current_user }
+
+          if current_user.can?(:update_workspace, workspace)
+            # Pass along the value to the next step
+            Result.ok(value)
+          else
+            Result.err(Unauthorized.new)
+          end
+        end
+      end
+    end
+  end
+end

ee/lib/remote_development/workspaces/update/main.rb

+# frozen_string_literal: true
+
+module RemoteDevelopment
+  module Workspaces
+    module Update
+      class Main
+        include Messages
+
+        extend MessageSupport
+        private_class_method :generate_error_response_from_message
+
+        # @param [Hash] value
+        # @return [Hash]
+        # @raise [UnmatchedResultError]
+        def self.main(value)
+          initial_result = Result.ok(value)
+          result =
+            initial_result
+              .and_then(Authorizer.method(:authorize))
+              .and_then(Updater.method(:update))
+
+          case result
+          in { err: Unauthorized => message }
+            generate_error_response_from_message(message: message, reason: :unauthorized)
+          in { err: WorkspaceUpdateFailed => message }
+            generate_error_response_from_message(message: message, reason: :bad_request)
+          in { ok: WorkspaceUpdateSuccessful => message }
+            message.context => { workspace: Workspace } # Type-check the payload before returning it
+            { status: :success, payload: message.context }
+          else
+            raise UnmatchedResultError.new(result: result)
+          end
+        end
+      end
+    end
+  end
+end

ee/lib/remote_development/workspaces/update/update_processor.rb

-# frozen_string_literal: true
-
-module RemoteDevelopment
-  module Workspaces
-    module Update
-      class UpdateProcessor
-        # @param [RemoteDevelopment::Workspace] workspace
-        # @param [Hash] params
-        # @return [Array<(Hash | nil, RemoteDevelopment::Error | nil)>]
-        def process(workspace:, params:)
-          if workspace.update(params)
-            payload = { workspace: workspace }
-            [payload, nil]
-          else
-            err_msg = "Error(s) updating Workspace: #{workspace.errors.full_messages.join(', ')}"
-            error = Error.new(message: err_msg, reason: :bad_request)
-            [nil, error]
-          end
-        end
-      end
-    end
-  end
-end

ee/lib/remote_development/workspaces/update/updater.rb

+# frozen_string_literal: true
+
+module RemoteDevelopment
+  module Workspaces
+    module Update
+      class Updater
+        include Messages
+
+        # @param [Hash] value
+        # @return [Result]
+        def self.update(value)
+          value => { workspace: RemoteDevelopment::Workspace => workspace, params: Hash => params }
+          if workspace.update(params)
+            Result.ok(WorkspaceUpdateSuccessful.new({ workspace: workspace }))
+          else
+            Result.err(WorkspaceUpdateFailed.new({ errors: workspace.errors }))
+          end
+        end
+      end
+    end
+  end
+end

ee/spec/lib/remote_development/message_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+
+RSpec.describe RemoteDevelopment::Message, feature_category: :remote_development do
+  describe '#==' do
+    it 'implements equality' do
+      expect(described_class.new({ a: 1 })).to eq(described_class.new(a: 1))
+      expect(described_class.new({ a: 1 })).not_to eq(described_class.new(a: 2))
+    end
+  end
+
+  describe 'validation' do
+    it 'requires context to be a Hash' do
+      # noinspection RubyMismatchedArgumentType - Intentionally passing wrong type to check runtime type validation
+      expect { described_class.new(1) }.to raise_error(ArgumentError, "context must be a Hash")
+    end
+  end
+end

ee/spec/lib/remote_development/message_support_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+
+RSpec.describe RemoteDevelopment::MessageSupport, feature_category: :remote_development do
+  let(:object) { Object.new.extend(described_class) }
+
+  describe '.generate_error_response_from_message' do
+    context 'for an unsupported context which is not pattern matched' do
+      let(:message) { RemoteDevelopment::Message.new(context: { unsupported: 'unmatched' }) }
+
+      it 'raises an error' do
+        expect { object.generate_error_response_from_message(message: message, reason: :does_not_matter) }
+          .to raise_error(/Unexpected message context/)
+      end
+    end
+  end
+end

ee/spec/lib/remote_development/unmatched_result_error_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+
+RSpec.describe RemoteDevelopment::UnmatchedResultError, feature_category: :remote_development do
+  let(:unmatched_message_class) { stub_const('UnmatchedMessage', Class.new(RemoteDevelopment::Message)) }
+  let(:unmatched_message) { unmatched_message_class.new }
+
+  context "for an 'ok' Result" do
+    it 'has a correct message' do
+      expected_msg = "Failed to pattern match 'ok' Result containing message of type: UnmatchedMessage"
+
+      expect do
+        raise described_class.new(result: Result.ok(unmatched_message))
+      end.to raise_error(described_class, expected_msg)
+    end
+  end
+
+  context "for an 'err' Result" do
+    it 'has a correct message' do
+      expected_msg = "Failed to pattern match 'err' Result containing message of type: UnmatchedMessage"
+
+      expect do
+        raise described_class.new(result: Result.err(unmatched_message))
+      end.to raise_error(described_class, expected_msg)
+    end
+  end
+end

ee/spec/lib/remote_development/workspaces/update/authorizer_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe RemoteDevelopment::Workspaces::Update::Authorizer, feature_category: :remote_development do
+  include ResultMatchers
+
+  let(:workspace) { build_stubbed(:workspace) }
+  let(:user) { build_stubbed(:user) }
+  let(:user_can_update_workspace) { true }
+  let(:params) { instance_double(Hash) }
+  let(:value) { { workspace: workspace, current_user: user, params: params } }
+
+  subject(:result) do
+    described_class.authorize(value)
+  end
+
+  before do
+    allow(user).to receive(:can?).with(:update_workspace, workspace).and_return(user_can_update_workspace)
+  end
+
+  context 'when user is authorized' do
+    it 'returns an ok Result containing the original value which was passed' do
+      expect(result).to eq(Result.ok(value))
+    end
+  end
+
+  context 'when user is not authorized' do
+    let(:user_can_update_workspace) { false }
+
+    it 'returns an err Result containing an unauthorized message with an empty context' do
+      expect(result).to be_err_result(RemoteDevelopment::Messages::Unauthorized.new)
+    end
+  end
+end