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.

Sign up for GitHub

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

Allow to do a change_table to change partition key #64

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Allow to do a change_table to change partition key #64

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
987337a
allow to do a change_table to change partition key
louiseGrandjonc Sep 11, 2019
99c6fbd
check that the primary key has n column to avoid recreating each time…
louiseGrandjonc Sep 11, 2019
f35c0a4
clean query
louiseGrandjonc Sep 12, 2019
855a382
change spec schema.rb to remove useless block
louiseGrandjonc Sep 12, 2019
8cf51b0
clean code
louiseGrandjonc Sep 12, 2019
0b01179
start file to have distribute statement in structure.sql
louiseGrandjonc Sep 12, 2019
f5f2f42
rake task to add distributed tables and reference tables at the end o…
louiseGrandjonc Sep 13, 2019
876f95c
add an option to add or not distribute statements
louiseGrandjonc Sep 13, 2019
0e5db11
compat with rails 4
louiseGrandjonc Sep 13, 2019
6e7d33e
clean code
louiseGrandjonc Sep 13, 2019
0159335
Merge branch 'master' into update-pkey
louiseGrandjonc Sep 16, 2019
419f234
split task to add unit tests
louiseGrandjonc Sep 17, 2019
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
41 changes: 37 additions & 4 deletions lib/activerecord-multi-tenant/migrations.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,45 @@ module ActiveRecord
module ConnectionAdapters # :nodoc:
module SchemaStatements
alias :orig_create_table :create_table
alias :orig_change_table :change_table

def change_primary_key(table_name, options)
if !options[:partition_key] || options[:partition_key].to_s == 'id'
return
end

pkey_columns = ['id', options[:partition_key]]

# we are here comparing the columns in the primary key on the database and the one in the migration file
columns_result = execute("select kcu.column_name as key_column " \
"from information_schema.table_constraints tco "\
"join information_schema.key_column_usage kcu " \
"ON kcu.constraint_name = tco.constraint_name " \
"AND kcu.constraint_schema = tco.constraint_schema "\
"WHERE tco.constraint_type = 'PRIMARY KEY' " \
"AND tco.constraint_name = '#{table_name}_pkey'")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use a bind parameter here, instead of string substitution?

Whilst its unlikely that someone will do SQL injection here, it seems better to avoid problems with table_name called '; DROP TABLE xyz; SELECT '


if columns_result.present?
columns = []
columns_result.values.each do |c| columns.push(c[0]) end
Copy link
Contributor

Choose a reason for hiding this comment

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

For one liners usually in Ruby you'd use the { .. } syntax:

columns_result.values.each { |c| columns.push(c[0]) }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is there anyway to do this without initializing a constant? in python you can do
columns = [e[0] for e in columns_result.values]

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you could do it like this:

columns = columns_result.values.map { |c| c[0] }

or without the block, utilizing the fact that there is a first method on arrays:

columns = columns_result.values.map(&:first)


if columns.length != pkey_columns.length
execute "ALTER TABLE #{table_name} DROP CONSTRAINT IF EXISTS #{table_name}_pkey"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here regarding SQL injection - I think there are methods to escape identifier names in ActiveRecord::Base that we can use

Copy link
Contributor Author

@louiseGrandjonc louiseGrandjonc Sep 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, I tried looking into it, the method to sanitize raw sql is sanitize_sql_array, except here, we are taking the name of the table in the ALTER TABLE which ends up like this:

query = ActiveRecord::Base::sanitize_sql_array(["ALTER TABLE $1 DROP CONSTRAINT IF EXISTS $2_pkey;", table_name, table_name])
execute(query)

And this triggers errors because SQL doesn't suppose that format

If you have an idea how to do change the queries, let me know

execute "ALTER TABLE #{table_name} ADD PRIMARY KEY(\"#{options[:partition_key]}\", id)"
end
end

end

def create_table(table_name, options = {}, &block)
ret = orig_create_table(table_name, options.except(:partition_key), &block)
if options[:partition_key] && options[:partition_key].to_s != 'id'
execute "ALTER TABLE #{table_name} DROP CONSTRAINT #{table_name}_pkey"
execute "ALTER TABLE #{table_name} ADD PRIMARY KEY(id, \"#{options[:partition_key]}\")"
Copy link
Contributor

Choose a reason for hiding this comment

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

Of course, to be fair my own code was not up to my comments :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

haha, noted, I'll work on avoiding SQL injection ;)

end
change_primary_key(table_name, options)
ret
end

def change_table(table_name, options, &block)
ret = orig_change_table(table_name, options.except(:partition_key), &block)
change_primary_key(table_name, options)
ret
end
end
Expand Down
15 changes: 13 additions & 2 deletions spec/schema.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@
t.column :name, :string
end

create_table :project_categories, force: true, partition_key: :account_id do |t|
create_table :project_categories, force: true do |t|
t.column :name, :string
t.column :account_id, :integer
t.column :project_id, :integer
Expand All @@ -106,9 +106,20 @@
create_distributed_table :partition_key_not_model_tasks, :non_model_id
create_distributed_table :subclass_tasks, :non_model_id
create_distributed_table :uuid_records, :organization_id
create_distributed_table :project_categories, :account_id
create_distributed_table :allowed_places, :account_id
create_reference_table :categories

change_table :project_categories, partition_key: :account_id do |t|
end
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can not pass the block at all, unless that throws an error:

change_table :project_categories, partition_key: :account_id



# primary key shoud not be recreated
change_table :project_categories, partition_key: :account_id do |t|
t.column :subtitle, :string
end


create_distributed_table :project_categories, :account_id
end

class Account < ActiveRecord::Base
Expand Down