migrate exercise show and update to ActiveStorage

kkoehn · kkoehn · commit ccf63fec051e · 2025-09-06T22:38:10.000+02:00
diff --git a/app/controllers/code_ocean/files_controller.rb b/app/controllers/code_ocean/files_controller.rb
@@ -25,23 +25,23 @@ def show_protected_upload
       @file = CodeOcean::File.find(params[:id])
       authorize!
       # The `@file.name_with_extension` is assembled based on the user-selected file type, not on the actual file name stored on disk.
-      raise Pundit::NotAuthorizedError if @embed_options[:disable_download] || @file.filepath != params[:filename] || @file.native_file.blank?
+      raise Pundit::NotAuthorizedError if @embed_options[:disable_download] || @file.filepath != params[:filename] || @file.attachment.blank?
 
-      real_location = Pathname(@file.native_file.current_path).realpath
-      send_file(real_location, type: 'application/octet-stream', filename: @file.name_with_extension, disposition: 'attachment')
+      url = rails_blob_path(@file.attachment, disposition: 'attachment', expires_in: 5.minutes)
+      redirect_to url, allow_other_host: true
     end
 
     def render_protected_upload
       # Set @current_user with a new *learner* for Pundit checks
       @current_user = ExternalUser.new
 
       @file = authorize AuthenticatedUrlHelper.retrieve!(CodeOcean::File, request)
-
       # The `@file.name_with_extension` is assembled based on the user-selected file type, not on the actual file name stored on disk.
-      raise Pundit::NotAuthorizedError unless @file.filepath == params[:filename] || @file.native_file.present?
+      raise Pundit::NotAuthorizedError unless @file.filepath == params[:filename] || @file.attachment.present?
+
+      url = rails_blob_path(@file.attachment, disposition: 'inline', expires_in: 5.minutes)
 
-      real_location = Pathname(@file.native_file.current_path).realpath
-      send_file(real_location, type: @file.native_file.content_type, filename: @file.name_with_extension)
+      redirect_to url, allow_other_host: true
     end
 
     def create
diff --git a/app/controllers/concerns/file_parameters.rb b/app/controllers/concerns/file_parameters.rb
@@ -33,8 +33,8 @@ def reject_illegal_file_attributes(exercise, params)
   private :reject_illegal_file_attributes
 
   def file_attributes
-    %w[content context_id feedback_message file_id file_type_id hidden id name native_file path read_only role weight
-       file_template_id hidden_feedback]
+    %w[content context_id feedback_message file_id file_type_id hidden id name path read_only role weight
+       file_template_id hidden_feedback attachment]
   end
   private :file_attributes
 end
diff --git a/app/controllers/exercises_controller.rb b/app/controllers/exercises_controller.rb
@@ -304,12 +304,12 @@ def exercise_params_with_tags
   def handle_file_uploads
     if exercise_params
       exercise_params[:files_attributes].try(:each) do |_index, file_attributes|
-        if file_attributes[:content].respond_to?(:read)
+        if file_attributes[:attachment].respond_to?(:read)
           if FileType.find_by(id: file_attributes[:file_type_id]).try(:binary?)
-            file_attributes[:native_file] = file_attributes[:content]
             file_attributes[:content] = nil
           else
-            file_attributes[:content] = file_attributes[:content].read.detect_encoding!.encode.delete("\x00")
+            file_attributes[:content] = file_attributes[:attachment].read.detect_encoding!.encode.delete("\x00")
+            file_attributes[:attachment] = nil
           end
         end
       end
diff --git a/app/controllers/submissions_controller.rb b/app/controllers/submissions_controller.rb
@@ -65,7 +65,7 @@ def download
   def download_file
     raise Pundit::NotAuthorizedError if @embed_options[:disable_download]
 
-    if @file.native_file?
+    if @file.attachment.attached?
       redirect_to protected_upload_path(id: @file.id, filename: @file.filepath), status: :see_other
     else
       response.set_header('Content-Length', @file.size)
@@ -96,7 +96,7 @@ def render_file
     end
 
     # Finally grant access and send the file
