From 24bc52385c0fd058b87e9dd036367334afa534f4 Mon Sep 17 00:00:00 2001 From: maarten Date: Wed, 24 Jul 2019 17:03:08 +0200 Subject: [PATCH 01/23] WIP: Add query methods to the transformer AST --- spec/arel/sql_to_arel_spec.rb | 2 ++ spec/arel/transformer_spec.rb | 55 +++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+) diff --git a/spec/arel/sql_to_arel_spec.rb b/spec/arel/sql_to_arel_spec.rb index 2af57614..c3b2e805 100644 --- a/spec/arel/sql_to_arel_spec.rb +++ b/spec/arel/sql_to_arel_spec.rb @@ -211,6 +211,8 @@ pg_node: 'PgQuery::RANGE_SUBSELECT' visit 'select', '1 FROM "public"."table_is_range_var" "alias", ONLY "b"', pg_node: 'PgQuery::RANGE_VAR' + # TODO: this one breaks + # visit 'select', '1 FROM "b", (SELECT 1) "a", "c"', pg_node: 'PgQuery::RANGE_VAR' visit 'select', '1', pg_node: 'PgQuery::RAW_STMT' visit 'sql', 'REFRESH MATERIALIZED VIEW view WITH NO DATA', pg_node: 'PgQuery::REFRESH_MAT_VIEW_STMT', diff --git a/spec/arel/transformer_spec.rb b/spec/arel/transformer_spec.rb index 108256af..61871972 100644 --- a/spec/arel/transformer_spec.rb +++ b/spec/arel/transformer_spec.rb @@ -154,4 +154,59 @@ expect(original_arel).to be_identical_arel(new_arel) end + + it 'replaces table references with subqueries' do + sql = <<~SQL + SELECT + COUNT(posts.id), + ARRAY_AGG(posts.title) + FROM + posts + INNER JOIN + comments + ON + comments.post_id = posts.id + AND + comments.message ILIKE '%hello%' + WHERE + posts.public = TRUE + AND + posts.id IN (SELECT id FROM posts WHERE id = 3) + GROUP BY + comments.created_at + SQL + + # sql = <<~SQL + # SELECT + # id + # FROM + # posts + # SQL + + # sql = <<~SQL + # SELECT + # id + # FROM + # (SELECT id FROM posts) posts, + # users + # SQL + + # sql = <<~SQL + # SELECT + # id + # FROM + # users,posts + # SQL + + # only the tables inside JOIN and FROM should be replaced + # tables inside the JOIN ... ON expression should not be replaced + + arel = Arel.sql_to_arel(sql) + tree = Arel.transformer(arel.first) + + binding.pry + + tree.query(class: Arel::Nodes::JoinSource) + + end end From 18a006a38f7de1ead74cd41e7cf209d411e0456f Mon Sep 17 00:00:00 2001 From: maarten Date: Fri, 26 Jul 2019 18:48:09 +0200 Subject: [PATCH 02/23] Add contextual information to transformer nodes --- lib/arel/extensions/delete_statement.rb | 2 +- lib/arel/transformer/node.rb | 13 +++++- lib/arel/transformer/visitor.rb | 61 +++++++++++++++++++++++++ spec/arel/transformer_spec.rb | 61 ++++--------------------- 4 files changed, 83 insertions(+), 54 deletions(-) diff --git a/lib/arel/extensions/delete_statement.rb b/lib/arel/extensions/delete_statement.rb index fdb4f4ac..16c3af79 100644 --- a/lib/arel/extensions/delete_statement.rb +++ b/lib/arel/extensions/delete_statement.rb @@ -3,7 +3,7 @@ module Arel module Nodes - # https://www.postgresql.org/docs/9.5/sql-insert.html + # https://www.postgresql.org/docs/10/sql-delete.html class DeleteStatement module DeleteStatementExtension attr_accessor :using diff --git a/lib/arel/transformer/node.rb b/lib/arel/transformer/node.rb index 92c36ae2..5cef3113 100644 --- a/lib/arel/transformer/node.rb +++ b/lib/arel/transformer/node.rb @@ -7,6 +7,7 @@ class Node attr_reader :fields attr_reader :children attr_reader :root_node + attr_reader :context def initialize(object) @object = object @@ -15,6 +16,7 @@ def initialize(object) @fields = [] @children = {} @dirty = false + @context = {} end def inspect @@ -63,7 +65,7 @@ def add(path_node, node) def to_sql(engine = Table.engine) return nil if children.empty? - target_object = object.is_a?(Arel::SelectManager) ? object.ast : object + target_object = object.is_a?(Arel::TreeManager) ? object.ast : object collector = Arel::Collectors::SQLString.new collector = engine.connection.visitor.accept target_object, collector collector.value @@ -73,6 +75,15 @@ def [](key) @children.fetch(key) end + def child_at(path_items) + selected_node = self + path_items.each do |path_item| + selected_node = selected_node[path_item] + return nil if selected_node.nil? + end + selected_node + end + protected attr_writer :path diff --git a/lib/arel/transformer/visitor.rb b/lib/arel/transformer/visitor.rb index bd92681c..78ed58de 100644 --- a/lib/arel/transformer/visitor.rb +++ b/lib/arel/transformer/visitor.rb @@ -42,6 +42,8 @@ def process_node(arel_node, path_node) node = Arel::Transformer::Node.new(arel_node) current_node.add(path_node, node) + update_context(node) + with_node node do visit arel_node end @@ -54,6 +56,65 @@ def visit(object) def current_node @node_stack.last end + + # Context we can add: + # - this attribute is using in a projection, group, order, where + # - this node belongs to this "group", like SELECT statement class + # - helps for subscriptions: "How are table A and B related?" -> "Give me references to table A" -> "Are there references here which point to B?" + # - this Arel::Table node is a way of brining more data into the SQL query (FROM/JOIN) versus something else + # - this Arel::Attributes::Attribute belongs to this reference (allows to re-link unqualified columns) + # - + def update_context(node) + return unless node.object.is_a?(Arel::Table) + + context = node.context.merge!(range_variable: false, column_reference: false) + + # Using Arel::Table as SELECT ... FROM + if node.parent.object.is_a?(Arel::Nodes::JoinSource) + context[:range_variable] = true + # Using Arel::Table as SELECT ... FROM [
] + elsif node.parent.object.is_a?(Array) && node.parent.parent.object.is_a?(Arel::Nodes::JoinSource) + context[:range_variable] = true + # Using Arel::Table as SELECT ... INNER JOIN
ON TRUE + elsif node.parent.object.is_a?(Arel::Nodes::Join) + context[:range_variable] = true + + # Using Arel::Table as an attribute SELECT
.id ... + elsif node.parent.object.is_a?(Arel::Attributes::Attribute) + context[:column_reference] = true + + # Using Arel::Table in an INSERT INTO
+ elsif node.parent.object.is_a?(Arel::Nodes::InsertStatement) + context[:range_variable] = true + + # Using Arel::Table in an UPDATE
... + elsif node.parent.object.is_a?(Arel::Nodes::UpdateStatement) + context[:range_variable] = true + elsif node.parent.object.is_a?(Array) && node.parent.parent.object.is_a?(Arel::Nodes::UpdateStatement) + # Arel::Table in UPDATE ... FROM [
] + context[:range_variable] = true + + # Using Arel::Table in an DELETE FROM
+ elsif node.parent.object.is_a?(Arel::Nodes::DeleteStatement) + context[:range_variable] = true + elsif node.parent.object.is_a?(Array) && node.parent.parent.object.is_a?(Arel::Nodes::DeleteStatement) + # Arel::Table in DELETE ... USING [
] + context[:range_variable] = true + + # Using Arel::Table as an "alias" for WITH
AS (SELECT 1) SELECT 1 + elsif node.parent.object.is_a?(Arel::Nodes::As) && node.parent.parent.parent.object.is_a?(Arel::Nodes::With) + context[:alias] = true + # Using Arel::Table as an "alias" for WITH RECURSIVE
AS (SELECT 1) SELECT 1 + elsif node.parent.object.is_a?(Arel::Nodes::As) && node.parent.parent.parent.object.is_a?(Arel::Nodes::WithRecursive) + context[:alias] = true + # Using Arel::Table as an "alias" for SELECT INTO
... + elsif node.parent.object.is_a?(Arel::Nodes::Into) + context[:alias] = true + + else + raise "Unknown location for table #{node.inspect}, #{sql}" + end + end end # rubocop:enable Naming/MethodName end diff --git a/spec/arel/transformer_spec.rb b/spec/arel/transformer_spec.rb index 61871972..c16be3a3 100644 --- a/spec/arel/transformer_spec.rb +++ b/spec/arel/transformer_spec.rb @@ -155,58 +155,15 @@ expect(original_arel).to be_identical_arel(new_arel) end - it 'replaces table references with subqueries' do - sql = <<~SQL - SELECT - COUNT(posts.id), - ARRAY_AGG(posts.title) - FROM - posts - INNER JOIN - comments - ON - comments.post_id = posts.id - AND - comments.message ILIKE '%hello%' - WHERE - posts.public = TRUE - AND - posts.id IN (SELECT id FROM posts WHERE id = 3) - GROUP BY - comments.created_at - SQL - - # sql = <<~SQL - # SELECT - # id - # FROM - # posts - # SQL - - # sql = <<~SQL - # SELECT - # id - # FROM - # (SELECT id FROM posts) posts, - # users - # SQL - - # sql = <<~SQL - # SELECT - # id - # FROM - # users,posts - # SQL - - # only the tables inside JOIN and FROM should be replaced - # tables inside the JOIN ... ON expression should not be replaced - - arel = Arel.sql_to_arel(sql) - tree = Arel.transformer(arel.first) - - binding.pry - - tree.query(class: Arel::Nodes::JoinSource) + it 'adds additional context to Arel::Table nodes' do + result = Arel.sql_to_arel('SELECT posts.id FROM posts') + original_arel = result.first + + tree = Arel.transformer(original_arel) + from_table_node = tree.child_at(["ast", "cores", 0, "source", "left"]) + projection_table_node = tree.child_at(["ast", "cores", 0, "projections", 0, "relation"]) + expect(from_table_node.context).to eq({ range_variable: true, column_reference: false }) + expect(projection_table_node.context).to eq({ range_variable: false, column_reference: true }) end end From 1354ce93216e7bd3089cb9056eba5c593c14865b Mon Sep 17 00:00:00 2001 From: maarten Date: Fri, 26 Jul 2019 22:00:13 +0200 Subject: [PATCH 03/23] Extracted context enhancer for Arel::Table --- .rubocop.yml | 3 + lib/arel/sql_to_arel/pg_query_visitor.rb | 4 +- .../pg_query_visitor/frame_options.rb | 2 +- .../context_enhancer/arel_table.rb | 64 ++++++++++++++++ lib/arel/transformer/node.rb | 2 +- lib/arel/transformer/path_node.rb | 2 +- lib/arel/transformer/visitor.rb | 76 +++++-------------- spec/arel/transformer_spec.rb | 8 +- .../prints_a_pretty_ast.approved.txt | 56 +++++++------- spec/pg_search_spec.rb | 2 +- .../dummy/config/environments/development.rb | 2 +- .../support/dummy/config/environments/test.rb | 2 +- 12 files changed, 125 insertions(+), 98 deletions(-) create mode 100644 lib/arel/transformer/context_enhancer/arel_table.rb diff --git a/.rubocop.yml b/.rubocop.yml index 7c5e8a4b..0d4f45cc 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -31,6 +31,9 @@ Metrics/BlockLength: Style/Documentation: Enabled: false +Style/TrailingCommaInHashLiteral: + EnforcedStyleForMultiline: comma + Style/TrailingCommaInArrayLiteral: EnforcedStyleForMultiline: comma diff --git a/lib/arel/sql_to_arel/pg_query_visitor.rb b/lib/arel/sql_to_arel/pg_query_visitor.rb index b2dad442..56ee8d1b 100644 --- a/lib/arel/sql_to_arel/pg_query_visitor.rb +++ b/lib/arel/sql_to_arel/pg_query_visitor.rb @@ -540,12 +540,12 @@ def visit_LockingClause(strength:, wait_policy:) 1 => 'FOR KEY SHARE', 2 => 'FOR SHARE', 3 => 'FOR NO KEY UPDATE', - 4 => 'FOR UPDATE' + 4 => 'FOR UPDATE', }.fetch(strength) wait_policy_clause = { 0 => '', 1 => ' SKIP LOCKED', - 2 => ' NOWAIT' + 2 => ' NOWAIT', }.fetch(wait_policy) Arel::Nodes::Lock.new Arel.sql("#{strength_clause}#{wait_policy_clause}") diff --git a/lib/arel/sql_to_arel/pg_query_visitor/frame_options.rb b/lib/arel/sql_to_arel/pg_query_visitor/frame_options.rb index 29e642c1..3d7d5f43 100644 --- a/lib/arel/sql_to_arel/pg_query_visitor/frame_options.rb +++ b/lib/arel/sql_to_arel/pg_query_visitor/frame_options.rb @@ -56,7 +56,7 @@ def arel(frame_options, start_offset, end_offset) 'FRAMEOPTION_START_VALUE_PRECEDING' => 0x00400, 'FRAMEOPTION_END_VALUE_PRECEDING' => 0x00800, 'FRAMEOPTION_START_VALUE_FOLLOWING' => 0x01000, - 'FRAMEOPTION_END_VALUE_FOLLOWING' => 0x02000 + 'FRAMEOPTION_END_VALUE_FOLLOWING' => 0x02000, }.freeze def biggest_detractable_number(number, candidates) diff --git a/lib/arel/transformer/context_enhancer/arel_table.rb b/lib/arel/transformer/context_enhancer/arel_table.rb new file mode 100644 index 00000000..005c13cc --- /dev/null +++ b/lib/arel/transformer/context_enhancer/arel_table.rb @@ -0,0 +1,64 @@ +module Arel + module Transformer + module ContextEnhancer + class ArelTable + # Context we can add: + # - this attribute is using in a projection, group, order, where + # - this node belongs to this "group", like SELECT statement class + # - helps for subscriptions: "How are table A and B related?" -> "Give me references to table A" -> "Are there references here which point to B?" + # - this Arel::Table node is a way of brining more data into the SQL query (FROM/JOIN) versus something else + # - this Arel::Attributes::Attribute belongs to this reference (allows to re-link unqualified columns) + # - + def self.call(node) + context = node.context.merge!(range_variable: false, column_reference: false) + + # Using Arel::Table as SELECT ... FROM
+ if node.parent.object.is_a?(Arel::Nodes::JoinSource) + context[:range_variable] = true + # Using Arel::Table as SELECT ... FROM [
] + elsif node.parent.object.is_a?(Array) && node.parent.parent.object.is_a?(Arel::Nodes::JoinSource) + context[:range_variable] = true + # Using Arel::Table as SELECT ... INNER JOIN
ON TRUE + elsif node.parent.object.is_a?(Arel::Nodes::Join) + context[:range_variable] = true + + # Using Arel::Table as an attribute SELECT
.id ... + elsif node.parent.object.is_a?(Arel::Attributes::Attribute) + context[:column_reference] = true + + # Using Arel::Table in an INSERT INTO
+ elsif node.parent.object.is_a?(Arel::Nodes::InsertStatement) + context[:range_variable] = true + + # Using Arel::Table in an UPDATE
... + elsif node.parent.object.is_a?(Arel::Nodes::UpdateStatement) + context[:range_variable] = true + elsif node.parent.object.is_a?(Array) && node.parent.parent.object.is_a?(Arel::Nodes::UpdateStatement) + # Arel::Table in UPDATE ... FROM [
] + context[:range_variable] = true + + # Using Arel::Table in an DELETE FROM
+ elsif node.parent.object.is_a?(Arel::Nodes::DeleteStatement) + context[:range_variable] = true + elsif node.parent.object.is_a?(Array) && node.parent.parent.object.is_a?(Arel::Nodes::DeleteStatement) + # Arel::Table in DELETE ... USING [
] + context[:range_variable] = true + + # Using Arel::Table as an "alias" for WITH
AS (SELECT 1) SELECT 1 + elsif node.parent.object.is_a?(Arel::Nodes::As) && node.parent.parent.parent.object.is_a?(Arel::Nodes::With) + context[:alias] = true + # Using Arel::Table as an "alias" for WITH RECURSIVE
AS (SELECT 1) SELECT 1 + elsif node.parent.object.is_a?(Arel::Nodes::As) && node.parent.parent.parent.object.is_a?(Arel::Nodes::WithRecursive) + context[:alias] = true + # Using Arel::Table as an "alias" for SELECT INTO
... + elsif node.parent.object.is_a?(Arel::Nodes::Into) + context[:alias] = true + + else + raise "Unknown AST location for table #{node.inspect}, #{sql}" + end + end + end + end + end +end diff --git a/lib/arel/transformer/node.rb b/lib/arel/transformer/node.rb index 5cef3113..b79ab82f 100644 --- a/lib/arel/transformer/node.rb +++ b/lib/arel/transformer/node.rb @@ -75,7 +75,7 @@ def [](key) @children.fetch(key) end - def child_at(path_items) + def child_at_path(path_items) selected_node = self path_items.each do |path_item| selected_node = selected_node[path_item] diff --git a/lib/arel/transformer/path_node.rb b/lib/arel/transformer/path_node.rb index cff9e85c..c9e209d0 100644 --- a/lib/arel/transformer/path_node.rb +++ b/lib/arel/transformer/path_node.rb @@ -16,7 +16,7 @@ def arguments? def inspect case value when String - "\"#{value}\"" + "'#{value}'" else value.inspect end diff --git a/lib/arel/transformer/visitor.rb b/lib/arel/transformer/visitor.rb index 78ed58de..742fd2da 100644 --- a/lib/arel/transformer/visitor.rb +++ b/lib/arel/transformer/visitor.rb @@ -1,13 +1,25 @@ +require_relative './context_enhancer/arel_table' + module Arel module Transformer # rubocop:disable Naming/MethodName class Visitor < Arel::Visitors::Dot - def accept(object) + DEFAULT_CONTEXT_ENHANCERS = { + Arel::Table => Arel::Transformer::ContextEnhancer::ArelTable, + }.freeze + + attr_reader :context_enhancers + + def accept(object, context_enhancers = DEFAULT_CONTEXT_ENHANCERS) + @context_enhancers = context_enhancers + root_node = Arel::Transformer::Node.new(object) accept_with_root(object, root_node) end - def accept_with_root(object, root_node) + def accept_with_root(object, root_node, context_enhancers = DEFAULT_CONTEXT_ENHANCERS) + @context_enhancers = context_enhancers + with_node(root_node) do visit object end @@ -57,63 +69,11 @@ def current_node @node_stack.last end - # Context we can add: - # - this attribute is using in a projection, group, order, where - # - this node belongs to this "group", like SELECT statement class - # - helps for subscriptions: "How are table A and B related?" -> "Give me references to table A" -> "Are there references here which point to B?" - # - this Arel::Table node is a way of brining more data into the SQL query (FROM/JOIN) versus something else - # - this Arel::Attributes::Attribute belongs to this reference (allows to re-link unqualified columns) - # - def update_context(node) - return unless node.object.is_a?(Arel::Table) - - context = node.context.merge!(range_variable: false, column_reference: false) - - # Using Arel::Table as SELECT ... FROM
- if node.parent.object.is_a?(Arel::Nodes::JoinSource) - context[:range_variable] = true - # Using Arel::Table as SELECT ... FROM [
] - elsif node.parent.object.is_a?(Array) && node.parent.parent.object.is_a?(Arel::Nodes::JoinSource) - context[:range_variable] = true - # Using Arel::Table as SELECT ... INNER JOIN
ON TRUE - elsif node.parent.object.is_a?(Arel::Nodes::Join) - context[:range_variable] = true - - # Using Arel::Table as an attribute SELECT
.id ... - elsif node.parent.object.is_a?(Arel::Attributes::Attribute) - context[:column_reference] = true - - # Using Arel::Table in an INSERT INTO
- elsif node.parent.object.is_a?(Arel::Nodes::InsertStatement) - context[:range_variable] = true - - # Using Arel::Table in an UPDATE
... - elsif node.parent.object.is_a?(Arel::Nodes::UpdateStatement) - context[:range_variable] = true - elsif node.parent.object.is_a?(Array) && node.parent.parent.object.is_a?(Arel::Nodes::UpdateStatement) - # Arel::Table in UPDATE ... FROM [
] - context[:range_variable] = true - - # Using Arel::Table in an DELETE FROM
- elsif node.parent.object.is_a?(Arel::Nodes::DeleteStatement) - context[:range_variable] = true - elsif node.parent.object.is_a?(Array) && node.parent.parent.object.is_a?(Arel::Nodes::DeleteStatement) - # Arel::Table in DELETE ... USING [
] - context[:range_variable] = true - - # Using Arel::Table as an "alias" for WITH
AS (SELECT 1) SELECT 1 - elsif node.parent.object.is_a?(Arel::Nodes::As) && node.parent.parent.parent.object.is_a?(Arel::Nodes::With) - context[:alias] = true - # Using Arel::Table as an "alias" for WITH RECURSIVE
AS (SELECT 1) SELECT 1 - elsif node.parent.object.is_a?(Arel::Nodes::As) && node.parent.parent.parent.object.is_a?(Arel::Nodes::WithRecursive) - context[:alias] = true - # Using Arel::Table as an "alias" for SELECT INTO
... - elsif node.parent.object.is_a?(Arel::Nodes::Into) - context[:alias] = true - - else - raise "Unknown location for table #{node.inspect}, #{sql}" - end + enhancer = context_enhancers[node.object.class] + return if enhancer.nil? + + enhancer.call(node) end end # rubocop:enable Naming/MethodName diff --git a/spec/arel/transformer_spec.rb b/spec/arel/transformer_spec.rb index c16be3a3..ec18154f 100644 --- a/spec/arel/transformer_spec.rb +++ b/spec/arel/transformer_spec.rb @@ -160,10 +160,10 @@ original_arel = result.first tree = Arel.transformer(original_arel) - from_table_node = tree.child_at(["ast", "cores", 0, "source", "left"]) - projection_table_node = tree.child_at(["ast", "cores", 0, "projections", 0, "relation"]) + from_table_node = tree.child_at_path(['ast', 'cores', 0, 'source', 'left']) + projection_table_node = tree.child_at_path(['ast', 'cores', 0, 'projections', 0, 'relation']) - expect(from_table_node.context).to eq({ range_variable: true, column_reference: false }) - expect(projection_table_node.context).to eq({ range_variable: false, column_reference: true }) + expect(from_table_node.context).to eq(range_variable: true, column_reference: false) + expect(projection_table_node.context).to eq(range_variable: false, column_reference: true) end end diff --git a/spec/fixtures/approvals/areltransformer/prints_a_pretty_ast.approved.txt b/spec/fixtures/approvals/areltransformer/prints_a_pretty_ast.approved.txt index 8f31270d..f89fc7a1 100644 --- a/spec/fixtures/approvals/areltransformer/prints_a_pretty_ast.approved.txt +++ b/spec/fixtures/approvals/areltransformer/prints_a_pretty_ast.approved.txt @@ -2,96 +2,96 @@ sql = SELECT 1, 2 FROM "posts" WHERE "id" = 1 parent = nil ast = - only = - schema_name = - relpersistence = - > right = - > projections = - 1 = - > wheres = - name = - > > right = - > @@ -99,29 +99,29 @@ > > windows = - into = - > > limit = - orders = - offset = - union = - > diff --git a/spec/pg_search_spec.rb b/spec/pg_search_spec.rb index bc18f5cb..76ed2c7c 100644 --- a/spec/pg_search_spec.rb +++ b/spec/pg_search_spec.rb @@ -57,7 +57,7 @@ class Salami < ActiveRecord::Base pg_search_scope :tasty_search, associated_against: { cheeses: %i[kind brand], - cracker: :kind + cracker: :kind, } end diff --git a/spec/support/dummy/config/environments/development.rb b/spec/support/dummy/config/environments/development.rb index 7b5ddb21..e4653ee6 100644 --- a/spec/support/dummy/config/environments/development.rb +++ b/spec/support/dummy/config/environments/development.rb @@ -19,7 +19,7 @@ config.cache_store = :memory_store config.public_file_server.headers = { - 'Cache-Control' => "public, max-age=#{2.days.to_i}" + 'Cache-Control' => "public, max-age=#{2.days.to_i}", } else config.action_controller.perform_caching = false diff --git a/spec/support/dummy/config/environments/test.rb b/spec/support/dummy/config/environments/test.rb index bf3aeb3f..b2bff210 100644 --- a/spec/support/dummy/config/environments/test.rb +++ b/spec/support/dummy/config/environments/test.rb @@ -15,7 +15,7 @@ # Configure public file server for tests with Cache-Control for performance. config.public_file_server.enabled = true config.public_file_server.headers = { - 'Cache-Control' => "public, max-age=#{1.hour.to_i}" + 'Cache-Control' => "public, max-age=#{1.hour.to_i}", } # Show full error reports and disable caching. From 21bd87ae0b92bc891419d502dd4af91d8b59d3fc Mon Sep 17 00:00:00 2001 From: maarten Date: Fri, 26 Jul 2019 22:01:07 +0200 Subject: [PATCH 04/23] Sort rubocop --- .rubocop.yml | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index 0d4f45cc..af101286 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -3,18 +3,12 @@ AllCops: - 'gemfiles/arel_gems.gemfile' - 'gemfiles/default.gemfile' -Style/FrozenStringLiteralComment: - Enabled: false - Bundler/OrderedGems: Enabled: false Gemspec/OrderedDependencies: Enabled: false -Style/MultilineBlockChain: - Enabled: false - Metrics/LineLength: Enabled: true Max: 100 @@ -31,6 +25,9 @@ Metrics/BlockLength: Style/Documentation: Enabled: false +Style/MultilineBlockChain: + Enabled: false + Style/TrailingCommaInHashLiteral: EnforcedStyleForMultiline: comma @@ -40,6 +37,9 @@ Style/TrailingCommaInArrayLiteral: Style/TrailingCommaInArguments: EnforcedStyleForMultiline: comma +Style/FrozenStringLiteralComment: + Enabled: false + Layout/MultilineMethodCallIndentation: Enabled: true EnforcedStyle: indented From 71ad35e6537eaabfcf3fcc6d558bb6388c44dbbd Mon Sep 17 00:00:00 2001 From: maarten Date: Fri, 26 Jul 2019 23:08:51 +0200 Subject: [PATCH 05/23] Fixed lint issues --- .../context_enhancer/arel_table.rb | 53 +++++++++++-------- 1 file changed, 32 insertions(+), 21 deletions(-) diff --git a/lib/arel/transformer/context_enhancer/arel_table.rb b/lib/arel/transformer/context_enhancer/arel_table.rb index 005c13cc..a8ec0c87 100644 --- a/lib/arel/transformer/context_enhancer/arel_table.rb +++ b/lib/arel/transformer/context_enhancer/arel_table.rb @@ -2,62 +2,73 @@ module Arel module Transformer module ContextEnhancer class ArelTable - # Context we can add: - # - this attribute is using in a projection, group, order, where - # - this node belongs to this "group", like SELECT statement class - # - helps for subscriptions: "How are table A and B related?" -> "Give me references to table A" -> "Are there references here which point to B?" - # - this Arel::Table node is a way of brining more data into the SQL query (FROM/JOIN) versus something else - # - this Arel::Attributes::Attribute belongs to this reference (allows to re-link unqualified columns) - # - + # rubocop:disable Metrics/PerceivedComplexity + # rubocop:disable Metrics/CyclomaticComplexity + # rubocop:disable Metrics/AbcSize def self.call(node) context = node.context.merge!(range_variable: false, column_reference: false) + parent_object = node.parent.object # Using Arel::Table as SELECT ... FROM
- if node.parent.object.is_a?(Arel::Nodes::JoinSource) + if parent_object.is_a?(Arel::Nodes::JoinSource) context[:range_variable] = true + # Using Arel::Table as SELECT ... FROM [
] - elsif node.parent.object.is_a?(Array) && node.parent.parent.object.is_a?(Arel::Nodes::JoinSource) + elsif parent_object.is_a?(Array) && + node.parent.parent.object.is_a?(Arel::Nodes::JoinSource) context[:range_variable] = true + # Using Arel::Table as SELECT ... INNER JOIN
ON TRUE - elsif node.parent.object.is_a?(Arel::Nodes::Join) + elsif parent_object.is_a?(Arel::Nodes::Join) context[:range_variable] = true # Using Arel::Table as an attribute SELECT
.id ... - elsif node.parent.object.is_a?(Arel::Attributes::Attribute) + elsif parent_object.is_a?(Arel::Attributes::Attribute) context[:column_reference] = true # Using Arel::Table in an INSERT INTO
- elsif node.parent.object.is_a?(Arel::Nodes::InsertStatement) + elsif parent_object.is_a?(Arel::Nodes::InsertStatement) context[:range_variable] = true # Using Arel::Table in an UPDATE
... - elsif node.parent.object.is_a?(Arel::Nodes::UpdateStatement) + elsif parent_object.is_a?(Arel::Nodes::UpdateStatement) context[:range_variable] = true - elsif node.parent.object.is_a?(Array) && node.parent.parent.object.is_a?(Arel::Nodes::UpdateStatement) - # Arel::Table in UPDATE ... FROM [
] + + # Arel::Table in UPDATE ... FROM [
] + elsif parent_object.is_a?(Array) && + node.parent.parent.object.is_a?(Arel::Nodes::UpdateStatement) context[:range_variable] = true # Using Arel::Table in an DELETE FROM
- elsif node.parent.object.is_a?(Arel::Nodes::DeleteStatement) + elsif parent_object.is_a?(Arel::Nodes::DeleteStatement) context[:range_variable] = true - elsif node.parent.object.is_a?(Array) && node.parent.parent.object.is_a?(Arel::Nodes::DeleteStatement) - # Arel::Table in DELETE ... USING [
] + + # Arel::Table in DELETE ... USING [
] + elsif parent_object.is_a?(Array) && + node.parent.parent.object.is_a?(Arel::Nodes::DeleteStatement) context[:range_variable] = true # Using Arel::Table as an "alias" for WITH
AS (SELECT 1) SELECT 1 - elsif node.parent.object.is_a?(Arel::Nodes::As) && node.parent.parent.parent.object.is_a?(Arel::Nodes::With) + elsif parent_object.is_a?(Arel::Nodes::As) && + node.parent.parent.parent.object.is_a?(Arel::Nodes::With) context[:alias] = true + # Using Arel::Table as an "alias" for WITH RECURSIVE
AS (SELECT 1) SELECT 1 - elsif node.parent.object.is_a?(Arel::Nodes::As) && node.parent.parent.parent.object.is_a?(Arel::Nodes::WithRecursive) + elsif parent_object.is_a?(Arel::Nodes::As) && + node.parent.parent.parent.object.is_a?(Arel::Nodes::WithRecursive) context[:alias] = true + # Using Arel::Table as an "alias" for SELECT INTO
... - elsif node.parent.object.is_a?(Arel::Nodes::Into) + elsif parent_object.is_a?(Arel::Nodes::Into) context[:alias] = true else raise "Unknown AST location for table #{node.inspect}, #{sql}" end end + # rubocop:enable Metrics/PerceivedComplexity + # rubocop:enable Metrics/CyclomaticComplexity + # rubocop:enable Metrics/AbcSize end end end From 0a1e052b9076fde0897e30ba923bb3097e058b64 Mon Sep 17 00:00:00 2001 From: maarten Date: Fri, 26 Jul 2019 23:16:55 +0200 Subject: [PATCH 06/23] Moved the context enhancer spec --- .../transformer/context_enhancer/arel_table_spec.rb | 13 +++++++++++++ spec/arel/transformer_spec.rb | 12 ------------ 2 files changed, 13 insertions(+), 12 deletions(-) create mode 100644 spec/arel/transformer/context_enhancer/arel_table_spec.rb diff --git a/spec/arel/transformer/context_enhancer/arel_table_spec.rb b/spec/arel/transformer/context_enhancer/arel_table_spec.rb new file mode 100644 index 00000000..a1a91618 --- /dev/null +++ b/spec/arel/transformer/context_enhancer/arel_table_spec.rb @@ -0,0 +1,13 @@ +describe Arel::Transformer::ContextEnhancer::ArelTable do + it 'adds additional context to Arel::Table nodes' do + result = Arel.sql_to_arel('SELECT posts.id FROM posts') + original_arel = result.first + + tree = Arel.transformer(original_arel) + from_table_node = tree.child_at_path(['ast', 'cores', 0, 'source', 'left']) + projection_table_node = tree.child_at_path(['ast', 'cores', 0, 'projections', 0, 'relation']) + + expect(from_table_node.context).to eq(range_variable: true, column_reference: false) + expect(projection_table_node.context).to eq(range_variable: false, column_reference: true) + end +end diff --git a/spec/arel/transformer_spec.rb b/spec/arel/transformer_spec.rb index ec18154f..108256af 100644 --- a/spec/arel/transformer_spec.rb +++ b/spec/arel/transformer_spec.rb @@ -154,16 +154,4 @@ expect(original_arel).to be_identical_arel(new_arel) end - - it 'adds additional context to Arel::Table nodes' do - result = Arel.sql_to_arel('SELECT posts.id FROM posts') - original_arel = result.first - - tree = Arel.transformer(original_arel) - from_table_node = tree.child_at_path(['ast', 'cores', 0, 'source', 'left']) - projection_table_node = tree.child_at_path(['ast', 'cores', 0, 'projections', 0, 'relation']) - - expect(from_table_node.context).to eq(range_variable: true, column_reference: false) - expect(projection_table_node.context).to eq(range_variable: false, column_reference: true) - end end From 8dee8548e3b7060b9310de3ee918cc1a656a96d5 Mon Sep 17 00:00:00 2001 From: maarten Date: Fri, 26 Jul 2019 23:17:06 +0200 Subject: [PATCH 07/23] Fixed exception message in ArelTable context enhancer --- lib/arel/transformer/context_enhancer/arel_table.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/arel/transformer/context_enhancer/arel_table.rb b/lib/arel/transformer/context_enhancer/arel_table.rb index a8ec0c87..6a3d159a 100644 --- a/lib/arel/transformer/context_enhancer/arel_table.rb +++ b/lib/arel/transformer/context_enhancer/arel_table.rb @@ -63,7 +63,7 @@ def self.call(node) context[:alias] = true else - raise "Unknown AST location for table #{node.inspect}, #{sql}" + raise "Unknown AST location for table #{node.inspect}, #{node.root_node.to_sql}" end end # rubocop:enable Metrics/PerceivedComplexity From 14ab884a8c3a7d78a2c95aa49644dd181cc02ce4 Mon Sep 17 00:00:00 2001 From: maarten Date: Sun, 28 Jul 2019 11:47:01 +0200 Subject: [PATCH 08/23] Fix comparison for select,update,insert and delete manager --- lib/arel/extensions/delete_manager.rb | 2 +- lib/arel/extensions/insert_manager.rb | 2 +- lib/arel/extensions/select_manager.rb | 2 +- lib/arel/extensions/update_manager.rb | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/arel/extensions/delete_manager.rb b/lib/arel/extensions/delete_manager.rb index fb7a0153..ebefc06c 100644 --- a/lib/arel/extensions/delete_manager.rb +++ b/lib/arel/extensions/delete_manager.rb @@ -4,7 +4,7 @@ module Arel class DeleteManager < Arel::TreeManager def ==(other) - @ast == other.ast && @ctx == other.ctx + other.is_a?(self.class) && @ast == other.ast && @ctx == other.ctx end protected diff --git a/lib/arel/extensions/insert_manager.rb b/lib/arel/extensions/insert_manager.rb index f4cc1d2b..7dedb450 100644 --- a/lib/arel/extensions/insert_manager.rb +++ b/lib/arel/extensions/insert_manager.rb @@ -4,7 +4,7 @@ module Arel class InsertManager < Arel::TreeManager def ==(other) - @ast == other.ast + other.is_a?(self.class) && @ast == other.ast end end diff --git a/lib/arel/extensions/select_manager.rb b/lib/arel/extensions/select_manager.rb index b5539d94..90c74ee1 100644 --- a/lib/arel/extensions/select_manager.rb +++ b/lib/arel/extensions/select_manager.rb @@ -4,7 +4,7 @@ module Arel class SelectManager def ==(other) - @ast == other.ast && @ctx == other.ctx + other.is_a?(self.class) && @ast == other.ast && @ctx == other.ctx end protected diff --git a/lib/arel/extensions/update_manager.rb b/lib/arel/extensions/update_manager.rb index 87337356..dc986ddf 100644 --- a/lib/arel/extensions/update_manager.rb +++ b/lib/arel/extensions/update_manager.rb @@ -4,7 +4,7 @@ module Arel class UpdateManager < Arel::TreeManager def ==(other) - @ast == other.ast && @ctx == other.ctx + other.is_a?(self.class) && @ast == other.ast && @ctx == other.ctx end protected From b49a7e454bf4ac76dc09f316d79f436cab60ba0e Mon Sep 17 00:00:00 2001 From: maarten Date: Sun, 28 Jul 2019 11:48:00 +0200 Subject: [PATCH 09/23] Added .query method to Node --- lib/arel/transformer.rb | 1 + lib/arel/transformer/node.rb | 5 +++ lib/arel/transformer/query.rb | 59 +++++++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+) create mode 100644 lib/arel/transformer/query.rb diff --git a/lib/arel/transformer.rb b/lib/arel/transformer.rb index 38fd2b48..2580e594 100644 --- a/lib/arel/transformer.rb +++ b/lib/arel/transformer.rb @@ -1,6 +1,7 @@ require_relative './transformer/node' require_relative './transformer/path' require_relative './transformer/path_node' +require_relative './transformer/query' require_relative './transformer/visitor' module Arel diff --git a/lib/arel/transformer/node.rb b/lib/arel/transformer/node.rb index b79ab82f..289f7f92 100644 --- a/lib/arel/transformer/node.rb +++ b/lib/arel/transformer/node.rb @@ -84,6 +84,10 @@ def child_at_path(path_items) selected_node end + def query(**kwargs) + Arel::Transformer::Query.call(self, kwargs) + end + protected attr_writer :path @@ -93,6 +97,7 @@ def child_at_path(path_items) # rubocop:disable Metrics/AbcSize # rubocop:disable Metrics/CyclomaticComplexity # rubocop:disable Metrics/PerceivedComplexity + # TODO: make the scalar values, string/boolean/integer/float more dense def recursive_inspect(string, indent = 1) string << "<#{inspect_name} #{path.inspect}\n" string << "#{spacing(indent)}sql = #{to_sql}\n" unless to_sql.nil? diff --git a/lib/arel/transformer/query.rb b/lib/arel/transformer/query.rb new file mode 100644 index 00000000..de996a1c --- /dev/null +++ b/lib/arel/transformer/query.rb @@ -0,0 +1,59 @@ +module Arel + module Transformer + class Query + def self.call(node, kwargs) + node_attributes = [:context, :parent] + node_args = kwargs.slice(*node_attributes) + object_args = kwargs.except(*node_attributes) + + # TODO: + # parent refers to a Node, so the matcher probably needs to be a hash: + # t.query(parent: { class: Arel::Table }) + + # query(context: { foo: :bar }) + # should match a node with a context of { foo: :bar, papi: :chulo } + # we're matching Hash against Hash, so in that case choose the "includes" strategy? + + # query(parent: Arel::Table) + # which is short hand for + # query(parent: { class: { Arel::Table } }) + # and also short hand for + # query(parent: { object: { class: { Arel::Table } } } }) + # query(parent: { context: { range_variable: true } }) + + node.each.select do |child_node| + next unless matches?(child_node, node_args) + + matches?(child_node.object, object_args) + end + end + + private + + # { context: { kerk: :shine } } + # Node.context shine> + def self.matches?(object, test) + # If test value is a Hash, maybe go recursive. + # all else test values are direct compare? + case test + when Hash + # When the object is a Hash, compare hashes using subset + # otherwise check if all members exist on object + case object + when Hash + test <= object + else + test.all? do |test_key, test_value| + next false unless object.respond_to?(test_key) + + object_attribute_value = object.public_send(test_key) + matches? object_attribute_value, test_value + end + end + else + object == test + end + end + end + end +end From 852f878c8b70c3db9ca35abd4104e8223d8d01ba Mon Sep 17 00:00:00 2001 From: maarten Date: Sun, 28 Jul 2019 11:48:33 +0200 Subject: [PATCH 10/23] Added AddchemaToTable transformer --- lib/arel/transformer/add_schema_to_table.rb | 25 +++++++++++++++++++ .../transformer/add_schema_to_table_spec.rb | 10 ++++++++ .../context_enhancer/arel_table_spec.rb | 2 ++ 3 files changed, 37 insertions(+) create mode 100644 lib/arel/transformer/add_schema_to_table.rb create mode 100644 spec/arel/transformer/add_schema_to_table_spec.rb diff --git a/lib/arel/transformer/add_schema_to_table.rb b/lib/arel/transformer/add_schema_to_table.rb new file mode 100644 index 00000000..cde288ab --- /dev/null +++ b/lib/arel/transformer/add_schema_to_table.rb @@ -0,0 +1,25 @@ +module Arel + module Transformer + class AddSchemaToTable + attr_reader :schema_name + + def initialize(schema_name) + @schema_name = schema_name + end + + def call(arel) + tree = Arel.transformer(arel) + + tree.query( + class: Arel::Table, + schema_name: nil, + context: { range_variable: true }, + ).each do |node| + node['schema_name'].replace(schema_name) + end + + tree.object + end + end + end +end diff --git a/spec/arel/transformer/add_schema_to_table_spec.rb b/spec/arel/transformer/add_schema_to_table_spec.rb new file mode 100644 index 00000000..5981efc2 --- /dev/null +++ b/spec/arel/transformer/add_schema_to_table_spec.rb @@ -0,0 +1,10 @@ +describe Arel::Transformer::AddSchemaToTable do + it 'works' do + transformer = Arel::Transformer::AddSchemaToTable.new('secret') + sql = 'SELECT posts.id FROM posts' + result = Arel.sql_to_arel(sql) + new_sql = transformer.call(result.first).to_sql + + expect(new_sql).to eq 'SELECT "posts"."id" FROM "secret"."posts"' + end +end diff --git a/spec/arel/transformer/context_enhancer/arel_table_spec.rb b/spec/arel/transformer/context_enhancer/arel_table_spec.rb index a1a91618..e14d711d 100644 --- a/spec/arel/transformer/context_enhancer/arel_table_spec.rb +++ b/spec/arel/transformer/context_enhancer/arel_table_spec.rb @@ -10,4 +10,6 @@ expect(from_table_node.context).to eq(range_variable: true, column_reference: false) expect(projection_table_node.context).to eq(range_variable: false, column_reference: true) end + + # TODO: add all cases here :scream: end From c10744e0480adcece808708bdd67a719c83ea530 Mon Sep 17 00:00:00 2001 From: maarten Date: Sun, 28 Jul 2019 11:48:54 +0200 Subject: [PATCH 11/23] Add the required require --- lib/arel/transformer.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/arel/transformer.rb b/lib/arel/transformer.rb index 2580e594..7c558e70 100644 --- a/lib/arel/transformer.rb +++ b/lib/arel/transformer.rb @@ -4,6 +4,8 @@ require_relative './transformer/query' require_relative './transformer/visitor' +require_relative './transformer/add_schema_to_table' + module Arel module Transformer end From 89021d50ed62e3377cb51493fa195d5e6c962a6d Mon Sep 17 00:00:00 2001 From: maarten Date: Mon, 29 Jul 2019 16:56:01 +0200 Subject: [PATCH 12/23] Hack to support context argument --- lib/arel/transformer/add_schema_to_table.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/arel/transformer/add_schema_to_table.rb b/lib/arel/transformer/add_schema_to_table.rb index cde288ab..7f63d7bf 100644 --- a/lib/arel/transformer/add_schema_to_table.rb +++ b/lib/arel/transformer/add_schema_to_table.rb @@ -7,7 +7,7 @@ def initialize(schema_name) @schema_name = schema_name end - def call(arel) + def call(arel, _context) tree = Arel.transformer(arel) tree.query( From efb24ba9d9010452849b6813fc33cb211a7806a5 Mon Sep 17 00:00:00 2001 From: maarten Date: Tue, 30 Jul 2019 18:24:58 +0200 Subject: [PATCH 13/23] Add support for schemas in function calls --- lib/arel/extensions/function.rb | 3 +++ lib/arel/extensions/table.rb | 2 +- lib/arel/sql_to_arel/pg_query_visitor.rb | 17 +++++++++++++---- spec/arel/sql_to_arel_spec.rb | 1 + .../transformer/add_schema_to_table_spec.rb | 2 +- 5 files changed, 19 insertions(+), 6 deletions(-) diff --git a/lib/arel/extensions/function.rb b/lib/arel/extensions/function.rb index 4b188ee4..be7c0a51 100644 --- a/lib/arel/extensions/function.rb +++ b/lib/arel/extensions/function.rb @@ -10,6 +10,8 @@ module FunctionExtension attr_accessor :filter attr_accessor :within_group attr_accessor :variardic + # postgres only: https://www.postgresql.org/docs/10/ddl-schemas.html + attr_accessor :schema_name def initialize(expr, aliaz = nil) super @@ -31,6 +33,7 @@ class ToSql # rubocop:disable Metrics/CyclomaticComplexity # rubocop:disable Metrics/AbcSize def aggregate(name, o, collector) + collector << "#{o.schema_name}." if o.schema_name collector << "#{name}(" collector << 'DISTINCT ' if o.distinct collector << 'VARIADIC ' if o.variardic diff --git a/lib/arel/extensions/table.rb b/lib/arel/extensions/table.rb index 28f4949e..1ef2e078 100644 --- a/lib/arel/extensions/table.rb +++ b/lib/arel/extensions/table.rb @@ -7,7 +7,7 @@ class Table module TableExtension # postgres only: https://www.postgresql.org/docs/9.5/sql-select.html attr_accessor :only - # postgres only: https://www.postgresql.org/docs/9.5/ddl-schemas.html + # postgres only: https://www.postgresql.org/docs/10/ddl-schemas.html attr_accessor :schema_name # postgres only: https://www.postgresql.org/docs/9.1/catalog-pg-class.html attr_accessor :relpersistence diff --git a/lib/arel/sql_to_arel/pg_query_visitor.rb b/lib/arel/sql_to_arel/pg_query_visitor.rb index 56ee8d1b..d04f9c96 100644 --- a/lib/arel/sql_to_arel/pg_query_visitor.rb +++ b/lib/arel/sql_to_arel/pg_query_visitor.rb @@ -427,11 +427,20 @@ def visit_FuncCall( [Arel::Nodes::Overlaps.new(start1, end1, start2, end2)] else - if function_names.length > 1 - boom "Don't know how to handle function names `#{function_names}`" - end + case function_names.length + when 2 + if function_names.first == PG_CATALOG + boom "Missing postgres function `#{function_names.last}`" + end - Arel::Nodes::NamedFunction.new(function_names.first, args) + func = Arel::Nodes::NamedFunction.new(function_names.last, args) + func.schema_name = function_names.first + func + when 1 + Arel::Nodes::NamedFunction.new(function_names.first, args) + else + boom "Don't know how to handle function names length `#{function_names.length}`" + end end func.distinct = (agg_distinct.nil? ? false : true) unless func.is_a?(::Array) diff --git a/spec/arel/sql_to_arel_spec.rb b/spec/arel/sql_to_arel_spec.rb index 9d98b445..3b44c9ea 100644 --- a/spec/arel/sql_to_arel_spec.rb +++ b/spec/arel/sql_to_arel_spec.rb @@ -109,6 +109,7 @@ visit 'select', '1.9', pg_node: 'PgQuery::FLOAT' visit 'select', 'SUM("a") AS some_a_sum', pg_node: 'PgQuery::FUNC_CALL' visit 'select', 'RANK("b")', pg_node: 'PgQuery::FUNC_CALL' + visit 'select', 'public.rank("c")', pg_node: 'PgQuery::FUNC_CALL' visit 'select', 'COUNT("c")', pg_node: 'PgQuery::FUNC_CALL' visit 'select', 'GENERATE_SERIES(1, 5)', pg_node: 'PgQuery::FUNC_CALL' visit 'select', 'MAX("d")', pg_node: 'PgQuery::FUNC_CALL' diff --git a/spec/arel/transformer/add_schema_to_table_spec.rb b/spec/arel/transformer/add_schema_to_table_spec.rb index 5981efc2..176c17e7 100644 --- a/spec/arel/transformer/add_schema_to_table_spec.rb +++ b/spec/arel/transformer/add_schema_to_table_spec.rb @@ -3,7 +3,7 @@ transformer = Arel::Transformer::AddSchemaToTable.new('secret') sql = 'SELECT posts.id FROM posts' result = Arel.sql_to_arel(sql) - new_sql = transformer.call(result.first).to_sql + new_sql = transformer.call(result.first, nil).to_sql expect(new_sql).to eq 'SELECT "posts"."id" FROM "secret"."posts"' end From 1b0e0cff1851c99f45a5163faaa1844a12773c4f Mon Sep 17 00:00:00 2001 From: maarten Date: Tue, 30 Jul 2019 20:31:08 +0200 Subject: [PATCH 14/23] Added specs for updated tree managers --- spec/arel/extensions/delete_manager_spec.rb | 7 +++++++ spec/arel/extensions/insert_manager_spec.rb | 7 +++++++ spec/arel/extensions/select_manager_spec.rb | 7 +++++++ spec/arel/extensions/update_manager_spec.rb | 7 +++++++ 4 files changed, 28 insertions(+) diff --git a/spec/arel/extensions/delete_manager_spec.rb b/spec/arel/extensions/delete_manager_spec.rb index f97eb37c..98ecb284 100644 --- a/spec/arel/extensions/delete_manager_spec.rb +++ b/spec/arel/extensions/delete_manager_spec.rb @@ -13,5 +13,12 @@ expect(tree1).to_not eq(tree2) end + + it 'works for comparing other objects' do + tree = Arel::DeleteManager.new.tap { |d| d.from(Arel::Table.new('posts')) } + other_object = 'foo' + + expect(tree).to_not eq other_object + end end end diff --git a/spec/arel/extensions/insert_manager_spec.rb b/spec/arel/extensions/insert_manager_spec.rb index 40f818d9..ff606c2b 100644 --- a/spec/arel/extensions/insert_manager_spec.rb +++ b/spec/arel/extensions/insert_manager_spec.rb @@ -13,5 +13,12 @@ expect(tree1).to_not eq(tree2) end + + it 'works for comparing other objects' do + tree = Arel::InsertManager.new.tap { |i| i.into(Arel::Table.new('posts')) } + other_object = 'foo' + + expect(tree).to_not eq other_object + end end end diff --git a/spec/arel/extensions/select_manager_spec.rb b/spec/arel/extensions/select_manager_spec.rb index fe6bbfe1..3953411f 100644 --- a/spec/arel/extensions/select_manager_spec.rb +++ b/spec/arel/extensions/select_manager_spec.rb @@ -13,5 +13,12 @@ expect(tree1).to_not eq(tree2) end + + it 'works for comparing other objects' do + tree = Arel::SelectManager.new(Arel::Table.new('posts')) + other_object = 'foo' + + expect(tree).to_not eq other_object + end end end diff --git a/spec/arel/extensions/update_manager_spec.rb b/spec/arel/extensions/update_manager_spec.rb index e4e5905d..4010ac8b 100644 --- a/spec/arel/extensions/update_manager_spec.rb +++ b/spec/arel/extensions/update_manager_spec.rb @@ -13,5 +13,12 @@ expect(tree1).to_not eq(tree2) end + + it 'works for comparing other objects' do + tree = Arel::UpdateManager.new.tap { |u| u.table(Arel::Table.new('posts')) } + other_object = 'foo' + + expect(tree).to_not eq other_object + end end end From 5501264bd6d176e9e7bd2cd01067bce9c4509084 Mon Sep 17 00:00:00 2001 From: maarten Date: Tue, 30 Jul 2019 20:41:15 +0200 Subject: [PATCH 15/23] Added specs for the error branches of FuncCall --- .../arel/sql_to_arel/pg_query_visitor_spec.rb | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/spec/arel/sql_to_arel/pg_query_visitor_spec.rb b/spec/arel/sql_to_arel/pg_query_visitor_spec.rb index 136853df..53f9504f 100644 --- a/spec/arel/sql_to_arel/pg_query_visitor_spec.rb +++ b/spec/arel/sql_to_arel/pg_query_visitor_spec.rb @@ -81,6 +81,35 @@ end end + describe 'visit_FuncCall' do + it 'raises an exception when given an unknown pg_catalog function' do + funcname = [ + { 'String' => { 'str' => 'pg_catalog' } }, + { 'String' => { 'str' => 'some_new_function' } }, + ] + + expect do + described_class.new.send(:visit_FuncCall, funcname: funcname) + end.to raise_error do |error| + expect(error.message).to include('Missing postgres function `some_new_function`') + end + end + + it 'raises an exception when there are more than 2 function names' do + funcname = [ + { 'String' => { 'str' => 'some_schema' } }, + { 'String' => { 'str' => 'some_function' } }, + { 'String' => { 'str' => 'something_unknown' } }, + ] + + expect do + described_class.new.send(:visit_FuncCall, funcname: funcname) + end.to raise_error do |error| + expect(error.message).to include("Don't know how to handle function names length `3`") + end + end + end + describe 'visit_TypeName' do it 'raises an exception when typemod is not -1' do expect do From 9380796686e1395330b291741b591e0a66a06446 Mon Sep 17 00:00:00 2001 From: maarten Date: Tue, 30 Jul 2019 21:40:51 +0200 Subject: [PATCH 16/23] Add missing attributes to the SelectCore dot visitor --- lib/arel/extensions/select_statement.rb | 3 +++ .../areltransformer/prints_a_pretty_ast.approved.txt | 12 ++++++++++++ 2 files changed, 15 insertions(+) diff --git a/lib/arel/extensions/select_statement.rb b/lib/arel/extensions/select_statement.rb index 1128bb7b..77dc3a23 100644 --- a/lib/arel/extensions/select_statement.rb +++ b/lib/arel/extensions/select_statement.rb @@ -32,7 +32,10 @@ module SelectStatementExtension def visit_Arel_Nodes_SelectStatement(o) super + visit_edge o, 'lock' + visit_edge o, 'with' visit_edge o, 'union' + visit_edge o, 'values_lists' end end diff --git a/spec/fixtures/approvals/areltransformer/prints_a_pretty_ast.approved.txt b/spec/fixtures/approvals/areltransformer/prints_a_pretty_ast.approved.txt index f89fc7a1..e011d322 100644 --- a/spec/fixtures/approvals/areltransformer/prints_a_pretty_ast.approved.txt +++ b/spec/fixtures/approvals/areltransformer/prints_a_pretty_ast.approved.txt @@ -120,9 +120,21 @@ + lock = + + with = + union = + values_lists = + > > From 31c050352c09aee9b540fcbc7559a49fdf7c12b3 Mon Sep 17 00:00:00 2001 From: maarten Date: Tue, 30 Jul 2019 21:48:39 +0200 Subject: [PATCH 17/23] Added TODO to Arel middleware --- lib/arel/transformer/add_schema_to_table.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/arel/transformer/add_schema_to_table.rb b/lib/arel/transformer/add_schema_to_table.rb index 7f63d7bf..59ae2f19 100644 --- a/lib/arel/transformer/add_schema_to_table.rb +++ b/lib/arel/transformer/add_schema_to_table.rb @@ -7,6 +7,7 @@ def initialize(schema_name) @schema_name = schema_name end + # https://github.com/mvgijssel/arel_toolkit/issues/110 def call(arel, _context) tree = Arel.transformer(arel) From bd15cf2ee581e80bad907c87f99868efdf4e4033 Mon Sep 17 00:00:00 2001 From: maarten Date: Tue, 30 Jul 2019 21:48:50 +0200 Subject: [PATCH 18/23] Added specs for ArelTable context enhancer --- .../context_enhancer/arel_table_spec.rb | 120 +++++++++++++++++- 1 file changed, 119 insertions(+), 1 deletion(-) diff --git a/spec/arel/transformer/context_enhancer/arel_table_spec.rb b/spec/arel/transformer/context_enhancer/arel_table_spec.rb index e14d711d..f213b761 100644 --- a/spec/arel/transformer/context_enhancer/arel_table_spec.rb +++ b/spec/arel/transformer/context_enhancer/arel_table_spec.rb @@ -11,5 +11,123 @@ expect(projection_table_node.context).to eq(range_variable: false, column_reference: true) end - # TODO: add all cases here :scream: + it 'works for a single table inside FROM' do + sql = 'SELECT * FROM posts' + tree = Arel.transformer(Arel.sql_to_arel(sql).first) + table_node = tree.child_at_path(['ast', 'cores', 0, 'source', 'left']) + + expect(table_node.context).to eq(range_variable: true, column_reference: false) + end + + it 'works for multiple tables inside FROM' do + sql = 'SELECT * FROM posts, comments' + tree = Arel.transformer(Arel.sql_to_arel(sql).first) + + posts_table_node = tree.child_at_path ['ast', 'cores', 0, 'source', 'left', 0] + comments_table_node = tree.child_at_path ['ast', 'cores', 0, 'source', 'left', 1] + + expect(posts_table_node.context).to eq(range_variable: true, column_reference: false) + expect(comments_table_node.context).to eq(range_variable: true, column_reference: false) + end + + it 'works for joining tables' do + sql = 'SELECT * FROM posts INNER JOIN comments ON true LEFT JOIN users ON true' + tree = Arel.transformer(Arel.sql_to_arel(sql).first) + + users_table_node = tree.child_at_path ['ast', 'cores', 0, 'source', 'right', 1, 'left'] + comments_table_node = tree.child_at_path ['ast', 'cores', 0, 'source', 'right', 0, 'left'] + + expect(users_table_node.context).to eq(range_variable: true, column_reference: false) + expect(comments_table_node.context).to eq(range_variable: true, column_reference: false) + end + + it 'works for SELECT projections' do + sql = 'SELECT posts.id' + tree = Arel.transformer(Arel.sql_to_arel(sql).first) + attribute_table_node = tree.child_at_path(['ast', 'cores', 0, 'projections', 0, 'relation']) + + expect(attribute_table_node.context).to eq(range_variable: false, column_reference: true) + end + + it 'works for INSERT INTO table' do + sql = 'INSERT INTO posts (public) VALUES (true)' + tree = Arel.transformer(Arel.sql_to_arel(sql).first) + insert_table_node = tree.child_at_path(%w[ast relation]) + + expect(insert_table_node.context).to eq(range_variable: true, column_reference: false) + end + + it 'works for UPDATE table' do + sql = 'UPDATE posts SET public = TRUE' + tree = Arel.transformer(Arel.sql_to_arel(sql).first) + update_table_node = tree.child_at_path(%w[ast relation]) + + expect(update_table_node.context).to eq(range_variable: true, column_reference: false) + end + + it 'works for the UPDATE FROM tables' do + sql = 'UPDATE posts SET public = TRUE FROM users, comments' + tree = Arel.transformer(Arel.sql_to_arel(sql).first) + users_table_node = tree.child_at_path(['ast', 'froms', 0]) + comments_table_node = tree.child_at_path(['ast', 'froms', 1]) + + expect(users_table_node.context).to eq(range_variable: true, column_reference: false) + expect(comments_table_node.context).to eq(range_variable: true, column_reference: false) + end + + it 'works for DELETE table' do + sql = 'DELETE FROM posts' + tree = Arel.transformer(Arel.sql_to_arel(sql).first) + delete_table_node = tree.child_at_path(%w[ast relation]) + + expect(delete_table_node.context).to eq(range_variable: true, column_reference: false) + end + + it 'works for DELETE USING tables' do + sql = 'DELETE FROM posts USING users, comments' + tree = Arel.transformer(Arel.sql_to_arel(sql).first) + users_table_node = tree.child_at_path(['ast', 'using', 0]) + comments_table_node = tree.child_at_path(['ast', 'using', 1]) + + expect(users_table_node.context).to eq(range_variable: true, column_reference: false) + expect(comments_table_node.context).to eq(range_variable: true, column_reference: false) + end + + it 'works for a CTE table' do + sql = 'WITH posts AS (SELECT 1) SELECT * FROM posts' + tree = Arel.transformer(Arel.sql_to_arel(sql).first) + cte_node = tree.child_at_path(['ast', 'with', 'expr', 0, 'left']) + + expect(cte_node.context).to eq(range_variable: false, column_reference: false, alias: true) + end + + it 'works for a recursive CTE table' do + sql = 'WITH RECURSIVE posts AS (SELECT 1) SELECT * FROM posts' + tree = Arel.transformer(Arel.sql_to_arel(sql).first) + cte_node = tree.child_at_path(['ast', 'with', 'expr', 0, 'left']) + + expect(cte_node.context).to eq(range_variable: false, column_reference: false, alias: true) + end + + it 'works for a SELECT INTO table' do + sql = 'SELECT INTO public_posts FROM posts WHERE posts.public = TRUE' + tree = Arel.transformer(Arel.sql_to_arel(sql).first) + into_table_node = tree.child_at_path(['ast', 'cores', 0, 'into', 'expr']) + + expect(into_table_node.context) + .to eq(range_variable: false, column_reference: false, alias: true) + end + + it 'raises an error for an arel table in an unknown location' do + sql = 'SELECT 1' + tree = Arel.transformer(Arel.sql_to_arel(sql).first) + limit_node = tree.child_at_path(%w[ast limit]) + + expect do + limit_node.replace(Arel::Table.new('strange_table')) + end.to raise_error do |error| + expect(error.message) + .to include("Unknown AST location for table Date: Tue, 30 Jul 2019 22:03:54 +0200 Subject: [PATCH 19/23] Remove TODO --- lib/arel/transformer/node.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/arel/transformer/node.rb b/lib/arel/transformer/node.rb index 289f7f92..33e9fb59 100644 --- a/lib/arel/transformer/node.rb +++ b/lib/arel/transformer/node.rb @@ -97,7 +97,6 @@ def query(**kwargs) # rubocop:disable Metrics/AbcSize # rubocop:disable Metrics/CyclomaticComplexity # rubocop:disable Metrics/PerceivedComplexity - # TODO: make the scalar values, string/boolean/integer/float more dense def recursive_inspect(string, indent = 1) string << "<#{inspect_name} #{path.inspect}\n" string << "#{spacing(indent)}sql = #{to_sql}\n" unless to_sql.nil? From 6c0b4f9d48e3c6dd85252acdec6850c02b59800f Mon Sep 17 00:00:00 2001 From: maarten Date: Tue, 30 Jul 2019 22:04:04 +0200 Subject: [PATCH 20/23] Remove comments --- lib/arel/transformer/query.rb | 25 +------------------------ 1 file changed, 1 insertion(+), 24 deletions(-) diff --git a/lib/arel/transformer/query.rb b/lib/arel/transformer/query.rb index de996a1c..470cbf24 100644 --- a/lib/arel/transformer/query.rb +++ b/lib/arel/transformer/query.rb @@ -2,25 +2,10 @@ module Arel module Transformer class Query def self.call(node, kwargs) - node_attributes = [:context, :parent] + node_attributes = %i[context parent] node_args = kwargs.slice(*node_attributes) object_args = kwargs.except(*node_attributes) - # TODO: - # parent refers to a Node, so the matcher probably needs to be a hash: - # t.query(parent: { class: Arel::Table }) - - # query(context: { foo: :bar }) - # should match a node with a context of { foo: :bar, papi: :chulo } - # we're matching Hash against Hash, so in that case choose the "includes" strategy? - - # query(parent: Arel::Table) - # which is short hand for - # query(parent: { class: { Arel::Table } }) - # and also short hand for - # query(parent: { object: { class: { Arel::Table } } } }) - # query(parent: { context: { range_variable: true } }) - node.each.select do |child_node| next unless matches?(child_node, node_args) @@ -28,17 +13,9 @@ def self.call(node, kwargs) end end - private - - # { context: { kerk: :shine } } - # Node.context shine> def self.matches?(object, test) - # If test value is a Hash, maybe go recursive. - # all else test values are direct compare? case test when Hash - # When the object is a Hash, compare hashes using subset - # otherwise check if all members exist on object case object when Hash test <= object From 870aef1c6dd9616fb0846c6a1d8c849a631efa39 Mon Sep 17 00:00:00 2001 From: maarten Date: Tue, 30 Jul 2019 22:30:19 +0200 Subject: [PATCH 21/23] Added specs for Query --- spec/arel/transformer/query_spec.rb | 58 +++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 spec/arel/transformer/query_spec.rb diff --git a/spec/arel/transformer/query_spec.rb b/spec/arel/transformer/query_spec.rb new file mode 100644 index 00000000..438549cd --- /dev/null +++ b/spec/arel/transformer/query_spec.rb @@ -0,0 +1,58 @@ +describe Arel::Transformer::Query do + it 'works for a node context' do + sql = 'SELECT 1 FROM posts' + tree = Arel.transformer(Arel.sql_to_arel(sql).first) + table_node = tree.child_at_path(['ast', 'cores', 0, 'source', 'left']) + + expect(tree.query(context: table_node.context)).to eq [table_node] + end + + it 'works quering only a subset of the context' do + sql = 'SELECT 1 FROM posts' + tree = Arel.transformer(Arel.sql_to_arel(sql).first) + table_node = tree.child_at_path(['ast', 'cores', 0, 'source', 'left']) + + expect(tree.query(context: { range_variable: true })).to eq [table_node] + end + + it 'works for a node parent' do + sql = 'SELECT 1 FROM posts' + tree = Arel.transformer(Arel.sql_to_arel(sql).first) + table_node = tree.child_at_path(['ast', 'cores', 0, 'source', 'left']) + + expect(tree.query(parent: { object: { class: Arel::Nodes::JoinSource } }).first) + . to eq table_node + end + + it 'works node object attributes' do + sql = 'SELECT 1 FROM posts' + tree = Arel.transformer(Arel.sql_to_arel(sql).first) + table_node = tree.child_at_path(['ast', 'cores', 0, 'source', 'left']) + + expect(tree.query(name: 'posts', relpersistence: 'p')). to eq [table_node] + end + + it 'searches from the current node, not from the root_node' do + sql = 'SELECT 1 FROM posts WHERE public = TRUE' + tree = Arel.transformer(Arel.sql_to_arel(sql).first) + table_node = tree.child_at_path(['ast', 'cores', 0, 'source', 'left']) + where_node = tree.child_at_path(['ast', 'cores', 0, 'wheres']) + + expect(tree.query(class: Arel::Table, name: 'posts')).to eq [table_node] + expect(where_node.query(class: Arel::Table, name: 'posts')).to eq [] + end + + it 'does not break when comparing incompatible objects' do + sql = 'SELECT 1 FROM posts LIMIT 10' + tree = Arel.transformer(Arel.sql_to_arel(sql).first) + + expect(tree.query(class: Arel::Nodes::Limit, expr: [1])).to eq [] + end + + it 'does not break when searching for unknown attributes' do + sql = 'SELECT 1 FROM posts' + tree = Arel.transformer(Arel.sql_to_arel(sql).first) + + expect(tree.query(klass: Arel::Table)).to eq [] + end +end From 8d7226b5a41dab88b44103e7bb900d66d70b6c12 Mon Sep 17 00:00:00 2001 From: maarten Date: Tue, 30 Jul 2019 22:34:50 +0200 Subject: [PATCH 22/23] Updated specs for AddSchemaToTable --- .../transformer/add_schema_to_table_spec.rb | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/spec/arel/transformer/add_schema_to_table_spec.rb b/spec/arel/transformer/add_schema_to_table_spec.rb index 176c17e7..28a845fb 100644 --- a/spec/arel/transformer/add_schema_to_table_spec.rb +++ b/spec/arel/transformer/add_schema_to_table_spec.rb @@ -1,10 +1,23 @@ describe Arel::Transformer::AddSchemaToTable do - it 'works' do + it 'adds the given schema name to all range variable tables' do transformer = Arel::Transformer::AddSchemaToTable.new('secret') - sql = 'SELECT posts.id FROM posts' + sql = 'SELECT posts.id FROM posts INNER JOIN comments ON comments.post_id = posts.id' result = Arel.sql_to_arel(sql) new_sql = transformer.call(result.first, nil).to_sql - expect(new_sql).to eq 'SELECT "posts"."id" FROM "secret"."posts"' + expect(new_sql) + .to eq 'SELECT "posts"."id" FROM "secret"."posts" INNER JOIN "secret"."comments" ' \ + 'ON "comments"."post_id" = "posts"."id"' + end + + it 'does not override existing schema name' do + transformer = Arel::Transformer::AddSchemaToTable.new('secret') + sql = 'SELECT posts.id FROM posts INNER JOIN public.comments ON comments.post_id = posts.id' + result = Arel.sql_to_arel(sql) + new_sql = transformer.call(result.first, nil).to_sql + + expect(new_sql) + .to eq 'SELECT "posts"."id" FROM "secret"."posts" INNER JOIN "public"."comments" ' \ + 'ON "comments"."post_id" = "posts"."id"' end end From cde022d11f19697cb74a6bb7390b2774957772e4 Mon Sep 17 00:00:00 2001 From: maarten Date: Tue, 30 Jul 2019 22:38:47 +0200 Subject: [PATCH 23/23] Remove broken spec --- spec/arel/sql_to_arel_spec.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/spec/arel/sql_to_arel_spec.rb b/spec/arel/sql_to_arel_spec.rb index 3b44c9ea..dbd240d5 100644 --- a/spec/arel/sql_to_arel_spec.rb +++ b/spec/arel/sql_to_arel_spec.rb @@ -216,8 +216,6 @@ pg_node: 'PgQuery::RANGE_SUBSELECT' visit 'select', '1 FROM "public"."table_is_range_var" "alias", ONLY "b"', pg_node: 'PgQuery::RANGE_VAR' - # TODO: this one breaks - # visit 'select', '1 FROM "b", (SELECT 1) "a", "c"', pg_node: 'PgQuery::RANGE_VAR' visit 'select', '1', pg_node: 'PgQuery::RAW_STMT' visit 'sql', 'REFRESH MATERIALIZED VIEW view WITH NO DATA', pg_node: 'PgQuery::REFRESH_MAT_VIEW_STMT',