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

fix: escape single quote with backslash #95

Merged

Conversation

evtilley
Copy link

@evtilley evtilley commented Jun 9, 2021

Currently, this extension uses the Liquibase default behavior of escaping single quotes with ''. This change overrides that behavior to use \' instead.

Fixes #94.

@google-cla google-cla bot added the cla: yes CLA signed label Jun 9, 2021
@mescanne
Copy link
Collaborator

Evtilley - thanks for this. It looks like the right thing to do, replacing the default JDBC escape method which works differently.

It would be good to add a test for it. I was working with it and our loadData changeset uses usePreparedStatements: true and it handles ' characters just fine. Could you try your test with usePreparedStatements: true?

We should still fix this, but I'd like a test that shows it working. We can duplicate the existing loadData test or create a simpler one based on your test case.

@evtilley
Copy link
Author

Thanks @mescanne . Using prepared statements will avoid this issue.

I took a shot at writing a unit test, and it turns out that this behavior can't be reproduced using liquibase-core 4.3.1, which this project currently uses. It can be reproduced in 4.3.2, which changes the logic that decides when to use prepared statements (see this commit).

Should I take on the dependency upgrade in this PR?

@mescanne
Copy link
Collaborator

Go for it. It isn't clear to me the intent behind that commit -- regardless, this is focused on the string escape logic, so if we can get a minimal test for the escape string we'll be good. We can upgrade to 4.3.2 if it helps reproduce it.

@mescanne
Copy link
Collaborator

Great job! Do you need a release for this? Or just use prepared statements for now, and this will roll into the next release.

@mescanne mescanne merged commit 700db30 into cloudspannerecosystem:master Jun 11, 2021
@evtilley
Copy link
Author

Thanks @mescanne! Yes, please create a release.

@evtilley evtilley deleted the fix-single-quote-escaping branch June 11, 2021 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes CLA signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

loadData not escaping single quotes correctly
2 participants