Refactor DML actions handling #73
Conversation
| tableName := stateChange.Value.(string) | ||
|
|
||
| tab := home.TabbedPane.GetTabByName(tableName) | ||
| tabReference := fmt.Sprintf("%s.%s", databaseName, tableName) |
There was a problem hiding this comment.
Can't you use .*ResultsTable. GetDatabaseAndTableName()
If not, it means maybe you could add a method to format db string, table string to format, then it would be called here and in GetDatabaseAndTableName
| return nil | ||
| } | ||
|
|
||
| if command == commands.Edit { |
There was a problem hiding this comment.
You could use a switch here
Either to create two switch with
switch command {
case commands.Edit:
case commands....
case commands....
default:
}
```go
switch eventKey {
case 4:
case 21:
}The second could be in the default of the first one if these switch have to be exclusive.
Or you could use the extended switch syntax
switch {
case command == commands.Edit:
case command == commands.…
case eventKey = 4:
case eventKey = 21:
default:
}| query := "DELETE FROM " | ||
| query += db.formatTableName(database, table) | ||
| query += fmt.Sprintf(" WHERE %s = ?", primaryKeyColumnName) | ||
| _, err := db.Connection.Exec(query, primaryKeyValue) |
There was a problem hiding this comment.
| query := "DELETE FROM " | |
| query += db.formatTableName(database, table) | |
| query += fmt.Sprintf(" WHERE %s = ?", primaryKeyColumnName) | |
| _, err := db.Connection.Exec(query, primaryKeyValue) | |
| query := fmt.Sprintf("DELETE FROM %s WHERE %s = ?", db.formatTableName(database, table), primaryKeyColumnName) | |
| _, err := db.Connection.Exec(query, primaryKeyValue) |
There was a problem hiding this comment.
The thing is gosec G201 rule, you think it should be disabled?
There was a problem hiding this comment.
i think you fixed it the right way.
https://securego.io/docs/rules/g201-g202
I'm unsure if there is "clean way" when you are sprintf to set the table name
You may look at ORM how they do.
But now you understood how to write clean SQL queries with ? and arguments, maybe you could add a //nolint:gosec // disable it because of usage of table
There was a problem hiding this comment.
Using a buffer is another way to contact string, it doesn't help at all with SQL injection.
You can use the buffer of course, as it's fast.
I think that's why they are using it
There was a problem hiding this comment.
By the time i'm more concerned about readability than SQL Injection, i think it does not makes sense (unless i'm missing something, if i do, please advice) to think that's a problem on a sql client, you will not self inject sql.
There was a problem hiding this comment.
You fixed the biggest cause of bug by using ? caused by injection.
For example, when updating the content of a field that contains quote in a JSON.
Such as `{"foo":true}. With previous code, the update would be KO, as it would not escape the quotes.
It could cause data corruption and bug
There was a problem hiding this comment.
You are correct, using parameters we avoid those things.
Thank you, sir. :)
drivers/mysql.go
Outdated
| queryStr := "INSERT INTO " | ||
| queryStr += db.formatTableName(change.Database, change.Table) | ||
| queryStr += fmt.Sprintf(" (%s) VALUES (%s)", strings.Join(columns, ", "), strings.Join(valuesPlaceholder, ", ")) |
There was a problem hiding this comment.
You could/should a sprintf here to make things easier to read
See #73 (comment)
|
any progress? do you need help? |
|
I just lost my main job, i'm busy right now preparing for interviews and those things. When I have more stability I will come back and get this done. Thanks for asking. |
|
Wow, take care of you |
jorgerojas26
force-pushed
the
refactor-dml
branch
from
17ab68e
to
04acbd2
Compare
goodluck bro |
jorgerojas26
changed the title
The main goal of this refactor is to improve code quality (a bit, i'm not super fluent in go) and allow for EMPTY, NULL and DEFAULT values to be used in the DML statements.
Please feel free to make suggestions.