rework error instantiation

- 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

config/initializers/10-patches.rb

@@ -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

@@ -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.
+        # Used to read it otherwise.
+        def identifier(identifier = nil)
+          @identifier = identifier if identifier
+
+          @identifier
+        end
+
+        private
 
-      ##
-      # Allows defining this error class's identifier once.
-      # Used to read it otherwise.
-      def self.identifier(identifier = nil)
-        @identifier ||= identifier
+        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

@@ -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

@@ -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

@@ -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

@@ -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

@@ -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

@@ -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

@@ -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

@@ -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

@@ -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

@@ -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

@@ -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

@@ -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

@@ -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