-    if @file.native_file?
+    if @file.attachment.attached?
       url = render_protected_upload_url(id: @file.id, filename: @file.filepath)
       redirect_to AuthenticatedUrlHelper.sign(url, @file), status: :see_other
     else
diff --git a/app/models/code_ocean/file.rb b/app/models/code_ocean/file.rb
@@ -16,7 +16,6 @@ class File < ApplicationRecord
 
     after_initialize :set_default_values
     before_validation :clear_weight, unless: :teacher_defined_assessment?
-    before_validation :hash_content, if: :content_present?
     before_validation :set_ancestor_values, if: :incomplete_descendent?
 
     attr_writer :size
@@ -36,7 +35,7 @@ class File < ApplicationRecord
     has_many :events_synchronized_editor, class_name: 'Event::SynchronizedEditor'
     alias descendants files
 
-    mount_uploader :native_file, FileUploader
+    has_one_attached :attachment
 
     scope :editable, -> { where(read_only: false) }
     scope :visible, -> { where(hidden: false) }
@@ -50,7 +49,6 @@ class File < ApplicationRecord
 
     validates :feedback_message, if: :teacher_defined_assessment?, presence: true
     validates :feedback_message, absence: true, unless: :teacher_defined_assessment?
-    validates :hashed_content, if: :content_present?, presence: true
     validates :hidden, inclusion: [true, false]
     validates :hidden_feedback, inclusion: [true, false]
     validates :name, presence: true
@@ -73,21 +71,13 @@ class File < ApplicationRecord
     end
 
     def read
-      if native_file?
-        return nil unless native_file_location_valid?
-
-        native_file.read
+      if attachment.attached?
+        attachment.download
       else
         content
       end
     end
 
-    def native_file_location_valid?
-      real_location = Pathname(native_file.current_path).realpath
-      upload_location = Pathname(::File.join(native_file.root, 'uploads')).realpath
-      real_location.fnmatch? ::File.join(upload_location.to_s, '**')
-    end
-
     def ancestor_id
       file_id || id
     end
@@ -102,7 +92,7 @@ def teacher_defined_assessment?
     end
 
     def content_present?
-      content? || native_file?
+      content? || attachment.attached?
     end
     private :content_present?
 
@@ -122,11 +112,6 @@ def filepath_without_extension
       end
     end
 
-    def hash_content
-      self.hashed_content = Digest::MD5.new.hexdigest(read || '')
-    end
-    private :hash_content
-
     def incomplete_descendent?
       file_id.present? && file_type_id.blank?
     end
@@ -158,8 +143,8 @@ def visible?
     end
 
     def size
-      @size ||= if native_file?
-                  native_file.size
+      @size ||= if attachment.attached?
+                  attachment.byte_size
                 else
                   content.size
                 end
diff --git a/app/policies/code_ocean/file_policy.rb b/app/policies/code_ocean/file_policy.rb
@@ -7,8 +7,6 @@ def author?
     end
 
     def show?
-      return no_one if @record.native_file? && !@record.native_file_location_valid?
-
       if @record.context.is_a?(Exercise)
         admin? || author? || !@record.hidden
       else
@@ -17,8 +15,6 @@ def show?
     end
 
     def show_protected_upload?
-      return no_one if @record.native_file? && !@record.native_file_location_valid?
-
       if @record.context.is_a?(Exercise)
         admin? || author? || (!@record.context.unpublished && !@record.hidden)
       else
@@ -27,7 +23,6 @@ def show_protected_upload?
     end
 
     def render_protected_upload?
-      return no_one if @record.native_file? && !@record.native_file_location_valid?
       return no_one if @record.context.is_a?(Exercise) && (@record.context.unpublished || @record.hidden)
 
       # The AuthenticatedUrlHelper will check for more details, but we cannot determine a specific user
diff --git a/app/services/proforma_service/convert_exercise_to_task.rb b/app/services/proforma_service/convert_exercise_to_task.rb
@@ -211,7 +211,7 @@ def filename(file)
     end
 
     def add_content_to_task_file(file, task_file)
-      if file.native_file.present?
+      if file.attachment.attached?
         file_content = file.read
         task_file.content = file_content
         task_file.used_by_grader = false
