Merge pull request #11571 from rubhanazeem/deep_symbolize_keys

Safely navigate and compare nested hashes in the workflow feature

src/api/app/jobs/report_to_scm_job.rb

@@ -35,7 +35,7 @@ def matched_event_subscription
     EventSubscriptionsFinder.new
                             .for_scm_channel_with_token(event_type: @event_type, event_package: @event_package)
                             .select do |event_subscription|
-      Workflows::Filter.new(filters: event_subscription.payload.with_indifferent_access[:workflow_filters]).match?(@event)
+      Workflows::Filter.new(filters: event_subscription.payload['workflow_filters']).match?(@event)
     end
   end
 end

src/api/app/models/workflow.rb

@@ -2,9 +2,9 @@ class Workflow
   include ActiveModel::Model
 
   SUPPORTED_STEPS = {
-    'branch_package' => Workflow::Step::BranchPackageStep,
-    'link_package' => Workflow::Step::LinkPackageStep,
-    'configure_repositories' => Workflow::Step::ConfigureRepositories
+    branch_package: Workflow::Step::BranchPackageStep,
+    link_package: Workflow::Step::LinkPackageStep,
+    configure_repositories: Workflow::Step::ConfigureRepositories
   }.freeze
 
   SUPPORTED_FILTERS = [:architectures, :repositories].freeze
@@ -13,11 +13,10 @@ class Workflow
 
   attr_accessor :workflow_instructions, :scm_extractor_payload, :token
 
-  # Overwriting the initializer is needed to set `with_indifferent_access`
-  def initialize(workflow_instructions:, scm_extractor_payload:, token:)
-    @workflow_instructions = workflow_instructions.with_indifferent_access
-    @scm_extractor_payload = scm_extractor_payload.with_indifferent_access
-    @token = token
+  def initialize(attributes = {})
+    super
+    @workflow_instructions = attributes[:workflow_instructions].deep_symbolize_keys
+    @scm_extractor_payload = attributes[:scm_extractor_payload].deep_symbolize_keys
   end
 
   validates_with WorkflowStepsValidator
@@ -57,7 +56,7 @@ def filters
   end
 
   def workflow_steps
-    workflow_instructions.fetch('steps', {})
+    workflow_instructions.fetch(:steps, {})
   end
 
   private

src/api/app/models/workflow/step.rb

@@ -6,11 +6,10 @@ class Workflow::Step
 
   attr_reader :scm_extractor_payload, :step_instructions, :token
 
-  # Overwriting the initializer is needed to set `with_indifferent_access`
-  def initialize(scm_extractor_payload:, step_instructions:, token:)
-    @step_instructions = step_instructions&.with_indifferent_access || {}
-    @scm_extractor_payload = scm_extractor_payload&.with_indifferent_access || {}
-    @token = token
+  def initialize(attributes = {})
+    @step_instructions = attributes[:step_instructions]&.deep_symbolize_keys || {}
+    @scm_extractor_payload = attributes[:scm_extractor_payload]&.deep_symbolize_keys || {}
+    @token = attributes[:token]
   end
 
   def call(_options)
@@ -20,19 +19,19 @@ def call(_options)
   protected
 
   def source_project_name
-    step_instructions['source_project']
+    step_instructions[:source_project]
   end
 
   def target_project_name
     "home:#{@token.user.login}:#{source_project_name}:PR-#{scm_extractor_payload[:pr_number]}"
   end
 
   def source_package_name
-    step_instructions['source_package']
+    step_instructions[:source_package]
   end
 
   def target_package_name
-    return step_instructions['target_package'] if step_instructions['target_package'].present?
+    return step_instructions[:target_package] if step_instructions[:target_package].present?
 
     source_package_name
   end

src/api/app/models/workflow/step/configure_repositories.rb

@@ -32,18 +32,14 @@ def validate_step_instructions
   end
 
   def validate_repositories
