Github user nir-sopher commented on the issue:
https://github.com/apache/incubator-trafficcontrol/pull/325
Thank a lot Rob!
I'll go over your comments early next week.
As this is my first go written code, you are actually giving me my first
real life go lesson.
Any input is very welcomed.
Moreover, do we have some kind of coding conventions document? If one is
needed, I would be very happy to look for a reference online. The community
than can comment on it and further adjust it to its needs.
Nir
On Mar 24, 2017 3:37 AM, "Robert Butts" <[email protected]> wrote:
> *@rob05c* requested changes on this pull request.
>
> Ok, lots of comments. If this feels overwhelming, or you disagree with
> things, talk to me and we can work out what's critical and what isn't.
>
> Since this is experimental, we can be a little lenient, but most of these
> will need changed before it can be moved out of experimental, so I
> recommend doing them sooner rather than later.
>
> Some are critical, like the SQL injection vulnerabilities. This cannot be
> merged, even into experimental, without fixing those.
> ------------------------------
>
> In traffic_ops/experimental/go-api/api.go
>
<https://github.com/apache/incubator-trafficcontrol/pull/325#discussion_r107806717>
> :
>
> > +
> +
> +import (
> + "database/sql"
> + "encoding/json"
> + "flag"
> + "fmt"
> + "log"
> + "net/http"
> + "os"
> +
> + "github.com/gorilla/mux"
> + _ "github.com/lib/pq"
> +
> +//modules to seed endpoints
> + "./tenant"
>
> This must be an absolute path. Relative paths cause all kinds of pain in
> Go (and arguably any language).
> ------------------------------
>
> In traffic_ops/experimental/go-api/api.go
>
<https://github.com/apache/incubator-trafficcontrol/pull/325#discussion_r107807929>
> :
>
> > +Note that the below go modules are required, so you'll might need to
"go get" them:)
> +"github.com/gorilla/mux"
> +"github.com/lib/pq"
> +*/
> +
> +
> +import (
> + "database/sql"
> + "encoding/json"
> + "flag"
> + "fmt"
> + "log"
> + "net/http"
> + "os"
> +
> + "github.com/gorilla/mux"
>
> This is ok for an experimental app, but consider using your own routing.
> (We may create a trafficcontrol library for this in the future). Regex
> routing is 50 lines of Go, versus the cost of a large dependency like
> Gorilla. See https://gist.github.com/rob05c/15866a9d032fb07df0e1b27fb2f088
> e5 and https://github.com/apache/incubator-trafficcontrol/blob/
> master/traffic_monitor_golang/traffic_monitor/srvhttp/srvhttp.go
> ------------------------------
>
> In traffic_ops/experimental/go-api/api.go
>
<https://github.com/apache/incubator-trafficcontrol/pull/325#discussion_r107808306>
> :
>
> > +"github.com/gorilla/mux"
> +"github.com/lib/pq"
> +*/
> +
> +
> +import (
> + "database/sql"
> + "encoding/json"
> + "flag"
> + "fmt"
> + "log"
> + "net/http"
> + "os"
> +
> + "github.com/gorilla/mux"
> + _ "github.com/lib/pq"
>
> These must be vendored. If you haven't vendored Go before, this is done by
> creating a vendor directory in /go-api with the full import path, i.e.
mkdir
> -p vendor/github.com/lib, cloning into it with git clone
> https://github.com/lib/pq, and deleting the .git directory (DON'T forget
> this, conflicting git directories will break everything).
>
> In the future, common libraries like pq will be vendored in a parent
> directory, but they aren't there yet.
>
> Unvendored dependencies are both a security vulnerability and a point of
> failure when the code changes underneath us.
> ------------------------------
>
> In traffic_ops/experimental/go-api/api.go
>
<https://github.com/apache/incubator-trafficcontrol/pull/325#discussion_r107808431>
> :
>
> > +
> + http://www.apache.org/licenses/LICENSE-2.0
> +
> + Unless required by applicable law or agreed to in writing, software
> + distributed under the License is distributed on an "AS IS" BASIS,
> + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + See the License for the specific language governing permissions and
> + limitations under the License.
> +*/
> +
> +/*
> +This module it provides a server creating a RESTful API towards
Traffic-Ops DB.
> +It was created in order to POC the concept of API Gateway (added on
another PR) between the different modules of Traffic-Ops, specifically old perl
Traffic-Ops, and new (go?) one.
> +
> +At first step, only "tenant" table is covered (CRUD).
> +Other tables can be added, by adding modules (see tenant foe example),
and seeding their enpoints below.
>
> Typo "foe"
> ------------------------------
>
> In traffic_ops/experimental/go-api/api.go
>
<https://github.com/apache/incubator-trafficcontrol/pull/325#discussion_r107808481>
> :
>
> > +/*
> + Licensed under the Apache License, Version 2.0 (the "License");
> + you may not use this file except in compliance with the License.
> + You may obtain a copy of the License at
> +
> + http://www.apache.org/licenses/LICENSE-2.0
> +
> + Unless required by applicable law or agreed to in writing, software
> + distributed under the License is distributed on an "AS IS" BASIS,
> + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + See the License for the specific language governing permissions and
> + limitations under the License.
> +*/
> +
> +/*
> +This module it provides a server creating a RESTful API towards
Traffic-Ops DB.
>
> Typo, remove "it". Also, this isn't a module, it's an application.
> ------------------------------
>
> In traffic_ops/experimental/go-api/tenant/EndPointSeeder.go
>
<https://github.com/apache/incubator-trafficcontrol/pull/325#discussion_r107809703>
> :
>
> > +import (
> + "database/sql"
> + "encoding/json"
> + "net/http"
> + "fmt"
> + "time"
> +
> + "github.com/gorilla/mux"
> + _ "github.com/lib/pq"
> +)
> +
> +
> +//TODO(nirs) polymorphisem
> +
> +/////BASE - COMMON TO ALL SEEDERS (needs to move to another package)
> +/// Defintions
>
> This will break Godoc. Godoc treats comments immediately above
> declarations as applying to that declaration (EndpointSeeder in this
case).
>
> Consider adding Godoc comments to all your declarations (we like Godoc,
> but don't mandate it [yet]). See https://blog.golang.org/godoc-
> documenting-go-code
> ------------------------------
>
> In traffic_ops/experimental/go-api/tenant/EndPointSeeder.go
>
<https://github.com/apache/incubator-trafficcontrol/pull/325#discussion_r107810020>
> :
>
> > + "github.com/gorilla/mux"
> + _ "github.com/lib/pq"
> +)
> +
> +
> +//TODO(nirs) polymorphisem
> +
> +/////BASE - COMMON TO ALL SEEDERS (needs to move to another package)
> +/// Defintions
> +type EndpointSeeder struct{
> + myDb *sql.DB
> +}
> +
> +/// Interface
> +func NewEndpointSeeder(aDb *sql.DB) *EndpointSeeder {
> + endPointSeeder := new(EndpointSeeder)
>
> This isn't idiomatic Go. Avoid new, prefer endpointSeeder :=
> &EndpointSeeder{}
> ------------------------------
>
> In traffic_ops/experimental/go-api/tenant/EndPointSeeder.go
>
<https://github.com/apache/incubator-trafficcontrol/pull/325#discussion_r107810203>
> :
>
> > + "database/sql"
> + "encoding/json"
> + "net/http"
> + "fmt"
> + "time"
> +
> + "github.com/gorilla/mux"
> + _ "github.com/lib/pq"
> +)
> +
> +
> +//TODO(nirs) polymorphisem
> +
> +/////BASE - COMMON TO ALL SEEDERS (needs to move to another package)
> +/// Defintions
> +type EndpointSeeder struct{
>
> Do you expect EndpointSeeder to have more members in the future? If not,
> this should be changed to type EndpointSeeder sql.DB or type
> EndpointSeeder *sql.DB.
> ------------------------------
>
> In traffic_ops/experimental/go-api/tenant/EndPointSeeder.go
>
<https://github.com/apache/incubator-trafficcontrol/pull/325#discussion_r107810629>
> :
>
> > +//TODO(nirs) polymorphisem
> +
> +/////BASE - COMMON TO ALL SEEDERS (needs to move to another package)
> +/// Defintions
> +type EndpointSeeder struct{
> + myDb *sql.DB
> +}
> +
> +/// Interface
> +func NewEndpointSeeder(aDb *sql.DB) *EndpointSeeder {
> + endPointSeeder := new(EndpointSeeder)
> + endPointSeeder.myDb = aDb
> + return endPointSeeder
> +}
> +
> +func (aEndpointSeeder *EndpointSeeder) Seed (aRouter *mux.Router,
aApiPrefix string) {
>
> aEndpointSeeder and aRouter aren't idiomatic Go. The "a" is unnecessary,
> and short names are preferred. Consider seeder, endpointSeeder, or for a
> function this short, even es or s. See https://golang.org/doc/
> effective_go.html#names
> ------------------------------
>
> In traffic_ops/experimental/go-api/tenant/EndPointSeeder.go
>
<https://github.com/apache/incubator-trafficcontrol/pull/325#discussion_r107810958>
> :
>
> > + Id int `json:"id"`
> + Name string `json:"name"`
> + Active bool `json:"active"`
> + ParentId int `json:"parentId"`
> +}
> +
> +func (aEndpointSeeder *EndpointSeeder) getEndpoint(aResponseWriter
http.ResponseWriter, aRequest *http.Request) {
> + params := mux.Vars(aRequest)
> +
> + var id int
> + var name string
> + var active bool
> + var parent_id sql.NullInt64
> + var updated time.Time;
> +
> + err := aEndpointSeeder.myDb.QueryRow("SELECT * FROM tenant WHERE
id="+params["id"]).Scan(&id, &name, &active, &parent_id, &updated)
>
> This is a SQL injection vulnerability. Use SQL parameters, i.e.
.QueryRow(`SELECT
> id, name, active, parent_id, updated FROM tenant WHERE id = $1`,
> params["id"]`).
> ------------------------------
>
> In traffic_ops/experimental/go-api/tenant/EndPointSeeder.go
>
<https://github.com/apache/incubator-trafficcontrol/pull/325#discussion_r107811076>
> :
>
> > + Id int `json:"id"`
> + Name string `json:"name"`
> + Active bool `json:"active"`
> + ParentId int `json:"parentId"`
> +}
> +
> +func (aEndpointSeeder *EndpointSeeder) getEndpoint(aResponseWriter
http.ResponseWriter, aRequest *http.Request) {
> + params := mux.Vars(aRequest)
> +
> + var id int
> + var name string
> + var active bool
> + var parent_id sql.NullInt64
> + var updated time.Time;
> +
> + err := aEndpointSeeder.myDb.QueryRow("SELECT * FROM tenant WHERE
id="+params["id"]).Scan(&id, &name, &active, &parent_id, &updated)
>
> Don't use SELECT * in production code, SQL column orders are not
> guaranteed. Select the specific columns you need, i.e. SELECT id, name ...
> ------------------------------
>
> In traffic_ops/experimental/go-api/tenant/EndPointSeeder.go
>
<https://github.com/apache/incubator-trafficcontrol/pull/325#discussion_r107811947>
> :
>
> > +}
> +
> +func (aEndpointSeeder *EndpointSeeder) Seed (aRouter *mux.Router,
aApiPrefix string) {
> +
> + aRouter.HandleFunc(aApiPrefix+"/tenants",
aEndpointSeeder.getListEndpoint).Methods("GET")
> + aRouter.HandleFunc(aApiPrefix+"/tenants/{id}",
aEndpointSeeder.getEndpoint).Methods("GET")
> + aRouter.HandleFunc(aApiPrefix+"/tenants",
aEndpointSeeder.createEndpoint).Methods("POST")
> + aRouter.HandleFunc(aApiPrefix+"/tenants/{id}",
aEndpointSeeder.updateEndpoint).Methods("PUT")
> + aRouter.HandleFunc(aApiPrefix+"/tenants/{id}",
aEndpointSeeder.deleteEndpoint).Methods("DELETE")
> +}
> +
> +///Common utilities
> +func (aEndpointSeeder *EndpointSeeder) checkErr(aError error) {
> + //TODO log
> + if aError != nil {
> + panic(aError)
>
> A panic call is not acceptable in production code. It violates flow
> control, and further it crashes the application. Here, checkErr is called
> for errors from client requests, so clients can potentially crash the
> application with a bad request.
>
> A function like checkErr is acceptable for logging, but not panicing.
> Code calling checkErr should be refactored to return an error up the
> stack, until an appropriate caller, some initial HTTP handler func in this
> case, can log the error and return gracefully.
>
> When you're changing this to pass errors up, the preferred technique is
> for every function to add context via fmt.Errorf. E.g. if err :=
> processJson(); err { return fmt.Errorf("error processing JSON: %v", err) }
> ------------------------------
>
> In traffic_ops/experimental/go-api/tenant/EndPointSeeder.go
>
<https://github.com/apache/incubator-trafficcontrol/pull/325#discussion_r107812127>
> :
>
> > + aRouter.HandleFunc(aApiPrefix+"/tenants/{id}",
aEndpointSeeder.getEndpoint).Methods("GET")
> + aRouter.HandleFunc(aApiPrefix+"/tenants",
aEndpointSeeder.createEndpoint).Methods("POST")
> + aRouter.HandleFunc(aApiPrefix+"/tenants/{id}",
aEndpointSeeder.updateEndpoint).Methods("PUT")
> + aRouter.HandleFunc(aApiPrefix+"/tenants/{id}",
aEndpointSeeder.deleteEndpoint).Methods("DELETE")
> +}
> +
> +///Common utilities
> +func (aEndpointSeeder *EndpointSeeder) checkErr(aError error) {
> + //TODO log
> + if aError != nil {
> + panic(aError)
> + }
> +}
> +
> +
> +/////Class specific
>
> Not a huge deal, but these ///// comments aren't idiomatic to Go or
> Traffic Control.
> ------------------------------
>
> In traffic_ops/experimental/go-api/tenant/EndPointSeeder.go
>
<https://github.com/apache/incubator-trafficcontrol/pull/325#discussion_r107812373>
> :
>
> > + Name string `json:"name"`
> + Active bool `json:"active"`
> + ParentId int `json:"parentId"`
> +}
> +
> +func (aEndpointSeeder *EndpointSeeder) getEndpoint(aResponseWriter
http.ResponseWriter, aRequest *http.Request) {
> + params := mux.Vars(aRequest)
> +
> + var id int
> + var name string
> + var active bool
> + var parent_id sql.NullInt64
> + var updated time.Time;
> +
> + err := aEndpointSeeder.myDb.QueryRow("SELECT * FROM tenant WHERE
id="+params["id"]).Scan(&id, &name, &active, &parent_id, &updated)
> + if err == sql.ErrNoRows{//TODO make it work
>
> TODOs are ok, but can you briefly explain? What doesn't work?
> ------------------------------
>
> In traffic_ops/experimental/go-api/tenant/EndPointSeeder.go
>
<https://github.com/apache/incubator-trafficcontrol/pull/325#discussion_r107812486>
> :
>
> > + if err == sql.ErrNoRows{//TODO make it work
> + aResponseWriter.WriteHeader(http.StatusNotFound)
> + fmt.Fprint(aResponseWriter, params["id"]+" not found")
> + return
> + }
> + aEndpointSeeder.checkErr(err)
> +
> + var parent_id1 int
> + if parent_id.Valid {
> + parent_id1 = int(parent_id.Int64)
> + } else {
> + parent_id1 = 0
> + }
> +
> + json.NewEncoder(aResponseWriter).Encode(Tenant{Id: id, Name: name,
Active: active, ParentId: parent_id1})
> + return
>
> return is unnecessary here.
> ------------------------------
>
> In traffic_ops/experimental/go-api/tenant/EndPointSeeder.go
>
<https://github.com/apache/incubator-trafficcontrol/pull/325#discussion_r107812722>
> :
>
> > + if parent_id.Valid {
> + parent_id1 = int(parent_id.Int64)
> + } else {
> + parent_id1 = 0
> + }
> +
> + tenants = append(tenants, Tenant{Id: id, Name: name, Active:
active, ParentId: parent_id1})
> + }
> +
> + json.NewEncoder(aResponseWriter).Encode(tenants)
> +
> +}
> +
> +func (aEndpointSeeder *EndpointSeeder) createEndpoint(aResponseWriter
http.ResponseWriter, aRequest *http.Request) {
> + var tenant Tenant
> + _ = json.NewDecoder(aRequest.Body).Decode(&tenant)
>
> This error needs checked. Ignoring errors is never ok in production code.
> ------------------------------
>
> In traffic_ops/experimental/go-api/tenant/EndPointSeeder.go
>
<https://github.com/apache/incubator-trafficcontrol/pull/325#discussion_r107812849>
> :
>
> > + var lastInsertId int
> + err := aEndpointSeeder.myDb.QueryRow("INSERT INTO tenant(name,
active, parent_id, last_updated) VALUES($1,$2,$3,$4) returning id;",
tenant.Name, tenant.Active, tenantParent, "2012-12-09").Scan(&lastInsertId)
> + aEndpointSeeder.checkErr(err)
> + aEndpointSeeder.getListEndpoint(aResponseWriter, aRequest)
> +}
> +
> +
> +
> +func (aEndpointSeeder *EndpointSeeder) updateEndpoint(aResponseWriter
http.ResponseWriter, aRequest *http.Request) {
> + params := mux.Vars(aRequest)
> +
> + var tenant Tenant
> + _ = json.NewDecoder(aRequest.Body).Decode(&tenant)
> +
> + var tenantParent sql.NullInt64
> + if tenant.ParentId==0{
>
> This hasn't been formatted. Please run gofmt on these files.
> ------------------------------
>
> In traffic_ops/experimental/go-api/tenant/EndPointSeeder.go
>
<https://github.com/apache/incubator-trafficcontrol/pull/325#discussion_r107812877>
> :
>
> > + }
> +
> +
> + var lastInsertId int
> + err := aEndpointSeeder.myDb.QueryRow("INSERT INTO tenant(name,
active, parent_id, last_updated) VALUES($1,$2,$3,$4) returning id;",
tenant.Name, tenant.Active, tenantParent, "2012-12-09").Scan(&lastInsertId)
> + aEndpointSeeder.checkErr(err)
> + aEndpointSeeder.getListEndpoint(aResponseWriter, aRequest)
> +}
> +
> +
> +
> +func (aEndpointSeeder *EndpointSeeder) updateEndpoint(aResponseWriter
http.ResponseWriter, aRequest *http.Request) {
> + params := mux.Vars(aRequest)
> +
> + var tenant Tenant
> + _ = json.NewDecoder(aRequest.Body).Decode(&tenant)
>
> Error must be checked.
> ------------------------------
>
> In traffic_ops/experimental/go-api/tenant/EndPointSeeder.go
>
<https://github.com/apache/incubator-trafficcontrol/pull/325#discussion_r107812920>
> :
>
> > +
> +func (aEndpointSeeder *EndpointSeeder) updateEndpoint(aResponseWriter
http.ResponseWriter, aRequest *http.Request) {
> + params := mux.Vars(aRequest)
> +
> + var tenant Tenant
> + _ = json.NewDecoder(aRequest.Body).Decode(&tenant)
> +
> + var tenantParent sql.NullInt64
> + if tenant.ParentId==0{
> + tenantParent.Valid = false
> + }else{
> + tenantParent.Int64 = int64(tenant.ParentId)
> + tenantParent.Valid = true
> + }
> +
> + stmt, err := aEndpointSeeder.myDb.Prepare("update tenant set
name=$1, active=$2, parent_id=$3, last_updated=$4 where id="+params["id"])
>
> SQL injection vulnerability. See previous comment.
> ------------------------------
>
> In traffic_ops/experimental/go-api/tenant/EndPointSeeder.go
>
<https://github.com/apache/incubator-trafficcontrol/pull/325#discussion_r107813032>
> :
>
> > + var tenantParent sql.NullInt64
> + if tenant.ParentId==0{
> + tenantParent.Valid = false
> + }else{
> + tenantParent.Int64 = int64(tenant.ParentId)
> + tenantParent.Valid = true
> + }
> +
> + stmt, err := aEndpointSeeder.myDb.Prepare("update tenant set
name=$1, active=$2, parent_id=$3, last_updated=$4 where id="+params["id"])
> + if err == sql.ErrNoRows{//TODO make it work
> + aResponseWriter.WriteHeader(http.StatusNotFound)
> + return
> + }
> + aEndpointSeeder.checkErr(err)
> +
> + res, err := stmt.Exec(tenant.Name, tenant.Active, tenantParent,
"2012-12-09")
>
> Why 2012? If this is ok, can you pull it into a variable with an
> explanatory name, e.g. sampleDataDateStr := "2012-12-09"?
> ------------------------------
>
> In traffic_ops/experimental/go-api/tenant/EndPointSeeder.go
>
<https://github.com/apache/incubator-trafficcontrol/pull/325#discussion_r107813099>
> :
>
> > + }
> + aEndpointSeeder.checkErr(err)
> +
> + res, err := stmt.Exec(tenant.Name, tenant.Active, tenantParent,
"2012-12-09")
> + aEndpointSeeder.checkErr(err)
> +
> + _, err = res.RowsAffected()
> + aEndpointSeeder.checkErr(err)
> +
> + aEndpointSeeder.getEndpoint(aResponseWriter, aRequest)
> +}
> +
> +func (aEndpointSeeder *EndpointSeeder) deleteEndpoint(aResponseWriter
http.ResponseWriter, aRequest *http.Request) {
> + params := mux.Vars(aRequest)
> +
> + stmt, err := aEndpointSeeder.myDb.Prepare("delete from tenant where
id="+params["id"])
>
> SQL injection vulnerability. See previous comment.
> ------------------------------
>
> In traffic_ops/experimental/go-api/tenant/EndPointSeeder.go
>
<https://github.com/apache/incubator-trafficcontrol/pull/325#discussion_r107813158>
> :
>
> > +
> + for rows.Next() {
> + var id int
> + var name string
> + var active bool
> + var parent_id sql.NullInt64
> + var updated time.Time;
> +
> + err = rows.Scan(&id, &name, &active, &parent_id, &updated)
> + aEndpointSeeder.checkErr(err)
> +
> + var parent_id1 int
> + if parent_id.Valid {
> + parent_id1 = int(parent_id.Int64)
> + } else {
> + parent_id1 = 0
>
> This is unnecessary, Go defaults uninitialized integral values to 0.
>
> Actually, the if statement isn't necessary at all, you can simply
parent_id1
> := int(parent_id.Int64), because parent_id.Int64 will be
> default-initialized to 0, even if it isn't valid.
> ------------------------------
>
> In traffic_ops/experimental/go-api/tenant/EndPointSeeder.go
>
<https://github.com/apache/incubator-trafficcontrol/pull/325#discussion_r107813482>
> :
>
> > +func (aEndpointSeeder *EndpointSeeder) getListEndpoint(aResponseWriter
http.ResponseWriter, aRequest *http.Request) {
> + var tenants []Tenant
> + rows, err := aEndpointSeeder.myDb.Query("SELECT * FROM tenant")
> + aEndpointSeeder.checkErr(err)
> +
> + for rows.Next() {
> + var id int
> + var name string
> + var active bool
> + var parent_id sql.NullInt64
> + var updated time.Time;
> +
> + err = rows.Scan(&id, &name, &active, &parent_id, &updated)
> + aEndpointSeeder.checkErr(err)
> +
> + var parent_id1 int
>
> This name is un-idiomatic Go. Go private variables should be camelCase,
> and numbers like this are never helpful to readers. Maybe something like
var
> parentIdNullable sql.NullInt64 and var parentId int?
>
> See https://golang.org/doc/effective_go.html#names
> ------------------------------
>
> In traffic_ops/experimental/go-api/tenant/EndPointSeeder.go
>
<https://github.com/apache/incubator-trafficcontrol/pull/325#discussion_r107813615>
> :
>
> > + ParentId int `json:"parentId"`
> +}
> +
> +func (aEndpointSeeder *EndpointSeeder) getEndpoint(aResponseWriter
http.ResponseWriter, aRequest *http.Request) {
> + params := mux.Vars(aRequest)
> +
> + var id int
> + var name string
> + var active bool
> + var parent_id sql.NullInt64
> + var updated time.Time;
> +
> + err := aEndpointSeeder.myDb.QueryRow("SELECT * FROM tenant WHERE
id="+params["id"]).Scan(&id, &name, &active, &parent_id, &updated)
> + if err == sql.ErrNoRows{//TODO make it work
> + aResponseWriter.WriteHeader(http.StatusNotFound)
> + fmt.Fprint(aResponseWriter, params["id"]+" not found")
>
> params["id"] Is used a lot in this function. Consider putting it in a
> variable?
> ------------------------------
>
> In traffic_ops/experimental/go-api/api.go
>
<https://github.com/apache/incubator-trafficcontrol/pull/325#discussion_r107813835>
> :
>
> > +
> + Unless required by applicable law or agreed to in writing, software
> + distributed under the License is distributed on an "AS IS" BASIS,
> + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + See the License for the specific language governing permissions and
> + limitations under the License.
> +*/
> +
> +/*
> +This module it provides a server creating a RESTful API towards
Traffic-Ops DB.
> +It was created in order to POC the concept of API Gateway (added on
another PR) between the different modules of Traffic-Ops, specifically old perl
Traffic-Ops, and new (go?) one.
> +
> +At first step, only "tenant" table is covered (CRUD).
> +Other tables can be added, by adding modules (see tenant foe example),
and seeding their enpoints below.
> +To Run this module, call: go run api.go --server --db-config-file
> +e.g. go run api.go --server :8888 --db-config-file
../../app/conf/test/database.conf
>
> Consider printing this text as an example, when invalid or empty arguments
> are passed to the application? That way, users can see how to run the app,
> without having to look at the source code.
> ------------------------------
>
> In traffic_ops/experimental/go-api/api.go
>
<https://github.com/apache/incubator-trafficcontrol/pull/325#discussion_r107814537>
> :
>
> > + }
> +
> + dbConfigFD, err := os.Open(*dbConfigFileName)
> + if err != nil {
> + fmt.Println(err.Error())
> + os.Exit(1)
> + }
> +
> + var dbConfig DbConfig
> + jsonParser := json.NewDecoder(dbConfigFD)
> + if err := jsonParser.Decode(&dbConfig); err != nil {
> + fmt.Println(err.Error())
> + os.Exit(1)
> + }
> +
> + psqlInfo := fmt.Sprintf("host=%s port=%s user=%s "+"password=%s
dbname=%s sslmode=disable",dbConfig.Hostname, dbConfig.Port, dbConfig.User,
dbConfig.Password, dbConfig.DbName)
>
> Consider adding a flag for sslmode, which defaults to require. Setting
> sslmode=disable can be acceptable in certain circumstances, but it's a
> big security risk.
> ------------------------------
>
> In traffic_ops/experimental/go-api/api.go
>
<https://github.com/apache/incubator-trafficcontrol/pull/325#discussion_r107814559>
> :
>
> > + }
> +
> + dbConfigFD, err := os.Open(*dbConfigFileName)
> + if err != nil {
> + fmt.Println(err.Error())
> + os.Exit(1)
> + }
> +
> + var dbConfig DbConfig
> + jsonParser := json.NewDecoder(dbConfigFD)
> + if err := jsonParser.Decode(&dbConfig); err != nil {
> + fmt.Println(err.Error())
> + os.Exit(1)
> + }
> +
> + psqlInfo := fmt.Sprintf("host=%s port=%s user=%s "+"password=%s
dbname=%s sslmode=disable",dbConfig.Hostname, dbConfig.Port, dbConfig.User,
dbConfig.Password, dbConfig.DbName)
>
> The + is unnecessary, this can be one big string literal.
> ------------------------------
>
> In traffic_ops/experimental/go-api/tenant/EndPointSeeder.go
>
<https://github.com/apache/incubator-trafficcontrol/pull/325#discussion_r107814853>
> :
>
> > +/////BASE - COMMON TO ALL SEEDERS (needs to move to another package)
> +/// Defintions
> +type EndpointSeeder struct{
> + myDb *sql.DB
> +}
> +
> +/// Interface
> +func NewEndpointSeeder(aDb *sql.DB) *EndpointSeeder {
> + endPointSeeder := new(EndpointSeeder)
> + endPointSeeder.myDb = aDb
> + return endPointSeeder
> +}
> +
> +func (aEndpointSeeder *EndpointSeeder) Seed (aRouter *mux.Router,
aApiPrefix string) {
> +
> + aRouter.HandleFunc(aApiPrefix+"/tenants",
aEndpointSeeder.getListEndpoint).Methods("GET")
>
> Consider changing the method strings "GET", "POST", etc to use the
> constants in https://golang.org/pkg/net/http/#pkg-constants so string
> typos are impossible.
>
> â
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
>
<https://github.com/apache/incubator-trafficcontrol/pull/325#pullrequestreview-28801261>,
> or mute the thread
>
<https://github.com/notifications/unsubscribe-auth/AXGkYHnx62mOjo55Wa2nXRSXfT1iTjQSks5roxBWgaJpZM4MPwNL>
> .
>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