diff --git a/app/services/proforma_service/convert_task_to_exercise.rb b/app/services/proforma_service/convert_task_to_exercise.rb
@@ -119,7 +119,7 @@ def codeocean_file_from_task_file(file, parent_object = nil)
         xml_id_path: (parent_object.nil? ? [file.id] : [parent_object.id, file.id]).map(&:to_s)
       )
       if file.binary
-        codeocean_file.native_file = FileIO.new(file.content.dup.force_encoding('UTF-8'), File.basename(file.filename))
+        codeocean_file.attachment.attach(io: StringIO.new(file.content.dup.force_encoding('UTF-8')), filename: file.filename)
       else
         codeocean_file.content = file.content
       end
diff --git a/app/views/exercises/_code_field.html.slim b/app/views/exercises/_code_field.html.slim
@@ -1,6 +1,6 @@
 .mb-3
-  = form.label(attribute, class: 'form-label')
-  = form.text_area(attribute, class: 'code-field form-control', rows: 16, style: 'display:none;')
+  = form.label('content', class: 'form-label')
+  = form.text_area('content', class: 'code-field form-control', rows: 16, style: 'display:none;')
   = render(partial: 'editor_edit', locals: {exercise: @exercise})
   .card.border-warning.p-2.my-2
     = form.file_field(attribute, class: 'form-control', style: 'display: inline-block;')
diff --git a/app/views/exercises/_editor_frame.html.slim b/app/views/exercises/_editor_frame.html.slim
@@ -10,7 +10,7 @@ div class=(defined?(own_solution) ? 'own-frame' : 'frame') data-executable=file.
         - elsif file.file_type.video?
           = video_tag(file_path, controls: true)
       - else
-        = link_to(file.native_file.file.filename, file_path)
+        = link_to(file.attachment.filename, file_path)
   - else
     .editor-content.d-none data-file-id=file.ancestor_id = file.content
     div class=(defined?(own_solution) ? 'own-editor' : 'editor') data-file-id=file.ancestor_id data-indent-size=file.file_type.indent_size data-mode=file.file_type.editor_mode data-allow-auto-completion=exercise.allow_auto_completion.to_s data-id=file.id
diff --git a/app/views/exercises/_file_form.html.slim b/app/views/exercises/_file_form.html.slim
@@ -48,4 +48,5 @@ li.card.mt-2
         .mb-3
           = f.label(:weight, class: 'form-label')
           = f.number_field(:weight, class: 'form-control', min: 0, step: 'any')
-      = render('code_field', attribute: :content, form: f)
+      = render('code_field', attribute: :attachment, form: f)
+      / = render('code_field', attribute: :content, form: f)
diff --git a/app/views/shared/_file.html.slim b/app/views/shared/_file.html.slim
@@ -8,4 +8,4 @@
   = row(label: 'code_ocean/file.hidden_feedback', value: file.hidden_feedback)
   = row(label: 'code_ocean/file.feedback_message', value: render_markdown(file.feedback_message), class: 'm-0')
   = row(label: 'code_ocean/file.weight', value: file.weight)
-= row(label: 'code_ocean/file.content', value: file.native_file? ? link_to_if(policy(file).show?, file.native_file.file.filename, protected_upload_path(id: file.id, filename: file.filepath)) : code_tag(file.content, file.file_type.programming_language))
+= row(label: 'code_ocean/file.content', value: file.attachment.attached? ? link_to_if(policy(file).show?, file.attachment.filename, protected_upload_path(id: file.id, filename: file.filepath)) : code_tag(file.content, file.file_type.programming_language))
diff --git a/config/application.rb b/config/application.rb
@@ -55,11 +55,6 @@ class Application < Rails::Application
 
     config.action_mailer.preview_paths << Rails.root.join('spec/mailers/previews')
 
-    # Specify default options for Rails generators
-    # config.generators do |g|
-    #   g.orm :active_record, primary_key_type: :uuid
-    # end
-
     # Allow tables in addition to existing default tags
     config.action_view.sanitized_allowed_tags = ActionView::Base.sanitized_allowed_tags + %w[table thead tbody tfoot td tr details summary]
 
