Merge pull request #4562 from sanger/Y24-190-refactor-processors

Y24-190: Refactor JSONAPI::Resources Processors

app/controllers/api/v2/qc_files_controller.rb

@@ -10,21 +10,32 @@ class QcFilesController < JSONAPI::ResourceController
     end
 
     class QcFileProcessor < JSONAPI::Processor
+      before_create_resource :prepare_temporary_file
+
+      private
+
+      def prepare_temporary_file
+        filename, contents = required_attributes
+        tempfile = create_temporary_file(filename, contents)
+
+        context.merge!(filename:, tempfile:)
+      end
+
       # Validate that attributes contains both the filename and the contents of the QcFile.
-      def validate_required_attributes(attributes)
-        errors = []
+      def required_attributes
+        attributes = params[:data][:attributes]
 
         filename = attributes[:filename]
-        errors += JSONAPI::Exceptions::ParameterMissing.new('filename').errors if filename.nil?
+        raise JSONAPI::Exceptions::ParameterMissing, 'filename' if filename.nil?
 
         contents = attributes[:contents]
-        errors += JSONAPI::Exceptions::ParameterMissing.new('contents').errors if contents.nil?
+        raise JSONAPI::Exceptions::ParameterMissing, 'contents' if contents.nil?
 
-        [filename, contents, errors]
+        [filename, contents]
       end
 
       # Create a temporary file with the contents.
-      def create_tempfile(filename, contents)
+      def create_temporary_file(filename, contents)
         # The filename for a Tempfile is passed as an array with the basename and extension.
         # e.g. [ 'file', '.csv' ]
         filename_components = [File.basename(filename, '.*'), File.extname(filename)]
@@ -33,24 +44,6 @@ def create_tempfile(filename, contents)
           return file
         end
       end
-
-      # Override the default behaviour for a JSONAPI::Processor when creating a new resource.
-      # We need to parse the filename and contents attributes so that we can generate a temporary file for the new
-      # QcFile model object. Replacing the fields with these values after the new QcFile was created does not generate
-      # the file correctly in the database. The CarrierWave library needs these values sooner.
-      def create_resource
-        data = params[:data]
-        attributes = data[:attributes]
-        filename, contents, errors = validate_required_attributes(attributes)
-
-        return JSONAPI::ErrorsOperationResult.new(JSONAPI::BAD_REQUEST, errors) unless errors.empty?
-
-        tempfile = create_tempfile(filename, contents)
-        resource = QcFileResource.create_with_tempfile(context, tempfile, filename)
-        result = resource.replace_fields(data)
-
-        JSONAPI::ResourceOperationResult.new((result == :completed ? :created : :accepted), resource)
-      end
     end
   end
 end

app/controllers/api/v2/tag_layouts_controller.rb

@@ -10,25 +10,37 @@ class TagLayoutsController < JSONAPI::ResourceController
     end
 
     class TagLayoutProcessor < JSONAPI::Processor
+      around_create_resource :apply_template
+
+      private
+
+      # We need to check whether a template UUID was given, and, if so, copy its data into this
+      # new TagLayoutResource. The creation will be prevented if any data from the template is also
+      # included in the create request as additional values. e.g. a request has a template UUID and
+      # also specifies a direction or a tag_group. In this case, the error will indicate that the
+      # template UUID was not an allowed attribute.
+      def apply_template
+        template = merge_template_data
+        yield
+        record_template_use(template) unless template.nil?
+      end
+
       def find_template
         template_uuid = params[:data][:attributes][:tag_layout_template_uuid]
         return nil if template_uuid.nil? # No errors -- we just don't have a template.
 
         template = TagLayoutTemplate.with_uuid(template_uuid).first
 
-        if template.nil?
-          yield JSONAPI::Exceptions::InvalidFieldValue.new(:tag_layout_template_uuid, template_uuid).errors
-        end
+        raise JSONAPI::Exceptions::InvalidFieldValue.new(:tag_layout_template_uuid, template_uuid) if template.nil?
 
         template
       end
 
