From 5b136147ca81406df18c54d0974a93a700188b6e Mon Sep 17 00:00:00 2001 From: Thorben Denzer Date: Wed, 7 Jan 2026 10:41:00 +0100 Subject: [PATCH 1/4] feat(errors): add custom error system This system allows developers to wrap code in a function which takes an error scope. This scope can contain one or more application errors, each of which represent a step that can go wrong when the code is executed. When one of the errors expected in the scope is raised, it is resolved to a customized error message, which contains: - a HTTP response code - a title - a description - a link to documentation explaining the error This error is then raised and bubbled up to the controller, where it is returned to the user. Should an error be raised that is not expected in the scope, an UnexpectedError exception will be raised, which tells the developer that a known error was raised in a scope it was not expected to occur in. Should a non-ApplicationError exception be raised, an UnknownError will be raised, telling the user that an actual bug is present. --- .../api/v2/ansible_director_api_controller.rb | 13 ++++ .../errors/application_error.rb | 45 +++++++++++ .../errors/helpers.rb | 78 +++++++++++++++++++ .../errors/unexpected_error.rb | 11 +++ .../errors/unknown_error.rb | 11 +++ 5 files changed, 158 insertions(+) create mode 100644 app/lib/foreman_ansible_director/errors/application_error.rb create mode 100644 app/lib/foreman_ansible_director/errors/helpers.rb create mode 100644 app/lib/foreman_ansible_director/errors/unexpected_error.rb create mode 100644 app/lib/foreman_ansible_director/errors/unknown_error.rb diff --git a/app/controllers/foreman_ansible_director/api/v2/ansible_director_api_controller.rb b/app/controllers/foreman_ansible_director/api/v2/ansible_director_api_controller.rb index f36e0255..2c1497a0 100644 --- a/app/controllers/foreman_ansible_director/api/v2/ansible_director_api_controller.rb +++ b/app/controllers/foreman_ansible_director/api/v2/ansible_director_api_controller.rb @@ -5,6 +5,19 @@ module Api module V2 class AnsibleDirectorApiController < ::Api::V2::BaseController include ::Api::Version2 + include ::ForemanAnsibleDirector::Errors::Helpers + + rescue_from 'ForemanAnsibleDirector::Errors::ApplicationError' do |exception| + message = { + error_code: exception.error_code, + title: exception.title, + description: exception.description, + docs_link: exception.docs_link, + }.compact + + render_error('custom_error', status: exception.response_status, + locals: { message: message }) + end def find_organization @organization = Organization.current || find_optional_organization diff --git a/app/lib/foreman_ansible_director/errors/application_error.rb b/app/lib/foreman_ansible_director/errors/application_error.rb new file mode 100644 index 00000000..9d59d8d2 --- /dev/null +++ b/app/lib/foreman_ansible_director/errors/application_error.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +module ForemanAnsibleDirector + module Errors + class ApplicationError < StandardError + attr_reader :error_code, :title, :description, :response_status, :docs_link + + def initialize(error_code, title_args, description_args, args) + if args + title_args.merge!(args) + description_args.merge!(args) + end + error_object = ::ForemanAnsibleDirector::Errors.error_object(error_code, title_args, description_args) + @error_code = error_code + @response_status = error_object[:response_status] + @title = error_object[:title] + @description = error_object[:description] + @docs_link = error_object[:docs_link] + super() + end + end + + def self.error_object(error_code, title_args, description_args) + raw = ::ForemanAnsibleDirector::Errors::ERROR_OBJECTS[error_code] + + { + response_status: raw[:response_status], + title: raw[:title] % title_args, + description: raw[:description] % description_args, + docs_link: raw[:docs_link], + } + end + + def self.env(message) + # ^ADR-(?\d{3})-(?\d{3})-(?\d{3})-(?\d{3})$ + + if Rails.env.development? + return message # TODO: This should return more verbose output at some point + end + message + end + + ERROR_OBJECTS = HashWithIndifferentAccess.new({}) + end +end diff --git a/app/lib/foreman_ansible_director/errors/helpers.rb b/app/lib/foreman_ansible_director/errors/helpers.rb new file mode 100644 index 00000000..2e1fa207 --- /dev/null +++ b/app/lib/foreman_ansible_director/errors/helpers.rb @@ -0,0 +1,78 @@ +# frozen_string_literal: true + +module ForemanAnsibleDirector + module Errors + module Helpers + def app_error_for(error_code, title_args: {}, description_args: {}, args: nil) + ::ForemanAnsibleDirector::Errors::ApplicationError.new(error_code, title_args, description_args, args) + end + + class ErrorScope + def initialize(errors) + @errors = errors + end + + def try(&block) + @try_block = block + execute + end + + def execute + @try_block&.call + rescue ::ForemanAnsibleDirector::Errors::ApplicationError => e + # Disabling rubocop here, because the "thing" we want to do with the error is not implemented yet + # rubocop:disable Style/GuardClause + if @errors.include?(e.error_code) + # TODO: do something with expected error + raise e + else + raise ::ForemanAnsibleDirector::Errors::UnexpectedError + end + # rubocop:enable Style/GuardClause + rescue StandardError + raise ::ForemanAnsibleDirector::Errors::UnknownError + end + end + + class ErrorScopeRescue + def initialize(errors) + @errors = errors + end + + def try(&block) + @try_block = block + self + end + + def catch(&block) + @catch_block = block + execute + end + + def execute + @try_block&.call + rescue ::ForemanAnsibleDirector::Errors::ApplicationError => e + @catch_block&.call(e) + if @errors.include?(e.error_code) + # TODO: do something with expected error + raise e + end + rescue StandardError + raise ::ForemanAnsibleDirector::Errors::UnknownError + end + end + + def error_scope_rescue(errors) + result = ErrorScopeRescue.new(errors) + yield(result) if block_given? + result + end + + def error_scope(errors) + result = ErrorScope.new(errors) + yield(result) if block_given? + result + end + end + end +end diff --git a/app/lib/foreman_ansible_director/errors/unexpected_error.rb b/app/lib/foreman_ansible_director/errors/unexpected_error.rb new file mode 100644 index 00000000..8d2ef9ea --- /dev/null +++ b/app/lib/foreman_ansible_director/errors/unexpected_error.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module ForemanAnsibleDirector + module Errors + class UnexpectedError < StandardError + def initialize + super('An unexpected error has occurred.') + end + end + end +end diff --git a/app/lib/foreman_ansible_director/errors/unknown_error.rb b/app/lib/foreman_ansible_director/errors/unknown_error.rb new file mode 100644 index 00000000..c35aa66a --- /dev/null +++ b/app/lib/foreman_ansible_director/errors/unknown_error.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module ForemanAnsibleDirector + module Errors + class UnknownError < StandardError + def initialize + super('An unknown error has occurred. This is likely a bug you should report.') + end + end + end +end From ceb342ff372652180141a12c85b8c432dcbd4894 Mon Sep 17 00:00:00 2001 From: Thorben Denzer Date: Tue, 10 Feb 2026 11:47:35 +0100 Subject: [PATCH 2/4] feat(errors/assignment_service): handle errors of finder and find_target - finder: - (ADR-003-005-004-001) invalid target type Raised when the user supplied an unsupported target type - find_target: - (ADR-003-005-005-001) target not found Raised when the user supplied a target id which does not exist Introduces error codes: - ADR-003-005-004-001 - ADR-003-005-005-001 --- .../api/v2/assignments_controller.rb | 1 - .../errors/application_error.rb | 22 ++++++++++- .../assignment_service.rb | 39 ++++++++++++------- error_codes.yaml | 18 +++++++++ test/services/unit/assignment_service_test.rb | 13 ------- 5 files changed, 64 insertions(+), 29 deletions(-) create mode 100644 error_codes.yaml diff --git a/app/controllers/foreman_ansible_director/api/v2/assignments_controller.rb b/app/controllers/foreman_ansible_director/api/v2/assignments_controller.rb index c80988a1..8b019b9a 100644 --- a/app/controllers/foreman_ansible_director/api/v2/assignments_controller.rb +++ b/app/controllers/foreman_ansible_director/api/v2/assignments_controller.rb @@ -11,7 +11,6 @@ def assignments target_type: params[:target], target_id: params[:target_id] ) - # TODO: Null check target @assignments = target.resolved_ansible_content end diff --git a/app/lib/foreman_ansible_director/errors/application_error.rb b/app/lib/foreman_ansible_director/errors/application_error.rb index 9d59d8d2..82feb9e1 100644 --- a/app/lib/foreman_ansible_director/errors/application_error.rb +++ b/app/lib/foreman_ansible_director/errors/application_error.rb @@ -40,6 +40,26 @@ def self.env(message) message end - ERROR_OBJECTS = HashWithIndifferentAccess.new({}) + ERROR_OBJECTS = HashWithIndifferentAccess.new({ + "ADR-003-005-004-001": { + response_status: :not_found, + title: <<~TITLE, + Invalid target type %s specified. + TITLE + description: <<~DESC, + A target type with the name %s was not found. You likely misspelled it. + Allowed types are [HOST, HOSTGROUP, ACR, CONTENT]. + DESC + }, + "ADR-003-005-005-001": { + response_status: :not_found, + title: <<~TITLE, + Target not found. + TITLE + description: <<~DESC, + A target of type %s with id %s was not found. + DESC + }, + }) end end diff --git a/app/services/foreman_ansible_director/assignment_service.rb b/app/services/foreman_ansible_director/assignment_service.rb index 71eefe24..250eaac9 100644 --- a/app/services/foreman_ansible_director/assignment_service.rb +++ b/app/services/foreman_ansible_director/assignment_service.rb @@ -3,6 +3,8 @@ module ForemanAnsibleDirector class AssignmentService class << self + include ForemanAnsibleDirector::Errors::Helpers + def destroy_assignment(assignment) ActiveRecord::Base.transaction do assignment.destroy @@ -38,26 +40,35 @@ def create_bulk_assignments(assignments:) end def finder(type:) - case type + error_scope(['ADR-003-005-004-001']).try do + case type - when 'ACR' - ::ForemanAnsibleDirector::AnsibleCollectionRole - when 'CONTENT' - ::ForemanAnsibleDirector::ContentUnitVersion - when 'HOST' - Host::Managed - when 'HOSTGROUP' - Hostgroup - else - # TODO: Actual error message - raise "Invalid type: #{type}" + when 'ACR' + ::ForemanAnsibleDirector::AnsibleCollectionRole + when 'CONTENT' + ::ForemanAnsibleDirector::ContentUnitVersion + when 'HOST' + Host::Managed + when 'HOSTGROUP' + Hostgroup + else + raise app_error_for('ADR-003-005-004-001', args: { type: type }) + end end end def find_target(target_type:, target_id:) - # TODO: Null check target finder = finder(type: target_type) - finder.find_by(id: target_id) + error_scope(['ADR-003-005-005-001']).try do + target = finder.find_by(id: target_id) + unless target + raise app_error_for('ADR-003-005-005-001', args: { + target_type: target_type, + target_id: target_id, + }) + end + target + end end end end diff --git a/error_codes.yaml b/error_codes.yaml new file mode 100644 index 00000000..cfec7282 --- /dev/null +++ b/error_codes.yaml @@ -0,0 +1,18 @@ +003: + description: "Internal service" + children: + 005: + description: "AssignmentService" + children: + 004: + description: "finder" + children: + 001: + description: "Invalid target type" + details: "Raised when the target type is not supported" + 005: + description: "find_target" + children: + 001: + description: "Target not found" + details: "Raised when the target is not found" \ No newline at end of file diff --git a/test/services/unit/assignment_service_test.rb b/test/services/unit/assignment_service_test.rb index dd88d681..d4b7f565 100644 --- a/test/services/unit/assignment_service_test.rb +++ b/test/services/unit/assignment_service_test.rb @@ -137,11 +137,6 @@ class AssignmentServiceTest < ForemanAnsibleDirectorTestCase assert_equal Hostgroup, result end - test 'raises error for invalid type' do - assert_raises(RuntimeError, 'Invalid type: UNKNOWN') do - ::ForemanAnsibleDirector::AssignmentService.finder(type: 'UNKNOWN') - end - end end describe '#find_target' do @@ -189,14 +184,6 @@ class AssignmentServiceTest < ForemanAnsibleDirectorTestCase end - test 'returns nil for non-existent target' do - result = ::ForemanAnsibleDirector::AssignmentService.find_target( - target_type: 'CONTENT', - target_id: -1 - ) - - assert_nil result - end end end From 4457beff7f769836adc61db9239e0f3aa92592c5 Mon Sep 17 00:00:00 2001 From: Thorben Denzer Date: Tue, 10 Feb 2026 12:02:38 +0100 Subject: [PATCH 3/4] refactor(services): make all services inherit from AnsibleDirectorService This new base-class allows definition of service-global methods and imports. Includes Ansible director error helpers. --- .../foreman_ansible_director/ansible_director_service.rb | 9 +++++++++ .../foreman_ansible_director/assignment_service.rb | 4 +--- app/services/foreman_ansible_director/content_service.rb | 2 +- .../execution_environment_service.rb | 2 +- .../lifecycle_environment_path_service.rb | 2 +- .../lifecycle_environment_service.rb | 2 +- .../foreman_ansible_director/variable_service.rb | 2 +- 7 files changed, 15 insertions(+), 8 deletions(-) create mode 100644 app/services/foreman_ansible_director/ansible_director_service.rb diff --git a/app/services/foreman_ansible_director/ansible_director_service.rb b/app/services/foreman_ansible_director/ansible_director_service.rb new file mode 100644 index 00000000..a0b82aea --- /dev/null +++ b/app/services/foreman_ansible_director/ansible_director_service.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +module ForemanAnsibleDirector + class AnsibleDirectorService + class << self + include ::ForemanAnsibleDirector::Errors::Helpers + end + end +end diff --git a/app/services/foreman_ansible_director/assignment_service.rb b/app/services/foreman_ansible_director/assignment_service.rb index 250eaac9..6cce1d10 100644 --- a/app/services/foreman_ansible_director/assignment_service.rb +++ b/app/services/foreman_ansible_director/assignment_service.rb @@ -1,10 +1,8 @@ # frozen_string_literal: true module ForemanAnsibleDirector - class AssignmentService + class AssignmentService < ::ForemanAnsibleDirector::AnsibleDirectorService class << self - include ForemanAnsibleDirector::Errors::Helpers - def destroy_assignment(assignment) ActiveRecord::Base.transaction do assignment.destroy diff --git a/app/services/foreman_ansible_director/content_service.rb b/app/services/foreman_ansible_director/content_service.rb index 8841c662..b9cd419d 100644 --- a/app/services/foreman_ansible_director/content_service.rb +++ b/app/services/foreman_ansible_director/content_service.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module ForemanAnsibleDirector - class ContentService + class ContentService < ::ForemanAnsibleDirector::AnsibleDirectorService class << self def create_content_unit_revision(cuv_id:, git_ref:, diff --git a/app/services/foreman_ansible_director/execution_environment_service.rb b/app/services/foreman_ansible_director/execution_environment_service.rb index 6d3ed360..57f83aca 100644 --- a/app/services/foreman_ansible_director/execution_environment_service.rb +++ b/app/services/foreman_ansible_director/execution_environment_service.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module ForemanAnsibleDirector - class ExecutionEnvironmentService + class ExecutionEnvironmentService < ::ForemanAnsibleDirector::AnsibleDirectorService class << self def create_execution_environment(name:, base_image_url:, diff --git a/app/services/foreman_ansible_director/lifecycle_environment_path_service.rb b/app/services/foreman_ansible_director/lifecycle_environment_path_service.rb index 1b3c6cce..5c4dc25b 100644 --- a/app/services/foreman_ansible_director/lifecycle_environment_path_service.rb +++ b/app/services/foreman_ansible_director/lifecycle_environment_path_service.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module ForemanAnsibleDirector - class LifecycleEnvironmentPathService + class LifecycleEnvironmentPathService < ::ForemanAnsibleDirector::AnsibleDirectorService class << self def create_path(name:, description:, diff --git a/app/services/foreman_ansible_director/lifecycle_environment_service.rb b/app/services/foreman_ansible_director/lifecycle_environment_service.rb index 263c4d85..0fbc9e75 100644 --- a/app/services/foreman_ansible_director/lifecycle_environment_service.rb +++ b/app/services/foreman_ansible_director/lifecycle_environment_service.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module ForemanAnsibleDirector - class LifecycleEnvironmentService + class LifecycleEnvironmentService < ::ForemanAnsibleDirector::AnsibleDirectorService class << self def create_environment(lce_path:, name:, diff --git a/app/services/foreman_ansible_director/variable_service.rb b/app/services/foreman_ansible_director/variable_service.rb index 495b0e82..8914b1ab 100644 --- a/app/services/foreman_ansible_director/variable_service.rb +++ b/app/services/foreman_ansible_director/variable_service.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module ForemanAnsibleDirector - class VariableService + class VariableService < ::ForemanAnsibleDirector::AnsibleDirectorService class << self def create_variable(key:, type:, From 6dfa0542958249adbf9e5b2bd112d236b341efcd Mon Sep 17 00:00:00 2001 From: Thorben Denzer Date: Tue, 10 Feb 2026 12:15:44 +0100 Subject: [PATCH 4/4] refactor(services/assignment): use find_target in create_bulk_overrides Usage of find_target enables error-handling and deduplicates code. --- app/services/foreman_ansible_director/assignment_service.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/app/services/foreman_ansible_director/assignment_service.rb b/app/services/foreman_ansible_director/assignment_service.rb index 6cce1d10..bbf7f691 100644 --- a/app/services/foreman_ansible_director/assignment_service.rb +++ b/app/services/foreman_ansible_director/assignment_service.rb @@ -23,10 +23,8 @@ def create_bulk_assignments(assignments:) cleared_targets = [] ActiveRecord::Base.transaction do assignments.each do |assignment| - source_finder = ::ForemanAnsibleDirector::AssignmentService.finder(type: assignment[:source][:type]) - target_finder = ::ForemanAnsibleDirector::AssignmentService.finder(type: assignment[:target][:type]) - source = source_finder.find_by(id: assignment[:source][:id]) - target = target_finder.find_by(id: assignment[:target][:id]) + source = find_target(target_type: assignment[:source][:type], target_id: assignment[:source][:id]) + target = find_target(target_type: assignment[:target][:type], target_id: assignment[:target][:id]) unless target.id.in?(cleared_targets) ::ForemanAnsibleDirector::AnsibleContentAssignment.where(assignable: target).destroy_all