Skip to content

Issue #15 - Kept multiple whitespace in embedded params using gsub only on query template.#17

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sufleR merged 1 commit into from
Feb 4, 2023
Merged

Issue #15 - Kept multiple whitespace in embedded params using gsub only on query template. #17

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .travis.yml
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -14,7 +14,7 @@ gemfile:
- gemfiles/5.0.gemfile

rvm:
- '2.3.3'
- '2.6.0'

addons:
postgresql: '9.4'
Expand Down
10 changes: 8 additions & 2 deletions lib/sql_query.rb
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -34,7 +34,7 @@ def exec_query(prepare = true)
end

def sql
@sql ||= ERB.new(File.read(file_path)).result(binding)
@sql ||= prepare_query(false)
end

def pretty_sql
Expand All@@ -46,7 +46,7 @@ def quote(value)
end

def prepared_for_logs
sql.gsub(/(\n|\s)+/, ' ')
@sql_for_logs ||= prepare_query(true)

@sullerandrassullerandrasJul 2, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this is a breaking change! Since this removes the newlines before the erb binding, it removes the newlines from the erb snippets, which can break erb code like this for example:

<%
variable1 = 1
variable2 = 2
%>
select 1

It will fail with something like:

expected no Exception, got #<SyntaxError: (erb):1: syntax error, unexpected local variable or method, expecting end-of-input
......riable1 = 1 variable2 = 2...

To fix it we only have to add a semicolon, but still it's unexpected that a bugfix version breaks stuff.

The new code could also read the file twice (and execute erb bindings twice which may cause side effects!), while the old code only did it once.

Imo the only problem was that the execute method calls prepared_for_logs to generate the sql command (though you can disable it by calling query.execute(false)).
The name prepared_for_logs implies that it's meant to be used for logging the query, so i'm not sure why is that used to generate the executed sql command.
Imo the default should be to execute the raw query. I'm not even sure why there is an option to execute an altered query with removed new lines? What's the use case for that?

@Barbez95Barbez95Jul 3, 2024

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sullerandras I've described the problem into issue #15 where a embedded variable that contains a string with multiple consecutive whitespaces was wrongly changed.
I've suggest a fix with this PR to avoid the buggy behaviour.

I've checked your use case and I agree with you this is a breaking change that should be reported into changelog.

end

def partial(partial_name, partial_options = {})
Expand DownExpand Up@@ -75,6 +75,12 @@ def initialize

private

def prepare_query(for_logs)
query_template = File.read(file_path)
query_template = query_template.gsub(/(\n|\s)+/, ' ') if for_logs
ERB.new(query_template).result(binding)
end

def split_to_path_and_name(file)
if file.is_a?(Symbol)
['', file.to_s]
Expand Down
10 changes: 10 additions & 0 deletions spec/sql_query_spec.rb
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -201,6 +201,16 @@ class Model < ActiveRecord::Base
expect(query.prepared_for_logs)
.to eq("SELECT * FROM players WHERE email = 'e@mail.dev' ")
end

context 'when embedded params have multiple whitespaces' do
let(:options) { { email: ' e@mail.dev ' } }
let(:query) { described_class.new(file_name, options) }

it 'returns string without multiple whitespaces except embedded params' do
expect(query.prepared_for_logs)
.to eq("SELECT * FROM players WHERE email = ' e@mail.dev ' ")
end
end
end

describe '.config' do
Expand Down