-      def errors_if_key_present(data, key)
-        return [] if data[key.to_sym].blank?
+      def raise_if_key_present(data, key)
+        return if data[key.to_sym].blank?
 
-        JSONAPI::Exceptions::BadRequest.new(
-          "Cannot provide '#{key}' while also providing 'tag_layout_template_uuid'."
-        ).errors
+        raise JSONAPI::Exceptions::BadRequest,
+              "Cannot provide '#{key}' while also providing 'tag_layout_template_uuid'."
       end
 
       def merge_template_attributes(template)
@@ -37,7 +49,7 @@ def merge_template_attributes(template)
         %i[walking_by direction].each do |attr_key|
           next if template.send(attr_key).blank?
 
-          yield errors_if_key_present(data[:attributes], attr_key)
+          raise_if_key_present(data[:attributes], attr_key)
 
           data[:attributes][attr_key] = template.send(attr_key)
         end
@@ -49,16 +61,22 @@ def merge_template_to_one_relationships(template)
         %i[tag_group tag2_group].each do |rel_key|
           next if template.send(rel_key).blank?
 
-          yield errors_if_key_present(data[:attributes], "#{rel_key}_uuid")
-          yield errors_if_key_present(data[:to_one], rel_key)
+          raise_if_key_present(data[:attributes], "#{rel_key}_uuid")
+          raise_if_key_present(data[:to_one], rel_key)
 
           data[:to_one][rel_key] = template.send(rel_key).id
         end
       end
 
-      def merge_template_data(template, &)
-        merge_template_attributes(template, &)
-        merge_template_to_one_relationships(template, &)
+      def merge_template_data
+        template = find_template
+
+        unless template.nil?
+          merge_template_attributes(template)
+          merge_template_to_one_relationships(template)
+        end
+
+        template
       end
 
       def enforce_uniqueness?
@@ -69,30 +87,9 @@ def enforce_uniqueness?
         attributes.fetch(:enforce_uniqueness, tag2_group_present)
       end
 
-      def record_template_use(template, resource)
-        template&.record_template_use(TagLayout.find(resource.id).plate, enforce_uniqueness?)
-      end
-
-      # Override the default behaviour for a JSONAPI::Processor when creating a new resource.
-      # We need to check whether a template UUID was given, and, if so, copy its data into this
-      # new TagLayoutResource. The creation will be prevented if any data from the template is also
-      # included in the create request as additional values. e.g. a request has a template UUID and
-      # also specifies a direction or a tag_group. In this case, the error will indicate that the
-      # template UUID was not an allowed attribute.
-      def create_resource
-        errors = []
-        template = find_template { |new_errors| errors += new_errors }
-        merge_template_data(template) { |new_errors| errors += new_errors } unless template.nil?
-
-        return JSONAPI::ErrorsOperationResult.new(JSONAPI::BAD_REQUEST, errors) unless errors.empty?
-
-        # Perform the usual create actions.
-        resource = TagLayoutResource.create(context)
-        result = resource.replace_fields(params[:data])
-
-        record_template_use(template, resource)
-
-        JSONAPI::ResourceOperationResult.new((result == :completed ? :created : :accepted), resource)
+      def record_template_use(template)
+        plate = Plate.with_uuid(params.dig(:data, :attributes, :plate_uuid)).first
+        template.record_template_use(plate, enforce_uniqueness?)
       end
     end
   end

app/controllers/api/v2/transfers_controller.rb

@@ -10,40 +10,23 @@ class TransfersController < JSONAPI::ResourceController
     end
 
     class TransferProcessor < JSONAPI::Processor
-      # Get the Transfer model type needed to be created from the transfer template in the attributes.
-      # Also put the transfers from the template into the attributes if they exist.
-      def extract_template_data(attributes)
-        errors = []
+      before_create_resource :extract_template_data
+
+      private
 
