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

SQLite: add additional quote to table name only when it’s not quoted #49544

Merged
merged 1 commit into from Oct 10, 2023

Conversation

hieuk09
Copy link
Contributor

@hieuk09 hieuk09 commented Oct 8, 2023

Motivation / Background

In Rails 7.1, when using quoted table name in exec_insert for SQLite, the table name is quoted again, which raises error that the table does not exist. This breaks downstream gem like activerecord-import. This PR aims to fix the issue by avoid quoting when the table name is already quoted.

Detail

I added a test in activerecord/test/cases/adapters/sqlite3/sqlite3_adapter_test.rb to reproduce the issue. When running the below code without the fix, it will raise the below error

  • Code:
@conn.exec_insert("insert into \"ex\" (number) VALUES (?)", "SQL", vals)
  • Output
ActiveRecord::ConnectionAdapters::SQLite3AdapterTest#test_exec_insert_with_quote:
ActiveRecord::StatementInvalid: Could not find table '"ex"'
    /workspaces/rails/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb:460:in `table_structure'
    /workspaces/rails/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb:270:in `primary_keys'
    /workspaces/rails/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb:146:in `primary_key'
    /workspaces/rails/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:641:in `sql_for_insert'
    /workspaces/rails/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:152:in `exec_insert'
    /workspaces/rails/activerecord/test/cases/adapters/sqlite3/sqlite3_adapter_test.rb:102:in `block in test_exec_insert_with_quote'
    /workspaces/rails/activerecord/test/support/ddl_helper.rb:6:in `with_example_table'
    /workspaces/rails/activerecord/test/cases/adapters/sqlite3/sqlite3_adapter_test.rb:841:in `with_example_table'
    /workspaces/rails/activerecord/test/cases/adapters/sqlite3/sqlite3_adapter_test.rb:100:in `test_exec_insert_with_quote'

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@skipkayhil
Copy link
Member

What do you think about trying to improve extract_table_ref_from_insert_sql to not include the quotes? That seems like a more appropriate place to make this fix

@hieuk09
Copy link
Contributor Author

hieuk09 commented Oct 8, 2023

That may not work because table name can contain space, which will require the quote.

@byroot
Copy link
Member

byroot commented Oct 9, 2023

@hieuk09 Hartley is talking of the extraction method, not the quoting one. I think it's fair for the extraction method to canonicalize the table name, hence removing the quotes.

@hieuk09 hieuk09 force-pushed the bug/fix-sqlite3-table-name-quote branch from ae3b4a5 to ab1c6ec Compare October 10, 2023 00:24
@hieuk09 hieuk09 force-pushed the bug/fix-sqlite3-table-name-quote branch from ab1c6ec to 6da327c Compare October 10, 2023 00:28
@hieuk09
Copy link
Contributor Author

hieuk09 commented Oct 10, 2023

@byroot thanks for the clarification. I forgot that the table name will always be quoted when building the query, so having space in the name is no problem.

I updated the PR to remove quote in the table name, let me know if it works for you.

@byroot byroot merged commit ce2eb72 into rails:main Oct 10, 2023
4 checks passed
byroot added a commit that referenced this pull request Oct 10, 2023
SQLite: add additional quote to table name only when it’s not quoted
@byroot
Copy link
Member

byroot commented Oct 10, 2023

Thanks, backported to 7-1-stable.

@hieuk09 hieuk09 deleted the bug/fix-sqlite3-table-name-quote branch October 10, 2023 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants