-
Notifications
You must be signed in to change notification settings - Fork 0
Error system and error handling of AssignmentService #7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
5b13614
ceb342f
4457bef
6dfa054
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| # 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-(?<error_source>\d{3})-(?<source_id>\d{3})-(?<failure_source>\d{3})-(?<failure_id>\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({ | ||
| "ADR-003-005-004-001": { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please use a easier error code scheme like "ADR-001-001" instead of 4 columns |
||
| response_status: :not_found, | ||
| title: <<~TITLE, | ||
| Invalid target type %<type>s specified. | ||
| TITLE | ||
| description: <<~DESC, | ||
| A target type with the name %<type>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 %<target_type>s with id %<target_id>s was not found. | ||
| DESC | ||
| }, | ||
| }) | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. instead of hiding the real issue, what about just let it unhandled and pass it through? |
||
| end | ||
| end | ||
|
|
||
| class ErrorScopeRescue | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is currently not used. what is the purpose of this and how should it work? I think it would be better to handle the exception at the code section where they really happen |
||
| 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 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module ForemanAnsibleDirector | ||
| class AnsibleDirectorService | ||
| class << self | ||
| include ::ForemanAnsibleDirector::Errors::Helpers | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module ForemanAnsibleDirector | ||
| class AssignmentService | ||
| class AssignmentService < ::ForemanAnsibleDirector::AnsibleDirectorService | ||
| class << self | ||
| def destroy_assignment(assignment) | ||
| ActiveRecord::Base.transaction do | ||
|
|
@@ -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 | ||
|
|
@@ -38,26 +36,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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this duplicates the error codes like this https://github.com/ATIX-AG/foreman_ansible_director/pull/7/changes#diff-3fd51f00ed42919881336d2ff09ded12a6ced416213ec0dcb811917d5bfb7c44R61 I know, its about tell error_scope which error may occur. unknown error should "flow through" |
||
| 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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| 003: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this file needs to be auto-generated from the error objects |
||
| 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" | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should have "our own" erorr code system.
Please have a look at: https://github.com/theforeman/foreman/blob/develop/lib/foreman/exception.rb#L8
if we want to have something better than this, we should take the time to get this into foreman core so that other plugins (and core) can re-use this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree for multiple reasons:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just throw an exception without these error codes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General benefits of error codes aside, raising an exception in the business logic would require bloating of the BL with a message, a response code and a title.
This already fixes that.