+      # Put the Transfer model_type needed to be created from the transfer template in the context.
+      # Also put the transfers from the template into the attributes if they exist.
+      def extract_template_data
+        attributes = params[:data][:attributes]
         template_uuid = attributes[:transfer_template_uuid]
-        errors += JSONAPI::Exceptions::ParameterMissing.new('transfer_template_uuid').errors if template_uuid.nil?
+        raise JSONAPI::Exceptions::ParameterMissing, 'transfer_template_uuid' if template_uuid.nil?
 
         template = TransferTemplate.with_uuid(template_uuid).first
-        errors +=
-          JSONAPI::Exceptions::InvalidFieldValue.new('transfer_template_uuid', template_uuid).errors if template.nil?
-
-        return nil, errors if errors.present?
+        raise JSONAPI::Exceptions::InvalidFieldValue.new('transfer_template_uuid', template_uuid) if template.nil?
 
         # Modify attributes according to what we've found in the template.
         attributes[:transfers] = template.transfers if template.transfers.present?
-
-        [template.transfer_class_name.constantize, errors]
-      end
-
-      # Override the default behaviour for a JSONAPI::Processor when creating a new resource.
-      # We need to parse for a transfer_template_uuid attribute so that it can be used to determine the polymorphic
-      # type of the Transfer represented by a template. The create method is then passed the type to create.
-      def create_resource
-        data = params[:data]
-        attributes = data[:attributes]
-        model_type, errors = extract_template_data(attributes)
-
-        return JSONAPI::ErrorsOperationResult.new(JSONAPI::BAD_REQUEST, errors) unless errors.empty?
-
-        resource = TransferResource.create_with_model(context, model_type)
-        result = resource.replace_fields(data)
-
-        JSONAPI::ResourceOperationResult.new((result == :completed ? :created : :accepted), resource)
+        context[:model_type] = template.transfer_class_name.constantize
       end
     end
   end

app/resources/api/v2/qc_file_resource.rb

@@ -49,36 +49,10 @@ class QcFileResource < BaseResource
       #   @return [String] The content type, or MIME type, of the QC file.
       attribute :content_type, readonly: true
 
-      # @!attribute [rw] contents
-      #   @return [String] The String contents of the QC file.
-      #      This is usually the CSV data for the QC file.
-      #      This can only be written once on creation.
-      attribute :contents, write_once: true
-
-      def contents
-        # The contents comes from the uploaded_data managed by CarrierWave.
-        @model.current_data
-      end
-
-      def contents=(value)
-        # Do not update the model.
-        # File contents is set via the uploaded_data hash supplied during QcFile creation.
-      end
-
       # @!attribute [r] created_at
       #   @return [DateTime] The date and time the QC file was created.
       attribute :created_at, readonly: true
 
-      # @!attribute [rw] filename
-      #   @return [String] The filename of the QC file.
-      #      This can only be written once on creation.
-      attribute :filename, write_once: true
-
-      def filename=(value)
-        # Do not update the model.
-        # Filename is set via the uploaded_data hash supplied during QcFile creation.
-      end
-
       # @!attribute [r] size
       #   @return [Integer] The size of the QC file in bytes.
       attribute :size, readonly: true
@@ -109,16 +83,26 @@ def filename=(value)
       # Create method
       ###
 
