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

Refactor to avoid using SELECT * #107

Merged
merged 1 commit into from
Dec 11, 2022
Merged

Refactor to avoid using SELECT * #107

merged 1 commit into from
Dec 11, 2022

Conversation

JonathanPlasse
Copy link
Contributor

@JonathanPlasse JonathanPlasse commented Aug 30, 2022 •
edited
Loading

Close #105

mariocynicys
mariocynicys requested changes Aug 30, 2022
View reviewed changes
Copy link
Collaborator

@mariocynicys mariocynicys left a comment

Choose a reason for hiding this comment

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

Thanks :D
You seem to have missed a couple of instances (like this one).
ps. run git grep -n "SELECT.*\*" to get them all.

@JonathanPlasse
Copy link
Contributor Author

It is fixed.

mariocynicys
mariocynicys requested changes Aug 30, 2022
View reviewed changes
teos/src/dbm.rs Outdated Show resolved Hide resolved
teos/src/dbm.rs Outdated Show resolved Hide resolved
watchtower-plugin/src/dbm.rs Outdated Show resolved Hide resolved
watchtower-plugin/src/dbm.rs Outdated Show resolved Hide resolved
@JonathanPlasse
Copy link
Contributor Author

It is ready

sr-gi
sr-gi requested changes Sep 4, 2022
View reviewed changes
Copy link
Member

@sr-gi sr-gi left a comment

Choose a reason for hiding this comment

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

Some minimal changes.

There are two additional instances of SELECT * in the codebase, but they are used to check if a row exists, so I'm guessing they are OK.

Needs rebasing.

teos/src/dbm.rs Outdated
@@ -306,7 +316,9 @@ impl DBM {
) -> HashMap<UUID, ExtendedAppointment> {
let mut appointments = HashMap::new();

let mut sql = "SELECT * FROM appointments as a LEFT JOIN trackers as t ON a.UUID=t.UUID WHERE t.UUID IS NULL".to_string();
let mut sql =
"SELECT a.UUID, locator, encrypted_blob, to_self_delay, user_signature, start_block, user_id
Copy link
Member

Choose a reason for hiding this comment

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

Prepend all fields with a. for consistency.

e.g:

a.UUID, a.locator, ...

teos/src/dbm.rs Outdated
"SELECT t.*, a.user_id FROM trackers as t INNER JOIN appointments as a ON t.UUID=a.UUID WHERE t.UUID=(?)").unwrap();
let mut stmt = self
.connection.prepare(
"SELECT dispute_tx, penalty_tx, height, confirmed, a.user_id
Copy link
Member

Choose a reason for hiding this comment

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

Same here with t.:

t.dispute_tx, t.penalty_tx, ...

teos/src/dbm.rs Outdated
@@ -478,7 +495,9 @@ impl DBM {
) -> HashMap<UUID, TransactionTracker> {
let mut trackers = HashMap::new();

let mut sql = "SELECT t.*, a.user_id FROM trackers as t INNER JOIN appointments as a ON t.UUID=a.UUID".to_string();
let mut sql = "SELECT t.UUID, dispute_tx, penalty_tx, height, confirmed, a.user_id
Copy link
Member

Choose a reason for hiding this comment

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

Same as above: t....

watchtower-plugin/src/dbm.rs Outdated
@@ -533,7 +533,7 @@ impl DBM {
let mut appointments = Vec::new();
let mut stmt = self
.connection
.prepare(&format!("SELECT * FROM appointments as a, {} as t WHERE a.locator = t.locator AND t.tower_id = ?", table))
.prepare(&format!("SELECT a.locator, encrypted_blob, to_self_delay FROM appointments as a, {} as t WHERE a.locator = t.locator AND t.tower_id = ?", table))
Copy link
Member

Choose a reason for hiding this comment

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

And same with a....

watchtower-plugin/src/dbm.rs Outdated
@@ -533,7 +533,7 @@ impl DBM {
let mut appointments = Vec::new();
let mut stmt = self
.connection
.prepare(&format!("SELECT * FROM appointments as a, {} as t WHERE a.locator = t.locator AND t.tower_id = ?", table))
.prepare(&format!("SELECT a.locator, encrypted_blob, to_self_delay FROM appointments as a, {} as t WHERE a.locator = t.locator AND t.tower_id = ?", table))
Copy link
Member

Choose a reason for hiding this comment

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

And same with a....

sr-gi
sr-gi requested changes Sep 4, 2022
View reviewed changes
Copy link
Member

@sr-gi sr-gi left a comment

Choose a reason for hiding this comment

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

Some minimal changes.

There are two additional instances of SELECT * in the codebase, but they are used to check if a row exists, so I'm guessing they are OK.

Needs rebasing.

@JonathanPlasse
Copy link
Contributor Author

Applied changes and rebased.

@sr-gi
Copy link
Member

sr-gi commented Dec 9, 2022

@JonathanPlasse Same with this one, sorry I missed it. Maybe fix #110 first and once that's merged rebase this so we can merge it too.

@sr-gi
Copy link
Member

sr-gi commented Dec 10, 2022

Needs rebase

@JonathanPlasse
Copy link
Contributor Author

Rebased.

@sr-gi sr-gi merged commit cfca86f into talaia-labs:master Dec 11, 2022
@JonathanPlasse JonathanPlasse deleted the refactor-select-star branch December 11, 2022 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sr-gi sr-gi sr-gi approved these changes

@mariocynicys mariocynicys mariocynicys approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Avoid using "SELECT *" in the codebase
3 participants
@JonathanPlasse @sr-gi @mariocynicys