Skip to content

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

Merged
sufleR merged 1 commit intosufleR:masterfrom
Barbez95:master
Feb 4, 2023
Merged

Issue #15 - Kept multiple whitespace in embedded params using gsub only on query template.#17
sufleR merged 1 commit intosufleR:masterfrom
Barbez95:master

Conversation

@Barbez95
Copy link

Updated travis.yml to Ruby 2.6.0 to avoid bundler install error.

…sub only on query template. Updated travis.yml to Ruby 2.6.0 to avoid bundler install error.
@Barbez95
Copy link
Author

Barbez95 commented Jan 21, 2023

Hi @sufleR ! I'm totally new to pull-requests on Github and Travis CI, so I hope I haven't done some silly mistakes!

  • About the fix: in order to keep a similar logic to current version, I've isolated the read logic of query template file and applied gsub only on the template content (before params get embedded).

  • About specs: in order to test them into Travis CI, I had to update Ruby version due to a bundler install error (it requires at least Ruby 2.6.0). I've added a simple spec which includes the case of a embedded params with multiple whitespaces.

Let me know what do you think about the changes!
Thanks

@sufleR sufleR merged commit f272a88 into sufleR:master Feb 4, 2023

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

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?

Copy link
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants