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 71 composite pks #837

Merged
merged 2 commits into from
Apr 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,16 @@ tmtags
## VIM
*.swp

## Idea
.idea

## PROJECT::GENERAL
coverage
rdoc
pkg
*.gem
*.lock
.byebug_history

## PROJECT::SPECIFIC
log/*.log
Expand Down
7 changes: 5 additions & 2 deletions lib/activerecord-import/import.rb
Original file line number Diff line number Diff line change
Expand Up @@ -941,7 +941,7 @@ def load_association_ids(model)
association = association.target
next if association.blank? || model.public_send(column_name).present?

association_primary_key = Array(association_reflection.association_primary_key)[column_index]
association_primary_key = Array(association_reflection.association_primary_key.tr("[]:", "").split(", "))[column_index]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without a composite PK, the association_reflection.association_primary_key has a value of the format "id", whereas with a composite PK in ActiveRecord 7.1 the value is of the format "[:id, :account_id]" (again a String). The .tr loses the [, :, ] characters, so that the .split(", ") will give us an array of the two colums: ["id", "account_id"]

model.public_send("#{column_name}=", association.send(association_primary_key))
end
end
Expand Down Expand Up @@ -996,7 +996,10 @@ def find_associated_objects_for_import(associated_objects_by_class, model)

changed_objects = association.select { |a| a.new_record? || a.changed? }
changed_objects.each do |child|
child.public_send("#{association_reflection.foreign_key}=", model.id)
Array(association_reflection.inverse_of&.foreign_key || association_reflection.foreign_key).each_with_index do |column, index|
child.public_send("#{column}=", Array(model.id)[index])
end

Comment on lines +999 to +1002
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another note for the reviewers.

Let's now consider we want to recursively import the model CompositeBook and its children CompositeChapter.

Here is the has_many relation on CompositeBook, mentioning the names of the columns id and author_id. These are the PK columns in the composite_books table.

As we are setting the ids in the children, we need the columns as they are called in the children table (composite_chapters), which is here. So we need the inverse relation (the belongs_to in CompositeChapter), which has the names composite_book_id and author_id.

In order not to break things, I kept the original association_reflection.foreign_key as a fallback.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍 If an association doesn't explicitly state the inverse_of does AR infer that relationship so that this still works?

Copy link
Contributor Author

@fragkakis fragkakis Apr 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkowens I tried it out, and it still works without the inverse_of attribute being declared on the has_many and belongs_to relationships. I don't know how AR infers it, but it does, at least in my case, where the names of the relations are "standard".

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's awesome 😄

# For polymorphic associations
association_name = if model.class.respond_to?(:polymorphic_name)
model.class.polymorphic_name
Expand Down
7 changes: 7 additions & 0 deletions test/models/author.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# frozen_string_literal: true

class Author < ActiveRecord::Base
if ENV['AR_VERSION'].to_f >= 7.1
has_many :composite_books, query_constraints: [:id, :author_id], inverse_of: :author
end
end
7 changes: 5 additions & 2 deletions test/models/book.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@

class Book < ActiveRecord::Base
belongs_to :topic, inverse_of: :books
belongs_to :tag, foreign_key: [:tag_id, :parent_id] unless ENV["SKIP_COMPOSITE_PK"]

if ENV['AR_VERSION'].to_f <= 7.0
belongs_to :tag, foreign_key: [:tag_id, :parent_id] unless ENV["SKIP_COMPOSITE_PK"]
else
belongs_to :tag, query_constraints: [:tag_id, :parent_id] unless ENV["SKIP_COMPOSITE_PK"]
end
has_many :chapters, inverse_of: :book
has_many :discounts, as: :discountable
has_many :end_notes, inverse_of: :book
Expand Down
19 changes: 19 additions & 0 deletions test/models/composite_book.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# frozen_string_literal: true

class CompositeBook < ActiveRecord::Base
self.primary_key = %i[id author_id]
belongs_to :author
if ENV['AR_VERSION'].to_f <= 7.0
unless ENV["SKIP_COMPOSITE_PK"]
has_many :composite_chapters, inverse_of: :composite_book,
foreign_key: [:id, :author_id]
end
else
has_many :composite_chapters, inverse_of: :composite_book,
query_constraints: [:id, :author_id]
end

def self.sequence_name
"composite_book_id_seq"
end
end
9 changes: 9 additions & 0 deletions test/models/composite_chapter.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# frozen_string_literal: true

class CompositeChapter < ActiveRecord::Base
if ENV['AR_VERSION'].to_f >= 7.1
belongs_to :composite_book, inverse_of: :composite_chapters,
query_constraints: [:composite_book_id, :author_id]
end
validates :title, presence: true
end
16 changes: 12 additions & 4 deletions test/models/customer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,17 @@

class Customer < ActiveRecord::Base
unless ENV["SKIP_COMPOSITE_PK"]
has_many :orders,
inverse_of: :customer,
primary_key: %i(account_id id),
foreign_key: %i(account_id customer_id)
if ENV['AR_VERSION'].to_f <= 7.0
has_many :orders,
inverse_of: :customer,
primary_key: %i(account_id id),
foreign_key: %i(account_id customer_id)
else
has_many :orders,
inverse_of: :customer,
primary_key: %i(account_id id),
query_constraints: %i(account_id customer_id)
end

end
end
15 changes: 11 additions & 4 deletions test/models/order.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,16 @@

class Order < ActiveRecord::Base
unless ENV["SKIP_COMPOSITE_PK"]
belongs_to :customer,
inverse_of: :orders,
primary_key: %i(account_id id),
foreign_key: %i(account_id customer_id)
if ENV['AR_VERSION'].to_f <= 7.0
belongs_to :customer,
inverse_of: :orders,
primary_key: %i(account_id id),
foreign_key: %i(account_id customer_id)
else
belongs_to :customer,
inverse_of: :orders,
primary_key: %i(account_id id),
query_constraints: %i(account_id customer_id)
end
end
end
7 changes: 6 additions & 1 deletion test/models/tag.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
# frozen_string_literal: true

class Tag < ActiveRecord::Base
self.primary_keys = :tag_id, :publisher_id unless ENV["SKIP_COMPOSITE_PK"]
if ENV['AR_VERSION'].to_f <= 7.0
self.primary_keys = :tag_id, :publisher_id unless ENV["SKIP_COMPOSITE_PK"]
else
self.primary_key = [:tag_id, :publisher_id] unless ENV["SKIP_COMPOSITE_PK"]
end
self.primary_key = [:tag_id, :publisher_id] unless ENV["SKIP_COMPOSITE_PK"]
has_many :books, inverse_of: :tag
has_many :tag_aliases, inverse_of: :tag
end
6 changes: 5 additions & 1 deletion test/models/tag_alias.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

class TagAlias < ActiveRecord::Base
unless ENV["SKIP_COMPOSITE_PK"]
belongs_to :tag, foreign_key: [:tag_id, :parent_id], required: true
if ENV['AR_VERSION'].to_f <= 7.0
belongs_to :tag, foreign_key: [:tag_id, :parent_id], required: true
else
belongs_to :tag, query_constraints: [:tag_id, :parent_id], required: true
end
end
end
34 changes: 34 additions & 0 deletions test/schema/postgresql_schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -57,4 +57,38 @@
end

add_index :alarms, [:device_id, :alarm_type], unique: true, where: 'status <> 0'

unless ENV["SKIP_COMPOSITE_PK"]
create_table :authors, force: :cascade do |t|
t.string :name
end

execute %(
DROP SEQUENCE IF EXISTS composite_book_id_seq CASCADE;
CREATE SEQUENCE composite_book_id_seq
AS integer
START WITH 1
INCREMENT BY 1
NO MINVALUE
NO MAXVALUE
CACHE 1;

DROP TABLE IF EXISTS composite_books;
CREATE TABLE composite_books (
id bigint DEFAULT nextval('composite_book_id_seq'::regclass) NOT NULL,
title character varying,
author_id bigint
);

ALTER TABLE ONLY composite_books ADD CONSTRAINT fk_rails_040a418131 FOREIGN KEY (author_id) REFERENCES authors(id);
).split.join(' ').strip
end

create_table :composite_chapters, force: :cascade do |t|
t.string :title
t.integer :composite_book_id, null: false
t.integer :author_id, null: false
t.datetime :created_at
t.datetime :updated_at
end
end
12 changes: 12 additions & 0 deletions test/support/postgresql/import_examples.rb
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,18 @@ def should_support_postgresql_import_functionality
assert_equal db_customer.orders.last, db_order
assert_not_equal db_order.customer_id, nil
end

it "should import models with auto-incrementing ID successfully" do
author = Author.create!(name: "Foo Barson")

books = []
2.times do |i|
books << CompositeBook.new(author_id: author.id, title: "book #{i}")
end
assert_difference "CompositeBook.count", +2 do
CompositeBook.import books
end
end
end
end
end
Expand Down
18 changes: 18 additions & 0 deletions test/support/shared_examples/recursive_import.rb
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,24 @@ def should_support_recursive_import
assert_equal 1, tags[0].tag_id
assert_equal 2, tags[1].tag_id
end

if ENV['AR_VERSION'].to_f >= 7.1
it "should import models with auto-incrementing ID successfully with recursive set to true" do
author = Author.create!(name: "Foo Barson")
books = []
2.times do |i|
books << CompositeBook.new(author_id: author.id, title: "Book #{i}", composite_chapters: [
CompositeChapter.new(title: "Book #{i} composite chapter 1"),
CompositeChapter.new(title: "Book #{i} composite chapter 2"),
])
end
assert_difference "CompositeBook.count", +2 do
assert_difference "CompositeChapter.count", +4 do
CompositeBook.import books, recursive: true
end
end
end
end
end
end

Expand Down
4 changes: 3 additions & 1 deletion test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@
begin
require 'composite_primary_keys'
rescue LoadError
ENV["SKIP_COMPOSITE_PK"] = "true"
if ENV['AR_VERSION'].to_f <= 7.1
ENV['SKIP_COMPOSITE_PK'] = 'true'
end
end

# Support MySQL 5.7
Expand Down