-    # FIXME: Change Workflow::Step#initialize `with_indifferent_access` for `deep_symbolize_keys` to consistently work
-    #        with symbols, even in nested hashes. This is needed for all workflow steps.
-    return if step_instructions.deep_symbolize_keys[:repositories].all? { |repository| repository.keys.sort == REQUIRED_REPOSITORY_KEYS }
+    return if step_instructions[:repositories].all? { |repository| repository.keys.sort == REQUIRED_REPOSITORY_KEYS }
 
     required_repository_keys_sentence ||= REQUIRED_REPOSITORY_KEYS.map { |key| "'#{key}'" }.to_sentence
     errors.add(:base, "configure_repositories step: All repositories must have the #{required_repository_keys_sentence} keys")
   end
 
   def validate_architectures
-    # FIXME: Change Workflow::Step#initialize `with_indifferent_access` for `deep_symbolize_keys` to consistently work
-    #        with symbols, even in nested hashes. This is needed for all workflow steps.
-    architectures = step_instructions.deep_symbolize_keys[:repositories].map { |repository| repository.fetch(:architectures, []) }.flatten.uniq
+    architectures = step_instructions[:repositories].map { |repository| repository.fetch(:architectures, []) }.flatten.uniq
 
     # Store architectures to avoid fetching them again later in #call
     @supported_architectures = Architecture.where(name: architectures).to_a

src/api/app/services/scm_status_reporter.rb

@@ -2,8 +2,8 @@ class SCMStatusReporter
   attr_accessor :event_payload, :event_subscription_payload, :scm_token, :state
 
   def initialize(event_payload, event_subscription_payload, scm_token, event_type = nil)
-    @event_payload = event_payload.with_indifferent_access
-    @event_subscription_payload = event_subscription_payload.with_indifferent_access
+    @event_payload = event_payload.deep_symbolize_keys
+    @event_subscription_payload = event_subscription_payload.deep_symbolize_keys
     @scm_token = scm_token
 
     @state = event_type.nil? ? 'pending' : scm_final_state(event_type)

src/api/app/services/trigger_controller_service/scm_extractor.rb

@@ -6,7 +6,7 @@ class ScmExtractor
 
     def initialize(scm, event, payload)
       # TODO: What should we do when the user sends a wwwurlencoded payload? Raise an exception?
-      @payload = payload.with_indifferent_access
+      @payload = payload.deep_symbolize_keys
       @scm = scm
       @event = event
     end
@@ -30,51 +30,51 @@ def call
     private
 
     def allowed_github_event_and_action?
-      @event == 'pull_request' && @payload['action'].in?(ALLOWED_GITHUB_ACTIONS)
+      @event == 'pull_request' && @payload[:action].in?(ALLOWED_GITHUB_ACTIONS)
     end
 
     def allowed_gitlab_event_and_action?
-      @event == 'Merge Request Hook' && @payload['object_attributes']['action'].in?(ALLOWED_GITLAB_ACTIONS)
+      @event == 'Merge Request Hook' && @payload[:object_attributes][:action].in?(ALLOWED_GITLAB_ACTIONS)
     end
 
     def github_extractor_payload
-      host = URI.parse(@payload.dig('sender', 'url')).host
+      host = URI.parse(@payload.dig(:sender, :url)).host
       api_endpoint = if host.start_with?('api.github.com')
                        "https://#{host}"
                      else
                        "https://#{host}/api/v3/"
                      end
       {
         scm: 'github',
-        commit_sha: @payload.dig('pull_request', 'head', 'sha'),
-        pr_number: @payload['number'],
-        source_branch: @payload.dig('pull_request', 'head', 'ref'),
-        target_branch: @payload.dig('pull_request', 'base', 'ref'),
-        action: @payload['action'], # TODO: Names may differ, maybe we need to find our own naming (defer to service?)
-        source_repository_full_name: @payload.dig('pull_request', 'head', 'repo', 'full_name'),
-        target_repository_full_name: @payload.dig('pull_request', 'base', 'repo', 'full_name'),
+        commit_sha: @payload.dig(:pull_request, :head, :sha),
+        pr_number: @payload[:number],
+        source_branch: @payload.dig(:pull_request, :head, :ref),
+        target_branch: @payload.dig(:pull_request, :base, :ref),
+        action: @payload[:action], # TODO: Names may differ, maybe we need to find our own naming (defer to service?)
+        source_repository_full_name: @payload.dig(:pull_request, :head, :repo, :full_name),
+        target_repository_full_name: @payload.dig(:pull_request, :base, :repo, :full_name),
         event: @event,
         api_endpoint: api_endpoint
-      }.with_indifferent_access
+      }
     end
 
     def gitlab_extractor_payload
