references/openproject
1
0
Fork 0
mirror of https://github.com/opf/openproject.git synced 2026-06-14 03:30:14 +00:00
Code Issues Packages Projects Releases Wiki Activity

rework error instantiation

Browse Source 
- multiple errors are no longer a special case of specific errors
- there is a separate error class to represent multiple API errors
- convert ActiveRecord errors to API errors in a single place, ideally work with API errors from there on
This commit is contained in:
Jan Sandbrink
2015-07-02 15:13:15 +02:00
parent 3e87d10980
commit dcd2a3b03c
15 changed files with 140 additions and 138 deletions
Download Patch File Download Diff File Expand all files Collapse all files
config/initializers/10-patches.rb
+1 -3
View File
@@ -76,9 +76,7 @@ module ActiveModel
def add_with_storing_error_symbols(attribute, message = nil, options = {})
add_without_storing_error_symbols(attribute, message, options)
if message.is_a?(Symbol)
writable_error_symbols_for(attribute) << message
end
writable_error_symbols_for(attribute) << message
end
alias_method_chain :add, :storing_error_symbols
lib/api/errors/error_base.rb
+60 -18
View File
@@ -30,29 +30,71 @@
module API
module Errors
class ErrorBase < Grape::Exceptions::Base
attr_reader :code, :message, :details, :errors
attr_reader :code, :message, :details, :errors, :property
def self.create(errors)
if errors.has_key?(:base)
base_errors = errors.error_symbols_for(:base)
if base_errors.include?(:error_not_found)
return ::API::Errors::NotFound.new
elsif base_errors.include?(:error_unauthorized)
return ::API::Errors::Unauthorized.new
elsif base_errors.include?(:error_conflict)
return ::API::Errors::Conflict.new
class << self
##
# Converts the given ActiveRecord errors into an Array of Error objects
# (i.e. subclasses of ErrorBase)
# In case the given errors contain 'critical' errors, the resulting Array will only
# contain the critical error and no non-critical errors (avoiding information disclosure)
# That means: The returned errors are always safe for display towards a user
def create_errors(errors)
if errors.has_key?(:base)
base_errors = errors.error_symbols_for(:base)
if base_errors.include?(:error_not_found)
return [::API::Errors::NotFound.new]
elsif base_errors.include?(:error_unauthorized)
return [::API::Errors::Unauthorized.new]
elsif base_errors.include?(:error_conflict)
return [::API::Errors::Conflict.new]
end
end
convert_ar_to_api_errors(errors)
end
messages_by_attribute = ::API::Errors::Validation.create(errors)
::API::Errors::Validation.new(messages_by_attribute.values.map(&:message))
end
##
# Like :create_errors, but creates a single MultipleErrors error if
# more than one error would be returned otherwise.
def create_and_merge_errors(errors)
::API::Errors::MultipleErrors.create_if_many(create_errors(errors))
end
##
# Allows defining this error class's identifier once.
# Used to read it otherwise.
def self.identifier(identifier = nil)
@identifier ||= identifier
##
# Allows defining this error class's identifier.
# Used to read it otherwise.
def identifier(identifier = nil)
@identifier = identifier if identifier
@identifier
end
private
def convert_ar_to_api_errors(errors)
api_errors = []
errors.keys.each do |attribute|
errors.error_symbols_for(attribute).each do |symbol_or_message|
if symbol_or_message == :error_readonly
api_errors << ::API::Errors::UnwritableProperty.new(attribute)
else
partial_message = if symbol_or_message.is_a?(Symbol)
errors.generate_message(attribute, symbol_or_message)
else
symbol_or_message
end
full_message = errors.full_message(attribute, partial_message)
api_errors << ::API::Errors::Validation.new(attribute, full_message)
end
end
end
api_errors
end
end
def initialize(code, message)
lib/api/errors/multiple_errors.rb
+49
View File
@@ -0,0 +1,49 @@
#-- encoding: UTF-8
#-- copyright
# OpenProject is a project management system.
# Copyright (C) 2012-2015 the OpenProject Foundation (OPF)
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License version 3.
#
# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
# Copyright (C) 2006-2013 Jean-Philippe Lang
# Copyright (C) 2010-2013 the ChiliProject Team
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License
# as published by the Free Software Foundation; either version 2
# of the License, or (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the Free Software
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
#
# See doc/COPYRIGHT.rdoc for more details.
#++
module API
module Errors
class MultipleErrors < ErrorBase
identifier 'urn:openproject-org:api:v3:errors:MultipleErrors'
def self.create_if_many(errors)
raise ArgumentError, 'expected at least one error' unless errors.any?
return errors.first if errors.count == 1
MultipleErrors.new(errors)
end
def initialize(errors)
super 422, I18n.t('api_v3.errors.multiple_errors')
@errors = errors
end
end
end
end
lib/api/errors/unwritable_property.rb
+4 -31
View File
@@ -32,38 +32,11 @@ module API
class UnwritableProperty < ErrorBase
identifier 'urn:openproject-org:api:v3:errors:PropertyIsReadOnly'
def initialize(invalid_attributes)
attributes = Array(invalid_attributes)
def initialize(property)
super 422, I18n.t('api_v3.errors.writing_read_only_attributes')
fail ArgumentError, 'UnwritableProperty error must contain at least one invalid attribute!' if attributes.empty?
if attributes.length == 1
message = if attributes.length == 1
begin
I18n.t("api_v3.errors.validation.#{attributes.first}", raise: true)
rescue I18n::MissingTranslationData
I18n.t('api_v3.errors.writing_read_only_attributes')
end
else
I18n.t('api_v3.errors.multiple_errors')
end
else
message = I18n.t('api_v3.errors.multiple_errors')
end
super 422, message
evaluate_attributes(attributes, invalid_attributes)
end
def evaluate_attributes(attributes, invalid_attributes)
if attributes.length > 1
invalid_attributes.each do |attribute|
@errors << UnwritableProperty.new(attribute)
end
else
@details = { attribute: attributes[0].to_s.camelize(:lower) }
end
@property = property
@details = { attribute: property }
end
end
end
lib/api/errors/validation.rb
+4 -51
View File
@@ -32,58 +32,11 @@ module API
class Validation < ErrorBase
identifier 'urn:openproject-org:api:v3:errors:PropertyConstraintViolation'
def self.create(errors)
errors = merge_error_properties(errors)
def initialize(property, full_message)
super 422, full_message
errors.keys.each_with_object({}) do |attribute, hash|
messages = errors[attribute].each_with_object([]) do |message, message_list|
# Let's assume that standard validation errors never end with a
# punctuation mark. Then it should be fair enough to assume that we
# don't need to prepend the error key if the error ends with a
# punctuation mark. Let's hope that this is true for the languages
# we'll support in OpenProject.
if message =~ /(\.|\?|\!)\z/
message_list << message
else
message_list << errors.full_message(attribute, message) + '.'
end
end
hash[attribute.to_s.camelize(:lower)] = ::API::Errors::Validation.new(messages)
end
end
# Merges property error messages (e.g. for status and status_id)
def self.merge_error_properties(errors)
merged_errors = errors.dup
errors.keys.each do |property|
match = /(?<property>\w+)_id/.match(property)
if match
key = match[:property].to_sym
error = Array(errors[key]) + errors[property]
merged_errors.set(key, error)
merged_errors.delete(property)
end
end
merged_errors
end
def initialize(messages)
messages = Array(messages)
if messages.length == 1
message = messages[0]
else
message = I18n.t('api_v3.errors.multiple_errors')
end
super 422, message
messages.each { |m| @errors << Validation.new(m) } if messages.length > 1
@property = property
@details = { attribute: property }
end
end
end
lib/api/v3/activities/activities_api.rb
+1 -1
View File
@@ -56,7 +56,7 @@ module API
representer
else
fail ::API::Errors::ErrorBase.create(activity.errors)
fail ::API::Errors::ErrorBase.create_and_merge_errors(activity.errors)
end
end
lib/api/v3/attachments/attachments_by_work_package_api.rb
+2 -1
View File
@@ -42,6 +42,7 @@ module API
unless metadata.file_name
raise ::API::Errors::Validation.new(
:file_name,
"fileName #{I18n.t('activerecord.errors.messages.blank')}.")
end
@@ -80,7 +81,7 @@ module API
attachment = make_attachment(metadata, file)
unless attachment.save
raise ::API::Errors::ErrorBase.create(attachment.errors)
raise ::API::Errors::ErrorBase.create_and_merge_errors(attachment.errors)
end
::API::V3::Attachments::AttachmentRepresenter.new(attachment)
lib/api/v3/errors/error_representer.rb
+1 -3
View File
@@ -55,9 +55,7 @@ module API
end
def error_identifier
return 'urn:openproject-org:api:v3:errors:MultipleErrors' unless Array(represented.errors).empty?
represented.class.identifier if represented.class.respond_to?(:identifier)
represented.class.identifier
end
end
end
lib/api/v3/relations/relations_api.rb
+1 -1
View File
@@ -52,7 +52,7 @@ module API
representer = RelationRepresenter.new(relation, work_package: relation.to)
representer
else
fail ::API::Errors::Validation.new(I18n.t('api_v3.errors.invalid_relation'))
fail ::API::Errors::Validation.new(nil, I18n.t('api_v3.errors.invalid_relation'))
end
end
lib/api/v3/work_packages/form_representer.rb
+5 -4
View File
@@ -31,7 +31,7 @@ module API
module V3
module WorkPackages
class FormRepresenter < ::API::Decorators::Single
def initialize(model, current_user: nil, errors:)
def initialize(model, current_user: nil, errors: [])
@errors = errors
super(model, current_user: current_user)
@@ -59,9 +59,10 @@ module API
end
def validation_errors
::API::Errors::Validation.create(@errors).inject({}) do |h, (k, v)|
h[k] = ::API::V3::Errors::ErrorRepresenter.new(v)
h
@errors.group_by(&:property).inject({}) do |hash, (property, errors)|
error = ::API::Errors::MultipleErrors.create_if_many(errors)
hash[property] = ::API::V3::Errors::ErrorRepresenter.new(error)
hash
end
end
end
lib/api/v3/work_packages/watchers_api.rb
+1 -4
View File
@@ -81,10 +81,7 @@ module API
Services::CreateWatcher.new(@work_package, user).run(
success: -> (result) { status(200) unless result[:created] },
failure: -> (watcher) {
messages = watcher.errors.map do |attribute, message|
watcher.errors.full_message(attribute, message) + '.'
end
raise ::API::Errors::Validation.new(messages)
raise ::API::Errors::ErrorBase.create_and_merge_errors(watcher.errors)
}
)
lib/api/v3/work_packages/work_packages_api.rb
+2 -2
View File
@@ -75,7 +75,7 @@ module API
work_package_representer
else
fail ::API::Errors::ErrorBase.create(contract.errors)
fail ::API::Errors::ErrorBase.create_and_merge_errors(contract.errors)
end
end
@@ -86,7 +86,7 @@ module API
Activities::ActivityRepresenter.new(work_package.journals.last,
current_user: current_user)
else
fail ::API::Errors::Validation.new(work_package)
fail ::API::Errors::ErrorBase.create_and_merge_errors(work_package.errors)
end
end
end
lib/api/v3/work_packages/work_packages_by_project_api.rb
+1 -1
View File
@@ -56,7 +56,7 @@ module API
WorkPackages::WorkPackageRepresenter.create(work_package, current_user: current_user)
else
fail ::API::Errors::ErrorBase.create(work_package.errors)
fail ::API::Errors::ErrorBase.create_and_merge_errors(work_package.errors)
end
end
lib/api/v3/work_packages/work_packages_shared_helpers.rb
+4 -4
View File
@@ -61,14 +61,14 @@ module API
contract = contract_class.new(work_package, current_user)
contract.validate
api_error = ::API::Errors::ErrorBase.create(contract.errors)
api_errors = ::API::Errors::ErrorBase.create_errors(contract.errors)
# errors for invalid data (e.g. validation errors) are handled inside the form
if api_error.code == 422
if api_errors.all? { |error| error.code == 422 }
status 200
form_class.new(work_package, current_user: current_user, errors: contract.errors)
form_class.new(work_package, current_user: current_user, errors: api_errors)
else
fail api_error
fail ::API::Errors::MultipleErrors.create_if_many(api_errors)
end
end
end
spec/lib/api/v3/work_packages/form_representer_spec.rb
+4 -14
View File
@@ -31,7 +31,7 @@ require 'spec_helper'
describe ::API::V3::WorkPackages::FormRepresenter do
include API::V3::Utilities::PathHelper
let(:errors) { Hash.new }
let(:errors) { [] }
let(:work_package) {
FactoryGirl.build(:work_package,
id: 42,
@@ -58,23 +58,13 @@ describe ::API::V3::WorkPackages::FormRepresenter do
context 'with errors' do
let(:subject_error_message) { 'Subject can\'t be blank!' }
let(:status_error_message) { 'Status can\'t be blank!' }
let(:errors) {
{ subject: [subject_error_message], status: [status_error_message] }
}
let(:subject_error) { ::API::Errors::Validation.new(subject_error_message) }
let(:status_error) { ::API::Errors::Validation.new(status_error_message) }
let(:errors) { [subject_error, status_error] }
let(:subject_error) { ::API::Errors::Validation.new(:subject, subject_error_message) }
let(:status_error) { ::API::Errors::Validation.new(:status, status_error_message) }
let(:api_subject_error) { ::API::V3::Errors::ErrorRepresenter.new(subject_error) }
let(:api_status_error) { ::API::V3::Errors::ErrorRepresenter.new(status_error) }
let(:api_errors) { { subject: api_subject_error, status: api_status_error } }
before do
allow(work_package).to receive(:errors).and_return(errors)
allow(errors).to(
receive(:full_message).with(:subject, anything).and_return(subject_error_message))
allow(errors).to(
receive(:full_message).with(:status, anything).and_return(status_error_message))
end
it { is_expected.to be_json_eql(api_errors.to_json).at_path('_embedded/validationErrors') }
end
end