diff --git a/db/migrate/20250827203150_create_active_storage_tables.active_storage.rb b/db/migrate/20250827203150_create_active_storage_tables.active_storage.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 # This migration comes from active_storage (originally 20170806125915)
 class CreateActiveStorageTables < ActiveRecord::Migration[7.0]
   def change
@@ -19,7 +21,7 @@ def change
         t.datetime :created_at, null: false
       end
 
-      t.index [ :key ], unique: true
+      t.index [:key], unique: true
     end
 
     create_table :active_storage_attachments, id: primary_key_type do |t|
@@ -33,25 +35,26 @@ def change
         t.datetime :created_at, null: false
       end
 
-      t.index [ :record_type, :record_id, :name, :blob_id ], name: :index_active_storage_attachments_uniqueness, unique: true
+      t.index %i[record_type record_id name blob_id], name: :index_active_storage_attachments_uniqueness, unique: true
       t.foreign_key :active_storage_blobs, column: :blob_id
     end
 
     create_table :active_storage_variant_records, id: primary_key_type do |t|
       t.belongs_to :blob, null: false, index: false, type: foreign_key_type
       t.string :variation_digest, null: false
 
-      t.index [ :blob_id, :variation_digest ], name: :index_active_storage_variant_records_uniqueness, unique: true
+      t.index %i[blob_id variation_digest], name: :index_active_storage_variant_records_uniqueness, unique: true
       t.foreign_key :active_storage_blobs, column: :blob_id
     end
   end
 
   private
-    def primary_and_foreign_key_types
-      config = Rails.configuration.generators
-      setting = config.options[config.orm][:primary_key_type]
-      primary_key_type = setting || :primary_key
-      foreign_key_type = setting || :bigint
-      [ primary_key_type, foreign_key_type ]
-    end
+
+  def primary_and_foreign_key_types
+    config = Rails.configuration.generators
+    setting = config.options[config.orm][:primary_key_type]
+    primary_key_type = setting || :primary_key
+    foreign_key_type = setting || :bigint
+    [primary_key_type, foreign_key_type]
+  end
 end
diff --git a/spec/controllers/code_ocean/files_controller_spec.rb b/spec/controllers/code_ocean/files_controller_spec.rb
@@ -19,11 +19,12 @@
         let(:file) { submission.collect_files.detect {|file| file.file_type.file_extension == '.mp4' } }
 
         expect_assigns(file: :file)
-        expect_content_type('application/octet-stream')
-        expect_http_status(:ok)
+        expect_redirect
 
-        it 'sets the correct filename' do
-          expect(response.headers['Content-Disposition']).to include("attachment; filename=\"#{file.name_with_extension}\"")
+        it 'redirects to ActiveStorage blob with correct filename' do
+          location = response.headers['Location'] || response.location
+          expect(location).to include(file.name_with_extension)
+          expect(location).to include('disposition=attachment')
         end
       end
     end
diff --git a/spec/controllers/exercises_controller_spec.rb b/spec/controllers/exercises_controller_spec.rb
@@ -91,15 +91,15 @@
       let(:perform_request) { proc { post :create, params: {exercise: exercise_attributes.merge(files_attributes:)} } }
 
       context 'when specifying the file content within the form' do
-        let(:files_attributes) { {'0' => build(:file).attributes.except('context_id', 'context_type', 'created_at', 'hashed_content', 'updated_at')} }
+        let(:files_attributes) { {'0' => build(:file).attributes.except('context_id', 'context_type', 'created_at', 'hashed_content', 'updated_at', 'native_file')} }
 
         it 'creates the file' do
           expect { perform_request.call }.to change(CodeOcean::File, :count)
         end
       end
 
       context 'when uploading a file' do
-        let(:files_attributes) { {'0' => build(:file, file_type:).attributes.except('context_id', 'context_type', 'created_at', 'hashed_content', 'updated_at').merge(content: uploaded_file)} }
+        let(:files_attributes) { {'0' => build(:file, file_type:).attributes.except('context_id', 'context_type', 'created_at', 'hashed_content', 'updated_at', 'native_file').merge(attachment: uploaded_file)} }
 
         context 'when uploading a binary file' do
           let(:file_path) { Rails.root.join('db/seeds/audio_video/devstories.mp4') }
