From 28c9d9b41482509a20ddb32fa52000a29241d355 Mon Sep 17 00:00:00 2001 From: Lukas Fittl Date: Tue, 4 Feb 2025 23:22:52 -0800 Subject: [PATCH 1/9] Tests: Replace "is_expected" with "expect(subject)" in multi-line blocks This is more idiomatic, per the RSpec/ImplicitSubject rule. --- spec/lib/param_refs_spec.rb | 8 ++++---- spec/lib/pg_query/deparse_spec.rb | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/lib/param_refs_spec.rb b/spec/lib/param_refs_spec.rb index 591c521e..c848ca6d 100644 --- a/spec/lib/param_refs_spec.rb +++ b/spec/lib/param_refs_spec.rb @@ -13,7 +13,7 @@ let(:query) { 'SELECT * FROM x WHERE y = $1::text AND z < now() - INTERVAL $2' } it do - is_expected.to eq( + expect(subject).to eq( [ { "location"=>26, @@ -34,9 +34,9 @@ let(:query) { 'SELECT * FROM a WHERE x = $1 AND y = $12 AND z = $255' } it do - is_expected.to eq [{"location"=>26, "length"=>2}, - {"location"=>37, "length"=>3}, - {"location"=>49, "length"=>4}] + expect(subject).to eq [{"location"=>26, "length"=>2}, + {"location"=>37, "length"=>3}, + {"location"=>49, "length"=>4}] end end end diff --git a/spec/lib/pg_query/deparse_spec.rb b/spec/lib/pg_query/deparse_spec.rb index edebb712..dd837c68 100644 --- a/spec/lib/pg_query/deparse_spec.rb +++ b/spec/lib/pg_query/deparse_spec.rb @@ -1072,7 +1072,7 @@ end it do - is_expected.to eq( + expect(subject).to eq( 'CREATE TABLE types (a real, b double precision, c numeric(2, 3), d char(4), e char(5), f varchar(6), g varchar(7))' ) end @@ -1086,7 +1086,7 @@ end it do - is_expected.to eq( + expect(subject).to eq( 'CREATE TABLE types (a geometry(point) NOT NULL)' ) end From 1be5e0b49cd64ec02ee206d5243089bfc5700ddc Mon Sep 17 00:00:00 2001 From: Lukas Fittl Date: Tue, 4 Feb 2025 23:23:33 -0800 Subject: [PATCH 2/9] Tests: Avoid using triple quote (''' or """) syntax This is unnecessary in ruby, single quotes are sufficient for multi-line strings. See https://dev.to/jbranchaud/there-is-no-triple-quote-multi-line-string-syntax-in-ruby-4gj5 --- spec/lib/pg_query/deparse_spec.rb | 156 ++++++++++++++++-------------- 1 file changed, 82 insertions(+), 74 deletions(-) diff --git a/spec/lib/pg_query/deparse_spec.rb b/spec/lib/pg_query/deparse_spec.rb index dd837c68..211eb0e6 100644 --- a/spec/lib/pg_query/deparse_spec.rb +++ b/spec/lib/pg_query/deparse_spec.rb @@ -653,7 +653,7 @@ context 'WITH' do let(:query) do - ''' + ' WITH moved AS ( DELETE FROM employees @@ -661,7 +661,7 @@ ) INSERT INTO employees_of_mary SELECT * FROM moved; - ''' + ' end it { is_expected.to eq oneline_query } @@ -693,7 +693,7 @@ context 'HAVING' do let(:query) do - ''' + ' INSERT INTO employees SELECT * FROM people WHERE 1 = 1 @@ -703,7 +703,7 @@ LIMIT 10 OFFSET 15 FOR UPDATE - ''' + ' end it { is_expected.to eq oneline_query } @@ -717,9 +717,9 @@ context 'with locks' do let(:query) do - ''' + ' SELECT * FROM people FOR UPDATE OF name, email - ''' + ' end it { is_expected.to eq oneline_query } @@ -727,9 +727,9 @@ context 'with cast varchar' do let(:query) do - ''' + ' SELECT name::varchar(255) FROM people - ''' + ' end it { is_expected.to eq oneline_query } @@ -737,9 +737,9 @@ context 'with cast varchar and no arguments' do let(:query) do - ''' + ' SELECT name::varchar FROM people - ''' + ' end it { is_expected.to eq oneline_query } @@ -747,9 +747,9 @@ context 'with cast numeric' do let(:query) do - ''' + ' SELECT age::numeric(5, 2) FROM people - ''' + ' end it { is_expected.to eq oneline_query } @@ -757,9 +757,9 @@ context 'with cast numeric and no arguments' do let(:query) do - ''' + ' SELECT age::numeric FROM people - ''' + ' end it { is_expected.to eq oneline_query } @@ -783,14 +783,14 @@ context 'WITH' do let(:query) do - ''' + ' WITH archived AS ( DELETE FROM employees WHERE manager_name = \'Mary\' ) UPDATE users SET archived = true WHERE users.id IN (SELECT user_id FROM moved) - ''' + ' end it { is_expected.to eq oneline_query } @@ -798,7 +798,7 @@ context 'WITH FROM UPDATE' do let(:query) do - ''' + ' WITH archived AS ( DELETE FROM employees @@ -806,7 +806,7 @@ RETURNING user_id ) UPDATE users SET archived = true FROM archived WHERE archived.user_id = id RETURNING id - ''' + ' end it { is_expected.to eq oneline_query } @@ -814,7 +814,7 @@ context 'from generated sequence' do let(:query) do - ''' + ' INSERT INTO jackdanger_card_totals (id, amount_cents, created_at) SELECT series.i, @@ -823,7 +823,7 @@ \'2015-08-25 00:00:00 -0700\'::timestamp + ((\'2015-08-25 23:59:59 -0700\'::timestamp - \'2015-08-25 00:00:00 -0700\'::timestamp) * random())) FROM generate_series(1, 10000) series(i); - ''' + ' end it { is_expected.to eq oneline_query } @@ -857,14 +857,14 @@ context 'WITH' do let(:query) do - ''' + ' WITH archived AS ( DELETE FROM employees WHERE manager_name = \'Mary\' ) DELETE FROM users WHERE users.id IN (SELECT user_id FROM moved) - ''' + ' end it { is_expected.to eq oneline_query } @@ -874,27 +874,29 @@ context 'CREATE CAST' do context 'with function' do let(:query) do - """ + " CREATE CAST (bigint AS int4) WITH FUNCTION int4(bigint) AS ASSIGNMENT - """.strip + ".strip end it { is_expected.to eq query } end + context 'without function' do let(:query) do - """ + " CREATE CAST (bigint AS int4) WITHOUT FUNCTION AS IMPLICIT - """.strip + ".strip end it { is_expected.to eq query } end + context 'with inout' do let(:query) do - """ + " CREATE CAST (bigint AS int4) WITH INOUT AS ASSIGNMENT - """.strip + ".strip end it { is_expected.to eq query } @@ -904,13 +906,13 @@ context 'CREATE DOMAIN' do context 'with check' do let(:query) do - """ + " CREATE DOMAIN us_postal_code AS text CHECK ( \"VALUE\" ~ '^\d{5}$' OR \"VALUE\" ~ '^\d{5}-\d{4}$' ); - """.strip + ".strip end it { is_expected.to eq oneline_query } @@ -921,11 +923,11 @@ # Taken from http://www.postgresql.org/docs/8.3/static/queries-table-expressions.html context 'with inline function definition' do let(:query) do - """ + " CREATE FUNCTION getfoo(int) RETURNS SETOF users AS $$ SELECT * FROM \"users\" WHERE users.id = $1; $$ LANGUAGE sql - """.strip + ".strip end it { is_expected.to eq query } @@ -933,11 +935,11 @@ context 'with or replace' do let(:query) do - """ + " CREATE OR REPLACE FUNCTION getfoo(int) RETURNS SETOF users AS $$ SELECT * FROM \"users\" WHERE users.id = $1; $$ LANGUAGE sql - """.strip + ".strip end it { is_expected.to eq query } @@ -945,11 +947,11 @@ context 'with immutable' do let(:query) do - """ + " CREATE OR REPLACE FUNCTION getfoo(int) RETURNS SETOF users AS $$ SELECT * FROM \"users\" WHERE users.id = $1; $$ LANGUAGE sql IMMUTABLE - """.strip + ".strip end it { is_expected.to eq query } @@ -957,11 +959,11 @@ context 'with STRICT (aka return null on null input)' do let(:query) do - """ + " CREATE OR REPLACE FUNCTION getfoo(int) RETURNS SETOF users AS $$ SELECT * FROM \"users\" WHERE users.id = $1; $$ LANGUAGE sql IMMUTABLE RETURNS NULL ON NULL INPUT - """.strip + ".strip end it { is_expected.to eq query } @@ -969,11 +971,11 @@ context 'with called on null input' do let(:query) do - """ + " CREATE OR REPLACE FUNCTION getfoo(int) RETURNS SETOF users AS $$ SELECT * FROM \"users\" WHERE users.id = $1; $$ LANGUAGE sql IMMUTABLE CALLED ON NULL INPUT - """.strip + ".strip end it { is_expected.to eq query } @@ -981,11 +983,11 @@ context 'without parameters' do let(:query) do - """ + " CREATE OR REPLACE FUNCTION getfoo() RETURNS text AS $$ SELECT name FROM \"users\" LIMIT 1 $$ LANGUAGE sql IMMUTABLE CALLED ON NULL INPUT - """.strip + ".strip end it { is_expected.to eq query } @@ -1014,12 +1016,12 @@ context 'with cschema_element' do let(:query) do - """ + " CREATE SCHEMA hollywood CREATE TABLE films (title text, release date, awards text[]) CREATE VIEW winners AS SELECT title, release FROM films WHERE awards IS NOT NULL - """.strip + ".strip end it { is_expected.to eq oneline_query } @@ -1029,7 +1031,7 @@ context 'CREATE TABLE' do context 'top-level' do let(:query) do - ''' + ' CREATE UNLOGGED TABLE cities ( name text, population real, @@ -1038,7 +1040,7 @@ postal_code int, foreign_id bigint ); - ''' + ' end it { is_expected.to eq oneline_query } @@ -1046,7 +1048,7 @@ context 'with common types' do let(:query) do - ''' + ' CREATE TABLE IF NOT EXISTS distributors ( name varchar(40) DEFAULT \'Luso Films\', len interval hour to second(3), @@ -1058,7 +1060,7 @@ timetz time with time zone, CONSTRAINT name_len PRIMARY KEY (name, len) ); - ''' + ' end it { is_expected.to eq oneline_query } @@ -1066,9 +1068,9 @@ context 'with alternate typecasts' do let(:query) do - """ + " CREATE TABLE types (a float(2), b float(49), c NUMERIC(2, 3), d character(4), e char(5), f varchar(6), g character varying(7)); - """ + " end it do @@ -1080,9 +1082,9 @@ context 'with custom typecasts with arguments' do let(:query) do - """ + " CREATE TABLE types (a geometry(point) not null); - """ + " end it do @@ -1094,11 +1096,11 @@ context 'with column definition options' do let(:query) do - ''' + ' CREATE TABLE tablename ( colname int NOT NULL DEFAULT nextval(\'tablename_colname_seq\') ); - ''' + ' end it { is_expected.to eq oneline_query } @@ -1106,11 +1108,11 @@ context 'inheriting' do let(:query) do - ''' + ' CREATE TABLE capitals ( state char(2) ) INHERITS (cities); - ''' + ' end it { is_expected.to eq oneline_query } @@ -1352,7 +1354,7 @@ context 'ALTER TABLE' do context 'with column modifications' do let(:query) do - ''' + ' ALTER TABLE distributors DROP CONSTRAINT distributors_pkey, ADD CONSTRAINT distributors_pkey PRIMARY KEY USING INDEX dist_id_temp_idx, @@ -1365,7 +1367,7 @@ ALTER COLUMN tstamp SET STATISTICS -5, ADD COLUMN some_int int NOT NULL, DROP IF EXISTS other_column CASCADE; - ''' + ' end it { is_expected.to eq oneline_query } @@ -1510,11 +1512,11 @@ context 'COMMENTS' do let(:query) do - ''' + ' CREATE TABLE remove_comments ( id int -- inline comment in multiline ); - ''' + ' end it { is_expected.to eq('CREATE TABLE remove_comments (id int)') } @@ -1593,37 +1595,40 @@ context 'SET' do context 'with integer value' do let(:query) do - ''' + ' SET statement_timeout TO 10000; - ''' + ' end it { is_expected.to eq oneline_query } end + context 'with string value' do let(:query) do - ''' + ' SET search_path TO my_schema, public; - ''' + ' end it { is_expected.to eq oneline_query } end + context 'with local scope' do let(:query) do - ''' + ' SET LOCAL search_path TO my_schema, public; - ''' + ' end it { is_expected.to eq oneline_query } end + # Because SESSION is default, it is removed by the query parser. context 'with session scope' do let(:query) do - ''' + ' SET SESSION search_path TO 10000; - ''' + ' end it { is_expected.to eq "SET search_path TO 10000" } @@ -1772,16 +1777,19 @@ it { is_expected.to eq oneline_query } end + context 'plans' do let(:query) { 'DISCARD PLANS' } it { is_expected.to eq oneline_query } end + context 'sequences' do let(:query) { 'DISCARD SEQUENCES' } it { is_expected.to eq oneline_query } end + context 'temp' do let(:query) { 'DISCARD TEMP' } @@ -2110,10 +2118,10 @@ context 'for single query' do let(:query) do - ''' + ' SELECT m.name AS mname, pname FROM manufacturers m LEFT JOIN LATERAL get_product_names(m.id) pname ON true - ''' + ' end it { is_expected.to eq oneline_query } @@ -2121,12 +2129,12 @@ context 'for multiple queries' do let(:query) do - ''' + ' SELECT m.name AS mname, pname FROM manufacturers m LEFT JOIN LATERAL get_product_names(m.id) pname ON true; INSERT INTO manufacturers_daily (a, b) SELECT a, b FROM manufacturers; - ''' + ' end it { is_expected.to eq oneline_query } @@ -2134,13 +2142,13 @@ context 'for multiple queries with a semicolon inside a value' do let(:query) do - ''' + ' SELECT m.name AS mname, pname FROM manufacturers m LEFT JOIN LATERAL get_product_names(m.id) pname ON true; UPDATE users SET name = \'bobby; drop tables\'; INSERT INTO manufacturers_daily (a, b) SELECT a, b FROM manufacturers; - ''' + ' end it { is_expected.to eq oneline_query } From 7d1182a8705950888ddb5f19ee648cfce74bf478 Mon Sep 17 00:00:00 2001 From: Lukas Fittl Date: Tue, 4 Feb 2025 23:24:38 -0800 Subject: [PATCH 3/9] Tests: Drop pending "ignores aliases referenced in query" fingerprint test This has been pending for a long time, and resolving this seems out of scope for what pg_query does when it runs its fingerprinting. We don't gain much by keeping this, and it clutters the test output. --- spec/lib/fingerprint_spec.rb | 6 ------ 1 file changed, 6 deletions(-) diff --git a/spec/lib/fingerprint_spec.rb b/spec/lib/fingerprint_spec.rb index f7198e27..97c9a40d 100644 --- a/spec/lib/fingerprint_spec.rb +++ b/spec/lib/fingerprint_spec.rb @@ -69,12 +69,6 @@ def fingerprint_defs expect(fingerprint("SELECT a AS b UNION SELECT x AS y")).to eq fingerprint("SELECT a AS c UNION SELECT x AS z") end - it "ignores aliases referenced in query" do - pending - expect(fingerprint("SELECT s1.id FROM snapshots s1")).to eq fingerprint("SELECT s2.id FROM snapshots s2") - expect(fingerprint("SELECT a AS b ORDER BY b")).to eq fingerprint("SELECT a AS c ORDER BY c") - end - it "ignores param references" do expect(fingerprint("SELECT $1")).to eq fingerprint("SELECT $2") end From 62e60cb144d7f9f7b6e747cebd84ecd40a39ca42 Mon Sep 17 00:00:00 2001 From: Lukas Fittl Date: Tue, 4 Feb 2025 23:29:41 -0800 Subject: [PATCH 4/9] Parse: Drop stale "FIXME" for statements_and_cte_names_for_with_clause Its not clear what the open item here is, and this is most likely a accidental leftover from when this code was modified in e4eff25. --- lib/pg_query/parse.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pg_query/parse.rb b/lib/pg_query/parse.rb index 0b0483bf..a21bae10 100644 --- a/lib/pg_query/parse.rb +++ b/lib/pg_query/parse.rb @@ -339,7 +339,7 @@ def load_objects! # rubocop:disable Metrics/CyclomaticComplexity @cte_names.uniq! end - def statements_and_cte_names_for_with_clause(with_clause) # FIXME + def statements_and_cte_names_for_with_clause(with_clause) statements = [] cte_names = [] From 28a787e69ae3b9a6d95272e2a68655799385543f Mon Sep 17 00:00:00 2001 From: Lukas Fittl Date: Tue, 4 Feb 2025 23:36:10 -0800 Subject: [PATCH 5/9] Truncate: Use "len" instead of "length" for PossibleTruncation struct All Struct objects have "length" defined (which is the number of fields in the Struct), and it seems sensible to not override this unnecessarily. --- lib/pg_query/truncate.rb | 36 +++++++++++++++++------------------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/lib/pg_query/truncate.rb b/lib/pg_query/truncate.rb index c7383ba8..1513812f 100644 --- a/lib/pg_query/truncate.rb +++ b/lib/pg_query/truncate.rb @@ -1,7 +1,6 @@ - module PgQuery class ParserResult - PossibleTruncation = Struct.new(:location, :node_type, :length, :is_array) + PossibleTruncation = Struct.new(:location, :node_type, :len, :is_array) # Truncates the query string to be below the specified length, first trying to # omit less important parts of the query, and only then cutting off the end. @@ -14,11 +13,11 @@ def truncate(max_length) # rubocop:disable Metrics/CyclomaticComplexity truncations = find_possible_truncations # Truncate the deepest possible truncation that is the longest first - truncations.sort_by! { |t| [-t.location.size, -t.length] } + truncations.sort_by! { |t| [-t.location.size, -t.len] } tree = dup_tree truncations.each do |truncation| - next if truncation.length < 3 + next if truncation.len < 3 find_tree_location(tree, truncation.location) do |node, _k| dummy_column_ref = PgQuery::Node.new(column_ref: PgQuery::ColumnRef.new(fields: [PgQuery::Node.new(string: PgQuery::String.new(sval: '…'))])) @@ -64,30 +63,29 @@ def find_possible_truncations # rubocop:disable Metrics/CyclomaticComplexity case k when :target_list next unless node.is_a?(PgQuery::SelectStmt) || node.is_a?(PgQuery::UpdateStmt) || node.is_a?(PgQuery::OnConflictClause) - length = if node.is_a?(PgQuery::SelectStmt) - select_target_list_len(v) - else # UpdateStmt / OnConflictClause - update_target_list_len(v) - end - truncations << PossibleTruncation.new(location, :target_list, length, true) + len = if node.is_a?(PgQuery::SelectStmt) + select_target_list_len(v) + else # UpdateStmt / OnConflictClause + update_target_list_len(v) + end + truncations << PossibleTruncation.new(location, :target_list, len, true) when :where_clause next unless node.is_a?(PgQuery::SelectStmt) || node.is_a?(PgQuery::UpdateStmt) || node.is_a?(PgQuery::DeleteStmt) || node.is_a?(PgQuery::CopyStmt) || node.is_a?(PgQuery::IndexStmt) || node.is_a?(PgQuery::RuleStmt) || node.is_a?(PgQuery::InferClause) || node.is_a?(PgQuery::OnConflictClause) - - length = PgQuery.deparse_expr(v).size - truncations << PossibleTruncation.new(location, :where_clause, length, false) + len = PgQuery.deparse_expr(v).size + truncations << PossibleTruncation.new(location, :where_clause, len, false) when :values_lists - length = select_values_lists_len(v) - truncations << PossibleTruncation.new(location, :values_lists, length, false) + len = select_values_lists_len(v) + truncations << PossibleTruncation.new(location, :values_lists, len, false) when :ctequery next unless node.is_a?(PgQuery::CommonTableExpr) - length = PgQuery.deparse_stmt(v[v.node.to_s]).size - truncations << PossibleTruncation.new(location, :ctequery, length, false) + len = PgQuery.deparse_stmt(v[v.node.to_s]).size + truncations << PossibleTruncation.new(location, :ctequery, len, false) when :cols next unless node.is_a?(PgQuery::InsertStmt) - length = cols_len(v) - truncations << PossibleTruncation.new(location, :cols, length, true) + len = cols_len(v) + truncations << PossibleTruncation.new(location, :cols, len, true) end end From 980ca6c20c2766d915082f0b1421ce4f43d9775c Mon Sep 17 00:00:00 2001 From: Lukas Fittl Date: Tue, 4 Feb 2025 23:40:40 -0800 Subject: [PATCH 6/9] Various stylistic fixe to accomodate Rubocop This is in anticipation of a follow-up commit that updates the Rubocop version to the latest and introduces new stylistic checks that seem acceptable to enable. --- lib/pg_query/fingerprint.rb | 8 +++----- lib/pg_query/param_refs.rb | 2 +- lib/pg_query/parse.rb | 4 +--- lib/pg_query/parse_error.rb | 1 + lib/pg_query/scan.rb | 1 + lib/pg_query/treewalker.rb | 8 ++------ 6 files changed, 9 insertions(+), 15 deletions(-) diff --git a/lib/pg_query/fingerprint.rb b/lib/pg_query/fingerprint.rb index 53230e60..4f953ff1 100644 --- a/lib/pg_query/fingerprint.rb +++ b/lib/pg_query/fingerprint.rb @@ -61,7 +61,7 @@ def fingerprint_value(val, hash, parent_node_name, parent_field_name, need_to_wr def ignored_node_type?(node) [A_Const, Alias, ParamRef, SetToDefault, IntList, OidList].include?(node.class) || - node.is_a?(TypeCast) && (node.arg.node == :a_const || node.arg.node == :param_ref) + (node.is_a?(TypeCast) && %i[a_const param_ref].include?(node.arg.node)) end def node_protobuf_field_name_to_json_name(node_class, field) @@ -112,12 +112,10 @@ def fingerprint_node(node, hash, parent_node_name = nil, parent_field_name = nil fingerprint_value(val.gsub(/\d{2,}/, ''), hash, postgres_node_name, postgres_field_name, true) next end - when 'stmt_len' - next if node.is_a?(RawStmt) - when 'stmt_location' + when 'stmt_len', 'stmt_location' next if node.is_a?(RawStmt) when 'kind' - if node.is_a?(A_Expr) && (val == :AEXPR_OP_ANY || val == :AEXPR_IN) + if node.is_a?(A_Expr) && %i[AEXPR_OP_ANY AEXPR_IN].include?(val) fingerprint_value(:AEXPR_OP, hash, postgres_node_name, postgres_field_name, true) next end diff --git a/lib/pg_query/param_refs.rb b/lib/pg_query/param_refs.rb index 0a9bd74a..d00661e5 100644 --- a/lib/pg_query/param_refs.rb +++ b/lib/pg_query/param_refs.rb @@ -7,7 +7,7 @@ def param_refs # rubocop:disable Metrics/CyclomaticComplexity case node when PgQuery::ParamRef # Ignore param refs inside type casts, as these are already handled - next if location[-3..-1] == %i[type_cast arg param_ref] + next if location[-3..] == %i[type_cast arg param_ref] results << { 'location' => node.location, 'length' => param_ref_length(node) } diff --git a/lib/pg_query/parse.rb b/lib/pg_query/parse.rb index a21bae10..35ffdd8a 100644 --- a/lib/pg_query/parse.rb +++ b/lib/pg_query/parse.rb @@ -24,9 +24,7 @@ def self.parse(query) end class ParserResult - attr_reader :query - attr_reader :tree - attr_reader :warnings + attr_reader :query, :tree, :warnings def initialize(query, tree, warnings = []) @query = query diff --git a/lib/pg_query/parse_error.rb b/lib/pg_query/parse_error.rb index da99e049..7d72ec4d 100644 --- a/lib/pg_query/parse_error.rb +++ b/lib/pg_query/parse_error.rb @@ -1,6 +1,7 @@ module PgQuery class ParseError < ArgumentError attr_reader :location + def initialize(message, source_file, source_line, location) super("#{message} (#{source_file}:#{source_line})") @location = location diff --git a/lib/pg_query/scan.rb b/lib/pg_query/scan.rb index cf5a6a93..e18e91ff 100644 --- a/lib/pg_query/scan.rb +++ b/lib/pg_query/scan.rb @@ -1,6 +1,7 @@ module PgQuery class ScanError < ArgumentError attr_reader :location + def initialize(message, source_file, source_line, location) super("#{message} (#{source_file}:#{source_line})") @location = location diff --git a/lib/pg_query/treewalker.rb b/lib/pg_query/treewalker.rb index 4ee9d3c1..04fac46e 100644 --- a/lib/pg_query/treewalker.rb +++ b/lib/pg_query/treewalker.rb @@ -11,13 +11,9 @@ class ParserResult # multiple parser runs, assuming the same pg_query release and no modifications to the parse tree. def walk!(&block) if block.arity == 1 - treewalker!(@tree) do |node| - yield(node) - end + treewalker!(@tree, &block) else - treewalker_with_location!(@tree) do |parent_node, parent_field, node, location| - yield(parent_node, parent_field, node, location) - end + treewalker_with_location!(@tree, &block) end end From 26804d3db93554143d84e769596c4c8a68427dcb Mon Sep 17 00:00:00 2001 From: Lukas Fittl Date: Tue, 4 Feb 2025 23:45:06 -0800 Subject: [PATCH 7/9] Gemfile: Use more idiomatic $LOAD_PATH code, add MFA requirement metadata --- pg_query.gemspec | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pg_query.gemspec b/pg_query.gemspec index 1ae7b5ad..56e5fa97 100644 --- a/pg_query.gemspec +++ b/pg_query.gemspec @@ -1,4 +1,4 @@ -$LOAD_PATH.push File.expand_path('../lib', __FILE__) +$LOAD_PATH.push File.expand_path('lib', __dir__) require 'pg_query/version' Gem::Specification.new do |s| @@ -12,6 +12,8 @@ Gem::Specification.new do |s| s.license = 'BSD-3-Clause' s.homepage = 'https://github.com/pganalyze/pg_query' + s.metadata['rubygems_mfa_required'] = 'true' + s.required_ruby_version = '>= 3.0' s.extensions = %w[ext/pg_query/extconf.rb] From 428a466fcc70ebd04fafd6f10bc4bd4bd007744a Mon Sep 17 00:00:00 2001 From: Lukas Fittl Date: Tue, 4 Feb 2025 23:41:36 -0800 Subject: [PATCH 8/9] Move required development gems directly to Gemfile This is considered more idiomatic, and gets flagged by newer Rubocop versions. --- Gemfile | 6 ++++++ pg_query.gemspec | 5 ----- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/Gemfile b/Gemfile index fa75df15..144f1cf4 100644 --- a/Gemfile +++ b/Gemfile @@ -1,3 +1,9 @@ source 'https://rubygems.org' gemspec + +gem 'rake-compiler', '~> 0' +gem 'rspec', '~> 3.0' +gem 'rubocop', '0.49.1' +gem 'rubocop-rspec', '1.15.1' +gem 'simplecov', '~> 0' diff --git a/pg_query.gemspec b/pg_query.gemspec index 56e5fa97..61e4f517 100644 --- a/pg_query.gemspec +++ b/pg_query.gemspec @@ -25,10 +25,5 @@ Gem::Specification.new do |s| s.rdoc_options = %w[--main README.md --exclude ext/] s.extra_rdoc_files = %w[CHANGELOG.md README.md] - s.add_development_dependency 'rake-compiler', '~> 0' - s.add_development_dependency 'rspec', '~> 3.0' - s.add_development_dependency 'rubocop', '0.49.1' - s.add_development_dependency 'rubocop-rspec', '1.15.1' - s.add_development_dependency 'simplecov', '~> 0' s.add_dependency 'google-protobuf', '>= 3.25.3' end From a5a4fa7ccfe5dbb31727120e28bb253748396a3c Mon Sep 17 00:00:00 2001 From: Lukas Fittl Date: Tue, 4 Feb 2025 23:48:23 -0800 Subject: [PATCH 9/9] Update rubocop to 1.71, rubocop-rspec to 3.4 This also lets us drop the Ruby 3.0 requirement for rubocop on CI, since we can now use rubocop on all released Ruby versions. Further, turn off some of the newly introduced cops that seem to add unnecessary noise, and opt in to new cops by default (this avoids a warning, and seems fine since we lock the rubocop version). Also, drop inline disabling of the "Lint/EmptyWhen" lint, which is no longer needed. --- .github/workflows/ci.yml | 1 - .rubocop.yml | 51 ++++++++++++++++++++++++++++------------ Gemfile | 4 ++-- Gemfile.lock | 38 +++++++++++++++++++----------- lib/pg_query/parse.rb | 6 ++--- 5 files changed, 65 insertions(+), 35 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 103d7a4a..9edb44dc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -28,4 +28,3 @@ jobs: run: bundle exec rake test - name: RuboCop run: bundle exec rake lint - if: matrix.ruby == '3.0' diff --git a/.rubocop.yml b/.rubocop.yml index d3baba06..f4db37f1 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1,6 +1,7 @@ require: rubocop-rspec AllCops: + NewCops: enable Exclude: - ext/pg_query/extconf.rb - vendor/**/* @@ -9,29 +10,40 @@ AllCops: - lib/pg_query/json_field_names.rb - Rakefile -Layout/AlignHash: +Layout/HashAlignment: Exclude: - spec/**/* -Style/WordArray: +Layout/SpaceAroundOperators: Exclude: - spec/**/* -Style/StringLiterals: +Layout/SpaceInsideHashLiteralBraces: Exclude: - spec/**/* -Layout/SpaceAroundOperators: +Style/WordArray: Exclude: - spec/**/* -Layout/SpaceInsideHashLiteralBraces: +Style/StringLiterals: Exclude: - spec/**/* -Performance/RegexpMatch: - Exclude: - - ext/pg_query/extconf.rb +Layout/LineLength: + Enabled: false + +Layout/EmptyLineAfterGuardClause: + Enabled: false + +Style/FormatStringToken: + Enabled: false + +Style/StringConcatenation: + Enabled: false + +Style/IfUnlessModifier: + Enabled: false Style/FrozenStringLiteralComment: Enabled: false @@ -39,16 +51,18 @@ Style/FrozenStringLiteralComment: Style/SafeNavigation: Enabled: false -Documentation: +Style/Documentation: Enabled: false -LineLength: +# extend self preserves private methods, +# module_function buggily removes them. +Style/ModuleFunction: Enabled: false -MethodLength: +Metrics/MethodLength: Enabled: false -ClassLength: +Metrics/ClassLength: Enabled: false Metrics/AbcSize: @@ -75,9 +89,16 @@ RSpec/NestedGroups: RSpec/DescribedClass: Enabled: false -# extend self preserves private methods, -# module_function buggily removes them. -Style/ModuleFunction: +RSpec/ContextWording: + Enabled: false + +RSpec/MatchArray: + Enabled: false + +RSpec/NamedSubject: + Enabled: false + +RSpec/SpecFilePathFormat: Enabled: false Lint/ImplicitStringConcatenation: diff --git a/Gemfile b/Gemfile index 144f1cf4..0aaea6a8 100644 --- a/Gemfile +++ b/Gemfile @@ -4,6 +4,6 @@ gemspec gem 'rake-compiler', '~> 0' gem 'rspec', '~> 3.0' -gem 'rubocop', '0.49.1' -gem 'rubocop-rspec', '1.15.1' +gem 'rubocop', '~> 1.71' +gem 'rubocop-rspec', '~> 3.4' gem 'simplecov', '~> 0' diff --git a/Gemfile.lock b/Gemfile.lock index b92019ce..100ccfad 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -14,15 +14,18 @@ GEM google-protobuf (4.29.3) bigdecimal rake (>= 13) + json (2.9.1) + language_server-protocol (3.17.0.4) parallel (1.26.3) - parser (2.7.2.0) + parser (3.3.7.0) ast (~> 2.4.1) - powerpack (0.1.3) - rainbow (2.2.2) - rake + racc + racc (1.8.1) + rainbow (3.1.1) rake (13.2.1) rake-compiler (0.9.9) rake + regexp_parser (2.10.0) rspec (3.13.0) rspec-core (~> 3.13.0) rspec-expectations (~> 3.13.0) @@ -36,15 +39,20 @@ GEM diff-lcs (>= 1.2.0, < 2.0) rspec-support (~> 3.13.0) rspec-support (3.13.2) - rubocop (0.49.1) + rubocop (1.71.2) + json (~> 2.3) + language_server-protocol (>= 3.17.0) parallel (~> 1.10) - parser (>= 2.3.3.1, < 3.0) - powerpack (~> 0.1) - rainbow (>= 1.99.1, < 3.0) + parser (>= 3.3.0.2) + rainbow (>= 2.2.2, < 4.0) + regexp_parser (>= 2.9.3, < 3.0) + rubocop-ast (>= 1.38.0, < 2.0) ruby-progressbar (~> 1.7) - unicode-display_width (~> 1.0, >= 1.0.1) - rubocop-rspec (1.15.1) - rubocop (>= 0.42.0) + unicode-display_width (>= 2.4.0, < 4.0) + rubocop-ast (1.38.0) + parser (>= 3.3.1.0) + rubocop-rspec (3.4.0) + rubocop (~> 1.61) ruby-progressbar (1.13.0) simplecov (0.22.0) docile (~> 1.1) @@ -52,7 +60,9 @@ GEM simplecov_json_formatter (~> 0.1) simplecov-html (0.13.1) simplecov_json_formatter (0.1.4) - unicode-display_width (1.8.0) + unicode-display_width (3.1.4) + unicode-emoji (~> 4.0, >= 4.0.4) + unicode-emoji (4.0.4) PLATFORMS ruby @@ -61,8 +71,8 @@ DEPENDENCIES pg_query! rake-compiler (~> 0) rspec (~> 3.0) - rubocop (= 0.49.1) - rubocop-rspec (= 1.15.1) + rubocop (~> 1.71) + rubocop-rspec (~> 3.4) simplecov (~> 0) BUNDLED WITH diff --git a/lib/pg_query/parse.rb b/lib/pg_query/parse.rb index 35ffdd8a..1549b0c6 100644 --- a/lib/pg_query/parse.rb +++ b/lib/pg_query/parse.rb @@ -174,7 +174,7 @@ def load_objects! # rubocop:disable Metrics/CyclomaticComplexity # The following statement types are DDL (changing table structure) when :alter_table_stmt case statement.alter_table_stmt.objtype - when :OBJECT_INDEX # Index # rubocop:disable Lint/EmptyWhen + when :OBJECT_INDEX # Index # ignore `ALTER INDEX index_name` else from_clause_items << { item: PgQuery::Node.new(range_var: statement.alter_table_stmt.relation), type: :ddl } @@ -228,11 +228,11 @@ def load_objects! # rubocop:disable Metrics/CyclomaticComplexity when :grant_stmt objects = statement.grant_stmt.objects case statement.grant_stmt.objtype - when :OBJECT_COLUMN # Column # rubocop:disable Lint/EmptyWhen + when :OBJECT_COLUMN # Column # FIXME when :OBJECT_TABLE # Table from_clause_items += objects.map { |o| { item: o, type: :ddl } } - when :OBJECT_SEQUENCE # Sequence # rubocop:disable Lint/EmptyWhen + when :OBJECT_SEQUENCE # Sequence # FIXME end when :lock_stmt