Skip to content

Commit 33c17f7

Browse files
committed
fix SQL generation issues and add comprehensive tests
- Add GROUP BY support in string builder compilation path - Implement HAVING clause support for both Arel and string builder - Add validation to prevent empty array IN clauses (syntax error) - Add comprehensive tests for compiler, validator, runner, explain_gate - Fix query_spec tests for safe? and binds methods
1 parent 1e6681c commit 33c17f7

6 files changed

Lines changed: 1581 additions & 7 deletions

File tree

lib/code_to_query/compiler.rb

Lines changed: 110 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,18 @@ def compile_with_arel(intent)
182182
end
183183
end
184184

185+
if (having_filters = intent['having']).present?
186+
having_filters.each do |h|
187+
agg_node = build_arel_aggregate(table, h)
188+
next unless agg_node
189+
190+
key = h['param'] || "having_#{h['column']}"
191+
bind_spec << { key: key, column: h['column'], cast: nil }
192+
condition = build_arel_having_condition(agg_node, h['op'], key)
193+
query = query.having(condition) if condition
194+
end
195+
end
196+
185197
if (limit = determine_appropriate_limit(intent))
186198
query = query.take(limit)
187199
end
@@ -300,6 +312,11 @@ def compile_with_string_building(intent)
300312
sub_where << "#{rcol} BETWEEN #{placeholder1} AND #{placeholder2}"
301313
when 'in'
302314
key = rf['param'] || rf['column']
315+
values = params_hash[key] || params_hash[key.to_s] || params_hash[key.to_sym]
316+
if values.is_a?(Array) && values.empty?
317+
raise ArgumentError, "IN clause requires non-empty array for column '#{rf['column']}'"
318+
end
319+
303320
placeholder = placeholder_for_adapter(placeholder_index)
304321
bind_spec << ({ key: key, column: rf['column'], cast: :array })
305322
placeholder_index += 1
@@ -363,6 +380,11 @@ def compile_with_string_building(intent)
363380
sub_where << "#{rcol} BETWEEN #{placeholder1} AND #{placeholder2}"
364381
when 'in'
365382
key = rf['param'] || rf['column']
383+
values = params_hash[key] || params_hash[key.to_s] || params_hash[key.to_sym]
384+
if values.is_a?(Array) && values.empty?
385+
raise ArgumentError, "IN clause requires non-empty array for column '#{rf['column']}'"
386+
end
387+
366388
placeholder = placeholder_for_adapter(placeholder_index)
367389
bind_spec << { key: key, column: rf['column'], cast: :array }
368390
placeholder_index += 1
@@ -394,7 +416,11 @@ def compile_with_string_building(intent)
394416
"#{col} BETWEEN #{placeholder1} AND #{placeholder2}"
395417
when 'in'
396418
key = f['param'] || f['column']
397-
# For IN clauses, we'll need to handle arrays specially
419+
values = params_hash[key] || params_hash[key.to_s] || params_hash[key.to_sym]
420+
if values.is_a?(Array) && values.empty?
421+
raise ArgumentError, "IN clause requires non-empty array for column '#{f['column']}'"
422+
end
423+
398424
placeholder = placeholder_for_adapter(placeholder_index)
399425
bind_spec << { key: key, column: f['column'], cast: :array }
400426
placeholder_index += 1
@@ -412,6 +438,22 @@ def compile_with_string_building(intent)
412438
sql_parts << "WHERE #{where_fragments.join(' AND ')}" if where_fragments.any?
413439
end
414440

441+
if (group_columns = intent['group_by']).present?
442+
group_fragments = group_columns.map { |col| quote_ident(col) }
443+
sql_parts << "GROUP BY #{group_fragments.join(', ')}"
444+
end
445+
446+
if (having_filters = intent['having']).present?
447+
having_fragments = having_filters.map do |h|
448+
agg_expr = build_aggregate_expression(h)
449+
placeholder = placeholder_for_adapter(placeholder_index)
450+
bind_spec << { key: h['param'] || "having_#{h['column']}", column: h['column'], cast: nil }
451+
placeholder_index += 1
452+
"#{agg_expr} #{h['op']} #{placeholder}"
453+
end
454+
sql_parts << "HAVING #{having_fragments.join(' AND ')}"
455+
end
456+
415457
if (orders = intent['order']).present?
416458
order_fragments = orders.map do |o|
417459
dir = o['dir'].to_s.downcase == 'desc' ? 'DESC' : 'ASC'
@@ -621,6 +663,73 @@ def parse_function_column(expr)
621663
end
622664
end
623665

666+
def build_aggregate_expression(having_spec)
667+
func = having_spec['function'].to_s.upcase
668+
col = having_spec['column']
669+
670+
case func
671+
when 'COUNT'
672+
col ? "COUNT(#{quote_ident(col)})" : 'COUNT(*)'
673+
when 'SUM'
674+
"SUM(#{quote_ident(col)})"
675+
when 'AVG'
676+
"AVG(#{quote_ident(col)})"
677+
when 'MAX'
678+
"MAX(#{quote_ident(col)})"
679+
when 'MIN'
680+
"MIN(#{quote_ident(col)})"
681+
else
682+
'COUNT(*)'
683+
end
684+
end
685+
686+
def build_arel_aggregate(table, having_spec)
687+
func = having_spec['function'].to_s.downcase
688+
col = having_spec['column']
689+
690+
case func
691+
when 'count'
692+
col ? table[col].count : Arel.star.count
693+
when 'sum'
694+
return nil unless col
695+
696+
table[col].sum
697+
when 'avg'
698+
return nil unless col
699+
700+
table[col].average
701+
when 'max'
702+
return nil unless col
703+
704+
table[col].maximum
705+
when 'min'
706+
return nil unless col
707+
708+
table[col].minimum
709+
else
710+
Arel.star.count
711+
end
712+
end
713+
714+
def build_arel_having_condition(agg_node, operator, key)
715+
bind_param = Arel::Nodes::BindParam.new(key)
716+
717+
case operator
718+
when '='
719+
agg_node.eq(bind_param)
720+
when '!='
721+
agg_node.not_eq(bind_param)
722+
when '>'
723+
agg_node.gt(bind_param)
724+
when '>='
725+
agg_node.gteq(bind_param)
726+
when '<'
727+
agg_node.lt(bind_param)
728+
when '<='
729+
agg_node.lteq(bind_param)
730+
end
731+
end
732+
624733
def normalize_params_with_model(intent)
625734
params = (intent['params'] || {}).dup
626735
return params unless defined?(ActiveRecord::Base)

0 commit comments

Comments
 (0)