search

drwl / annotaterb

A Ruby Gem that adds annotations to your Rails models and route files.
Other
123 stars 12 forks source link

Error on composite foreign key constraints #121

Closed dmke closed 2 months ago

dmke commented 3 months ago

I have a schema with the following FK constraint setup:

class SetupFailure < ActiveRecord::Migration[7.1]
  def up
    create_table :tenants do |t|
      t.string :name, null: false
      t.timestamps null: false
    end

    create_table :customers do |t|
      t.belongs_to :tenant, null: false
      t.string :name, null: false
      t.timestamps null: false
    end

    create_table :documents do |t|
      t.belongs_to :tenant, null: false
      t.belongs_to :customer, null: false
      t.string :title
      t.timestamps null: false
    end

    # remove FK documents(customer_id) => customers(id)
    remove_foreign_key :documents, :customers, column: :customer_id

    # replace with documents(tenant_id,customer_id) => customers(tenant_id,id)
    execute <<~SQL.squish
      alter table documents
      add constraint #{connection.send(:foreign_key_name, :documents, column: :customer_id)}
      foreign key (tenant_id,customer_id)
      references customers(tenant_id, id)
    SQL
  end
end

The application supports multi-tenancy; tenants have customers, and customers have documents.

We use this FK to ensure on the DB layer, that the tenant_id matches on both the document and customer (i.e. document.tenant_id == document.customer.tenant_id).

Commands