@@ -112,7 +112,7 @@
 
           it 'assigns the native file' do
             perform_request.call
-            expect(Exercise.last.files.first.native_file).to be_a(FileUploader)
+            expect(assigns(:exercise).files.first.attachment).to be_attached
           end
         end
 
@@ -127,7 +127,7 @@
 
           it 'assigns the file content' do
             perform_request.call
-            expect(Exercise.last.files.first.content).to eq(File.read(file_path))
+            expect(assigns(:exercise).files.first.content).to eq(File.read(file_path))
           end
         end
       end
@@ -690,10 +690,17 @@
   describe 'POST #import_confirm' do
     let(:file_id) { 'file_id' }
     let(:post_request) { post :import_confirm, params: {file_id: file_id} }
+    let(:blob_double) { instance_double(ActiveStorage::Blob) }
+    let(:tempfile) { Tempfile.new }
+
+    before do
+      allow(ActiveStorage::Blob).to receive(:find_by).and_return(blob_double)
+      allow(blob_double).to receive(:open).and_yield(tempfile)
+    end
 
     context 'when the import is successful' do
       before do
-        allow(ProformaService::Import).to receive(:call).with(zip: 'mocked_file', user: user).and_return(exercise)
+        allow(ProformaService::Import).to receive(:call).with(zip: tempfile, user: user).and_return(exercise)
         allow(exercise).to receive(:save!).and_return(true)
       end
 
@@ -730,7 +737,7 @@
       it 'logs the error and renders internal error JSON' do
         post_request
 
-        expect(Sentry).to have_received(:capture_exception).with(instance_of(StandardError))
+        expect(Sentry).to have_received(:capture_exception).with(a_kind_of(StandardError))
         expect(response).to have_http_status(:ok)
         parsed_response = response.parsed_body
         expect(parsed_response['status']).to eq('failure')
diff --git a/spec/factories/code_ocean/file.rb b/spec/factories/code_ocean/file.rb
@@ -16,7 +16,13 @@ module CodeOcean
       trait(:image) do
         file_type factory: :dot_png
         name { 'poster' }
-        native_file { Rack::Test::UploadedFile.new(Rails.root.join('db/seeds/audio_video/poster.png'), 'image/png') }
+        after(:build) do |file|
+          file.attachment.attach(
+            io: Rails.root.join('db/seeds/audio_video/poster.png').open,
+            filename: 'poster.png',
+            content_type: 'image/png'
+          )
+        end
       end
     end
 
diff --git a/spec/factories/exercise.rb b/spec/factories/exercise.rb
diff --git a/spec/lib/runner/strategy/docker_container_pool_spec.rb b/spec/lib/runner/strategy/docker_container_pool_spec.rb
diff --git a/spec/models/code_ocean/file_spec.rb b/spec/models/code_ocean/file_spec.rb
diff --git a/spec/services/proforma_service/convert_task_to_exercise_spec.rb b/spec/services/proforma_service/convert_task_to_exercise_spec.rb
diff --git a/spec/system/editor_system_spec.rb b/spec/system/editor_system_spec.rb

Original file line number	Diff line number	Diff line change
`@@ -119,7 +119,7 @@ def codeocean_file_from_task_file(file, parent_object = nil)`
`119`	`119`	`xml_id_path: (parent_object.nil? ? [file.id] : [parent_object.id, file.id]).map(&:to_s)`
`120`	`120`	`)`
`121`	`121`	`if file.binary`
`122`		`- codeocean_file.native_file = FileIO.new(file.content.dup.force_encoding('UTF-8'), File.basename(file.filename))`
	`122`	`+ codeocean_file.attachment.attach(io: StringIO.new(file.content.dup.force_encoding('UTF-8')), filename: file.filename)`
`123`	`123`	`else`
`124`	`124`	`codeocean_file.content = file.content`
`125`	`125`	`end`