-      # @!method create_with_tempfile
-      #   Create a new QcFile resource with the uploaded data from a temporary file. This is called by the controller
-      #   when a create request for a QcFile is made. It ensures the contents of the file have been written to a
-      #   new TempFile instance.
-      # @param context [Hash] The context for the request.
-      # @param tempfile [Tempfile] A temporary file containing the uploaded data.
-      # @param filename [String] The filename for the uploaded data.
-      # @return [QcFileResource] The new QcFile resource.
-      def self.create_with_tempfile(context, tempfile, filename)
-        opts = { uploaded_data: { tempfile:, filename: } }
+      # @!attribute [rw] contents
+      #   @return [String] The String contents of the QC file.
+      #      This is usually the CSV data for the QC file.
+      #      This can only be written once on creation.
+      attribute :contents, write_once: true
+      attr_writer :contents # Do not store the value on the model. It is stored by the CarrierWave gem via a Tempfile.
+
+      def contents
+        # The contents comes from the uploaded_data managed by CarrierWave.
+        @model.current_data
+      end
+
+      # @!attribute [rw] filename
+      #   @return [String] The filename of the QC file.
+      #      This can only be written once on creation.
+      attribute :filename, write_once: true
+      attr_writer :filename # Do not store the value on the model. This value is consumed by the QcFileProcessor.
+
+      def self.create(context)
+        opts = { uploaded_data: context.slice(:filename, :tempfile) }
         new(QcFile.new(opts), context)
       end
     end

app/resources/api/v2/tag_layout_resource.rb

@@ -25,17 +25,6 @@ class TagLayoutResource < BaseResource
       #   @note This attribute is required.
       attribute :direction
 
-      # @!attribute [w] enforce_uniqueness
-      #   A flag indicating whether to set `enforce_uniqueness` on {TagLayout::TemplateSubmission}s when a template is
-      #   used to create the TagLayout.
-      #   @param value [Boolean] Whether to enforce uniqueness within template submissions.
-      attribute :enforce_uniqueness, writeonly: true
-
-      def enforce_uniqueness=(value)
-        # Do not update the model.
-        # This value is used by the controller if a template UUID was given and is not used by the TagLayout directly.
-      end
-
       # @!attribute [rw] initial_tag
       #   An offset for the tag set indicating which tag to start with in the layout.
       #   @return [Integer]
@@ -94,18 +83,6 @@ def tag2_group_uuid=(value)
         @model.tag2_group = TagGroup.with_uuid(value).first
       end
 
-      # @!attribute [w] tag_layout_template_uuid
-      #   @param value [String] the UUID of a TagLayoutTemplate to use for attributes of this TagLayout resource.
-      #     Providing this UUID while also providing values for attributes and relationships which can be extracted from
-      #     a {TagLayoutTemplateResource} will generate an error indicating that the UUID should not have been provided.
-      attribute :tag_layout_template_uuid, writeonly: true
-
-      def tag_layout_template_uuid=(value)
-        # Do not update the model.
-        # This value is used by the controller to apply request data to the TagLayout from the indicated template.
-        # It is not stored on the Transfer model.
-      end
-
       # @!attribute [rw] tags_per_well
       #   The number of tags in each well.
       #   This is only used and/or returned by specific tag layout {walking_by} algorithms.
@@ -165,6 +142,27 @@ def user_uuid=(value)
       #   @return [Api::V2::UserResource] The user who initiated this state change.
       #   @note This relationship is required.
       has_one :user
+
+      ###
+      # Template attributes
+      ###
+
+      # These are consumed by the TagLayoutProcessor and not a concern of the resource.
+      # They are included here to allow their presence in the JSON:API request body and to document their use cases.
+
+      # @!attribute [w] tag_layout_template_uuid
+      #   @param value [String] the UUID of a TagLayoutTemplate to use for attributes of this TagLayout resource.
+      #     Providing this UUID while also providing values for attributes and relationships which can be extracted from
+      #     a {TagLayoutTemplateResource} will generate an error indicating that the UUID should not have been provided.
+      attribute :tag_layout_template_uuid, writeonly: true
+      attr_writer :tag_layout_template_uuid # Not stored on the model
+
+      # @!attribute [w] enforce_uniqueness
+      #   A flag indicating whether to set `enforce_uniqueness` on {TagLayout::TemplateSubmission}s when a template is
+      #   used to create the TagLayout.
+      #   @param value [Boolean] Whether to enforce uniqueness within template submissions.
+      attribute :enforce_uniqueness, writeonly: true
+      attr_writer :enforce_uniqueness # Not stored on the model
     end
   end
 end