-
Notifications
You must be signed in to change notification settings - Fork 466
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
Dev guide v2 Delete Data updates #9998
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor java comment
v21.1/delete-data.md
Outdated
You can delete multiple rows from a table in several ways: | ||
{% include copy-clipboard.html %} | ||
~~~ go | ||
// tx is a *sql.Tx from "database/sql" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could also use a *sql.DB
here, which matches the other examples that don't use an explicit transaction
v21.1/delete-data.md
Outdated
- Using [`TRUNCATE`](truncate.html) instead of [`DELETE`](delete.html) to delete all of the rows from a table, as recommended in our [performance best practices](performance-best-practices-overview.html#use-truncate-instead-of-delete-to-delete-all-rows-in-a-table). | ||
try (Connection connection = ds.getConnection()) { | ||
connection.createStatement().executeUpdate( | ||
"DELETE from promo_codes WHERE code IN('" + codeOne + "','" + codeTwo + "','" + codeThree + "')"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's risky to put in raw strings like this into SQL statements, since it allows for SQL injection. of course, it doesn't matter for this toy example, but we might as well use a best practice here so the reader doesn't copy+paste something dangerous.
PreparedStatement p = connection.prepareStatement("DELETE from promo_codes WHERE code IN(?, ?, ?)");
p.setString(1, codeOne);
p.setString(2, codeTwo);
p.setString(3, codeThree);
p.executeUpdate();
(it also seems unrealistic to write real JDBC code that way, since a framework on top would take care of this, but at least this way is "safe")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is my personal preference, and also would be a big project, but i wonder if we could think about using JDBI for stuff like this -- where we want to run raw SQL with Java. it's a lightweight library that helps with query building and stuff. wouldn't say this is a big priority though.
it would allow:
handle.execute("DELETE from promo_codes WHERE code IN(?, ?, ?)", codeOne, codeTwo, codeThree);
b010a23
to
471a80c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TFTR @rafiss !
Updated the code samples with your feedback.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)
v21.1/delete-data.md, line 166 at r3 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
we could also use a
*sql.DB
here, which matches the other examples that don't use an explicit transaction
Done.
v21.1/delete-data.md, line 192 at r3 (raw file):
it's risky to put in raw strings like this into SQL statements, since it allows for SQL injection.
That makes sense (the python and golang examples do this correctly for this reason).
I've updated the Java samples.
it also seems unrealistic to write real JDBC code that way, since a framework on top would take care of this, but at least this way is "safe"
Yeah.... it makes me wonder how useful even providing these low-level driver examples actually is.
i wonder if we could think about using JDBI for stuff like this
That seems like a bigger project. But just skimming their docs and looking at the sample you gave makes me think it could be worth investing some time into, just to make these kinds of simple examples a little more succinct. I've made an issue: #10014
Fixes #9864