rob05c commented on a change in pull request #2862: Add Traffic Ops Golang federations endpoints URL: https://github.com/apache/trafficcontrol/pull/2862#discussion_r222175241
########## File path: traffic_ops/traffic_ops_golang/federations/allfederations.go ########## @@ -0,0 +1,131 @@ +package federations + +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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. + */ + +import ( + "database/sql" + "errors" + "net/http" + + "github.com/apache/trafficcontrol/lib/go-tc" + "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/api" +) + +func GetAll(w http.ResponseWriter, r *http.Request) { + inf, userErr, sysErr, errCode := api.NewInfo(r, nil, nil) + if userErr != nil || sysErr != nil { + api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr) + return + } + + feds := []FedInfo{} + err := error(nil) + allFederations := []tc.IAllFederation{} + + if cdnParam, ok := inf.Params["cdnName"]; ok { + cdnName := tc.CDNName(cdnParam) + feds, err = getAllFederationsForCDN(inf.Tx.Tx, cdnName) + if err != nil { + api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, errors.New("federations.GetAll getting all federations: "+err.Error())) + return + } + allFederations = append(allFederations, tc.AllFederationCDN{CDNName: &cdnName}) + } else { + feds, err = getAllFederations(inf.Tx.Tx) + if err != nil { + api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, errors.New("federations.GetAll getting all federations by CDN: "+err.Error())) + return + } + } + + fedsResolvers, err := getFederationResolvers(inf.Tx.Tx, fedInfoIDs(feds)) + if err != nil { + api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, errors.New("federations.Get getting federations resolvers: "+err.Error())) + return + } + allFederations = addResolvers(allFederations, feds, fedsResolvers) + + api.WriteResp(w, r, allFederations) +} + +func getAllFederations(tx *sql.Tx) ([]FedInfo, error) { + qry := ` +SELECT + fds.federation, + fd.ttl, + fd.cname, + ds.xml_id +FROM + federation_deliveryservice fds + JOIN deliveryservice ds ON ds.id = fds.deliveryservice + JOIN federation fd ON fd.id = fds.federation +ORDER BY + ds.xml_id +` + rows, err := tx.Query(qry) + if err != nil { + return nil, errors.New("all federations querying: " + err.Error()) + } + defer rows.Close() + + feds := []FedInfo{} + for rows.Next() { + f := FedInfo{} + if err := rows.Scan(&f.ID, &f.TTL, &f.CName, &f.DS); err != nil { + return nil, errors.New("all federations scanning: " + err.Error()) + } + feds = append(feds, f) + } + return feds, nil +} + +func getAllFederationsForCDN(tx *sql.Tx, cdn tc.CDNName) ([]FedInfo, error) { + qry := ` +SELECT + fds.federation, + fd.ttl, + fd.cname, + ds.xml_id +FROM + federation_deliveryservice fds + JOIN deliveryservice ds ON ds.id = fds.deliveryservice + JOIN federation fd ON fd.id = fds.federation + JOIN cdn on cdn.id = ds.cdn_id +WHERE + cdn.name = $1 +ORDER BY + ds.xml_id +` + rows, err := tx.Query(qry, cdn) Review comment: I like the idea of automatically closing, but throwing away type safety with `interface{}` feels like we're just trading one class of bugs for another. >Right now we're kind of all over the place in terms of sql vs sqlx, Query vs Queryx vs NamedQuery, Scan vs StructScan I don't really understand this. Each of those serves a different purpose—the convenience of struct-field mapping, the power of raw SQL, and using native Go instead of incurring the myriad costs of a library where it isn't needed. Insisting on "consistency" and only allowing one or some of those would be like shoving a round peg in a square hole, and limit our power and flexibility. What about something like this? ```Go func WithRows(f func(rows *sql.Rows) error, tx *sql.Tx, query string, args ...interface{}) error { rows, err := tx.Query(query, args...) if err != nil { return errors.New("querying: " + err.Error()) } defer rows.Close() return f(rows) } func WithRowsX(f func(rows *sqlx.Rows) error, tx *sqlx.Tx, query string, args ...interface{}) error {…} ``` ```Go func getUnassignedParametersByProfileID(tx *sql.Tx, profileID int) ([]tc.ProfileParameterByName, error) { qry := `SELECT pa.id, pa.name, pa.value, pa.config_file, pa.secure, pa.last_updated FROM parameter pa WHERE pa.id NOT IN (SELECT parameter FROM profile_parameter pp WHERE pp.profile = $1)` params := []tc.ProfileParameterByName{} err := api.WithRows(func(rows *sql.Rows) error { for rows.Next() { p := tc.ProfileParameterByName{} if err := rows.Scan(&p.ID, &p.Name, &p.Value, &p.ConfigFile, &p.Secure, &p.LastUpdated); err != nil { return errors.New("scanning profile id parameters: " + err.Error()) } params = append(params, p) } return nil }, tx, qry, profileID) if err != nil { return nil, errors.New("querying profile name parameters: " + err.Error()) } return params, nil } ``` That gives you the safety of an enforced `Close`, while still retaining the type safety (notwithstanding the `args` in the standard library), and the flexibility of allowing callers to use `Rows` however they want. I actually wrote something very similar in the Riak library, `func WithClusterTx(tx *sql.Tx, authOpts *riak.AuthOptions, f func(StorageCluster) error) error`: https://github.com/apache/trafficcontrol/blob/master/traffic_ops/traffic_ops_golang/riaksvc/riak_services.go#L265 Personally, I think I decided it wasn't worth the cost. The readability cost is pretty high; it requires additional indentation, which is harder to read; it requires programmers be familiar with first-class functions, which are difficult for a lot of people coming from imperative programming backgrounds to get a handle on; and it's just un-idiomatic Go, the Go language doesn't have destructors, and the `Closer` interface is common in the standard library, unsafe or not, using lambdas to hack in destructors just feels unexpected and not simple and easy code, and that much more difficult for a new programmer on the project to learn. But, that's my personal feeling. And I'm not strongly opposed, the pattern just feels a little worse, not a lot worse. I certainly agree with the value of automatically closing. If you feel strongly about it, I don't know that I'll give you a +1, but I probably won't -1 it. I would really like to see the type safety preserved, though. Incidentally, we can do the same thing with `APIInfo`: ```Go // WithInfo executes the given function f, with an APIInfo object containing the commonly needed API data. // The http.Request r, requiredParams, and intParams are used to create the APIInfo object, parsing the parameters from the list of required parameters and integer parameters. If there is an error, for example creating the transaction, or if the user didn't pass required parameters, an appropriate error is returned to the client and f is not called. // The http.ResponseWriter w is used to write the error to the client, if there is an error getting the APIInfo data, or if the func f returns an error. // If f returns a non-nil error, the appropriate error is written to the client. The func f must return nil errors if it has written to the client, and likewise must return a non-nil error if a response was not written to the client. func WithInfo(w http.ResponseWriter, r *http.Request, requiredParams []string, intParams []string, f func(inf *APIInfo) (error, error, int)) { inf, userErr, sysErr, errCode := NewInfo(r, []string{"id"}, []string{"id"}) if userErr != nil || sysErr != nil { HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr) return } defer inf.Close() userErr, sysErr, errCode = f(inf) if userErr != nil || sysErr != nil { HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr) return } } ``` ```Go func GetUnassigned(w http.ResponseWriter, r *http.Request) { api.WithInfo(w, r, []string{"id"}, []string{"id"}, func(inf *api.APIInfo) (error, error, int) { params, err := getUnassignedParametersByProfileID(inf.Tx.Tx, inf.IntParams["id"]) if err != nil { return nil, errors.New("getting params: " + err.Error()), http.StatusInternalServerError } api.WriteResp(w, r, params) return nil, nil, http.StatusOK }) } ``` ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
