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.

Sign up for GitHub

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

Jump to bottom

Call sp_rename as prepared statement #14272

Merged
merged 1 commit into from
Oct 10, 2022
Merged

Call sp_rename as prepared statement #14272

merged 1 commit into from
Oct 10, 2022

Conversation

vlad-lyutenko
Copy link
Contributor

@vlad-lyutenko vlad-lyutenko commented Sep 23, 2022 •
edited by hashhar
Loading

Description

This change made call to sp_rename for renaming columns as prepared statement,
to be safe with sql injections.

Plus it support different tricky column names during renaming,
inspired by this discussion:
#13878 (comment)

So now we can do things like this from trino:
alter table test2 rename column "[crazy; drop table nation]" to "[crazy; drop ""table nation]";
alter table test2 rename column "[crazy; drop ""table nation]" to "[crazy; drop []table nation]";
alter table test2 rename column [column4] to """[crazy;"" "" drop []table nation]";
select """[crazy;"" "" drop []table nation]" from test2;

Non-technical explanation

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text:

# SQL Server
* Allow renaming column names containing special characters. ({issue}`14272`)

@cla-bot cla-bot bot added the cla-signed label Sep 23, 2022
hashhar
hashhar reviewed Sep 23, 2022
View reviewed changes
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skimmed.

plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java Outdated Show resolved Hide resolved
@vlad-lyutenko
Copy link
Contributor Author

vlad-lyutenko commented Sep 26, 2022 •
edited
Loading

I checked situation with double double quote it works like this:
create table "[te"""" st3]" ("column1" int, "column2" int);
show tables;
Table
------------
[te"" st3]

alter table dbo."[te"""" st3]" rename column "column1" to "c";
RENAME COLUMN

Same works for column names

@vlad-lyutenko vlad-lyutenko mentioned this pull request Sep 30, 2022
Merged
@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/sp-rename-to-statement branch from 0b23c3f to 0979aab Compare October 4, 2022 09:53
@vlad-lyutenko
Copy link
Contributor Author

@findepi @hashhar @ssheikin Some tests added and comments addressed, please take a look

@hashhar
Copy link
Member

hashhar commented Oct 4, 2022

@vlad-lyutenko since we've diverged a lot from the initial implementation I think squashing the fixups would be helpful for the review. (and there's been a force push anyway).

hashhar
hashhar reviewed Oct 4, 2022
View reviewed changes
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good % comments.

Didn't review commit by commit yet.

plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java Outdated Show resolved Hide resolved
plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java Outdated Show resolved Hide resolved
plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java Outdated Show resolved Hide resolved
@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/sp-rename-to-statement branch from a39765c to 5501a0f Compare October 4, 2022 13:10
@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/sp-rename-to-statement branch 2 times, most recently from f0fbb3c to c5200a3 Compare October 7, 2022 09:11
@vlad-lyutenko
Copy link
Contributor Author

@findepi @hashhar rebased after merging #14458

@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/sp-rename-to-statement branch from c5200a3 to eb34eff Compare October 7, 2022 09:16
findepi
findepi reviewed Oct 7, 2022
View reviewed changes
plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java Outdated Show resolved Hide resolved
plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java Outdated Show resolved Hide resolved
plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerConnectorTest.java Outdated Show resolved Hide resolved
plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerConnectorTest.java Show resolved Hide resolved
plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerConnectorTest.java Show resolved Hide resolved
plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerConnectorTest.java Outdated Show resolved Hide resolved
plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerConnectorTest.java Outdated Show resolved Hide resolved
@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/sp-rename-to-statement branch from eb34eff to 015f661 Compare October 7, 2022 11:08
ssheikin
ssheikin reviewed Oct 9, 2022
View reviewed changes
plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java
Comment on lines +314 to +317
renameTable.setString(1, fullTableFromName);
renameTable.setString(2, newRemoteTableName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fullTableFromName -> oldTableName

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oldTableFullName (it's not just the table name)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually I didn't like full I've spent some time to understand that it's full name of the table which has to be renamed.
oldTableFullName - table is not old. Full name is old.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tableOldFullName?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest I don't think there are any good names here - I'll just go with what exists already. 😄 We can rename when we come up with better name.

@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/sp-rename-to-statement branch from 015f661 to eba1262 Compare October 10, 2022 09:12
@hashhar hashhar merged commit 4674cc5 into trinodb:master Oct 10, 2022
@hashhar hashhar mentioned this pull request Oct 10, 2022
Closed
@github-actions github-actions bot added this to the 400 milestone Oct 10, 2022
@colebow colebow mentioned this pull request Oct 11, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tangjiangling tangjiangling tangjiangling left review comments

@hashhar hashhar hashhar approved these changes

@ssheikin ssheikin ssheikin left review comments

@findepi findepi findepi left review comments

@wendigo wendigo Awaiting requested review from wendigo

Assignees
No one assigned
Labels
cla-signed
Milestone
400
Development

Successfully merging this pull request may close these issues.
6 participants
@vlad-lyutenko @hashhar @wendigo @findepi @ssheikin @tangjiangling