-      http_url = @payload.dig('project', 'http_url')
+      http_url = @payload.dig(:project, :http_url)
       uri = URI.parse(http_url)
       {
         scm: 'gitlab',
-        object_kind: @payload['object_kind'],
+        object_kind: @payload[:object_kind],
         http_url: http_url,
-        commit_sha: @payload.dig('object_attributes', 'last_commit', 'id'),
-        pr_number: @payload.dig('object_attributes', 'iid'),
-        source_branch: @payload.dig('object_attributes', 'source_branch'),
-        target_branch: @payload.dig('object_attributes', 'target_branch'),
-        action: @payload.dig('object_attributes', 'action'), # TODO: Names may differ, maybe we need to find our own naming (defer to service?)
-        project_id: @payload.dig('object_attributes', 'source_project_id'),
-        path_with_namespace: @payload.dig('project', 'path_with_namespace'),
+        commit_sha: @payload.dig(:object_attributes, :last_commit, :id),
+        pr_number: @payload.dig(:object_attributes, :iid),
+        source_branch: @payload.dig(:object_attributes, :source_branch),
+        target_branch: @payload.dig(:object_attributes, :target_branch),
+        action: @payload.dig(:object_attributes, :action), # TODO: Names may differ, maybe we need to find our own naming (defer to service?)
+        project_id: @payload.dig(:object_attributes, :source_project_id),
+        path_with_namespace: @payload.dig(:project, :path_with_namespace),
         event: @event,
         api_endpoint: "#{uri.scheme}://#{uri.host}"
-      }.with_indifferent_access
+      }
     end
   end
 end

src/api/app/services/workflows/filter.rb

@@ -1,7 +1,7 @@
 module Workflows
   class Filter
     def initialize(filters:)
-      filters ||= {}
+      filters = filters.blank? ? {} : filters.deep_symbolize_keys
       @repository_filters = filters[:repositories]
       @architecture_filters = filters[:architectures]
     end

src/api/app/validators/workflow_filters_validator.rb

@@ -1,7 +1,7 @@
 class WorkflowFiltersValidator < ActiveModel::Validator
   def validate(record)
     @scm_extractor_payload = record.scm_extractor_payload
-    @workflow_instructions = record.workflow_instructions.with_indifferent_access
+    @workflow_instructions = record.workflow_instructions
 
     valid_filters?
   end

src/api/spec/models/workflow_spec.rb

@@ -200,7 +200,7 @@
           scm: 'github',
           action: 'opened',
           event: 'pull_request'
-        }.with_indifferent_access
+        }
       end
 
       it { expect(subject).to be_valid }
@@ -215,7 +215,7 @@
           scm: 'github',
           action: 'invalid_action',
           event: 'invalid_event'
-        }.with_indifferent_access
+        }
       end
 
       it { expect(subject).not_to(be_valid) }

src/api/spec/support/shared_contexts/a_scm_payload_hash.rb

@@ -10,7 +10,7 @@
       action: 'opened',
       repository_full_name: 'openSUSE/open-build-service',
       event: 'pull_request'
-    }.with_indifferent_access
+    }
   end
   let(:gitlab_extractor_payload) do
     {
@@ -25,6 +25,6 @@
       project_id: 1,
       path_with_namespace: 'gitlabhq/gitlab-test',
       event: 'Merge Request Hook'
-    }.with_indifferent_access
+    }
   end
 end