From 3929d4541944f2e0a196b83df25c36e30c6f3097 Mon Sep 17 00:00:00 2001 From: Ernie Miller Date: Wed, 5 Jun 2013 15:23:00 -0400 Subject: [PATCH] Stop creating SQL literals for aliases. PostgreSQL requires that the aliases aren't quoted, but MySQL and SQLite don't seem to care at all, and it seems like we would do better to avoid creating SqlLiteral nodes where we don't need to. I ran the mysql/postgresql/sqlite3 tests for ActiveRecord against this branch and everything passes. Of course, we all know that doesn't necessarily mean anything. This is tangentially related to ernie/squeel#246, but still doesn't address the "reserved word" complaint there. Might make sense to have an Alias node for special visitation vs the SqlLiteral one, perhaps. Or maybe we just don't care if someone wants to alias things using reserved words in pg, because that's just cray-cray. --- lib/arel/alias_predication.rb | 4 +-- lib/arel/nodes/extract.rb | 4 +-- lib/arel/nodes/function.rb | 4 +-- lib/arel/select_manager.rb | 2 +- lib/arel/visitors/postgresql.rb | 49 +++++++++++++++++++++++++++++ test/nodes/test_as.rb | 6 ---- test/nodes/test_count.rb | 2 +- test/nodes/test_extract.rb | 2 +- test/nodes/test_named_function.rb | 2 -- test/nodes/test_over.rb | 2 +- test/nodes/test_sum.rb | 2 +- test/test_select_manager.rb | 10 ++---- test/visitors/test_postgres.rb | 51 +++++++++++++++++++++++++++++++ 13 files changed, 113 insertions(+), 27 deletions(-) diff --git a/lib/arel/alias_predication.rb b/lib/arel/alias_predication.rb index f42e9ee1..d377bc20 100644 --- a/lib/arel/alias_predication.rb +++ b/lib/arel/alias_predication.rb @@ -1,7 +1,7 @@ module Arel module AliasPredication def as other - Nodes::As.new self, Nodes::SqlLiteral.new(other) + Nodes::As.new self, other end end -end \ No newline at end of file +end diff --git a/lib/arel/nodes/extract.rb b/lib/arel/nodes/extract.rb index 92fbde62..f55f5acf 100644 --- a/lib/arel/nodes/extract.rb +++ b/lib/arel/nodes/extract.rb @@ -11,11 +11,11 @@ class Extract < Arel::Nodes::Unary def initialize expr, field, aliaz = nil super(expr) @field = field - @alias = aliaz && SqlLiteral.new(aliaz) + @alias = aliaz end def as aliaz - self.alias = SqlLiteral.new(aliaz) + self.alias = aliaz self end diff --git a/lib/arel/nodes/function.rb b/lib/arel/nodes/function.rb index dcafbbf1..35d3cc1f 100644 --- a/lib/arel/nodes/function.rb +++ b/lib/arel/nodes/function.rb @@ -9,12 +9,12 @@ class Function < Arel::Nodes::Node def initialize expr, aliaz = nil super() @expressions = expr - @alias = aliaz && SqlLiteral.new(aliaz) + @alias = aliaz @distinct = false end def as aliaz - self.alias = SqlLiteral.new(aliaz) + self.alias = aliaz self end diff --git a/lib/arel/select_manager.rb b/lib/arel/select_manager.rb index ee9d34a5..7bc9be99 100644 --- a/lib/arel/select_manager.rb +++ b/lib/arel/select_manager.rb @@ -44,7 +44,7 @@ def exists end def as other - create_table_alias grouping(@ast), Nodes::SqlLiteral.new(other) + create_table_alias grouping(@ast), other end def where_clauses diff --git a/lib/arel/visitors/postgresql.rb b/lib/arel/visitors/postgresql.rb index 080e402e..a8fd5fc4 100644 --- a/lib/arel/visitors/postgresql.rb +++ b/lib/arel/visitors/postgresql.rb @@ -14,6 +14,55 @@ def visit_Arel_Nodes_DoesNotMatch o, a def visit_Arel_Nodes_DistinctOn o, a "DISTINCT ON ( #{visit o.expr, a} )" end + + def visit_Arel_Nodes_As o, a + "#{visit o.left, a} AS #{o.right}" + end + + def visit_Arel_Nodes_TableAlias o, a + "#{visit o.relation, a} #{o.name}" + end + + def visit_Arel_Nodes_NamedFunction o, a + "#{o.name}(#{o.distinct ? 'DISTINCT ' : ''}#{o.expressions.map { |x| + visit x, a + }.join(', ')})#{o.alias ? " AS #{o.alias}" : ''}" + end + + def visit_Arel_Nodes_Extract o, a + "EXTRACT(#{o.field.to_s.upcase} FROM #{visit o.expr, a})#{o.alias ? " AS #{o.alias}" : ''}" + end + + def visit_Arel_Nodes_Count o, a + "COUNT(#{o.distinct ? 'DISTINCT ' : ''}#{o.expressions.map { |x| + visit x, a + }.join(', ')})#{o.alias ? " AS #{o.alias}" : ''}" + end + + def visit_Arel_Nodes_Sum o, a + "SUM(#{o.distinct ? 'DISTINCT ' : ''}#{o.expressions.map { |x| + visit x, a }.join(', ')})#{o.alias ? " AS #{o.alias}" : ''}" + end + + def visit_Arel_Nodes_Exists o, a + "EXISTS (#{visit o.expressions, a})#{ + o.alias ? " AS #{o.alias}" : ''}" + end + + def visit_Arel_Nodes_Max o, a + "MAX(#{o.distinct ? 'DISTINCT ' : ''}#{o.expressions.map { |x| + visit x, a }.join(', ')})#{o.alias ? " AS #{o.alias}" : ''}" + end + + def visit_Arel_Nodes_Min o, a + "MIN(#{o.distinct ? 'DISTINCT ' : ''}#{o.expressions.map { |x| + visit x, a }.join(', ')})#{o.alias ? " AS #{o.alias}" : ''}" + end + + def visit_Arel_Nodes_Avg o, a + "AVG(#{o.distinct ? 'DISTINCT ' : ''}#{o.expressions.map { |x| + visit x, a }.join(', ')})#{o.alias ? " AS #{o.alias}" : ''}" + end end end end diff --git a/test/nodes/test_as.rb b/test/nodes/test_as.rb index b1dcccf7..a44c35df 100644 --- a/test/nodes/test_as.rb +++ b/test/nodes/test_as.rb @@ -10,12 +10,6 @@ module Nodes assert_equal attr, as.left assert_equal 'foo', as.right end - - it 'converts right to SqlLiteral if a string' do - attr = Table.new(:users)[:id] - as = attr.as('foo') - assert_kind_of Arel::Nodes::SqlLiteral, as.right - end end describe 'equality' do diff --git a/test/nodes/test_count.rb b/test/nodes/test_count.rb index 88d2a694..49ec7d3d 100644 --- a/test/nodes/test_count.rb +++ b/test/nodes/test_count.rb @@ -11,7 +11,7 @@ it 'should alias the count' do table = Arel::Table.new :users table[:id].count.as('foo').to_sql.must_be_like %{ - COUNT("users"."id") AS foo + COUNT("users"."id") AS 'foo' } end end diff --git a/test/nodes/test_extract.rb b/test/nodes/test_extract.rb index 80bb465f..91e827df 100644 --- a/test/nodes/test_extract.rb +++ b/test/nodes/test_extract.rb @@ -12,7 +12,7 @@ it 'should alias the extract' do table = Arel::Table.new :users table[:timestamp].extract('date').as('foo').to_sql.must_be_like %{ - EXTRACT(DATE FROM "users"."timestamp") AS foo + EXTRACT(DATE FROM "users"."timestamp") AS 'foo' } end end diff --git a/test/nodes/test_named_function.rb b/test/nodes/test_named_function.rb index 9d16f9cb..0998f6c5 100644 --- a/test/nodes/test_named_function.rb +++ b/test/nodes/test_named_function.rb @@ -14,7 +14,6 @@ def test_function_alias function = function.as('wth') assert_equal 'omg', function.name assert_equal 'zomg', function.expressions - assert_kind_of SqlLiteral, function.alias assert_equal 'wth', function.alias end @@ -22,7 +21,6 @@ def test_construct_with_alias function = NamedFunction.new 'omg', 'zomg', 'wth' assert_equal 'omg', function.name assert_equal 'zomg', function.expressions - assert_kind_of SqlLiteral, function.alias assert_equal 'wth', function.alias end diff --git a/test/nodes/test_over.rb b/test/nodes/test_over.rb index 0aac00b2..f27d625b 100644 --- a/test/nodes/test_over.rb +++ b/test/nodes/test_over.rb @@ -5,7 +5,7 @@ it 'should alias the expression' do table = Arel::Table.new :users table[:id].count.over.as('foo').to_sql.must_be_like %{ - COUNT("users"."id") OVER () AS foo + COUNT("users"."id") OVER () AS 'foo' } end end diff --git a/test/nodes/test_sum.rb b/test/nodes/test_sum.rb index d65cd31d..78c588f7 100644 --- a/test/nodes/test_sum.rb +++ b/test/nodes/test_sum.rb @@ -5,7 +5,7 @@ it 'should alias the sum' do table = Arel::Table.new :users table[:id].sum.as('foo').to_sql.must_be_like %{ - SUM("users"."id") AS foo + SUM("users"."id") AS 'foo' } end end diff --git a/test/test_select_manager.rb b/test/test_select_manager.rb index a449f78b..de2cae98 100644 --- a/test/test_select_manager.rb +++ b/test/test_select_manager.rb @@ -104,12 +104,6 @@ def test_join_sources assert_equal 'foo', as.right end - it 'converts right to SqlLiteral if a string' do - manager = Arel::SelectManager.new Table.engine - as = manager.as('foo') - assert_kind_of Arel::Nodes::SqlLiteral, as.right - end - it 'can make a subselect' do manager = Arel::SelectManager.new Table.engine manager.project Arel.star @@ -281,7 +275,7 @@ def test_join_sources manager.project SqlLiteral.new '*' m2 = Arel::SelectManager.new(manager.engine) m2.project manager.exists.as('foo') - m2.to_sql.must_be_like %{ SELECT EXISTS (#{manager.to_sql}) AS foo } + m2.to_sql.must_be_like %{ SELECT EXISTS (#{manager.to_sql}) AS 'foo' } end end @@ -667,7 +661,7 @@ def test_join_sources joins = users.join(counts).on(counts[:user_id].eq(10)) joins.to_sql.must_be_like %{ - SELECT FROM "users" INNER JOIN (SELECT "comments"."user_id" AS user_id, COUNT("comments"."user_id") AS count FROM "comments" GROUP BY "comments"."user_id") counts ON counts."user_id" = 10 + SELECT FROM "users" INNER JOIN (SELECT "comments"."user_id" AS 'user_id', COUNT("comments"."user_id") AS 'count' FROM "comments" GROUP BY "comments"."user_id") "counts" ON "counts"."user_id" = 10 } end diff --git a/test/visitors/test_postgres.rb b/test/visitors/test_postgres.rb index 921bd96c..a8d583ba 100644 --- a/test/visitors/test_postgres.rb +++ b/test/visitors/test_postgres.rb @@ -43,6 +43,57 @@ module Visitors core.set_quantifier = Arel::Nodes::Distinct.new assert_equal 'SELECT DISTINCT', @visitor.accept(core) end + + it 'should not quote aliases' do + aliaz = Arel::Nodes::As.new('zomg', 'wth') + assert_equal "'zomg' AS wth", @visitor.accept(aliaz) + end + + it 'should not quote table aliases' do + aliaz = Arel::Nodes::TableAlias.new('zomg', 'wth') + assert_equal "'zomg' wth", @visitor.accept(aliaz) + end + + it 'should not quote named function aliases' do + func = Arel::Nodes::NamedFunction.new(:max, ['zomg'], 'wth') + assert_equal "max('zomg') AS wth", @visitor.accept(func) + end + + it 'should not quote extract aliases' do + extract = Arel::Nodes::Extract.new('zomg', 'hi2u', 'wth') + assert_equal "EXTRACT(HI2U FROM 'zomg') AS wth", + @visitor.accept(extract) + end + + it 'should not quote count aliases' do + count = Arel::Nodes::Count.new(['zomg'], false, 'wth') + assert_equal "COUNT('zomg') AS wth", @visitor.accept(count) + end + + it 'should not quote sum aliases' do + sum = Arel::Nodes::Sum.new(['zomg'], 'wth') + assert_equal "SUM('zomg') AS wth", @visitor.accept(sum) + end + + it 'should not quote exists aliases' do + exists = Arel::Nodes::Exists.new(['zomg'], 'wth') + assert_equal "EXISTS ('zomg') AS wth", @visitor.accept(exists) + end + + it 'should not quote max aliases' do + max = Arel::Nodes::Max.new(['zomg'], 'wth') + assert_equal "MAX('zomg') AS wth", @visitor.accept(max) + end + + it 'should not quote min aliases' do + min = Arel::Nodes::Min.new(['zomg'], 'wth') + assert_equal "MIN('zomg') AS wth", @visitor.accept(min) + end + + it 'should not quote avg aliases' do + avg = Arel::Nodes::Avg.new(['zomg'], 'wth') + assert_equal "AVG('zomg') AS wth", @visitor.accept(avg) + end end end end