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.1: ActiveRecord::Base#query_constraints and composite keys are incompatible with ActiveStorage #51127

Open
Slotos opened this issue Feb 19, 2024 · 1 comment

Comments

@Slotos
Copy link

Slotos commented Feb 19, 2024

Steps to reproduce

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails"
  # If you want to test against edge Rails replace the previous line with this:
  # gem "rails", github: "rails/rails", branch: "main"

  gem "sqlite3"
end

require "active_record/railtie"
require "active_storage/engine"
require "tmpdir"

class TestApp < Rails::Application
  config.load_defaults Rails::VERSION::STRING.to_f

  config.root = __dir__
  config.hosts << "example.org"
  config.eager_load = false
  config.session_store :cookie_store, key: "cookie_store_key"
  config.secret_key_base = "secret_key_base"

  config.logger = Logger.new($stdout)
  Rails.logger  = config.logger

  config.active_storage.service = :local
  config.active_storage.service_configurations = {
    local: {
      root: Dir.tmpdir,
      service: "Disk"
    }
  }
end

ENV["DATABASE_URL"] = "sqlite3::memory:"

Rails.application.initialize!

require ActiveStorage::Engine.root.join("db/migrate/20170806125915_create_active_storage_tables.rb").to_s

ActiveRecord::Schema.define do
  CreateActiveStorageTables.new.change

  create_table :users, force: true do |t|
    t.bigint :tenant_id, null: false
    t.index [:tenant_id, :id], unique: true
  end

  create_table :cats, primary_key: [:breed, :color], force: true do |t|
    t.string :color, null: false
    t.string :breed, null: false
  end
end

class User < ActiveRecord::Base
  query_constraints :tenant_id, :id

  has_one_attached :profile
end

class Cat < ActiveRecord::Base
  has_one_attached :profile
end

require "minitest/autorun"

class BugTest < Minitest::Test
  def test_upload_and_download_query_constraint
    user = User.create!(
      tenant_id: 42,
      profile: {
        content_type: "text/plain",
        filename: "dummy.txt",
        io: ::StringIO.new("dummy"),
      }
    )

    assert_equal "dummy", user.profile.download
  end

  def test_upload_and_download_primary_key
    cat = Cat.create!(
      breed: "cat",
      color: "orange",
      profile: {
        content_type: "text/plain",
        filename: "dummy.txt",
        io: ::StringIO.new("dummy"),
      }
    )

    assert_equal "dummy", cat.profile.download
  end
end

Expected behavior

Since ActiveStorage storage tables are not configurable, it should simply work. Attaching a file should result in an attached file.

Actual behavior

query_constraints route results in an attempt to query non-existing columns on active_storage_attachments. Redefining relations on a model doesn't help, since inverse belong_to association then attempts to compare two columns to one in ActiveRecord::AutosaveAssociation#compute_primary_key.

Composite primary key route doesn't work, unless the key contains :id column.

Notes

This is kind-of solvable by adding exttra columns to active_storage_attachments, but that can get out of hand real fast.

Incidentally, allowing belongs_to to accept an array primary_key, would solve the problem by letting the record have a regular id for use by ActiveSupport, but configure relations between models using composite keys. Right now, this route doesn't work due to ActiveRecord::Reflection::BelongsToReflection#association_primary_key transforming primary_key option with to_s.

System configuration

Rails version: 7.1.3

Ruby version: 3.3.0

@rafaelfranca
Copy link
Member

@nvasilevski

Slotos added a commit to Slotos/rails that referenced this issue Feb 25, 2024
Resolves rails#50850

Resolves rails#51127 by allowing users to set all query constraints on
association definitions, leaving model in a state compatible with
active_storage (and active_text) associations.
Slotos added a commit to Slotos/rails that referenced this issue Feb 25, 2024
Resolves rails#50850

Resolves rails#51127 by allowing users to set all query constraints on
association definitions, leaving model in a state compatible with
active_storage (and active_text) associations.
Slotos added a commit to Slotos/rails that referenced this issue Feb 25, 2024
Resolves rails#50850

Resolves rails#51127 by allowing users to set all query constraints on
association definitions, leaving model in a state compatible with
active_storage (and active_text) associations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants