From c0d361745e70d153cbd3a9f120cfe6df0d5c655b Mon Sep 17 00:00:00 2001 From: KarthikeyanAppointy Date: Wed, 17 Jan 2024 18:03:56 +0530 Subject: [PATCH 1/2] chore: Added Code Review --- .DS_Store | Bin 0 -> 6148 bytes .idea/.gitignore | 8 +++++++ .idea/modules.xml | 8 +++++++ .idea/screening.iml | 9 ++++++++ .idea/vcs.xml | 6 +++++ Dockerfile | 9 ++++++++ api.go | 53 ++++++++++++++++++++++++++++++++++++++++++-- go.mod | 7 +++++- main.go | 33 +++++++++++++++++++++++++++ utils.go | 16 +++++++++++++ 10 files changed, 146 insertions(+), 3 deletions(-) create mode 100644 .DS_Store create mode 100644 .idea/.gitignore create mode 100644 .idea/modules.xml create mode 100644 .idea/screening.iml create mode 100644 .idea/vcs.xml diff --git a/.DS_Store b/.DS_Store new file mode 100644 index 0000000000000000000000000000000000000000..882fbd080531d22dde93c340e6b68cf4f3839cef GIT binary patch literal 6148 zcmeHKy-EW?5T4N!0yY5)E%ynOyul@$y^t5scoG9SE@15M4bu4lw$>JQHiEsC&!U3g z?2LL#4oeZ8f!%L*es=CYxZN8f;?ZR}AnFrQ4?~be=@2n@x;89WoGg3Hr-SinT`f8` z`K=~-_KfyvN;6uf&wugU_EmLJOy>0jw%+dQ?(ya3VVL^MZ~cq&7B_s4RN6w_8I^QK zCu!xKkb2v-S5M2UH?7^uuj;Sr2iLd5*R_bGJ{?JY7iYj3a0Z-#|H%OEY?0o!qR-BN zGvEw-G9c$ez!1z08^w5ZV2UjOFoQV?bm=7|Cm802jUqe{)>NRTvb7kj>97Zj%MBYv zO((YIgRSyM^TM$@_75?fI9K%98E^(t1~zrsm;3(^pG;?ypQiZA8E^*v7z3OY<6?xD zvb%Nd_2jM%7{?eQ64!_Vfu8&XU?JzoUUaHIh>p12uu+s<#2(Xu{v!|x@yQwZ0S4Xy D#dk=k literal 0 HcmV?d00001 diff --git a/.idea/.gitignore b/.idea/.gitignore new file mode 100644 index 0000000..13566b8 --- /dev/null +++ b/.idea/.gitignore @@ -0,0 +1,8 @@ +# Default ignored files +/shelf/ +/workspace.xml +# Editor-based HTTP Client requests +/httpRequests/ +# Datasource local storage ignored files +/dataSources/ +/dataSources.local.xml diff --git a/.idea/modules.xml b/.idea/modules.xml new file mode 100644 index 0000000..28c7770 --- /dev/null +++ b/.idea/modules.xml @@ -0,0 +1,8 @@ + + + + + + + + \ No newline at end of file diff --git a/.idea/screening.iml b/.idea/screening.iml new file mode 100644 index 0000000..5e764c4 --- /dev/null +++ b/.idea/screening.iml @@ -0,0 +1,9 @@ + + + + + + + + + \ No newline at end of file diff --git a/.idea/vcs.xml b/.idea/vcs.xml new file mode 100644 index 0000000..35eb1dd --- /dev/null +++ b/.idea/vcs.xml @@ -0,0 +1,6 @@ + + + + + + \ No newline at end of file diff --git a/Dockerfile b/Dockerfile index d1438c5..a6144ed 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,9 +1,17 @@ # docker file for building Go application FROM ubuntu:latest +# it is recommended to use alpine:X.X as base image for production +# as it is lightweight and secure # Install dependencies +# It is not recommended to install dependencies in the base image, +# For Every code build it will try to install dependencies RUN sudo apt install -y git go wget +# use of sudo is also not recommended in the base image + +# Use of Scratch image is recommended for production +# and provice minimal permissions. COPY . /app WORKDIR /app @@ -11,4 +19,5 @@ WORKDIR /app # Build the application RUN go build -o main . +# This command is wrong, it should be like ./main CMD [ "main" ] \ No newline at end of file diff --git a/api.go b/api.go index cbef2ef..3ea7e2a 100644 --- a/api.go +++ b/api.go @@ -5,15 +5,64 @@ import ( "net/http" ) +// The Name of the function should be more meaningful, and describe what it does. +// so instead of setupJsonApi, we can use initHandlers or setupHandlers to register the handlers. func setupJsonApi() { http.HandleFunc("/createUser", func(w http.ResponseWriter, r *http.Request) { - // create mysql connection - conn := createConnection() + // Add a Method check here, something like this + /* + if r.Method != http.MethodPut { + w.WriteHeader(http.StatusBadRequest) + _, err := w.Write([]byte("Invalid request method")) + if err != nil { + return + } + return + } + */ + + // should not create connection everytime a new request comes in. which will result it using more resources. + // It is recommended to create a struct and create a connection only once + // Use interface on the struct to create connection to the database. + conn := createConnection() // This is a bad practice. + + // it is not recommended to use form values, instead we should use json. name := r.FormValue("name") email := r.FormValue("email") + // After getting the data from the form, we should validate it. + // we should not proceed if the name or email is empty. + // something like this + /* + if name == "" || email == "" { + w.WriteHeader(http.StatusBadRequest) + _, err := w.Write([]byte("Invalid request")) + if err != nil { + return + } + return + } + */ + + // for security reasons, + // implementing something like this will cause serious security issues. like it can be used for sql injection. + // imagine if someone sends a request like this + // http://localhost:8080/updateUser?id=1 OR 1=1;DROP TABLE users + // it will drop the users table. + // so, we need to use prepared statements. query := "INSERT INTO users (name, email) VALUES (" + name + ", " + email + ")" + // something like this + // query := "INSERT INTO users (name, email) VALUES ($1, $2)" + + // _, err := conn.Exec(query, name, email) result, err := conn.Exec(query) + // we cannot proceed if there is an error. + // we should handle the error + // if there is an error, we should return the error to the client. + + // logging user details is not a good practice. because of HIPAA compliance. fmt.Println("result ", result, " err ", err.Error()) + + // Returning without status code is not a good practice in case of Rest APIs. w.Write([]byte("Created user successfully!")) }) http.HandleFunc("/updateUser", func(w http.ResponseWriter, r *http.Request) { diff --git a/go.mod b/go.mod index 541428f..552ef72 100644 --- a/go.mod +++ b/go.mod @@ -1,3 +1,8 @@ module screening -go 1.21.3 +// invalid go version, in go mod the version should be mentioned in the format 1.XX format. +// there should be a go sum file in the directory. +// From this i have understood that, this package is not created by using +// go mod init command. + +go 1.21.3 \ No newline at end of file diff --git a/main.go b/main.go index 18805dd..d332389 100644 --- a/main.go +++ b/main.go @@ -5,6 +5,39 @@ import ( ) func main() { + //Usage of default handler is not recommended + // as default serveMux is a global variable and any package can access it and modify it setupJsonApi() + // Since we are using NewServeMux, we can use HandleFunc + // HandleFunc registers the handler function for the given pattern in the DefaultServeMux. + // The documentation for ServeMux explains how patterns are matched. http.ListenAndServe(":80", nil) + // There is no Timeout Provided. So, it will wait for the request to complete. + // Which is not a good practice. since we are connecting to DB and it may take time. + // So, we need to provide a timeout. + + /* it should have been something like this + mux := http.NewServeMux() + setupJsonApi(mux) + srv := &http.Server{ + Addr: ":8080", + Handler: mux, + ReadTimeout: 10 * time.Second, + WriteTimeout: 10 * time.Second, + } + err := srv.ListenAndServe() + if err != nil { + log.Fatal(err) + } + */ + + //Usage of middleware is recommended. For example, + // we can use middleware to log the request details + // we can use middleware to handle the errors + // we can use middleware to handle the panic + // we can use middleware to handle the timeouts + // we can use middleware to handle the authentication + // we can use middleware to handle the authorization + + // It is recommended to use a Packages like gorilla/mux or go-Chi for routing. } diff --git a/utils.go b/utils.go index e65cd87..67290a2 100644 --- a/utils.go +++ b/utils.go @@ -5,9 +5,25 @@ import ( "fmt" ) +// Make the name of the function more meaningful, and describe what it does. +// so instead of createConnection, we can use createDBConnection or createDBClient // createConnection creates a connection to mysql database func createConnection() *sql.DB { + + // it is not recommended to hardcode the credentials. + // we should use environment variables to store the credentials. + // we should not use root user to connect to the database. + // we should create a user with limited privileges and use that user to connect to the database. db, err := sql.Open("mysql", "root:password@tcp(127.0.0.1:3306)/test") + // handling the error is important. + // it is not recommended to print the error what is the error is nil. fmt.Println("sql open " + err.Error()) + // it will panic the code. + + // Handle error something like this + // if err != nil { + // fmt.Println("sql open " + err.Error()) + // return nil + // } return db } From 9767b6523453739f71e0e4cb3dd895603266c1aa Mon Sep 17 00:00:00 2001 From: KarthikeyanAppointy Date: Wed, 17 Jan 2024 18:52:19 +0530 Subject: [PATCH 2/2] chore: initial commit of basic server --- .DS_Store | Bin 6148 -> 6148 bytes Dockerfile | 28 +++++---------- api.go | 104 ++++++++++++++++++++--------------------------------- go.mod | 10 +++--- go.sum | 4 +++ main.go | 74 +++++++++++++++++++++----------------- model.go | 7 ++++ utils.go | 39 ++++++++++---------- 8 files changed, 126 insertions(+), 140 deletions(-) create mode 100644 go.sum create mode 100644 model.go diff --git a/.DS_Store b/.DS_Store index 882fbd080531d22dde93c340e6b68cf4f3839cef..fb54d6343bf89c0a494f1ca9c1769e6fd8e49278 100644 GIT binary patch delta 243 zcmZoMXffEJ##le?0s{jB3xgg*IzuKyNp8N2OHxjL5>Sl8G``t_WzA7XRQVLV@&y@& z!O8i#1wcIv49pE6k`-unCPNBCDnnvUI+84dgd9*d4oMo>^83*)PoS2gs6bdhd8I!O bGsJmFDo`B2kg!>h=^N|B0=~`c9Dn%%!E!jN delta 243 zcmZoMXffEJ##mo`lYxPOg+Y%YogtHf#Lv$fX#wT-&iLW@NH)2_{$Ff`{6!; diff --git a/Dockerfile b/Dockerfile index a6144ed..2b838a2 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,23 +1,13 @@ # docker file for building Go application -FROM ubuntu:latest -# it is recommended to use alpine:X.X as base image for production -# as it is lightweight and secure - -# Install dependencies -# It is not recommended to install dependencies in the base image, -# For Every code build it will try to install dependencies -RUN sudo apt install -y git go wget -# use of sudo is also not recommended in the base image - - -# Use of Scratch image is recommended for production -# and provice minimal permissions. -COPY . /app - +FROM golang:1.20 AS builder WORKDIR /app +COPY . . +RUN CGO_ENABLED=0 GOOS=linux go build -o main . -# Build the application -RUN go build -o main . -# This command is wrong, it should be like ./main -CMD [ "main" ] \ No newline at end of file +FROM alpine:3.9 as base +RUN apk --no-cache add ca-certificates +RUN apk --no-cache add tzdata +WORKDIR /app +COPY --from=builder /app/main . +CMD [ "./main" ] \ No newline at end of file diff --git a/api.go b/api.go index 3ea7e2a..7007520 100644 --- a/api.go +++ b/api.go @@ -1,78 +1,52 @@ package main import ( - "fmt" + "encoding/json" + "github.com/go-chi/chi" "net/http" ) -// The Name of the function should be more meaningful, and describe what it does. -// so instead of setupJsonApi, we can use initHandlers or setupHandlers to register the handlers. -func setupJsonApi() { - http.HandleFunc("/createUser", func(w http.ResponseWriter, r *http.Request) { - // Add a Method check here, something like this - /* - if r.Method != http.MethodPut { - w.WriteHeader(http.StatusBadRequest) - _, err := w.Write([]byte("Invalid request method")) - if err != nil { - return - } - return - } - */ +func (s *Server) setupRoutes() { + s.router.Post("/createUser", s.createUserHandler) + s.router.Put("/updateUser/{id}", s.updateUserHandler) +} + +func (s *Server) createUserHandler(w http.ResponseWriter, r *http.Request) { + conn := s.getDBConnection() + defer conn.Close() - // should not create connection everytime a new request comes in. which will result it using more resources. - // It is recommended to create a struct and create a connection only once - // Use interface on the struct to create connection to the database. - conn := createConnection() // This is a bad practice. + var user User + if err := json.NewDecoder(r.Body).Decode(&user); err != nil { + http.Error(w, "Bad Request: Invalid JSON", http.StatusBadRequest) + return + } - // it is not recommended to use form values, instead we should use json. - name := r.FormValue("name") - email := r.FormValue("email") - // After getting the data from the form, we should validate it. - // we should not proceed if the name or email is empty. - // something like this - /* - if name == "" || email == "" { - w.WriteHeader(http.StatusBadRequest) - _, err := w.Write([]byte("Invalid request")) - if err != nil { - return - } - return - } - */ + query := `INSERT INTO users (name, email) VALUES ($1, $2)` + _, err := conn.Exec(query, user.Name, user.Email) + if err != nil { + http.Error(w, "Internal Server Error", http.StatusInternalServerError) + return + } + + w.Write([]byte("Created user successfully!")) +} - // for security reasons, - // implementing something like this will cause serious security issues. like it can be used for sql injection. - // imagine if someone sends a request like this - // http://localhost:8080/updateUser?id=1 OR 1=1;DROP TABLE users - // it will drop the users table. - // so, we need to use prepared statements. - query := "INSERT INTO users (name, email) VALUES (" + name + ", " + email + ")" - // something like this - // query := "INSERT INTO users (name, email) VALUES ($1, $2)" +func (s *Server) updateUserHandler(w http.ResponseWriter, r *http.Request) { + conn := s.getDBConnection() + defer conn.Close() - // _, err := conn.Exec(query, name, email) - result, err := conn.Exec(query) - // we cannot proceed if there is an error. - // we should handle the error - // if there is an error, we should return the error to the client. + var user User + if err := json.NewDecoder(r.Body).Decode(&user); err != nil { + http.Error(w, "Bad Request", http.StatusBadRequest) + return + } - // logging user details is not a good practice. because of HIPAA compliance. - fmt.Println("result ", result, " err ", err.Error()) + query := `UPDATE users SET name=$1, email=$2 WHERE id=$3` + _, err := conn.Exec(query, user.Name, user.Email, chi.URLParam(r, "id")) + if err != nil { + http.Error(w, "Internal Server Error", http.StatusInternalServerError) + return + } - // Returning without status code is not a good practice in case of Rest APIs. - w.Write([]byte("Created user successfully!")) - }) - http.HandleFunc("/updateUser", func(w http.ResponseWriter, r *http.Request) { - // create mysql connection - conn := createConnection() - name := r.FormValue("name") - email := r.FormValue("email") - query := "Update users set name=" + name + ", email=" + email + " where id=" + r.FormValue("id") - result, err := conn.Exec(query) - fmt.Println("result ", result, " err ", err.Error()) - w.Write([]byte("User updated successfully!")) - }) + w.Write([]byte("User updated successfully!")) } diff --git a/go.mod b/go.mod index 552ef72..b456bf7 100644 --- a/go.mod +++ b/go.mod @@ -1,8 +1,8 @@ module screening -// invalid go version, in go mod the version should be mentioned in the format 1.XX format. -// there should be a go sum file in the directory. -// From this i have understood that, this package is not created by using -// go mod init command. +go 1.20 -go 1.21.3 \ No newline at end of file +require ( + github.com/go-chi/chi v1.5.5 + github.com/go-sql-driver/mysql v1.7.1 +) diff --git a/go.sum b/go.sum new file mode 100644 index 0000000..bf728cc --- /dev/null +++ b/go.sum @@ -0,0 +1,4 @@ +github.com/go-chi/chi v1.5.5 h1:vOB/HbEMt9QqBqErz07QehcOKHaWFtuj87tTDVz2qXE= +github.com/go-chi/chi v1.5.5/go.mod h1:C9JqLr3tIYjDOZpzn+BCuxY8z8vmca43EeMgyZt7irw= +github.com/go-sql-driver/mysql v1.7.1 h1:lUIinVbN1DY0xBg0eMOzmmtGoHwWBbvnWubQUrtU8EI= +github.com/go-sql-driver/mysql v1.7.1/go.mod h1:OXbVy3sEdcQ2Doequ6Z5BW6fXNQTmx+9S1MCJN5yJMI= diff --git a/main.go b/main.go index d332389..cb59149 100644 --- a/main.go +++ b/main.go @@ -1,43 +1,53 @@ package main import ( + "database/sql" + "github.com/go-chi/chi" + "github.com/go-chi/chi/middleware" + _ "github.com/go-sql-driver/mysql" + "log" "net/http" + "sync" + "time" ) -func main() { - //Usage of default handler is not recommended - // as default serveMux is a global variable and any package can access it and modify it - setupJsonApi() - // Since we are using NewServeMux, we can use HandleFunc - // HandleFunc registers the handler function for the given pattern in the DefaultServeMux. - // The documentation for ServeMux explains how patterns are matched. - http.ListenAndServe(":80", nil) - // There is no Timeout Provided. So, it will wait for the request to complete. - // Which is not a good practice. since we are connecting to DB and it may take time. - // So, we need to provide a timeout. - - /* it should have been something like this - mux := http.NewServeMux() - setupJsonApi(mux) - srv := &http.Server{ - Addr: ":8080", - Handler: mux, - ReadTimeout: 10 * time.Second, - WriteTimeout: 10 * time.Second, +type Server struct { + router *chi.Mux + db *sql.DB + mu sync.Mutex +} + +func NewServer() *Server { + return &Server{ + router: chi.NewRouter(), } - err := srv.ListenAndServe() - if err != nil { - log.Fatal(err) +} + +func (s *Server) Init() { + s.router.Use(middleware.Logger) + s.router.Use(middleware.Recoverer) + + s.setupRoutes() + s.initDBConnection() +} + +func (s *Server) Start(port string) error { + server := &http.Server{ + Addr: port, + Handler: s.router, + ReadTimeout: 5 * time.Second, + WriteTimeout: 10 * time.Second, } - */ - //Usage of middleware is recommended. For example, - // we can use middleware to log the request details - // we can use middleware to handle the errors - // we can use middleware to handle the panic - // we can use middleware to handle the timeouts - // we can use middleware to handle the authentication - // we can use middleware to handle the authorization + log.Printf("Server started on port %s", port) + return server.ListenAndServe() +} - // It is recommended to use a Packages like gorilla/mux or go-Chi for routing. +func main() { + server := NewServer() + server.Init() + + if err := server.Start(":8080"); err != nil { + log.Fatalf("Error starting server: %v", err) + } } diff --git a/model.go b/model.go new file mode 100644 index 0000000..cbdc729 --- /dev/null +++ b/model.go @@ -0,0 +1,7 @@ +package main + +type User struct { + ID int `json:"id,omitempty"` + Name string `json:"name"` + Email string `json:"email"` +} diff --git a/utils.go b/utils.go index 67290a2..7dd27ed 100644 --- a/utils.go +++ b/utils.go @@ -3,27 +3,28 @@ package main import ( "database/sql" "fmt" + "log" + "os" ) -// Make the name of the function more meaningful, and describe what it does. -// so instead of createConnection, we can use createDBConnection or createDBClient -// createConnection creates a connection to mysql database -func createConnection() *sql.DB { +func (s *Server) getDBConnection() *sql.DB { + s.mu.Lock() + defer s.mu.Unlock() - // it is not recommended to hardcode the credentials. - // we should use environment variables to store the credentials. - // we should not use root user to connect to the database. - // we should create a user with limited privileges and use that user to connect to the database. - db, err := sql.Open("mysql", "root:password@tcp(127.0.0.1:3306)/test") - // handling the error is important. - // it is not recommended to print the error what is the error is nil. - fmt.Println("sql open " + err.Error()) - // it will panic the code. + return s.db +} + +func (s *Server) initDBConnection() { + dbHost := os.Getenv("DB_HOST") + dbPort := os.Getenv("DB_PORT") + dbUser := os.Getenv("DB_USER") + dbPassword := os.Getenv("DB_PASSWORD") + dbName := os.Getenv("DB_NAME") - // Handle error something like this - // if err != nil { - // fmt.Println("sql open " + err.Error()) - // return nil - // } - return db + dbSource := fmt.Sprintf("%s:%s@tcp(%s:%s)/%s", dbUser, dbPassword, dbHost, dbPort, dbName) + conn, err := sql.Open("mysql", dbSource) + if err != nil { + log.Fatalf("Failed to connect to the database: %v", err) + } + s.db = conn }