Need Code Review #9
Conversation
| ## How to test: | ||
|
|
||
| 1. Change into directory of server_test.go | ||
| 2. In the terminal, type "go test" |
There was a problem hiding this comment.
I tried running this and it prints error to the console several times, but than it passes which confused me at first. I thought it didn't work, probably because I don't have the same mysql setup as you. Maybe don't print error? or maybe it is passing but shouldn't be
% go test
Unable to make connection
Error from Database [email protected] is not in the database
Unable to make connection
Error from Database [email protected] is not in the database
PASS
ok github.com/blics18/SendGrid/server 0.117s
Also I would consider writing and committing a createTables.sql file that generates the schema.
|
|
||
| func createTable(numTable int, db *sql.DB) error { | ||
| stmt := fmt.Sprintf(`CREATE TABLE IF NOT EXISTS User%02d ( | ||
| id int NOT NULL AUTO_INCREMENT, |
There was a problem hiding this comment.
instead of creating the tables inside your service, you might want to take advantage of docker which you could configure to spin up a mysql instance and then populate it every time... e.g. https://github.com/jimmyjames85/bpmonitor
|
|
||
| p := MakeRandomUsers(numUsers, numEmails) | ||
|
|
||
| db, err := sql.Open("mysql", "root:SendGrid@tcp(localhost:3306)/UserStructs") |
There was a problem hiding this comment.
Instead of hardcoding the username and password, consider passing them in as an environment variable that could retrieve via os.Getenv("DB_HOST") or often we use https://github.com/kelseyhightower/envconfig
| } | ||
|
|
||
| func crossCheck(UserID *int, Email string) bool { | ||
| db, err := sql.Open("mysql", "root:SendGrid@tcp(localhost:3306)/UserStructs") |
There was a problem hiding this comment.
Here db is a connection pool. To gain the benefits of having a pool it is best to keep this open all the time...for the lifetime of the app. You could than access it as part of the server. e.g.
func (bf *boomFilter) crossCheck(UserID *int, Email string) bool {
rows, err := bf.db.Query(stmt)
.....
}
| db, err := sql.Open("mysql", "root:SendGrid@tcp(localhost:3306)/UserStructs") | ||
| if err != nil { | ||
| fmt.Println("Failed to get handle") | ||
| db.Close() |
| err = db.Ping() | ||
| if err != nil { | ||
| fmt.Println("Unable to make connection") | ||
| db.Close() |
There was a problem hiding this comment.
return from the function and bubble up the error
| } | ||
| } | ||
|
|
||
| func crossCheck(UserID *int, Email string) bool { |
There was a problem hiding this comment.
What if UserID is nil. We should check for that...
There was a problem hiding this comment.
Would we have to check for this still? Because in check before UserID gets passed to crossCheck, we already checked if it is nil
|
|
||
| const numTables int = 5 | ||
|
|
||
| stmt := fmt.Sprintf("SELECT uid, email FROM User%02d WHERE uid=%d AND email='%s'", (*UserID)%numTables, *UserID, Email) |
There was a problem hiding this comment.
This can get dangerous, trying interpolate parameters with Sprintf. To prevent sql injection you could do something like this
stmt := fmt.Sprintf("SELECT uid, email FROM User02 WHERE uid=? AND email=?")
rows, err := db.Query(stmt, *UserID, Email)
Now to dynamically determine the table name User02 based on the userid, I think you will have to use Sprintf. However you have control of the UserID and numTables which are both int. You do not have control over email that may include a single quote ' which can ruin entire query.
https://www.calhoun.io/what-is-sql-injection-and-how-do-i-avoid-it-in-go/
| db.Close() | ||
| } | ||
|
|
||
| const numTables int = 5 |
There was a problem hiding this comment.
Would be nice if this was configurable instead of hard coded...
| } | ||
|
|
||
| func main() { | ||
| port := ":8082" |
There was a problem hiding this comment.
Would be nice if this were configurable instead of hardcoded.
| filter *bloom.BloomFilter | ||
| } | ||
|
|
||
| func createBloomFilter() *bloomFilter { |
There was a problem hiding this comment.
typically this would be an exported function and instead of create it is idiomatic to use new
type BloomFilter struct {
filter *bloom.BloomFilter // this still is "private" or not exported
}
func NewBloomFilter() *BloomFilter{
...
}
| } | ||
| } | ||
|
|
||
| func crossCheck(UserID *int, Email string) bool { |
There was a problem hiding this comment.
This function needs to bubble up the errors it encounters. Consider changing this functions return signature
func crossCheck(UserID *int, Email string) (bool, error) {...}
This way if there is an error (e.g. with the database connection) then this would return an error, instead of false which may or may not be the case.
| rows, err := db.Query(stmt) | ||
| if err != nil { | ||
| fmt.Printf("Error from Database Connection") | ||
| return false |
There was a problem hiding this comment.
Returning false says the email is not in the database. But we don't know at this point. All we know is there was an error with communicating with the database
| const numTables int = 5 | ||
|
|
||
| stmt := fmt.Sprintf("SELECT uid, email FROM User%02d WHERE uid=%d AND email='%s'", (*UserID)%numTables, *UserID, Email) | ||
| rows, err := db.Query(stmt) |
| return false | ||
| } | ||
|
|
||
| return rows.Next() |
There was a problem hiding this comment.
Eventually we need to call rows.Close() in order to return the connection back to the db conn pool. Take a look at pg 6-11 The_Ultimate_Guide_To_Building_Database-Driven_Apps_with_Go.pdf It's a quick read and it explains what is going behind the scenes pretty well.
In any case consider
ret := rows.Next()
rows.Close()
return ret
| fmt.Println("Starting Client") | ||
|
|
||
| // populate the MySQL Database | ||
| db := client.PopulateDB() |
There was a problem hiding this comment.
The flow of this seems odd. It looks like client.PopulateDB creates a database object and doesn't close it, but we close it here. I think we should have another function func NewDB() (*sql.DB, error) that creates the db conn pool. Then if there is no error, you can pass db variable to PopulateDB, and DropTables functions.
| defer db.Close() | ||
|
|
||
| // populate the Bloom Filter from values in the MySQL Database | ||
| client.Populate() |
There was a problem hiding this comment.
This returns an error. We should check it.
| client.Populate() | ||
|
|
||
| // to check if values are in the Bloom Filter. Note: Remember to replace the value of b and the userID in Check to what they are in the MySQL DB. | ||
| // b := []string{"[email protected]"} |
| return db | ||
| } | ||
|
|
||
| func DropTables(db *sql.DB) error { |
There was a problem hiding this comment.
This is kinda scary...Why do we want to do this? The database is the only source of truth we have. For our use case, the tables hold the unsubscribe emails, and we def don't want to remove them.
| const numTables int = 5 | ||
|
|
||
| func createTable(numTable int, db *sql.DB) error { | ||
| stmt := fmt.Sprintf(`CREATE TABLE IF NOT EXISTS User%02d ( |
There was a problem hiding this comment.
The table names are User01, User02, ... which makes it seem like we only have 5 users. I think we want them to be Unsub01, Unsub02, ...
|
|
||
| defer stmtHandle.Close() | ||
|
|
||
| for i := 0; i < len(usr.Email); i++ { |
There was a problem hiding this comment.
you could also use range https://gobyexample.com/range
| db, err := sql.Open("mysql", "root:SendGrid@tcp(localhost:3306)/UserStructs") | ||
| if err != nil { | ||
| fmt.Printf("Failed to get handle\n") | ||
| db.Close() |
There was a problem hiding this comment.
If there was an err on sql.Open, we are not guaranteed that db is not nil. I.e. db.Close will also error, and possibly it might be a nil pointer err. Instead we should bubble up the error and exit the function.
| err = db.Ping() | ||
| if err != nil { | ||
| fmt.Println(err) | ||
| db.Close() |
There was a problem hiding this comment.
Here db.Close is the right thing todo, however we should bubble up the error and exit the function. Otherwise on line 75 insertToTables will attempt to connect to a closed connection.
| err := createTable(i, db) | ||
| if err != nil { | ||
| fmt.Println(err) | ||
| db.Close() |
There was a problem hiding this comment.
bubble up err, and exit function... also consider defer db.Close(), immediately after creating the db successfully
| body, err := ioutil.ReadAll(r.Body) | ||
| if err != nil { | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| w.Write([]byte("Could not read the body of the request")) |
|
|
||
| func (bf *bloomFilter) populateBF(w http.ResponseWriter, r *http.Request) { | ||
| if r.Body == nil { | ||
| w.Write([]byte("Could not read the body of the request")) |
There was a problem hiding this comment.
this will register as http.StatusOK. You need to write the header you want here, and if it is an error case, I would return from the function
| body, err := ioutil.ReadAll(r.Body) | ||
| if err != nil { | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| w.Write([]byte("Could not read the body of the request")) |
| stmt := fmt.Sprintf("SELECT TABLE_NAME AS tableName FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_TYPE = 'BASE TABLE' AND TABLE_SCHEMA='UserStructs'") | ||
| rows, err := db.Query(stmt) | ||
| if err != nil { | ||
| fmt.Println("Error from Database Connection") |
| err = db.Ping() | ||
| if err != nil { | ||
| fmt.Println(err) | ||
| db.Close() |
| db, err := sql.Open("mysql", "root:SendGrid@tcp(localhost:3306)/UserStructs") | ||
| if err != nil { | ||
| fmt.Printf("Failed to get handle\n") | ||
| db.Close() |
| stmt := fmt.Sprintf("SELECT uid, email FROM UserStructs.%s", tableName) | ||
| rows, err := db.Query(stmt) | ||
| if err != nil { | ||
| fmt.Printf("Error from Database Connection") |
| } else { | ||
| userMap[&id] = []string{email} | ||
| } | ||
| } |
There was a problem hiding this comment.
need to call rows.Close to return the connection back to the db conn pool.
| Email []string | ||
| } | ||
|
|
||
| func Check(userID int, emails []string) error { |
There was a problem hiding this comment.
Where is this function being called? Also should it return a list of users that are in the bf?
| return nil | ||
| } | ||
|
|
||
| func Clear() error { |
There was a problem hiding this comment.
Again where is this being called?
| } | ||
|
|
||
| func Populate() error { | ||
| db, err := sql.Open("mysql", "root:SendGrid@tcp(localhost:3306)/UserStructs") |
There was a problem hiding this comment.
consider passing in the credentials as arguments instead of hard coding them
3 participants
These are the things we need code review with:
We changed the file structure as well (where each file goes) and we were wondering if this is fine?