fix graphql error messages (#669)

* fix create sample mutation error messages

* Add rails route default port for dev envs, fixes generated upload urls

* Fix missing file in blob causing 500 error on attachFilesToSample graphql query

* update graphql schema

* Fix error outputs for graphql updateSampleMetadata

* update create sample mutation to user_error_type

* Update attach file to sample mutation to user error type

* Fix error messages for UpdateSampleMetadata mutations

* fix and add tests for update_sample_metadata

* delineate between metadata that is not_updated and metadata that is unchanged

* Add test to check that unchanged fields do not update provenance

* fix error on createDirectUpload mutation

* fix broken tests

* Downcase metadata fields on update service

* fix samples tests

* Add migration for downcasing metadata

* fix migration

* change description on create sample mutation to be optional

* Change RuntimeErrors to CoercionErrors

* refactor create_sample

* Fixing more errors

* handle RecordNotFound exceptions at schema level

* added test case for update sample metadata

* move exception handling into rescue_from block

* use find_by! to be consistent

* refactor common methods into base mutation

app/graphql/irida_schema.rb

@@ -52,7 +52,9 @@ def self.execute(query_str = nil, **kwargs)
 
   # Return a string UUID for `object`
   def self.id_from_object(object, _type = nil, _ctx = nil)
-    raise "#{object} does not implement `to_global_id`." unless object.respond_to?(:to_global_id)
+    unless object.respond_to?(:to_global_id)
+      raise GraphQL::CoercionError, "#{object} does not implement `to_global_id`."
+    end
 
     object.to_global_id
   end
@@ -69,10 +71,10 @@ def self.parse_gid(global_id, ctx = {})
     expected_types = Array(ctx[:expected_type])
     gid = GlobalID.parse(global_id)
 
-    raise "#{global_id} is not a valid IRIDA Next ID." unless gid
+    raise GraphQL::CoercionError, "#{global_id} is not a valid IRIDA Next ID." if !gid || gid.app != GlobalID.app
 
     if expected_types.any? && expected_types.none? { |type| gid.model_class.ancestors.include?(type) }
-      raise "#{global_id} is not a valid ID for #{expected_types.join(', ')}"
+      raise GraphQL::CoercionError, "#{global_id} is not a valid ID for #{expected_types.join(', ')}"
     end
 
     gid
@@ -106,4 +108,8 @@ def self.unauthorized_field(error)
   rescue_from(ActionPolicy::AuthorizationContextMissing) do
     raise GraphQL::ExecutionError, 'Unable to access object while accessing the API in guest mode'
   end
+
+  rescue_from(ActiveRecord::RecordNotFound) do |exception|
+    raise GraphQL::CoercionError, exception
+  end
 end

app/graphql/mutations/attach_files_to_sample.rb

@@ -13,47 +13,79 @@ class AttachFilesToSample < BaseMutation
              description: 'Persistent Unique Identifier of the sample. For example, `INXT_SAM_AAAAAAAAAA`.'
     validates required: { one_of: %i[sample_id sample_puid] }
 
-    field :errors, GraphQL::Types::JSON, null: false, description: 'Errors that prevented the mutation.'
-    field :sample, Types::SampleType, null: false, description: 'The updated sample.'
-    field :status, GraphQL::Types::JSON, null: false, description: 'The status of the mutation.'
-
-    def resolve(args)
-      sample = if args[:sample_id]
-                 IridaSchema.object_from_id(args[:sample_id], { expected_type: Sample })
-               else
-                 Sample.find_by(puid: args[:sample_puid])
-               end
+    field :errors, [Types::UserErrorType], null: false, description: 'A list of errors that prevented the mutation.'
+    field :sample, Types::SampleType, null: true, description: 'The updated sample.'
+    field :status, GraphQL::Types::JSON, null: true, description: 'The status of the mutation.'
+
+    def resolve(args) # rubocop:disable Metrics/MethodLength
+      sample = get_sample_from_id_or_puid_args(args)
+
+      if sample.nil?
+        return {
+          sample:,
+          status: nil,
+          errors: [{ path: ['sample'], message: 'not found by provided ID or PUID' }]
+        }
+      end
+
       files_attached = Attachments::CreateService.new(current_user, sample, { files: args[:files] }).execute
 
-      status, errors = attachment_status_and_errors(files_attached)
-      errors['query'] = sample.errors.full_messages if sample.errors.count.positive?
+      status, user_errors = attachment_status_and_errors(files_attached:, file_blob_id_list: args[:files])
+
+      # query level errors
+      sample.errors.each do |error|
+        user_errors.append(
+          {
+            path: ['sample', error.attribute.to_s.camelize(:lower)],
+            message: error.message
+          }
+        )
+      end
 
       {
         sample:,
         status:,
-        errors:
+        errors: user_errors
       }
     end
 
-    def attachment_status_and_errors(files_attached)
-      status = {}
-      errors = {}
+    def ready?(**_args)
+      authorize!(to: :mutate?, with: GraphqlPolicy, context: { user: context[:current_user], token: context[:token] })
+    end
+
+    private
+
+    def attachment_status_and_errors(files_attached:, file_blob_id_list:)
+      # initialize status hash such that all blob ids given by user are included
+      status = Hash[*file_blob_id_list.collect { |v| [v, nil] }.flatten]
+      user_errors = []
 
       files_attached.each do |attachment|
         id = attachment.file.blob.signed_id
         if attachment.persisted?
           status[id] = :success
         else
           status[id] = :error
-          errors[id] = attachment.errors.full_messages
+          attachment.errors.each do |error|
+            user_errors.append({ path: ['attachment', id], message: error.message })
+          end
         end
       end
 
-      [status, errors]
+      add_missing_blob_id_error(status:, user_errors:)
     end
 
-    def ready?(**_args)
-      authorize!(to: :mutate?, with: GraphqlPolicy, context: { user: context[:current_user], token: context[:token] })
+    def add_missing_blob_id_error(status:, user_errors:)
+      # any nil status is an error
+      status.each do |id, value|
+        next unless value.nil?
+
+        status[id] = :error
+        user_errors.append({ path: ['blob_id', id],
+                             message: 'Blob id could not be processed. Blob id is invalid or file is missing.' })
+      end
+
+      [status, user_errors]
     end
   end
 end

app/graphql/mutations/base_mutation.rb

@@ -9,5 +9,28 @@ class BaseMutation < GraphQL::Schema::RelayClassicMutation
     field_class Types::BaseField
     input_object_class Types::BaseInputObject
     object_class Types::BaseObject
+
+    protected
+
+    def get_sample_from_id_or_puid_args(args)
+      if args[:sample_id]
+        IridaSchema.object_from_id(args[:sample_id], { expected_type: Sample })
+      else
+        Sample.find_by!(puid: args[:sample_puid])
+      end
+    rescue ActiveRecord::RecordNotFound
+      nil
+    end
+
+    def get_project_from_id_or_puid_args(args)
+      if args[:project_id]
+        IridaSchema.object_from_id(args[:project_id], { expected_type: Project })
+      else
+        project_namespace = Namespaces::ProjectNamespace.find_by!(puid: args[:project_puid])
+        project_namespace&.project
+      end
+    rescue ActiveRecord::RecordNotFound
+      nil
+    end
   end
 end

app/graphql/mutations/create_direct_upload.rb

@@ -5,7 +5,8 @@ module Mutations
   class CreateDirectUpload < BaseMutation
     description 'Create blob to upload data to.'
 
-    argument :byte_size, Int, 'File size (bytes)', required: true
+    argument :byte_size, Int, 'File size (bytes)', required: true,
+                                                   validates: { numericality: { greater_than: 0 } }
     argument :checksum, String, 'MD5 file checksum as base64', required: true
     argument :content_type, String, 'File content type', required: true # rubocop:disable GraphQL/ExtractInputType
     argument :filename, String, 'Original file name', required: true # rubocop:disable GraphQL/ExtractInputType

app/graphql/mutations/create_sample.rb

@@ -5,7 +5,7 @@ module Mutations
   class CreateSample < BaseMutation
     null true
     description 'Create a new sample within an existing project.'
-    argument :description, String, description: 'The description to give the sample.'
+    argument :description, String, required: false, description: 'The description to give the sample.'
     argument :name, String, required: true, description: 'The name to give the sample.'
     argument :project_id, ID, # rubocop:disable GraphQL/ExtractInputType
              required: false,
@@ -15,37 +15,54 @@ class CreateSample < BaseMutation
              description: 'Persistent Unique Identifier of the project. For example, `INXT_PRJ_AAAAAAAAAA`.'
     validates required: { one_of: %i[project_id project_puid] }
 
-    field :errors, [String], null: false, description: 'A list of errors that prevented the mutation.'
+    field :errors, [Types::UserErrorType], null: false, description: 'A list of errors that prevented the mutation.'
     field :sample, Types::SampleType, description: 'The newly created sample.'
 
-    def resolve(args) # rubocop:disable Metrics/MethodLength
-      project = if args[:project_id]
-                  IridaSchema.object_from_id(args[:project_id], { expected_type: Project })
-                else
-                  Namespaces::ProjectNamespace.find_by!(puid: args[:project_puid]).project
-                end
-      sample = Samples::CreateService.new(current_user, project,
-                                          { name: args[:name], description: args[:description] }).execute
+    def resolve(args)
+      project = get_project_from_id_or_puid_args(args)
+
+      if project.nil? || !project.persisted?
+        user_errors = [{
+          path: ['project'],
+          message: 'Project not found by provided ID or PUID'
+        }]
+        return {
+          sample: nil,
+          errors: user_errors
+        }
+      end
+
+      create_sample(project, args)
+    end
+
+    def ready?(**_args)
+      authorize!(to: :mutate?, with: GraphqlPolicy, context: { user: context[:current_user], token: context[:token] })
+    end
+
+    private
+
+    def create_sample(project, args) # rubocop:disable Metrics/MethodLength
+      sample = Samples::CreateService.new(
+        current_user, project, { name: args[:name], description: args[:description] }
+      ).execute
+
       if sample.persisted?
         {
           sample:,
           errors: []
         }
       else
+        user_errors = sample.errors.map do |error|
+          {
+            path: ['sample', error.attribute.to_s.camelize(:lower)],
+            message: error.message
+          }
+        end
         {
           sample: nil,
-          errors: sample.errors.full_messages
+          errors: user_errors
         }
       end
-    rescue ActiveRecord::RecordNotFound
-      {
-        sample: nil,
-        errors: ['Project not found by provided ID or PUID']
-      }
-    end
-
-    def ready?(**_args)
-      authorize!(to: :mutate?, with: GraphqlPolicy, context: { user: context[:current_user], token: context[:token] })
     end
   end
 end

app/graphql/mutations/update_sample_metadata.rb

@@ -13,22 +13,64 @@ class UpdateSampleMetadata < BaseMutation
              description: 'Persistent Unique Identifier of the sample. For example, `INXT_SAM_AAAAAAAAAA`.'
     validates required: { one_of: %i[sample_id sample_puid] }
 
-    field :errors, [String], null: false, description: 'A list of errors that prevented the mutation.'
-    field :sample, Types::SampleType, null: false, description: 'The updated sample.'
-    field :status, GraphQL::Types::JSON, null: false, description: 'The status of the mutation.'
-
-    def resolve(args)
-      sample = if args[:sample_id]
-                 IridaSchema.object_from_id(args[:sample_id], { expected_type: Sample })
-               else
-                 Sample.find_by(puid: args[:sample_puid])
-               end
+    field :errors, [Types::UserErrorType], description: 'A list of errors that prevented the mutation.'
+    field :sample, Types::SampleType, null: true, description: 'The updated sample.'
+    field :status, GraphQL::Types::JSON, null: true, description: 'The status of the mutation.'
+
+    def resolve(args) # rubocop:disable Metrics/MethodLength,Metrics/AbcSize
+      sample = get_sample_from_id_or_puid_args(args)
+
+      if sample.nil?
+        user_errors = [{
+          path: ['sample'],
+          message: 'not found by provided ID or PUID'
+        }]
+        return {
+          sample:,
+          status: nil,
+          errors: user_errors
+        }
+      end
+
+      metadata = args[:metadata]
+      # convert string to hash if json string as given
+      metadata = JSON.parse(metadata) if metadata.is_a?(String)
+
+      unless metadata.is_a?(Hash)
+        user_errors = [{
+          path: ['metadata'],
+          message: 'is not JSON data'
+        }]
+        return {
+          sample:,
+          status: nil,
+          errors: user_errors
+        }
+      end
+
       metadata_changes = Samples::Metadata::UpdateService.new(sample.project, sample, current_user,
-                                                              { 'metadata' => args[:metadata] }).execute
+                                                              { 'metadata' => metadata }).execute
+
+      user_errors = sample.errors.map do |error|
+        {
+          path: ['sample', error.attribute.to_s.camelize(:lower)],
+          message: error.message
+        }
+      end
       {
         sample:,
         status: metadata_changes,
-        errors: sample.errors.full_messages
+        errors: user_errors
+      }
+    rescue JSON::ParserError => e
+      user_errors = [{
+        path: ['metadata'],
+        message: "JSON data is not formatted correctly. #{e.message}"
+      }]
+      {
+        sample: nil,
+        status: nil,
+        errors: user_errors
       }
     end