From c525a2b1309b30a8bd033847cc30a0448f7459ba Mon Sep 17 00:00:00 2001 From: sufleR Date: Tue, 23 Dec 2025 15:02:49 +0100 Subject: [PATCH 1/3] Add configurable SQL comment removal feature This commit introduces a comprehensive SQL comment removal system that is SQL-92 standard compliant and works across all major databases. Key Features: - Configurable comment removal strategies (:none, :oneline, :multiline, :all) - Configurable scope (:none, :prepared_for_logs, :all) - SQL standard compliant (SQL-92) - Preserves comments in quoted strings (single, double, dollar quotes) - Handles all edge cases (escaped quotes, inline comments, etc.) Implementation: - New CommentRemover service class using state machine pattern - Extracted Config class to separate file for better organization - Added Config#should_comments_be_removed? method for clean logic - Integrated into sql() and prepared_for_logs() methods Testing: - Comprehensive unit tests for CommentRemover (32 examples) - Integration tests for configuration scenarios (10 examples) - Config class tests (12 examples) - Test fixtures for various comment scenarios - 78 total tests, 98.38% code coverage - RuboCop compliant Documentation: - Updated CHANGELOG.md with feature description - Updated README.md with configuration options and examples Addresses https://github.com/sufleR/sql_query/issues/20 --- CHANGELOG.md | 2 +- README.md | 15 ++ lib/sql_query.rb | 30 ++- lib/sql_query/comment_remover.rb | 211 +++++++++++++++++ lib/sql_query/config.rb | 38 +++ spec/config_spec.rb | 64 ++++- .../with_comments_in_strings.sql.erb | 6 + spec/sql_queries/with_dollar_quotes.sql.erb | 5 + spec/sql_queries/with_mixed_comments.sql.erb | 4 + .../with_multiline_comments.sql.erb | 5 + .../with_single_line_comments.sql.erb | 4 + spec/sql_query/comment_remover_spec.rb | 218 ++++++++++++++++++ spec/sql_query_spec.rb | 135 +++++++++++ 13 files changed, 723 insertions(+), 14 deletions(-) create mode 100644 lib/sql_query/comment_remover.rb create mode 100644 lib/sql_query/config.rb create mode 100644 spec/sql_queries/with_comments_in_strings.sql.erb create mode 100644 spec/sql_queries/with_dollar_quotes.sql.erb create mode 100644 spec/sql_queries/with_mixed_comments.sql.erb create mode 100644 spec/sql_queries/with_multiline_comments.sql.erb create mode 100644 spec/sql_queries/with_single_line_comments.sql.erb create mode 100644 spec/sql_query/comment_remover_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 8896ebc..04b3a55 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## 0.7.5 / Unreleased -* [Added] +* [Added] Configurable SQL comment removal feature with `remove_comments` and `remove_comments_from` configuration options. Supports selective removal of single-line (`--`) and multi-line (`/* */`) comments while preserving comments within quoted strings (single, double, and PostgreSQL dollar quotes). Addresses https://github.com/sufleR/sql_query/issues/20 * [Deprecated] diff --git a/README.md b/README.md index 540c9db..5d02a20 100644 --- a/README.md +++ b/README.md @@ -99,6 +99,8 @@ SqlQuery.new('player/get_by_email', email: 'e@mail.dev') SqlQuery.configure do |config| config.path = '/app/sql_templates' config.adapter = ActiveRecord::Base + config.remove_comments = :all # :none, :oneline, :multiline, :all (default: :all) + config.remove_comments_from = :all # :none, :prepared_for_logs, :all (default: :all) end ``` @@ -107,6 +109,19 @@ end * adapter - class which implements connection method. +* remove_comments - Controls which types of SQL comments to remove: + * `:none` - Don't remove any comments + * `:oneline` - Remove only single-line comments (`--`) + * `:multiline` - Remove only multi-line comments (`/* */`) + * `:all` - Remove both types (default) + +* remove_comments_from - Controls where to apply comment removal: + * `:none` - Don't remove comments anywhere + * `:prepared_for_logs` - Remove comments only in `prepared_for_logs` method + * `:all` - Remove comments from all queries (default) + +**Note:** Comments within quoted strings (single quotes, double quotes, or PostgreSQL dollar quotes) are always preserved regardless of settings. + ### Partials You can prepare part of sql query in partial file and reuse it in multiple queries. diff --git a/lib/sql_query.rb b/lib/sql_query.rb index 23b7b04..2ae7c59 100644 --- a/lib/sql_query.rb +++ b/lib/sql_query.rb @@ -1,7 +1,10 @@ # frozen_string_literal: true require 'erb' +require_relative 'sql_query/config' +require_relative 'sql_query/comment_remover' +# rubocop:disable Metrics/ClassLength class SqlQuery attr_reader :connection @@ -34,7 +37,10 @@ def exec_query(prepare = true) end def sql - @sql ||= prepare_query(false) + @sql ||= begin + rendered_sql = prepare_query(false) + apply_comment_removal(rendered_sql, for_logs: false) + end end def pretty_sql @@ -46,7 +52,10 @@ def quote(value) end def prepared_for_logs - @prepared_for_logs ||= prepare_query(true) + @prepared_for_logs ||= begin + rendered_sql = prepare_query(true) + apply_comment_removal(rendered_sql, for_logs: true) + end end def partial(partial_name, partial_options = {}) @@ -65,15 +74,6 @@ def self.configure yield(config) end - class Config - attr_accessor :path, :adapter - - def initialize - @path = '/app/sql_queries' - @adapter = ActiveRecord::Base - end - end - private def prepare_query(for_logs) @@ -91,6 +91,13 @@ def split_to_path_and_name(file) end end + def apply_comment_removal(sql, for_logs:) + config = self.class.config + return sql unless config.should_comments_be_removed?(for_logs: for_logs) + + CommentRemover.new(config.remove_comments).remove(sql) + end + def pretty(value) # override inspect to be more human readable from console # code copy from ActiveRecord @@ -131,3 +138,4 @@ def path tmp_path end end +# rubocop:enable Metrics/ClassLength diff --git a/lib/sql_query/comment_remover.rb b/lib/sql_query/comment_remover.rb new file mode 100644 index 0000000..a7be52c --- /dev/null +++ b/lib/sql_query/comment_remover.rb @@ -0,0 +1,211 @@ +# frozen_string_literal: true + +class SqlQuery + # Service class responsible for removing SQL comments from queries + # while preserving content within quoted strings. + # + # Supports SQL-92 standard comment syntax: + # - Single-line comments: -- comment + # - Multi-line comments: /* comment */ + # + # Preserves content in: + # - Single-quoted strings: '...' + # - Double-quoted identifiers: "..." + # - Dollar-quoted strings (PostgreSQL): $$...$$ or $tag$...$tag$ + # + # @example + # remover = CommentRemover.new(:all) + # sql = "SELECT * FROM t -- comment\nWHERE id = 1" + # remover.remove(sql) + # # => "SELECT * FROM t \nWHERE id = 1" + # + # rubocop:disable Metrics/ClassLength + class CommentRemover + def initialize(strategy) + @strategy = strategy # :none, :oneline, :multiline, :all + end + + # Removes comments from SQL based on the configured strategy + # + # @param sql [String] the SQL string to process + # @return [String] SQL with comments removed (or unchanged if strategy is :none) + def remove(sql) + return sql if @strategy == :none + + state = init_state + result = [] + i = 0 + + i = process_character(sql, i, result, state) while i < sql.length + + result.join + end + + private + + def init_state + { + in_single: false, # Inside '...' + in_double: false, # Inside "..." + in_dollar: false, # Inside $$...$$ or $tag$...$tag$ + dollar_tag: nil, # Current dollar quote tag + in_line_comment: false, # Inside -- comment + in_block_comment: false, # Inside /* */ comment + escape_next: false # Next char is escaped (for backslash escapes) + } + end + + # rubocop:disable Metrics/MethodLength, Metrics/CyclomaticComplexity + # rubocop:disable Metrics/PerceivedComplexity, Metrics/AbcSize + def process_character(sql, index, result, state) + char = sql[index] + next_char = index + 1 < sql.length ? sql[index + 1] : nil + + # Handle escape sequences + if state[:escape_next] + result << char unless in_comment?(state) + state[:escape_next] = false + return index + 1 + end + + # Check for backslash escape + if char == '\\' && (state[:in_single] || state[:in_double]) + result << char unless in_comment?(state) + state[:escape_next] = true + return index + 1 + end + + # If in comment, check for comment end + if state[:in_line_comment] + if char == "\n" + state[:in_line_comment] = false + result << char # Preserve newline + end + # Skip comment content + return index + 1 + end + + if state[:in_block_comment] + if char == '*' && next_char == '/' + state[:in_block_comment] = false + return index + 2 # Skip */ + end + # Skip comment content + return index + 1 + end + + # Not in comment - check for comment start (only if not in quotes) + unless in_quote?(state) + if char == '-' && next_char == '-' && should_remove_oneline? + state[:in_line_comment] = true + return index + 2 # Skip -- + end + + if char == '/' && next_char == '*' && should_remove_multiline? + state[:in_block_comment] = true + return index + 2 # Skip /* + end + end + + # Handle quotes + if char == "'" && !state[:in_double] && !state[:in_dollar] + if state[:in_single] && next_char == "'" + # Escaped single quote (SQL style: '') + result << char << next_char + return index + 2 + else + state[:in_single] = !state[:in_single] + result << char + return index + 1 + end + end + + if char == '"' && !state[:in_single] && !state[:in_dollar] + if state[:in_double] && next_char == '"' + # Escaped double quote + result << char << next_char + return index + 2 + else + state[:in_double] = !state[:in_double] + result << char + return index + 1 + end + end + + # Handle dollar quotes (PostgreSQL) + if char == '$' && !state[:in_single] && !state[:in_double] + tag, tag_length = extract_dollar_tag(sql, index) + if tag + if state[:in_dollar] && tag == state[:dollar_tag] + # Closing dollar quote + state[:in_dollar] = false + state[:dollar_tag] = nil + tag_length.times { |i| result << sql[index + i] } + return index + tag_length + elsif !state[:in_dollar] + # Opening dollar quote + state[:in_dollar] = true + state[:dollar_tag] = tag + tag_length.times { |i| result << sql[index + i] } + return index + tag_length + end + end + end + + # Regular character + result << char + index + 1 + end + # rubocop:enable Metrics/MethodLength, Metrics/CyclomaticComplexity + # rubocop:enable Metrics/PerceivedComplexity, Metrics/AbcSize + + def in_quote?(state) + state[:in_single] || state[:in_double] || state[:in_dollar] + end + + def in_comment?(state) + state[:in_line_comment] || state[:in_block_comment] + end + + def should_remove_oneline? + @strategy == :oneline || @strategy == :all + end + + def should_remove_multiline? + @strategy == :multiline || @strategy == :all + end + + # Extracts dollar quote tag from position + # Returns [tag, length] or [nil, nil] + # Matches: $$ or $tag$ where tag is alphanumeric/underscore + # rubocop:disable Metrics/MethodLength + def extract_dollar_tag(sql, index) + return [nil, nil] unless sql[index] == '$' + + # Look for closing $ + i = index + 1 + tag_chars = [] + + while i < sql.length + char = sql[i] + if char == '$' + # Found closing $ + tag = tag_chars.empty? ? '' : tag_chars.join + length = i - index + 1 + return [tag, length] + elsif char =~ /[a-zA-Z0-9_]/ + tag_chars << char + i += 1 + else + # Invalid character for dollar quote tag + return [nil, nil] + end + end + + # No closing $ found + [nil, nil] + end + # rubocop:enable Metrics/MethodLength + end + # rubocop:enable Metrics/ClassLength +end diff --git a/lib/sql_query/config.rb b/lib/sql_query/config.rb new file mode 100644 index 0000000..80f7443 --- /dev/null +++ b/lib/sql_query/config.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +class SqlQuery + # Configuration class for SqlQuery behavior + # + # @example + # SqlQuery.configure do |config| + # config.path = '/app/sql_queries' + # config.adapter = ActiveRecord::Base + # config.remove_comments = :all + # config.remove_comments_from = :prepared_for_logs + # end + class Config + attr_accessor :path, :adapter, :remove_comments, :remove_comments_from + + def initialize + @path = '/app/sql_queries' + @adapter = ActiveRecord::Base + @remove_comments = :all # :none, :oneline, :multiline, :all + @remove_comments_from = :all # :none, :prepared_for_logs, :all + end + + # Determines if comments should be removed for a given context + # + # @param for_logs [Boolean] whether the query is being prepared for logs + # @return [Boolean] true if comments should be removed + def should_comments_be_removed?(for_logs:) + case remove_comments_from + when :prepared_for_logs + for_logs + when :all + true + else + false + end + end + end +end diff --git a/spec/config_spec.rb b/spec/config_spec.rb index 75a088b..70a9f7f 100644 --- a/spec/config_spec.rb +++ b/spec/config_spec.rb @@ -3,13 +3,73 @@ require 'spec_helper' describe SqlQuery::Config do + let(:config) { described_class.new } + describe 'initialize' do it 'sets path to default' do - expect(described_class.new.path).to eq('/app/sql_queries') + expect(config.path).to eq('/app/sql_queries') end it 'sets adapter to ActiveRecord::Base' do - expect(described_class.new.adapter).to eq(ActiveRecord::Base) + expect(config.adapter).to eq(ActiveRecord::Base) + end + + it 'sets remove_comments to :all by default' do + expect(config.remove_comments).to eq(:all) + end + + it 'sets remove_comments_from to :all by default' do + expect(config.remove_comments_from).to eq(:all) + end + end + + describe '#should_comments_be_removed?' do + context 'when remove_comments_from is :all' do + before { config.remove_comments_from = :all } + + it 'returns true for logs' do + expect(config.should_comments_be_removed?(for_logs: true)).to be true + end + + it 'returns true for non-logs' do + expect(config.should_comments_be_removed?(for_logs: false)).to be true + end + end + + context 'when remove_comments_from is :prepared_for_logs' do + before { config.remove_comments_from = :prepared_for_logs } + + it 'returns true for logs' do + expect(config.should_comments_be_removed?(for_logs: true)).to be true + end + + it 'returns false for non-logs' do + expect(config.should_comments_be_removed?(for_logs: false)).to be false + end + end + + context 'when remove_comments_from is :none' do + before { config.remove_comments_from = :none } + + it 'returns false for logs' do + expect(config.should_comments_be_removed?(for_logs: true)).to be false + end + + it 'returns false for non-logs' do + expect(config.should_comments_be_removed?(for_logs: false)).to be false + end + end + + context 'when remove_comments_from is an unknown value' do + before { config.remove_comments_from = :unknown } + + it 'returns false for logs' do + expect(config.should_comments_be_removed?(for_logs: true)).to be false + end + + it 'returns false for non-logs' do + expect(config.should_comments_be_removed?(for_logs: false)).to be false + end end end end diff --git a/spec/sql_queries/with_comments_in_strings.sql.erb b/spec/sql_queries/with_comments_in_strings.sql.erb new file mode 100644 index 0000000..a15c99c --- /dev/null +++ b/spec/sql_queries/with_comments_in_strings.sql.erb @@ -0,0 +1,6 @@ +SELECT + '-- not a comment' as text1, + '/* also not a comment */' as text2, + "column--name" as text3 +FROM players +WHERE email = <%= quote @email %> diff --git a/spec/sql_queries/with_dollar_quotes.sql.erb b/spec/sql_queries/with_dollar_quotes.sql.erb new file mode 100644 index 0000000..df38284 --- /dev/null +++ b/spec/sql_queries/with_dollar_quotes.sql.erb @@ -0,0 +1,5 @@ +SELECT + $$text with -- comment$$ as text1, + $tag$text with /* comment */$tag$ as text2 +FROM players +WHERE email = <%= quote @email %> diff --git a/spec/sql_queries/with_mixed_comments.sql.erb b/spec/sql_queries/with_mixed_comments.sql.erb new file mode 100644 index 0000000..dba06a8 --- /dev/null +++ b/spec/sql_queries/with_mixed_comments.sql.erb @@ -0,0 +1,4 @@ +-- Single line comment +SELECT * /* inline comment */ FROM players +WHERE email = <%= quote @email %> /* another comment */ +-- End comment diff --git a/spec/sql_queries/with_multiline_comments.sql.erb b/spec/sql_queries/with_multiline_comments.sql.erb new file mode 100644 index 0000000..1829971 --- /dev/null +++ b/spec/sql_queries/with_multiline_comments.sql.erb @@ -0,0 +1,5 @@ +/* This query selects all players + filtered by email address */ +SELECT * FROM players +WHERE email = <%= quote @email %> +/* End of query */ diff --git a/spec/sql_queries/with_single_line_comments.sql.erb b/spec/sql_queries/with_single_line_comments.sql.erb new file mode 100644 index 0000000..aa33b61 --- /dev/null +++ b/spec/sql_queries/with_single_line_comments.sql.erb @@ -0,0 +1,4 @@ +-- This is a single-line comment at the start +SELECT * FROM players +WHERE email = <%= quote @email %> -- filter by email +-- Another comment at the end diff --git a/spec/sql_query/comment_remover_spec.rb b/spec/sql_query/comment_remover_spec.rb new file mode 100644 index 0000000..ee707f3 --- /dev/null +++ b/spec/sql_query/comment_remover_spec.rb @@ -0,0 +1,218 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe SqlQuery::CommentRemover do + describe '#remove' do + context 'with strategy :none' do + let(:remover) { described_class.new(:none) } + + it 'returns SQL unchanged' do + sql = "SELECT * FROM t -- comment\nWHERE id = 1" + expect(remover.remove(sql)).to eq(sql) + end + end + + context 'with strategy :oneline' do + let(:remover) { described_class.new(:oneline) } + + it 'removes single-line comments' do + sql = "SELECT * FROM t -- comment\nWHERE id = 1" + expected = "SELECT * FROM t \nWHERE id = 1" + expect(remover.remove(sql)).to eq(expected) + end + + it 'removes comment at end of file without newline' do + sql = 'SELECT * FROM t --comment' + expected = 'SELECT * FROM t ' + expect(remover.remove(sql)).to eq(expected) + end + + it 'keeps multi-line comments' do + sql = "SELECT * FROM t /* comment */\nWHERE id = 1" + expect(remover.remove(sql)).to eq(sql) + end + + it 'preserves -- in single-quoted strings' do + sql = "SELECT '-- not a comment' FROM t" + expect(remover.remove(sql)).to eq(sql) + end + + it 'preserves -- in double-quoted identifiers' do + sql = 'SELECT "column--name" FROM t' + expect(remover.remove(sql)).to eq(sql) + end + + it 'preserves -- in dollar-quoted strings' do + sql = 'SELECT $$text with -- comment$$ FROM t' + expect(remover.remove(sql)).to eq(sql) + end + end + + context 'with strategy :multiline' do + let(:remover) { described_class.new(:multiline) } + + it 'removes multi-line comments' do + sql = "SELECT * /* comment */ FROM t\nWHERE id = 1" + expected = "SELECT * FROM t\nWHERE id = 1" + expect(remover.remove(sql)).to eq(expected) + end + + it 'removes multi-line comments spanning multiple lines' do + sql = "SELECT * /* comment\nspanning lines */ FROM t" + expected = 'SELECT * FROM t' + expect(remover.remove(sql)).to eq(expected) + end + + it 'keeps single-line comments' do + sql = "SELECT * FROM t -- comment\nWHERE id = 1" + expect(remover.remove(sql)).to eq(sql) + end + + it 'preserves /* */ in single-quoted strings' do + sql = "SELECT '/* not a comment */' FROM t" + expect(remover.remove(sql)).to eq(sql) + end + + it 'preserves /* */ in double-quoted identifiers' do + sql = 'SELECT "column/* name */" FROM t' + expect(remover.remove(sql)).to eq(sql) + end + + it 'preserves /* */ in dollar-quoted strings' do + sql = 'SELECT $$text with /* comment */$$ FROM t' + expect(remover.remove(sql)).to eq(sql) + end + end + + context 'with strategy :all' do + let(:remover) { described_class.new(:all) } + + it 'removes both single-line and multi-line comments' do + sql = "SELECT * /* inline */ FROM t -- end\nWHERE id = 1" + expected = "SELECT * FROM t \nWHERE id = 1" + expect(remover.remove(sql)).to eq(expected) + end + + it 'removes multiple comments of different types' do + sql = "-- start\nSELECT /* c1 */ * /* c2 */ FROM t -- end" + expected = "\nSELECT * FROM t " + expect(remover.remove(sql)).to eq(expected) + end + + it 'preserves comments in all quote types' do + sql = "SELECT '-- c1', \"-- c2\", $$-- c3$$ FROM t" + expect(remover.remove(sql)).to eq(sql) + end + end + + context 'with escaped quotes' do + let(:remover) { described_class.new(:all) } + + it 'handles SQL-style escaped single quotes' do + sql = "SELECT 'it''s' FROM t -- comment" + expected = "SELECT 'it''s' FROM t " + expect(remover.remove(sql)).to eq(expected) + end + + it 'handles SQL-style escaped double quotes' do + sql = 'SELECT "col""name" FROM t -- comment' + expected = 'SELECT "col""name" FROM t ' + expect(remover.remove(sql)).to eq(expected) + end + + it 'handles backslash-escaped quotes' do + sql = "SELECT 'it\\'s' FROM t -- comment" + expected = "SELECT 'it\\'s' FROM t " + expect(remover.remove(sql)).to eq(expected) + end + end + + context 'with dollar-quoted strings' do + let(:remover) { described_class.new(:all) } + + it 'preserves comments in simple dollar quotes' do + sql = 'SELECT $$text with -- comment$$ FROM t' + expect(remover.remove(sql)).to eq(sql) + end + + it 'preserves comments in tagged dollar quotes' do + sql = 'SELECT $tag$text with /* comment */$tag$ FROM t' + expect(remover.remove(sql)).to eq(sql) + end + + it 'handles multiple dollar-quoted strings' do + sql = 'SELECT $$-- c1$$, $t$/* c2 */$t$ FROM t -- real comment' + expected = 'SELECT $$-- c1$$, $t$/* c2 */$t$ FROM t ' + expect(remover.remove(sql)).to eq(expected) + end + + it 'handles nested dollar quotes with different tags' do + sql = 'SELECT $outer$text $inner$nested$inner$$outer$ FROM t' + expect(remover.remove(sql)).to eq(sql) + end + end + + context 'with inline comments' do + let(:remover) { described_class.new(:all) } + + it 'removes inline block comments' do + sql = 'SELECT /* comment */ column FROM t' + expected = 'SELECT column FROM t' + expect(remover.remove(sql)).to eq(expected) + end + + it 'removes multiple inline comments' do + sql = 'SELECT /* c1 */ col1, /* c2 */ col2 FROM t' + expected = 'SELECT col1, col2 FROM t' + expect(remover.remove(sql)).to eq(expected) + end + end + + context 'with comments containing quotes' do + let(:remover) { described_class.new(:all) } + + it 'handles single-line comments with quotes' do + sql = "SELECT * FROM t -- comment with 'quotes' and \"more\"\nWHERE id = 1" + expected = "SELECT * FROM t \nWHERE id = 1" + expect(remover.remove(sql)).to eq(expected) + end + + it 'handles multi-line comments with quotes' do + sql = "SELECT * FROM t /* comment with 'quotes' and \"more\" */ WHERE id = 1" + expected = 'SELECT * FROM t WHERE id = 1' + expect(remover.remove(sql)).to eq(expected) + end + end + + context 'with edge cases' do + let(:remover) { described_class.new(:all) } + + it 'handles empty SQL' do + expect(remover.remove('')).to eq('') + end + + it 'handles SQL with only comments' do + sql = "-- comment 1\n/* comment 2 */" + expected = "\n" + expect(remover.remove(sql)).to eq(expected) + end + + it 'handles SQL with no comments' do + sql = 'SELECT * FROM t WHERE id = 1' + expect(remover.remove(sql)).to eq(sql) + end + + it 'handles comment-like patterns that are not comments' do + sql = "SELECT * FROM t WHERE url = 'http://example.com' AND x = 1" + expect(remover.remove(sql)).to eq(sql) + end + + it 'preserves newlines from removed single-line comments' do + sql = "SELECT *\n-- comment\nFROM t" + expected = "SELECT *\n\nFROM t" + expect(remover.remove(sql)).to eq(expected) + end + end + end +end diff --git a/spec/sql_query_spec.rb b/spec/sql_query_spec.rb index 390153b..58fde99 100644 --- a/spec/sql_query_spec.rb +++ b/spec/sql_query_spec.rb @@ -213,9 +213,144 @@ class Model < ActiveRecord::Base end end + describe 'comment removal integration' do + after do + # Reset configuration to defaults + SqlQuery.configure do |config| + config.remove_comments = :all + config.remove_comments_from = :all + end + end + + context 'with remove_comments_from: :all' do + before do + SqlQuery.configure do |config| + config.remove_comments = :all + config.remove_comments_from = :all + end + end + + it 'removes comments from sql() method' do + query = described_class.new(:with_single_line_comments, email: 'test@example.com') + result = query.sql + expect(result).not_to include('--') + expect(result).to include('SELECT * FROM players') + end + + it 'removes comments from prepared_for_logs() method' do + query = described_class.new(:with_multiline_comments, email: 'test@example.com') + result = query.prepared_for_logs + expect(result).not_to include('/*') + expect(result).not_to include('*/') + end + end + + context 'with remove_comments_from: :prepared_for_logs' do + before do + SqlQuery.configure do |config| + config.remove_comments = :all + config.remove_comments_from = :prepared_for_logs + end + end + + it 'keeps comments in sql() method' do + query = described_class.new(:with_single_line_comments, email: 'test@example.com') + result = query.sql + expect(result).to include('--') + end + + it 'removes comments from prepared_for_logs() method' do + query = described_class.new(:with_single_line_comments, email: 'test@example.com') + result = query.prepared_for_logs + expect(result).not_to include('--') + end + end + + context 'with remove_comments_from: :none' do + before do + SqlQuery.configure do |config| + config.remove_comments = :all + config.remove_comments_from = :none + end + end + + it 'keeps comments in sql() method' do + query = described_class.new(:with_mixed_comments, email: 'test@example.com') + result = query.sql + expect(result).to include('--') + expect(result).to include('/*') + end + + it 'keeps comments in prepared_for_logs() method' do + query = described_class.new(:with_mixed_comments, email: 'test@example.com') + result = query.prepared_for_logs + expect(result).to include('--') + expect(result).to include('/*') + end + end + + context 'with remove_comments: :oneline' do + before do + SqlQuery.configure do |config| + config.remove_comments = :oneline + config.remove_comments_from = :all + end + end + + it 'removes only single-line comments' do + query = described_class.new(:with_mixed_comments, email: 'test@example.com') + result = query.sql + expect(result).not_to include('--') + expect(result).to include('/*') + end + end + + context 'with remove_comments: :multiline' do + before do + SqlQuery.configure do |config| + config.remove_comments = :multiline + config.remove_comments_from = :all + end + end + + it 'removes only multi-line comments' do + query = described_class.new(:with_mixed_comments, email: 'test@example.com') + result = query.sql + expect(result).to include('--') + expect(result).not_to include('/*') + expect(result).not_to include('*/') + end + end + + context 'preserving comments in quoted strings' do + before do + SqlQuery.configure do |config| + config.remove_comments = :all + config.remove_comments_from = :all + end + end + + it 'preserves comment syntax in string literals' do + query = described_class.new(:with_comments_in_strings, email: 'test@example.com') + result = query.sql + expect(result).to include("'-- not a comment'") + expect(result).to include("'/* also not a comment */'") + expect(result).to include('"column--name"') + end + end + end + describe '.config' do it 'returns configuration instance' do expect(described_class.config).to be_kind_of(SqlQuery::Config) end + + it 'has default remove_comments setting' do + expect(described_class.config.remove_comments).to eq(:all) + end + + it 'has default remove_comments_from setting' do + expect(described_class.config.remove_comments_from).to eq(:all) + end end end From 757b2e845131039952f0dd8864be7b36295697e1 Mon Sep 17 00:00:00 2001 From: sufleR Date: Tue, 23 Dec 2025 15:15:06 +0100 Subject: [PATCH 2/3] use oneliners to make class smaller and code easier to read --- lib/sql_query.rb | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/lib/sql_query.rb b/lib/sql_query.rb index 2ae7c59..65927d8 100644 --- a/lib/sql_query.rb +++ b/lib/sql_query.rb @@ -4,7 +4,6 @@ require_relative 'sql_query/config' require_relative 'sql_query/comment_remover' -# rubocop:disable Metrics/ClassLength class SqlQuery attr_reader :connection @@ -37,10 +36,7 @@ def exec_query(prepare = true) end def sql - @sql ||= begin - rendered_sql = prepare_query(false) - apply_comment_removal(rendered_sql, for_logs: false) - end + @sql ||= apply_comment_removal(prepare_query(false), for_logs: false) end def pretty_sql @@ -52,10 +48,7 @@ def quote(value) end def prepared_for_logs - @prepared_for_logs ||= begin - rendered_sql = prepare_query(true) - apply_comment_removal(rendered_sql, for_logs: true) - end + @prepared_for_logs ||= apply_comment_removal(prepare_query(true), for_logs: true) end def partial(partial_name, partial_options = {}) @@ -138,4 +131,3 @@ def path tmp_path end end -# rubocop:enable Metrics/ClassLength From 05dc35f1dcbf9ec140dd712b0d38e37a65651448 Mon Sep 17 00:00:00 2001 From: sufleR Date: Tue, 23 Dec 2025 15:28:34 +0100 Subject: [PATCH 3/3] Refactor: Centralize comment removal logic in CommentRemover - CommentRemover now accepts config object instead of strategy symbol - CommentRemover handles all removal decisions (both strategy and scope) - Simplified SqlQuery#apply_comment_removal to always call CommentRemover - All comment removal logic now centralized in one place - Updated all tests to pass config objects and for_logs parameter - Added comprehensive tests for remove_comments_from configuration - 84 tests, all passing, 98.33% coverage - RuboCop compliant --- lib/sql_query.rb | 5 +- lib/sql_query/comment_remover.rb | 23 ++-- spec/sql_query/comment_remover_spec.rb | 143 ++++++++++++++++++------- 3 files changed, 116 insertions(+), 55 deletions(-) diff --git a/lib/sql_query.rb b/lib/sql_query.rb index 65927d8..11dfa20 100644 --- a/lib/sql_query.rb +++ b/lib/sql_query.rb @@ -85,10 +85,7 @@ def split_to_path_and_name(file) end def apply_comment_removal(sql, for_logs:) - config = self.class.config - return sql unless config.should_comments_be_removed?(for_logs: for_logs) - - CommentRemover.new(config.remove_comments).remove(sql) + CommentRemover.new(self.class.config).remove(sql, for_logs: for_logs) end def pretty(value) diff --git a/lib/sql_query/comment_remover.rb b/lib/sql_query/comment_remover.rb index a7be52c..a5131da 100644 --- a/lib/sql_query/comment_remover.rb +++ b/lib/sql_query/comment_remover.rb @@ -14,23 +14,26 @@ class SqlQuery # - Dollar-quoted strings (PostgreSQL): $$...$$ or $tag$...$tag$ # # @example - # remover = CommentRemover.new(:all) + # config = SqlQuery.config + # remover = CommentRemover.new(config) # sql = "SELECT * FROM t -- comment\nWHERE id = 1" - # remover.remove(sql) + # remover.remove(sql, for_logs: true) # # => "SELECT * FROM t \nWHERE id = 1" # # rubocop:disable Metrics/ClassLength class CommentRemover - def initialize(strategy) - @strategy = strategy # :none, :oneline, :multiline, :all + def initialize(config) + @config = config end - # Removes comments from SQL based on the configured strategy + # Removes comments from SQL based on the configuration # # @param sql [String] the SQL string to process - # @return [String] SQL with comments removed (or unchanged if strategy is :none) - def remove(sql) - return sql if @strategy == :none + # @param for_logs [Boolean] whether the query is being prepared for logs + # @return [String] SQL with comments removed (or unchanged based on config) + def remove(sql, for_logs:) + return sql unless @config.should_comments_be_removed?(for_logs: for_logs) + return sql if @config.remove_comments == :none state = init_state result = [] @@ -168,11 +171,11 @@ def in_comment?(state) end def should_remove_oneline? - @strategy == :oneline || @strategy == :all + %i[oneline all].include?(@config.remove_comments) end def should_remove_multiline? - @strategy == :multiline || @strategy == :all + %i[multiline all].include?(@config.remove_comments) end # Extracts dollar quote tag from position diff --git a/spec/sql_query/comment_remover_spec.rb b/spec/sql_query/comment_remover_spec.rb index ee707f3..bbccd0e 100644 --- a/spec/sql_query/comment_remover_spec.rb +++ b/spec/sql_query/comment_remover_spec.rb @@ -3,215 +3,276 @@ require 'spec_helper' RSpec.describe SqlQuery::CommentRemover do + # Helper to create config with specific settings + def build_config(remove_comments:, remove_comments_from: :all) + config = SqlQuery::Config.new + config.remove_comments = remove_comments + config.remove_comments_from = remove_comments_from + config + end + describe '#remove' do context 'with strategy :none' do - let(:remover) { described_class.new(:none) } + let(:config) { build_config(remove_comments: :none) } + let(:remover) { described_class.new(config) } it 'returns SQL unchanged' do sql = "SELECT * FROM t -- comment\nWHERE id = 1" - expect(remover.remove(sql)).to eq(sql) + expect(remover.remove(sql, for_logs: true)).to eq(sql) end end context 'with strategy :oneline' do - let(:remover) { described_class.new(:oneline) } + let(:config) { build_config(remove_comments: :oneline) } + let(:remover) { described_class.new(config) } it 'removes single-line comments' do sql = "SELECT * FROM t -- comment\nWHERE id = 1" expected = "SELECT * FROM t \nWHERE id = 1" - expect(remover.remove(sql)).to eq(expected) + expect(remover.remove(sql, for_logs: true)).to eq(expected) end it 'removes comment at end of file without newline' do sql = 'SELECT * FROM t --comment' expected = 'SELECT * FROM t ' - expect(remover.remove(sql)).to eq(expected) + expect(remover.remove(sql, for_logs: true)).to eq(expected) end it 'keeps multi-line comments' do sql = "SELECT * FROM t /* comment */\nWHERE id = 1" - expect(remover.remove(sql)).to eq(sql) + expect(remover.remove(sql, for_logs: true)).to eq(sql) end it 'preserves -- in single-quoted strings' do sql = "SELECT '-- not a comment' FROM t" - expect(remover.remove(sql)).to eq(sql) + expect(remover.remove(sql, for_logs: true)).to eq(sql) end it 'preserves -- in double-quoted identifiers' do sql = 'SELECT "column--name" FROM t' - expect(remover.remove(sql)).to eq(sql) + expect(remover.remove(sql, for_logs: true)).to eq(sql) end it 'preserves -- in dollar-quoted strings' do sql = 'SELECT $$text with -- comment$$ FROM t' - expect(remover.remove(sql)).to eq(sql) + expect(remover.remove(sql, for_logs: true)).to eq(sql) end end context 'with strategy :multiline' do - let(:remover) { described_class.new(:multiline) } + let(:config) { build_config(remove_comments: :multiline) } + let(:remover) { described_class.new(config) } it 'removes multi-line comments' do sql = "SELECT * /* comment */ FROM t\nWHERE id = 1" expected = "SELECT * FROM t\nWHERE id = 1" - expect(remover.remove(sql)).to eq(expected) + expect(remover.remove(sql, for_logs: true)).to eq(expected) end it 'removes multi-line comments spanning multiple lines' do sql = "SELECT * /* comment\nspanning lines */ FROM t" expected = 'SELECT * FROM t' - expect(remover.remove(sql)).to eq(expected) + expect(remover.remove(sql, for_logs: true)).to eq(expected) end it 'keeps single-line comments' do sql = "SELECT * FROM t -- comment\nWHERE id = 1" - expect(remover.remove(sql)).to eq(sql) + expect(remover.remove(sql, for_logs: true)).to eq(sql) end it 'preserves /* */ in single-quoted strings' do sql = "SELECT '/* not a comment */' FROM t" - expect(remover.remove(sql)).to eq(sql) + expect(remover.remove(sql, for_logs: true)).to eq(sql) end it 'preserves /* */ in double-quoted identifiers' do sql = 'SELECT "column/* name */" FROM t' - expect(remover.remove(sql)).to eq(sql) + expect(remover.remove(sql, for_logs: true)).to eq(sql) end it 'preserves /* */ in dollar-quoted strings' do sql = 'SELECT $$text with /* comment */$$ FROM t' - expect(remover.remove(sql)).to eq(sql) + expect(remover.remove(sql, for_logs: true)).to eq(sql) end end context 'with strategy :all' do - let(:remover) { described_class.new(:all) } + let(:config) { build_config(remove_comments: :all) } + let(:remover) { described_class.new(config) } it 'removes both single-line and multi-line comments' do sql = "SELECT * /* inline */ FROM t -- end\nWHERE id = 1" expected = "SELECT * FROM t \nWHERE id = 1" - expect(remover.remove(sql)).to eq(expected) + expect(remover.remove(sql, for_logs: true)).to eq(expected) end it 'removes multiple comments of different types' do sql = "-- start\nSELECT /* c1 */ * /* c2 */ FROM t -- end" expected = "\nSELECT * FROM t " - expect(remover.remove(sql)).to eq(expected) + expect(remover.remove(sql, for_logs: true)).to eq(expected) end it 'preserves comments in all quote types' do sql = "SELECT '-- c1', \"-- c2\", $$-- c3$$ FROM t" - expect(remover.remove(sql)).to eq(sql) + expect(remover.remove(sql, for_logs: true)).to eq(sql) end end context 'with escaped quotes' do - let(:remover) { described_class.new(:all) } + let(:config) { build_config(remove_comments: :all) } + let(:remover) { described_class.new(config) } it 'handles SQL-style escaped single quotes' do sql = "SELECT 'it''s' FROM t -- comment" expected = "SELECT 'it''s' FROM t " - expect(remover.remove(sql)).to eq(expected) + expect(remover.remove(sql, for_logs: true)).to eq(expected) end it 'handles SQL-style escaped double quotes' do sql = 'SELECT "col""name" FROM t -- comment' expected = 'SELECT "col""name" FROM t ' - expect(remover.remove(sql)).to eq(expected) + expect(remover.remove(sql, for_logs: true)).to eq(expected) end it 'handles backslash-escaped quotes' do sql = "SELECT 'it\\'s' FROM t -- comment" expected = "SELECT 'it\\'s' FROM t " - expect(remover.remove(sql)).to eq(expected) + expect(remover.remove(sql, for_logs: true)).to eq(expected) end end context 'with dollar-quoted strings' do - let(:remover) { described_class.new(:all) } + let(:config) { build_config(remove_comments: :all) } + let(:remover) { described_class.new(config) } it 'preserves comments in simple dollar quotes' do sql = 'SELECT $$text with -- comment$$ FROM t' - expect(remover.remove(sql)).to eq(sql) + expect(remover.remove(sql, for_logs: true)).to eq(sql) end it 'preserves comments in tagged dollar quotes' do sql = 'SELECT $tag$text with /* comment */$tag$ FROM t' - expect(remover.remove(sql)).to eq(sql) + expect(remover.remove(sql, for_logs: true)).to eq(sql) end it 'handles multiple dollar-quoted strings' do sql = 'SELECT $$-- c1$$, $t$/* c2 */$t$ FROM t -- real comment' expected = 'SELECT $$-- c1$$, $t$/* c2 */$t$ FROM t ' - expect(remover.remove(sql)).to eq(expected) + expect(remover.remove(sql, for_logs: true)).to eq(expected) end it 'handles nested dollar quotes with different tags' do sql = 'SELECT $outer$text $inner$nested$inner$$outer$ FROM t' - expect(remover.remove(sql)).to eq(sql) + expect(remover.remove(sql, for_logs: true)).to eq(sql) end end context 'with inline comments' do - let(:remover) { described_class.new(:all) } + let(:config) { build_config(remove_comments: :all) } + let(:remover) { described_class.new(config) } it 'removes inline block comments' do sql = 'SELECT /* comment */ column FROM t' expected = 'SELECT column FROM t' - expect(remover.remove(sql)).to eq(expected) + expect(remover.remove(sql, for_logs: true)).to eq(expected) end it 'removes multiple inline comments' do sql = 'SELECT /* c1 */ col1, /* c2 */ col2 FROM t' expected = 'SELECT col1, col2 FROM t' - expect(remover.remove(sql)).to eq(expected) + expect(remover.remove(sql, for_logs: true)).to eq(expected) end end context 'with comments containing quotes' do - let(:remover) { described_class.new(:all) } + let(:config) { build_config(remove_comments: :all) } + let(:remover) { described_class.new(config) } it 'handles single-line comments with quotes' do sql = "SELECT * FROM t -- comment with 'quotes' and \"more\"\nWHERE id = 1" expected = "SELECT * FROM t \nWHERE id = 1" - expect(remover.remove(sql)).to eq(expected) + expect(remover.remove(sql, for_logs: true)).to eq(expected) end it 'handles multi-line comments with quotes' do sql = "SELECT * FROM t /* comment with 'quotes' and \"more\" */ WHERE id = 1" expected = 'SELECT * FROM t WHERE id = 1' - expect(remover.remove(sql)).to eq(expected) + expect(remover.remove(sql, for_logs: true)).to eq(expected) end end context 'with edge cases' do - let(:remover) { described_class.new(:all) } + let(:config) { build_config(remove_comments: :all) } + let(:remover) { described_class.new(config) } it 'handles empty SQL' do - expect(remover.remove('')).to eq('') + expect(remover.remove('', for_logs: true)).to eq('') end it 'handles SQL with only comments' do sql = "-- comment 1\n/* comment 2 */" expected = "\n" - expect(remover.remove(sql)).to eq(expected) + expect(remover.remove(sql, for_logs: true)).to eq(expected) end it 'handles SQL with no comments' do sql = 'SELECT * FROM t WHERE id = 1' - expect(remover.remove(sql)).to eq(sql) + expect(remover.remove(sql, for_logs: true)).to eq(sql) end it 'handles comment-like patterns that are not comments' do sql = "SELECT * FROM t WHERE url = 'http://example.com' AND x = 1" - expect(remover.remove(sql)).to eq(sql) + expect(remover.remove(sql, for_logs: true)).to eq(sql) end it 'preserves newlines from removed single-line comments' do sql = "SELECT *\n-- comment\nFROM t" expected = "SELECT *\n\nFROM t" - expect(remover.remove(sql)).to eq(expected) + expect(remover.remove(sql, for_logs: true)).to eq(expected) + end + end + + context 'with remove_comments_from configuration' do + let(:sql) { "SELECT * FROM t -- comment\nWHERE id = 1" } + let(:expected) { "SELECT * FROM t \nWHERE id = 1" } + + context 'when remove_comments_from is :all' do + let(:config) { build_config(remove_comments: :all, remove_comments_from: :all) } + let(:remover) { described_class.new(config) } + + it 'removes comments for logs' do + expect(remover.remove(sql, for_logs: true)).to eq(expected) + end + + it 'removes comments for non-logs' do + expect(remover.remove(sql, for_logs: false)).to eq(expected) + end + end + + context 'when remove_comments_from is :prepared_for_logs' do + let(:config) { build_config(remove_comments: :all, remove_comments_from: :prepared_for_logs) } + let(:remover) { described_class.new(config) } + + it 'removes comments for logs' do + expect(remover.remove(sql, for_logs: true)).to eq(expected) + end + + it 'keeps comments for non-logs' do + expect(remover.remove(sql, for_logs: false)).to eq(sql) + end + end + + context 'when remove_comments_from is :none' do + let(:config) { build_config(remove_comments: :all, remove_comments_from: :none) } + let(:remover) { described_class.new(config) } + + it 'keeps comments for logs' do + expect(remover.remove(sql, for_logs: true)).to eq(sql) + end + + it 'keeps comments for non-logs' do + expect(remover.remove(sql, for_logs: false)).to eq(sql) + end end end end