Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rails 7 Wrong schema_search_path in ActiveStorage::DirectUploadsController #196

Open
fdaugs opened this issue Feb 16, 2022 · 7 comments
Open

Comments

@fdaugs
Copy link

fdaugs commented Feb 16, 2022

Steps to reproduce

After upgrading our app to Rails 7.0.2 from 6.1.4, the direct upload no longer works together with the apartment. Blob records are always created in the default schema (or whatever is configured as schema_search_path in database.yml). And if you try to attach such a record to a model, an error is issued because the record cannot be found in the current tenant's schema.

Expected behavior

Blobs should be created the schema of the current tenant.

Actual behavior

The schema_search_path in the create method of the ActiveStorage::DirectUploadsController is set to the default and blobs are inserted in the default schema.

  • Database: postgres 11

  • Apartment version: 2.11.0 (I also tried the fix in Fix Rails 7 connection handling #194)

  • Apartment config (in config/initializers/apartment.rb or so):

    • use_schemas: true
    • use_sql: true
  • Rails (or ActiveRecord) version: 7.0.2

  • Ruby version: 2.7.5

@archonic
Copy link

I'm also seeing 404s on files and even more worrisome is that existing URLs are linking to attachments in other workspaces.

@acallaghan
Copy link

acallaghan commented Mar 20, 2022

I'm also seeing this problem with Rails 7.

For my application, ActiveStorage::Blob is in each schema, but after upgrading to Rails 7, Rails always chooses the public schema for calls to ActiveStorage::Blob Load

E.g.

[public,shared_extensions]   ActiveStorage::Blob Load (0.8ms)  SELECT "active_storage_blobs".* FROM "active_storage_blobs" WHERE "active_storage_blobs"."id" = $1 LIMIT $2  [["id", 28], ["LIMIT", 1]]

ActiveRecord::RecordNotFound (Couldn't find ActiveStorage::Blob with 'id'=28):

I'll try and muck around on a branch and find a fix

@acallaghan
Copy link

I've opened a PR that might be the fix: #198

@ThomAille
Copy link

Hi all,

It's not optimized and not very pretty as code, but here is the solution we developed to solve the problem

app/controllers/active_storage/direct_uploads_controller.rb

# frozen_string_literal: true

# Creates a new blob on the server side in anticipation of a direct-to-service upload from the client side.
# When the client-side upload is completed, the signed_blob_id can be submitted as part of the form to reference
# the blob that was created up front.
class ActiveStorage::DirectUploadsController < ActiveStorage::BaseController
  def create
    subdomain = Apartment::Tenant.current
    blob = nil
    Apartment::Tenant.switch(subdomain) do
      blob = ActiveStorage::Blob.create_before_direct_upload!(**blob_args)
    end
    render json: direct_upload_json(blob)
  end

  private
  def blob_args
    params.require(:blob).permit(:filename, :byte_size, :checksum, :content_type, metadata: {}).to_h.symbolize_keys
  end

  def direct_upload_json(blob)
    blob.as_json(root: false, methods: :signed_id).merge(direct_upload: {
      url: blob.service_url_for_direct_upload,
      headers: blob.service_headers_for_direct_upload
    })
  end
end

@archonic
Copy link

I'm uncertain why this would fix anything:

subdomain = Apartment::Tenant.current
Apartment::Tenant.switch(subdomain)

Wouldn't Apartment already be switched to the current tenant?

@ArthurWD
Copy link
Contributor

ArthurWD commented May 24, 2022

I'm uncertain why this would fix anything:

It seems like somewhere deep in active storage the schema is reset, without Apartment knowing about this.

We came up with this fix in an initializer:

module ActiveStorageApartmentFix
  extend ActiveSupport::Concern

  def key
    self[:key] ||= "#{Apartment::Tenant.current}/#{self.class.generate_unique_secure_token(length: 28)}"
  end

  class_methods do
    def create_before_direct_upload!(**args)
      Apartment::Tenant.switch(Apartment::Tenant.current) do
        super
      end
    end
  end
end

Rails.application.config.to_prepare do
  ActiveStorage::Blob.prepend(ActiveStorageApartmentFix)

  ActiveRecord::Base.class_eval do
    def self.find_signed!(signed_id, purpose: nil)
      Apartment::Tenant.switch(Apartment::Tenant.current) do
        super
      end
    end
  end
end

@dirkbull
Copy link

Thanks @ArthurWD for the quick fix. Small addition for those who need to avoid initializing Apartment while precompiling assets:

module ActiveStorageApartmentFix
  extend ActiveSupport::Concern

  def key
    self[:key] ||= "#{Apartment::Tenant.current}/#{self.class.generate_unique_secure_token(length: 28)}"
  end

  class_methods do
    def create_before_direct_upload!(**args)
      Apartment::Tenant.switch(Apartment::Tenant.current) do
        super
      end
    end
  end
end

Rails.application.config.to_prepare do
  next if ENV['APARTMENT_DISABLE_INIT']

  ActiveStorage::Blob.prepend(ActiveStorageApartmentFix)

  ActiveRecord::Base.class_eval do
    def self.find_signed!(signed_id, purpose: nil)
      Apartment::Tenant.switch(Apartment::Tenant.current) do
        super
      end
    end
  end
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants