Skip to content
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.

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

Quote mysql 8.0 reserved keyword rank in database dumper #4522

Merged
merged 1 commit into from Aug 25, 2019

Conversation

@tussosedan
Copy link
Contributor

commented Aug 23, 2019

Similarly to #4163. With this change all tests pass on mysql 8.0!
However, I suspect we may encounter additional issues with the "rank" keyword that are not covered by tests.

@jfly

This comment has been minimized.

Copy link
Member

commented Aug 24, 2019

Would a better fix be to quote the column name here? https://github.com/jfly/worldcubeassociation.org/blob/9e54ed58941020af47b8b0bad6112283b7931ef5/WcaOnRails/lib/database_dumper.rb#L745. I'm thinking something like this:

diff --git a/WcaOnRails/lib/database_dumper.rb b/WcaOnRails/lib/database_dumper.rb
index 1af1aa0d..a9268500 100644
--- a/WcaOnRails/lib/database_dumper.rb
+++ b/WcaOnRails/lib/database_dumper.rb
@@ -742,7 +742,7 @@ module DatabaseDumper
         end
 
         column_expressions = column_sanitizers.map do |column_name, column_sanitizer|
-          column_sanitizer == :copy ? "#{table_name}.#{column_name}" : "#{column_sanitizer} as #{column_name}"
+          column_sanitizer == :copy ? "#{table_name}.`#{column_name}`" : "#{column_sanitizer} as `#{column_name}`"
         end.join(", ")
 
         populate_table_sql = "INSERT INTO #{dump_db_name}.#{table_name} (#{column_sanitizers.keys.join(", ")}) SELECT #{column_expressions} FROM #{table_name} #{table_sanitizer[:where_clause]}"
@tussosedan

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2019

Hmm so just quote all the column names? Sure, possible. And then we'd have to quote all the column names in the tests too.

Please feel free to make the change, or let me know if you'd like me to do it.

@jfly

This comment has been minimized.

Copy link
Member

commented Aug 24, 2019

Go for it! Based on my understanding of the test, I don't think you'd have to quote them in the test.

@tussosedan

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2019

It's a bit more complicated than that, as column_sanitizers.keys.join(", ") is used for the SQL INSERT column list in https://github.com/jfly/worldcubeassociation.org/blob/9e54ed58941020af47b8b0bad6112283b7931ef5/WcaOnRails/lib/database_dumper.rb#L748

We could change actions_to_column_sanitizers to use something like column_sanitizers['`' + column + '`'] = action, but then we do need to change the tests as it changes the columns they expect.

Or we could add another variable to hold the column names list, and use it just for the INSERT above, something like this:
quoted_column_list = column_sanitizers.keys.map { |column_name| '`' + column_name + '`' }.join(", ")
This is probably cleaner. What do you think?

EDIT: this also made me realize that the SELECT itself is not an issue, as the "rank" there is prefixed with the table name, and does not raise an error.

@jfly

This comment has been minimized.

Copy link
Member

commented Aug 25, 2019

Both of those sound like good options to me. Please go ahead with whichever you prefer =)

(BTW, I found that there's a ActiveRecord::Base.connection.quote_column_name we can use instead of actually appending the backticks ourselves.)

@tussosedan tussosedan force-pushed the tussosedan:fix-reserved-keyword-in-dumper branch from af22a60 to da38af2 Aug 25, 2019
Copy link
Member

left a comment

LGTM. Just a couple of nits.

WcaOnRails/lib/database_dumper.rb Outdated Show resolved Hide resolved
WcaOnRails/lib/database_dumper.rb Outdated Show resolved Hide resolved
@tussosedan tussosedan force-pushed the tussosedan:fix-reserved-keyword-in-dumper branch from da38af2 to 95c8cfd Aug 25, 2019
@jfly jfly merged commit 82bcf1d into thewca:master Aug 25, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.0002%) to 96.115%
Details
@tussosedan tussosedan deleted the tussosedan:fix-reserved-keyword-in-dumper branch Aug 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.