Issues/1263 scope assets to project in scratch

app/controllers/api/scratch/assets_controller.rb

@@ -6,26 +6,60 @@ class AssetsController < ScratchController
       include ActiveStorage::SetCurrent
 
       skip_before_action :authorize_user, only: [:show]
-      skip_before_action :check_scratch_feature, only: [:show]
+      prepend_before_action :load_project_from_header, only: %i[show create]
 
       def show
         filename_with_extension = "#{params[:id]}.#{params[:format]}"
-        scratch_asset = ScratchAsset.find_by!(filename: filename_with_extension)
-        redirect_to scratch_asset.file.url(content_type: scratch_asset.file.content_type), allow_other_host: true
+        authorize! :show, @project_from_header
+
+        scratch_asset = ScratchAsset.find_visible_to_project(
+          project: @project_from_header,
+          user: current_user,
+          filename: filename_with_extension
+        )
+        raise ActiveRecord::RecordNotFound, 'Not Found' unless scratch_asset
+
+        redirect_to scratch_asset.file.url(content_type: scratch_asset.response_content_type), allow_other_host: true
       end
 
       def create
-        begin
-          filename_with_extension = "#{params[:id]}.#{params[:format]}"
-          ScratchAsset.find_or_create_by!(filename: filename_with_extension) do |a|
-            a.file.attach(io: request.body, filename: filename_with_extension)
+        authorize! :show, @project_from_header
+
+        filename_with_extension = "#{params[:id]}.#{params[:format]}"
+        scratch_asset = ScratchAsset.find_or_initialize_by(
+          project: @project_from_header,
+          uploaded_user_id: current_user.id,
+          filename: filename_with_extension
+        )
+
+        if scratch_asset.new_record?
+          begin
+            scratch_asset.save!
+            scratch_asset.file.attach(io: request.body, filename: filename_with_extension)
+          rescue ActiveRecord::RecordNotUnique
+            logger.info("Scratch asset already created during concurrent upload: #{filename_with_extension}")
+            ScratchAsset.find_by!(
+              project: @project_from_header,
+              uploaded_user_id: current_user.id,
+              filename: filename_with_extension
+            )
           end
-        rescue ActiveRecord::RecordNotUnique => e
-          logger.error(e)
         end
 
         render json: { status: 'ok', 'content-name': params[:id] }, status: :created
       end
+
+      private
+
+      def load_project_from_header
+        identifier = request.headers['X-Project-ID']
+        return render json: { error: 'X-Project-ID header is required' }, status: :bad_request if identifier.blank?
+
+        @project_from_header = Project.find_by!(
+          identifier:,
+          project_type: Project::Types::CODE_EDITOR_SCRATCH
+        )
+      end
     end
   end
 end

app/controllers/api/scratch/projects_controller.rb

@@ -3,14 +3,16 @@
 module Api
   module Scratch
     class ProjectsController < ScratchController
+      include RemixSelection
+
       skip_before_action :authorize_user, only: [:show]
       skip_before_action :check_scratch_feature, only: [:show]
       before_action :load_project, only: %i[show update]
 
       before_action :ensure_create_is_a_remix, only: %i[create]
 
       def show
-        render json: @project.scratch_component.content
+        render json: scratch_project_content_with_stage_first(@project.scratch_component.content.to_h)
       end
 
       def create
@@ -20,6 +22,16 @@ def create
         remix_params = create_params
         return render json: { error: I18n.t('errors.project.remixing.invalid_params') }, status: :bad_request if remix_params.dig(:scratch_component, :content).blank?
 
+        existing_remix = remix_for_user(original_project, current_user)
+        if existing_remix
+          scratch_component = existing_remix.scratch_component || existing_remix.build_scratch_component
+          scratch_component.content = scratch_content_params
+          existing_remix.save!
+          reassign_uploaded_scratch_assets(original_project:, remix_project: existing_remix)
+
+          return render json: { status: 'ok', 'content-name': existing_remix.identifier }, status: :ok
+        end
+
         remix_origin = request.origin || request.referer
 
         result = Project::CreateRemix.call(
@@ -30,6 +42,7 @@ def create
         )
 
         if result.success?
+          reassign_uploaded_scratch_assets(original_project:, remix_project: result[:project])
           render json: { status: 'ok', 'content-name': result[:project].identifier }, status: :ok
         else
           render json: { error: result[:error] }, status: :bad_request
@@ -68,6 +81,55 @@ def load_original_project(identifier)
       def scratch_content_params
         params.slice(:meta, :targets, :monitors, :extensions).to_unsafe_h
       end
+
+      def scratch_project_content_with_stage_first(content)
+        targets = content['targets']
+        return content unless targets.is_a?(Array)
+
+        # Scratch expects the stage target first and can fail to load otherwise.
+        stage_targets, other_targets = targets.partition do |target|
+          target.is_a?(Hash) && (target['isStage'] || target[:isStage])
+        end
+        return content if stage_targets.empty?
+
+        content.merge('targets' => stage_targets + other_targets)
+      end
+
+      def reassign_uploaded_scratch_assets(original_project:, remix_project:)
+        uploaded_user_id = current_user.id
+        return if skip_scratch_asset_reassignment?(
+          original_project:,
+          remix_project:,
+          uploaded_user_id:
+        )
+
+        ScratchAsset.where(project: original_project, uploaded_user_id:).find_each do |source_asset|
+          reassign_uploaded_scratch_asset(
+            source_asset:,
+            remix_project:,
+            uploaded_user_id:
+          )
+        end
+      rescue StandardError => e
+        Sentry.capture_exception(e)
+      end
+
+      def skip_scratch_asset_reassignment?(original_project:, remix_project:, uploaded_user_id:)
+        original_project.blank? ||
+          remix_project.blank? ||
+          uploaded_user_id.blank? ||
+          original_project.id == remix_project.id
+      end
+
+      def reassign_uploaded_scratch_asset(source_asset:, remix_project:, uploaded_user_id:)
+        if ScratchAsset.exists?(project: remix_project, uploaded_user_id:, filename: source_asset.filename)
+          source_asset.destroy!
+        else
+          source_asset.update!(project: remix_project)
+        end
+      rescue ActiveRecord::RecordNotUnique
+        source_asset.destroy!
+      end
     end
   end
 end

app/models/project.rb

@@ -86,6 +86,20 @@ def scratch_project?
     project_type == Types::CODE_EDITOR_SCRATCH
   end
 
+  def self_and_ancestors
+    projects = []
+    current_project = self
+    seen_ids = []
+
+    while current_project && seen_ids.exclude?(current_project.id)
+      projects << current_project
+      seen_ids << current_project.id
+      current_project = current_project.parent
+    end
+
+    projects
+  end
+
   private
 
   def check_unique_not_null

app/models/scratch_asset.rb

@@ -1,7 +1,47 @@
 # frozen_string_literal: true
 
 class ScratchAsset < ApplicationRecord
-  validates :filename, presence: true, uniqueness: true
-
+  belongs_to :project, optional: true
   has_one_attached :file
+
+  validates :filename, presence: true, uniqueness: { scope: %i[project_id uploaded_user_id] }
+  validates :uploaded_user_id, absence: true, if: :global?
+  validates :uploaded_user_id, presence: true, unless: :global?
+  validate :belongs_to_scratch_project
+
+  scope :global_assets, -> { where(project_id: nil, uploaded_user_id: nil) }
+
+  def self.find_visible_to_project(project:, user:, filename:)
+    lineage_projects = project.self_and_ancestors
+    current_user_id = user&.id
+    assets_by_project_id = where(filename:, project_id: lineage_projects.map(&:id)).group_by(&:project_id)
+
+    lineage_projects.each do |lineage_project|
+      project_assets_by_user_id = assets_by_project_id.fetch(lineage_project.id, []).index_by(&:uploaded_user_id)
+      visible_asset = project_assets_by_user_id[current_user_id] || project_assets_by_user_id[lineage_project.user_id]
+      return find(visible_asset.id) if visible_asset
+    end
+
+    global_assets.find_by(filename:)
+  end
+
+  def global?
+    project_id.nil?
+  end
+
+  def response_content_type
+    extension_is_svg = File.extname(filename).casecmp('.svg').zero?
+    return 'image/svg+xml' if global? && extension_is_svg
+    return 'application/octet-stream' if extension_is_svg
+
+    Marcel::MimeType.for(name: filename) || 'application/octet-stream'
+  end
+
+  private
+
+  def belongs_to_scratch_project
+    return if project.blank? || project.scratch_project?
+
+    errors.add(:project, 'must be a Scratch project')
+  end
 end

db/migrate/20260410110000_scope_scratch_assets_to_projects.rb