$ bundle exec annotaterb models
bundler: failed to load command: annotaterb (GEM_PATH/bin/annotaterb)
GEM_PATH/gems/annotaterb-4.9.0/lib/annotate_rb/model_annotator/foreign_key_annotation/annotation_builder.rb:34:in `sort_by': comparison of Array with Array failed (ArgumentError)
        from GEM_PATH/gems/annotaterb-4.9.0/lib/annotate_rb/model_annotator/foreign_key_annotation/annotation_builder.rb:34:in `build'
        from GEM_PATH/gems/annotaterb-4.9.0/lib/annotate_rb/model_annotator/annotation_builder.rb:44:in `build'
        from GEM_PATH/gems/annotaterb-4.9.0/lib/annotate_rb/model_annotator/project_annotator.rb:42:in `build_instructions_for_file'
        from GEM_PATH/gems/annotaterb-4.9.0/lib/annotate_rb/model_annotator/project_annotator.rb:17:in `block in annotate'
        from GEM_PATH/gems/annotaterb-4.9.0/lib/annotate_rb/model_annotator/project_annotator.rb:13:in `map'
        from GEM_PATH/gems/annotaterb-4.9.0/lib/annotate_rb/model_annotator/project_annotator.rb:13:in `annotate'
        from GEM_PATH/gems/annotaterb-4.9.0/lib/annotate_rb/model_annotator/annotator.rb:21:in `do_annotations'
        from GEM_PATH/gems/annotaterb-4.9.0/lib/annotate_rb/model_annotator/annotator.rb:8:in `do_annotations'
        from GEM_PATH/gems/annotaterb-4.9.0/lib/annotate_rb/commands/annotate_models.rb:17:in `call'
        from GEM_PATH/gems/annotaterb-4.9.0/lib/annotate_rb/runner.rb:24:in `run'
        from GEM_PATH/gems/annotaterb-4.9.0/lib/annotate_rb/runner.rb:7:in `run'
        from GEM_PATH/gems/annotaterb-4.9.0/exe/annotaterb:18:in `<top (required)>'
        from GEM_PATH/bin/annotaterb:25:in `load'
        from GEM_PATH/bin/annotaterb:25:in `<top (required)>'
        from GEM_PATH/gems/bundler-2.4.19/lib/bundler/cli/exec.rb:58:in `load'
        from GEM_PATH/gems/bundler-2.4.19/lib/bundler/cli/exec.rb:58:in `kernel_load'
        from GEM_PATH/gems/bundler-2.4.19/lib/bundler/cli/exec.rb:23:in `run'
        from GEM_PATH/gems/bundler-2.4.19/lib/bundler/cli.rb:492:in `exec'
        from GEM_PATH/gems/bundler-2.4.19/lib/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
        from GEM_PATH/gems/bundler-2.4.19/lib/bundler/vendor/thor/lib/thor/invocation.rb:127:in `invoke_command'
        from GEM_PATH/gems/bundler-2.4.19/lib/bundler/vendor/thor/lib/thor.rb:392:in `dispatch'
        from GEM_PATH/gems/bundler-2.4.19/lib/bundler/cli.rb:34:in `dispatch'
        from GEM_PATH/gems/bundler-2.4.19/lib/bundler/vendor/thor/lib/thor/base.rb:485:in `start'
        from GEM_PATH/gems/bundler-2.4.19/lib/bundler/cli.rb:28:in `start'
        from GEM_PATH/gems/bundler-2.4.19/exe/bundle:37:in `block in <top (required)>'
        from GEM_PATH/gems/bundler-2.4.19/lib/bundler/friendly_errors.rb:117:in `with_friendly_errors'
        from GEM_PATH/gems/bundler-2.4.19/exe/bundle:29:in `<top (required)>'
        from GEM_PATH/bin/bundle:23:in `load'
        from GEM_PATH/bin/bundle:23:in `<main>'

Debugging

I've added a debug-print to foreign_key_annotation/annotation_builder.rb:34:

          ...
          max_size = foreign_keys.map(&format_name).map(&:size).max + 1
          # debug
          puts "-- #{@model.table_name} --"
          foreign_keys.map { |fk| pp [format_name.call(fk), fk.column] }
          # /debug
          foreign_keys.sort_by { |fk| [format_name.call(fk), fk.column] }.each do |fk|
          ...

That gave the following output:

-- documents --
["fk_rails_...", ["tenant_id", "customer_id"]]
["fk_rails_...", "tenant_id"]

Possible Solution

Splatting the fk.columns did solve the error:

foreign_keys.sort_by { |fk| [format_name.call(fk), *fk.column] }.each do |fk|

Now the annotation is updated, but the display is a bit wonky. I'm not sure how improve that, though :)

# ...
# Foreign Keys
#
#  fk_rails_...  (tenant_id => tenants.id)
#  fk_rails_...  (["tenant_id", "customer_id"] => customers.["tenant_id", "id"])

Version

drwl commented 3 months ago

@dmke woah, thanks for this awesome write up + explanation + possible solutions. Let me try and replicate the issue so I can better understand what is going on.

drwl commented 3 months ago

@dmke Would you mind telling me what database you are using?

dmke commented 3 months ago

@drwl, I'm using PostgreSQL; any currently supported PostgreSQL version will do (I'm currently on 14.11).

I believe (haven't tested) both MariaDB and SQLite also allow to reference multiple columns in a foreign key, although it looks like having a column tuple as source in SQLite isn't allowed.

drwl commented 2 months ago

@dmke apologies for the delay. I was preparing for interviews and also got sick recently.

I'm trying to replicate the setup but having issues with the SetupFailure migration. So far, I found that I needed to add a foreign_key: true to the customer reference in documents table:

    create_table :documents do |t|
      t.belongs_to :tenant, null: false
      t.belongs_to :customer, null: false, foreign_key: true
      t.string :title
      t.timestamps null: false
    end

Now still, I have this issue:

Caused by:
PG::InvalidForeignKey: ERROR:  there is no unique constraint matching given keys for referenced table "customers"
/Users/leea/.rbenv/versions/3.0.5/lib/ruby/gems/3.0.0/gems/activerecord-7.1.3.4/lib/active_record/connection_adapters/postgresql/database_statements.rb:55:in `exec'
/Users/leea/.rbenv/versions/3.0.5/lib/ruby/gems/3.0.0/gems/activerecord-7.1.3.4/lib/active_record/connection_adapters/postgresql/database_statements.rb:55:in `block (2 levels) in raw_execute'
/Users/leea/.rbenv/versions/3.0.5/lib/ruby/gems/3.0.0/gems/activerecord-7.1.3.4/lib/active_record/connection_adapters/abstract_adapter.rb:1028:in `block in with_raw_connection'
/Users/leea/.rbenv/versions/3.0.5/lib/ruby/gems/3.0.0/gems/activesupport-7.1.3.4/lib/active_support/concurrency/null_lock.rb:9:in `synchronize'
/Users/leea/.rbenv/versions/3.0.5/lib/ruby/gems/3.0.0/gems/activerecord-7.1.3.4/lib/active_record/connection_adapters/abstract_adapter.rb:1000:in `with_raw_connection'
/Users/leea/.rbenv/versions/3.0.5/lib/ruby/gems/3.0.0/gems/activerecord-7.1.3.4/lib/active_record/connection_adapters/postgresql/database_statements.rb:54:in `block in raw_execute'
/Users/leea/.rbenv/versions/3.0.5/lib/ruby/gems/3.0.0/gems/activesupport-7.1.3.4/lib/active_support/notifications/instrumenter.rb:58:in `instrument'
/Users/leea/.rbenv/versions/3.0.5/lib/ruby/gems/3.0.0/gems/activerecord-7.1.3.4/lib/active_record/connection_adapters/abstract_adapter.rb:1143:in `log'
/Users/leea/.rbenv/versions/3.0.5/lib/ruby/gems/3.0.0/gems/activerecord-7.1.3.4/lib/active_record/connection_adapters/postgresql/database_statements.rb:53:in `raw_execute'
/Users/leea/.rbenv/versions/3.0.5/lib/ruby/gems/3.0.0/gems/activerecord-7.1.3.4/lib/active_record/connection_adapters/abstract/database_statements.rb:521:in `internal_execute'
/Users/leea/.rbenv/versions/3.0.5/lib/ruby/gems/3.0.0/gems/activerecord-7.1.3.4/lib/active_record/connection_adapters/abstract/database_statements.rb:131:in `execute'
/Users/leea/.rbenv/versions/3.0.5/lib/ruby/gems/3.0.0/gems/activerecord-7.1.3.4/lib/active_record/connection_adapters/abstract/query_cache.rb:25:in `execute'
/Users/leea/.rbenv/versions/3.0.5/lib/ruby/gems/3.0.0/gems/activerecord-7.1.3.4/lib/active_record/connection_adapters/postgresql/database_statements.rb:47:in `execute'
/Users/leea/.rbenv/versions/3.0.5/lib/ruby/gems/3.0.0/gems/activerecord-7.1.3.4/lib/active_record/migration/default_strategy.rb:10:in `method_missing'
/Users/leea/.rbenv/versions/3.0.5/lib/ruby/gems/3.0.0/gems/activerecord-7.1.3.4/lib/active_record/migration.rb:1047:in `block in method_missing'
/Users/leea/.rbenv/versions/3.0.5/lib/ruby/gems/3.0.0/gems/activerecord-7.1.3.4/lib/active_record/migration.rb:1017:in `block in say_with_time'

I suspect there's some issue with the raw execute sql code:

    execute <<~SQL.squish
      alter table documents
      add constraint #{connection.send(:foreign_key_name, :documents, column: :customer_id)}
      foreign key (tenant_id,customer_id)
      references customers(tenant_id, id)
    SQL

Any thoughts on how I can get around this? I'm using pg 1.5.6, Rails 7.1.3.4.

dmke commented 2 months ago

apologies for the delay

Oh don't worry :)

I'm trying to replicate the setup but having issues with the SetupFailure migration

Hm, I'll look into that.

I've extracted the migration from one of our production apps (maybe a little hastily). It could very well be that I've missed something.

dmke commented 2 months ago

It took some time, but the problem was essentially a missing unique constraint on documents(tenant_id, id) (composite FK source and destination columns must be unique).

The corrected migration looks like this:

class SetupFailure < ActiveRecord::Migration[7.1]
  def up
    create_table :tenants do |t|
      t.string :name, null: false
      t.timestamps null: false
    end

    create_table :customers do |t|
      t.belongs_to :tenant, null: false, foreign_key: true
      t.string :name, null: false
      t.timestamps null: false

      t.index %i[tenant_id id], unique: true
    end

    create_table :documents do |t|
      t.belongs_to :tenant, null: false, foreign_key: true
      t.belongs_to :customer, null: false, foreign_key: true
      t.string :title
      t.timestamps null: false

      t.index %i[tenant_id id], unique: true
    end

    # remove FK documents(customer_id) => customers(id)
    remove_foreign_key :documents, :customers, column: :customer_id

    # replace with documents(tenant_id,customer_id) => customers(tenant_id,id)
    execute <<~SQL.squish
      alter table documents
      add constraint #{connection.send(:foreign_key_name, :documents, column: :customer_id)}
      foreign key (tenant_id, customer_id)
      references customers(tenant_id, id)
    SQL
  end
end
drwl commented 2 months ago

@dmke so this migration seems to run but not give the expected output... I think.

This is the migration I'm running:

class AddSetupFailure < ActiveRecord::Migration[7.0]
  def up
    create_table :tenants do |t|
      t.string :name, null: false
      t.timestamps null: false
    end

    create_table :customers do |t|
      t.belongs_to :tenant, null: false, foreign_key: true
      t.string :name, null: false
      t.timestamps null: false

      t.index %i[tenant_id id], unique: true
    end

    create_table :documents do |t|
      t.belongs_to :tenant, null: false, foreign_key: true
      t.belongs_to :customer, null: false, foreign_key: true
      t.string :title
      t.timestamps null: false

      t.index %i[tenant_id id], unique: true
    end

    # remove FK documents(customer_id) => customers(id)
    remove_foreign_key :documents, :customers, column: :customer_id

    # replace with documents(tenant_id,customer_id) => customers(tenant_id,id)
    execute <<~SQL.squish
      alter table documents
      add constraint #{connection.send(:foreign_key_name, :documents, column: :customer_id)}
      foreign key (tenant_id, customer_id)
      references customers(tenant_id, id)
    SQL
  end

  def down
    drop_table :documents
    drop_table :customers
    drop_table :tenants
  end
end

Resulting in the following schema.rb:

ActiveRecord::Schema[7.0].define(version: 2024_06_16_002409) do
  # These are extensions that must be enabled in order to support this database
  enable_extension "plpgsql"

  create_table "customers", force: :cascade do |t|
    t.bigint "tenant_id", null: false
    t.string "name", null: false
    t.datetime "created_at", null: false
    t.datetime "updated_at", null: false
    t.index ["tenant_id", "id"], name: "index_customers_on_tenant_id_and_id", unique: true
    t.index ["tenant_id"], name: "index_customers_on_tenant_id"
  end

  create_table "documents", force: :cascade do |t|
    t.bigint "tenant_id", null: false
    t.bigint "customer_id", null: false
    t.string "title"
    t.datetime "created_at", null: false
    t.datetime "updated_at", null: false
    t.index ["customer_id"], name: "index_documents_on_customer_id"
    t.index ["tenant_id", "id"], name: "index_documents_on_tenant_id_and_id", unique: true
    t.index ["tenant_id"], name: "index_documents_on_tenant_id"
  end

  create_table "tenants", force: :cascade do |t|
    t.string "name", null: false
    t.datetime "created_at", null: false
    t.datetime "updated_at", null: false
  end

  add_foreign_key "customers", "tenants"
  add_foreign_key "documents", "customers", column: "tenant_id", primary_key: "tenant_id"
  add_foreign_key "documents", "tenants"
end

When I run annotaterb it runs successfully (without errors), but gives the wrong output:

# app/models/document.rb

# == Schema Information
#
# Table name: documents
#
#  id          :bigint           not null, primary key
#  title       :string
#  created_at  :datetime         not null
#  updated_at  :datetime         not null
#  customer_id :bigint           not null
#  tenant_id   :bigint           not null
#
# Indexes
#
#  index_documents_on_customer_id       (customer_id)
#  index_documents_on_tenant_id         (tenant_id)
#  index_documents_on_tenant_id_and_id  (tenant_id,id) UNIQUE
#
# Foreign Keys
#
#  fk_rails_...  (tenant_id => tenants.id)
#  fk_rails_...  (tenant_id => customers.tenant_id)
#
class Document < ApplicationRecord
end

That being said, I think I have enough information from your initial report on how to make changes. In terms of output, what do you think about?

# ...
# Foreign Keys
#
#  fk_rails_...  (tenant_id => tenants.id)
#  fk_rails_...  ([tenant_id, customer_id] => customers[tenant_id, id])

Looks like there's a PR in the old gem I can also reference: https://github.com/ctran/annotate_models/pull/1013

dmke commented 2 months ago

Resulting in the following schema.rb:

This is the kind of advanced schema manipulation which requires switching from db/schema.rv to db/structure.sql. You need to add the following to your config/application.rb:

config.active_record.schema_format = :sql

In terms of output, what do you think about?

Looks good :)

drwl commented 2 months ago

@dmke thanks for the suggestion about changing schema format option, I wasn't aware of that.

Just checking did you have any special relationship code in any of the models? I was able to successfully run migrations again and populate a structure.sql but still seeing the same foreign key definition (from my understanding it populates data from postgres)

[1] pry(#<AnnotateRb::ModelAnnotator::ForeignKeyAnnotation::AnnotationBuilder>)> foreign_keys
=> [#<struct ActiveRecord::ConnectionAdapters::ForeignKeyDefinition
  from_table="documents",
  to_table="tenants",
  options=
   {:column=>"tenant_id",
    :name=>"fk_rails_5ca55da786",
    :primary_key=>"id",
    :on_delete=>nil,
    :on_update=>nil,
    :deferrable=>false,
    :validate=>true}>,
 #<struct ActiveRecord::ConnectionAdapters::ForeignKeyDefinition
  from_table="documents",
  to_table="customers",
  options=
   {:column=>"tenant_id", # <-- expecting this to be an array
    :name=>"fk_rails_8b49d7b757",
    :primary_key=>"tenant_id",
    :on_delete=>nil,
    :on_update=>nil,
    :deferrable=>false,
    :validate=>true}>]
dmke commented 2 months ago

Hmm... apart from the obvious has_many/belongs_to calls in the Document and Customer models I don't think there was any special setup. I'd need to double check when I'm back in the office.

drwl commented 2 months ago

@dmke can you try using this branch and telling me if it works? https://github.com/drwl/annotaterb/pull/126

I had issues recreating the issue with arrays and sort_by, but it produces the expected output for mocked columns/foreign keys.

dmke commented 2 months ago

@drwl, #126 works great!

drwl commented 2 months ago

Thanks for testing it out. I'll cut a new release with the change soon.

drwl commented 2 months ago

@dmke just a fyi, I pushed v4.10.0 to Rubygems and it contains the change to support composite foreign keys.