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

Always quote schema name with backticks in MySQL queries. #777

Merged
merged 1 commit into from
Dec 4, 2022

Conversation

Choc13
Copy link
Contributor

@Choc13 Choc13 commented Dec 3, 2022

Proposed Changes

Adds backtick quotes around all schema names in MySQL queries. Addresses #776.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • Build and tests pass locally (Build passes, but I wasn't able to get all the tests to run locally yet. Will keep trying.)
  • I have added tests that prove my fix is effective or that my feature works (if appropriate)
  • I have added necessary documentation (if appropriate)

@Thorium
Copy link
Member

Thorium commented Dec 4, 2022

Thanks, this is definitely improvement.

Do you think the replace is too naive? (I know it was there already.) Like what if your table/column contains a weird name like `Person[backup]` ?

@Choc13
Copy link
Contributor Author

Choc13 commented Dec 4, 2022

That is a good point. I wondered the same but didn't change it as I saw a comment about [] being used as an internal representation so figured the replacement was there in order to switch to the MySQL specific quotations. However I think you are right that if the table/schéma did also contain those chars then they would be incorrectly substituted.

It seems like a separate change set though to remodel the internal quoting behaviour to address this. I would have thought it should be sufficient to not use any quote symbol internally and just do vendor specific quoting in each provider, but there are likely reasons I'm not aware of given I've only spent an hour or so looking over this code.

@Thorium
Copy link
Member

Thorium commented Dec 4, 2022

You are correct, thanks, will release this.

@Thorium Thorium merged commit 13774f8 into fsprojects:master Dec 4, 2022
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.

None yet

2 participants