@@ -0,0 +1,37 @@
+# frozen_string_literal: true
+
+class ScopeScratchAssetsToProjects < ActiveRecord::Migration[7.2]
+  disable_ddl_transaction!
+
+  def up
+    change_table :scratch_assets, bulk: true do |t|
+      t.uuid :project_id
+      t.uuid :uploaded_user_id
+    end
+
+    add_foreign_key :scratch_assets, :projects, column: :project_id
+
+    add_index :scratch_assets, :project_id, algorithm: :concurrently
+
+    add_index :scratch_assets,
+              %i[project_id uploaded_user_id filename],
+              unique: true,
+              where: 'project_id IS NOT NULL',
+              name: 'index_scratch_assets_on_project_uploaded_user_and_filename',
+              algorithm: :concurrently
+
+    add_index :scratch_assets,
+              :filename,
+              unique: true,
+              where: 'project_id IS NULL',
+              name: 'index_scratch_assets_on_global_filename',
+              algorithm: :concurrently
+
+    remove_index :scratch_assets, name: 'index_scratch_assets_on_filename', algorithm: :concurrently
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration,
+          'Scratch assets are scoped by project and uploader and cannot be safely collapsed back into one global row per filename'
+  end
+end

lib/scratch_asset_importer.rb

@@ -29,11 +29,13 @@ def import
   private
 
   def import_asset(asset_name)
-    return if ScratchAsset.exists?(filename: asset_name)
+    return if ScratchAsset.global_assets.exists?(filename: asset_name)
 
     sleep(ASSET_FETCHING_DELAY)
     asset = connection.get("#{asset_name}/get/")
-    ScratchAsset.create!(filename: asset_name).file.attach(io: StringIO.new(asset.body), filename: asset_name)
+    ScratchAsset.create!(filename: asset_name, project_id: nil, uploaded_user_id: nil)
+                .file
+                .attach(io: StringIO.new(asset.body), filename: asset_name)
   rescue StandardError => e
     Rails.logger.error("Failed to import asset #{asset_name}: #{e.message}")
   end

spec/factories/scratch_assets.rb

@@ -3,6 +3,8 @@
 FactoryBot.define do
   factory :scratch_asset do
     sequence(:filename) { Random.hex }
+    project { nil }
+    uploaded_user_id { project&.user_id }
 
     trait :with_file do
       transient { asset_path { file_fixture('test_image_1.png') } }

spec/features/scratch/creating_a_scratch_asset_spec.rb

@@ -5,32 +5,38 @@
 RSpec.describe 'Creating a Scratch asset', type: :request do
   let(:school) { create(:school) }
   let(:teacher) { create(:teacher, school:) }
+  let(:project) do
+    create(:project, project_type: Project::Types::CODE_EDITOR_SCRATCH, locale: nil, user_id: teacher.id).tap do |scratch_project|
+      create(:scratch_component, project: scratch_project)
+    end
+  end
   let(:auth_headers) { { 'Authorization' => UserProfileMock::TOKEN } }
+  let(:project_headers) { auth_headers.merge('X-Project-ID' => project.identifier) }
 
   before do
-    Flipper.disable :cat_mode
-    Flipper.disable_actor :cat_mode, school
+    Flipper.enable_actor :cat_mode, school
   end
 
   it 'responds 401 Unauthorized when no Authorization header is provided' do
-    post '/api/scratch/assets/example.svg'
+    post '/api/scratch/assets/example.svg', headers: { 'X-Project-ID' => project.identifier }
 
     expect(response).to have_http_status(:unauthorized)
   end
 
   it 'responds 404 Not Found when cat_mode is not enabled' do
     authenticated_in_hydra_as(teacher)
+    Flipper.disable :cat_mode
+    Flipper.disable_actor :cat_mode, school
 
-    post '/api/scratch/assets/example.svg', headers: auth_headers
+    post '/api/scratch/assets/example.svg', headers: project_headers
 
     expect(response).to have_http_status(:not_found)
   end
 
   it 'creates an asset when cat_mode is enabled and the required headers are provided' do
     authenticated_in_hydra_as(teacher)
-    Flipper.enable_actor :cat_mode, school
 
-    post '/api/scratch/assets/example.svg', headers: auth_headers
+    post '/api/scratch/assets/example.svg', headers: project_headers
 
     expect(response).to have_http_status(:created)