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

fix dumping expression indexes #27

Merged
merged 2 commits into from
Dec 31, 2018
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,7 @@ SchemaPlus::Core provides a state object and of callbacks to various phases of t

## Release Notes

* 2.2.3 Fix dumping complex expression based indexes in AR 5.x
* 2.2.2 Fixed dumping tables in postgresql in AR 5.2 when the PK is not a bigint.
* 2.2.1 Fixed expression index handling in AR5.x.
* 2.2.0 Added AR5.2 support. Thanks to [@jeremyyap](https://github.com/jeremyyap)
Expand Down
81 changes: 54 additions & 27 deletions lib/schema_plus/core/active_record/schema_dumper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,54 @@ def tables(_)
end
end

TABLE_COLUMN_MATCHES = [
[ # first match expression index case
%r{
^
t\.index \s*
"(?<index_cols>(?:[^"\\]|\\.)*?)" \s*
, \s*
name\: \s* [:'"](?<name>[^"\s]+)[,"]? \s*
,? \s*
(?<options>.*)
$
}x,
->(m) {
index_cols = m[:index_cols].gsub('\\"', '"')
SchemaDump::Table::Index.new name: m[:name], columns: index_cols, options: eval("{" + m[:options] + "}")
}
],
[ # general matching of columns
%r{
^
t\.(?<type>\S+) \s*
[:'"](?<name>[^"\s]+)[,"]? \s*
,? \s*
(?<options>.*)
$
}x,
->(m) {
SchemaDump::Table::Column.new name: m[:name], type: m[:type], options: eval("{" + m[:options] + "}"), comments: []
}
],
[ # index definitions with multiple columns
%r{
^
t\.index \s*
\[(?<index_cols>.*?)\] \s*
, \s*
name\: \s* [:'"](?<name>[^"\s]+)[,"]? \s*
,? \s*
(?<options>.*)
$
}x,
->(m) {
index_cols = m[:index_cols].tr(%q{'":}, '').strip.split(/\s*,\s*/)
SchemaDump::Table::Index.new name: m[:name], columns: index_cols, options: eval("{#{m[:options]}}")
}
]
].freeze

def table(table, _)
SchemaMonkey::Middleware::Dumper::Table.start(dumper: self, connection: @connection, dump: @dump, table: @dump.tables[table] = SchemaDump::Table.new(name: table)) do |env|
stream = StringIO.new
Expand All @@ -68,34 +116,13 @@ def table(table, _)
env.table.trailer = m[:trailer].split("\n").map(&:strip).reject{|s| s.blank?}
table_objects = m[:columns].strip.split("\n").map { |col|
cs = col.strip
m = cs.match %r{
^
t\.(?<type>\S+) \s*
[:'"](?<name>[^"\s]+)[,"]? \s*
,? \s*
(?<options>.*)
$
}x
if !m.nil?
SchemaDump::Table::Column.new name: m[:name], type: m[:type], options: eval("{" + m[:options] + "}"), comments: []
else
m = cs.match %r{
^
t\.index \s*
\[(?<index_cols>.*?)\] \s*
, \s*
name\: \s* [:'"](?<name>[^"\s]+)[,"]? \s*
,? \s*
(?<options>.*)
$
}x
if m.nil?
nil
else
index_cols = m[:index_cols].tr(%q{'":}, '').strip.split(/\s*,\s*/)
SchemaDump::Table::Index.new name: m[:name], columns: index_cols, options: eval("{#{m[:options]}}")
end
result = nil
Copy link
Member

Choose a reason for hiding this comment

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

At this point I think it's worth splitting up to different functions for each arity. This seems rather convoluted...

Copy link
Member Author

Choose a reason for hiding this comment

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

it's all about priority of matching here.. as I have the expression index case, the generic case, and the column case.

Copy link
Member Author

Choose a reason for hiding this comment

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

once thought I could do, is extract it out into a CONST array of regex/lambda and reduce this block to just iterating those.. Then it'll be a little cleaner? e.g.

COLUMN_MATCHERS = [
 [%r{...}, ->(m) {}],
 [%r{...}, ->(m) {}],
].freeze

COLUMN_MATCHERS.find do |r, l| 
    m = cs.match(r)
   l.call(m) unless m.nil?
end

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that would be awesome, thanks @urkle!

Copy link
Member Author

Choose a reason for hiding this comment

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

@lowjoel take a look now..

# find the first regex that matches and grab the column definition
TABLE_COLUMN_MATCHES.find do |(r, l)|
Copy link
Member

Choose a reason for hiding this comment

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

I think

result = nil
TABILE_COLUMN_MATCHES.find do |(r, l)|
  m = cs.match(r)
  result = ...
end
result

is equivalent to

TABLE_COLUMN_MATCHES.find do |(r, l)|
  m = cs.match(r)
  !m.nil? && l.call(m)  # or l.call(m) unless m.nil?
end

Copy link
Member Author

Choose a reason for hiding this comment

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

it's actually not.. as "find" returns the entry.. not the result of the block. (as I had that first and all tests failed)

Copy link
Member

Choose a reason for hiding this comment

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

Oops. I'll accept this.

This seems non-obvious though 🤔

m = cs.match r
result = m.nil? ? nil : l.call(m)
end
result
}.reject { |o| o.nil? }
env.table.columns = table_objects.select { |o| o.is_a? SchemaDump::Table::Column }
env.table.indexes = table_objects.select { |o| o.is_a? SchemaDump::Table::Index }
Expand Down
2 changes: 1 addition & 1 deletion lib/schema_plus/core/version.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
module SchemaPlus
module Core
VERSION = "2.2.2"
VERSION = "2.2.3"
end
end
14 changes: 14 additions & 0 deletions spec/dumper_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,20 @@ def after(env)

Then { expect(dump).to_not match(/create_table "inttable", id: :serial.*default:/m) }
end

context 'expression index handling', postgresql: :only do
before(:each) do
migration.create_table "expressions" do |t|
t.string :field
t.string :column
t.index 'lower(field), id', name: 'index_expression_field'
t.index 'lower("column"), id', name: 'index_expression_column'
end
end

Then { expect(dump).to match(/index "lower.+field.+, id", :name=>"index_expression_field"/) }
Then { expect(dump).to match(/index "lower.+"column\\".+, id", :name=>"index_expression_column"/) }
end
end

context TestDumper::Middleware::Dumper::Table do
Expand Down