zrhoffman commented on a change in pull request #5071:
URL: https://github.com/apache/trafficcontrol/pull/5071#discussion_r501175965
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/request/requests.go
##########
@@ -20,451 +22,843 @@ package request
*/
import (
+ "database/sql"
+ "encoding/json"
"errors"
"fmt"
-
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/util/ims"
"net/http"
- "strconv"
+ "strings"
"time"
+
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/util/ims"
+
"github.com/apache/trafficcontrol/lib/go-log"
+ "github.com/apache/trafficcontrol/lib/go-rfc"
"github.com/apache/trafficcontrol/lib/go-tc"
"github.com/apache/trafficcontrol/lib/go-util"
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/api"
- "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/auth"
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/dbhelpers"
+
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/deliveryservice"
+
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/routing/middleware"
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/tenant"
"github.com/jmoiron/sqlx"
+ "github.com/lib/pq"
)
-// TODeliveryServiceRequest is the type alias to define functions on
-type TODeliveryServiceRequest struct {
- api.APIInfoImpl `json:"-"`
- tc.DeliveryServiceRequestNullable
-}
+const selectQuery = `
+SELECT
+ a.username AS author,
+ e.username AS lastEditedBy,
+ s.username AS assignee,
+ r.assignee_id,
+ r.author_id,
+ r.change_type,
+ r.created_at,
+ r.id,
+ r.last_edited_by_id,
+ r.last_updated,
+ r.deliveryservice,
+ r.original,
+ r.status
+FROM deliveryservice_request r
+JOIN tm_user a ON r.author_id = a.id
+LEFT OUTER JOIN tm_user s ON r.assignee_id = s.id
+LEFT OUTER JOIN tm_user e ON r.last_edited_by_id = e.id
+`
-func (v *TODeliveryServiceRequest) SetLastUpdated(t tc.TimeNoMod) {
v.LastUpdated = &t }
-func (v *TODeliveryServiceRequest) InsertQuery() string { return
insertRequestQuery() }
-func (v *TODeliveryServiceRequest) UpdateQuery() string { return
updateRequestQuery() }
-func (v *TODeliveryServiceRequest) DeleteQuery() string {
- return `DELETE FROM deliveryservice_request WHERE id = :id`
-}
+const originalsQuery = deliveryservice.SelectDeliveryServicesQuery + `
+WHERE ds.id = ANY(:ids)
+`
-func (req TODeliveryServiceRequest) GetKeyFieldsInfo() []api.KeyFieldInfo {
- return []api.KeyFieldInfo{{"id", api.GetIntKey}}
-}
+const insertQuery = `
+INSERT INTO deliveryservice_request (
+ assignee_id,
+ author_id,
+ change_type,
+ last_edited_by_id,
+ deliveryservice,
+ original,
+ status
+) VALUES (
+ $1,
+ $2,
+ $3,
+ $2,
+ $4,
+ $5,
+ $6
+)
+RETURNING
+ id,
+ last_updated,
+ created_at
+`
-func (req TODeliveryServiceRequest) GetKeys() (map[string]interface{}, bool) {
- if req.ID == nil {
- return map[string]interface{}{"id": 0}, false
- }
- return map[string]interface{}{"id": *req.ID}, true
-}
+const updateQuery = `
+UPDATE deliveryservice_request
+SET
+ assignee_id = $1,
+ change_type = $2,
+ last_edited_by_id = $3,
+ deliveryservice = $4,
+ original = $5,
+ status = $6
+WHERE id = $7
+RETURNING
+ last_updated,
+ created_at
+`
-func (req *TODeliveryServiceRequest) SetKeys(keys map[string]interface{}) {
- i, _ := keys["id"].(int) //this utilizes the non panicking type
assertion, if the thrown away ok variable is false i will be the zero of the
type, 0 here.
- req.ID = &i
-}
+const deleteQuery = `
+DELETE
+FROM deliveryservice_request
+WHERE id=$1
+`
-// GetAuditName is part of the tc.Identifier interface
-func (req TODeliveryServiceRequest) GetAuditName() string {
- return req.getXMLID()
+//TODO: figure out how to modify 'AddTenancyCheck' so this isn't necessary
+const customTenancyCheck = `(
+ CASE r.change_type
+ WHEN 'delete' THEN CAST(r.original->>'tenantId' AS bigint) =
ANY(CAST(:accessibleTenants AS bigint[]))
+ ELSE CAST(r.deliveryservice->>'tenantId' AS bigint) =
ANY(CAST(:accessibleTenants AS bigint[]))
+ END
+)`
+
+func selectMaxLastUpdatedQuery(where string) string {
+ return `SELECT max(t) from (
+ SELECT max(r.last_updated) as t FROM deliveryservice_request r
+ JOIN tm_user a ON r.author_id = a.id
+ LEFT OUTER JOIN tm_user s ON r.assignee_id = s.id
+ LEFT OUTER JOIN tm_user e ON r.last_edited_by_id = e.id ` + where +
+ ` UNION ALL
+ select max(last_updated) as t from last_deleted l where
l.table_name='deliveryservice_request') as res`
}
-// GetType is part of the tc.Identifier interface
-func (req TODeliveryServiceRequest) GetType() string {
- return "deliveryservice_request"
+// getOriginals fetches the Delivery Services identified in 'ids' and sets
+// them as originals on the Delivery Services to which each ID maps in
+// needOriginals. It returns a response code to use if an error occurred, in
+// which case it also returns a user error and a system error.
+func getOriginals(ids []int, tx *sqlx.Tx, needOriginals
map[int][]*tc.DeliveryServiceRequestV30) (int, error, error) {
+ if len(ids) > 0 {
+ originals, userErr, sysErr, errCode :=
deliveryservice.GetDeliveryServices(originalsQuery,
map[string]interface{}{"ids": pq.Array(ids)}, tx)
+ if userErr != nil || sysErr != nil {
+ return errCode, userErr, sysErr
+ }
+
+ for _, original := range originals {
+ if original.ID == nil {
+ log.Warnf("Trying to fill in originals: found
Delivery Service with no ID")
+ } else if need, ok := needOriginals[*original.ID]; ok {
+ for _, n := range need {
+ n.Original = new(tc.DeliveryServiceV30)
+ *n.Original = original
+ }
+ } else {
+ log.Warnf("Trying to fill in originals: found
Delivery Service that wasn't identified by a DSR (#%d)", *original.ID)
+ }
+ }
+ }
+ return http.StatusOK, nil, nil
}
-// Read implements the api.Reader interface
-func (req *TODeliveryServiceRequest) Read(h http.Header, useIMS bool)
([]interface{}, error, error, int, *time.Time) {
- var maxTime time.Time
- var runSecond bool
- deliveryServiceRequests := []interface{}{}
+// Get is the GET handler for /deliveryservice_requests.
+func Get(w http.ResponseWriter, r *http.Request) {
+ inf, userErr, sysErr, errCode := api.NewInfo(r, nil, nil)
+ tx := inf.Tx.Tx
+ if userErr != nil || sysErr != nil {
+ api.HandleErr(w, r, tx, errCode, userErr, sysErr)
+ return
+ }
+ defer inf.Close()
+
+ // Middleware should've already handled this, so idk why this is a
pointer at all tbh
+ version := inf.Version
+ if version == nil {
+ middleware.NotImplementedHandler().ServeHTTP(w, r)
+ return
+ }
+
queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{
- "assignee": dbhelpers.WhereColumnInfo{Column: "s.username"},
- "assigneeId": dbhelpers.WhereColumnInfo{Column:
"r.assignee_id", Checker: api.IsInt},
- "author": dbhelpers.WhereColumnInfo{Column: "a.username"},
- "authorId": dbhelpers.WhereColumnInfo{Column: "r.author_id",
Checker: api.IsInt},
- "changeType": dbhelpers.WhereColumnInfo{Column:
"r.change_type"},
- "id": dbhelpers.WhereColumnInfo{Column: "r.id",
Checker: api.IsInt},
- "status": dbhelpers.WhereColumnInfo{Column: "r.status"},
- "xmlId": dbhelpers.WhereColumnInfo{Column:
"r.deliveryservice->>'xmlId'"},
- }
-
- p := req.APIInfo().Params
- if _, ok := req.APIInfo().Params["orderby"]; !ok {
- // if orderby not provided, default to orderby xmlId. Making a
copy of parameters to not modify input arg
- p = make(map[string]string, len(req.APIInfo().Params))
- for k, v := range req.APIInfo().Params {
- p[k] = v
- }
- p["orderby"] = "xmlId"
+ "assignee": {Column: "s.username"},
+ "assigneeId": {Column: "r.assignee_id", Checker: api.IsInt},
+ "author": {Column: "a.username"},
+ "authorId": {Column: "r.author_id", Checker: api.IsInt},
+ "changeType": {Column: "r.change_type"},
+ "id": {Column: "r.id", Checker: api.IsInt},
+ "status": {Column: "r.status"},
+ }
+ if _, ok := inf.Params["orderby"]; !ok {
+ inf.Params["orderby"] = "xmlId"
}
- where, orderBy, pagination, queryValues, errs :=
dbhelpers.BuildWhereAndOrderByAndPagination(p, queryParamsToQueryCols)
+ where, orderBy, pagination, queryValues, errs :=
dbhelpers.BuildWhereAndOrderByAndPagination(inf.Params, queryParamsToQueryCols)
if len(errs) > 0 {
- return nil, util.JoinErrs(errs), nil, http.StatusBadRequest, nil
+ api.HandleErr(w, r, tx, http.StatusBadRequest,
util.JoinErrs(errs), nil)
+ return
}
- if useIMS {
- runSecond, maxTime =
ims.TryIfModifiedSinceQuery(req.APIInfo().Tx, h, queryValues,
selectMaxLastUpdatedQuery(where))
+
+ // TODO: add this functionality to the query builder in dbhelpers
+ if xmlID, ok := inf.Params["xmlId"]; ok {
+ queryValues["xmlId"] = xmlID
+ if where != "" {
+ where += " AND "
+ } else {
+ where = "WHERE "
+ }
+ where += "(r.deliveryservice->>'xmlId' = :xmlId OR
r.original->>'xmlId' = :xmlId)"
+ }
+
+ maxTime := new(time.Time)
+ if inf.UseIMS(r) {
+ var runSecond bool
+ runSecond, *maxTime = ims.TryIfModifiedSinceQuery(inf.Tx,
r.Header, queryValues, selectMaxLastUpdatedQuery(where))
if !runSecond {
log.Debugln("IMS HIT")
- return deliveryServiceRequests, nil, nil,
http.StatusNotModified, &maxTime
+ api.WriteIMSHitResp(w, r, *maxTime)
+ return
}
log.Debugln("IMS MISS")
} else {
log.Debugln("Non IMS request")
}
- tenantIDs, err := tenant.GetUserTenantIDListTx(req.APIInfo().Tx.Tx,
req.APIInfo().User.TenantID)
+
+ tenantIDs, err := tenant.GetUserTenantIDListTx(tx, inf.User.TenantID)
if err != nil {
- return nil, nil, errors.New("dsr getting tenant list: " +
err.Error()), http.StatusInternalServerError, nil
+ sysErr = fmt.Errorf("dsr getting tenant list: %v", err)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil,
sysErr)
+ return
+ }
+
+ if where == "" {
+ where = "WHERE "
+ } else {
+ where += " AND "
}
- where, queryValues = dbhelpers.AddTenancyCheck(where, queryValues,
"CAST(r.deliveryservice->>'tenantId' AS bigint)", tenantIDs)
+ where += customTenancyCheck
+ queryValues["accessibleTenants"] = pq.Array(tenantIDs)
- query := selectDeliveryServiceRequestsQuery() + where + orderBy +
pagination
+ query := selectQuery + where + orderBy + pagination
log.Debugln("Query is ", query)
- rows, err := req.APIInfo().Tx.NamedQuery(query, queryValues)
+ rows, err := inf.Tx.NamedQuery(query, queryValues)
if err != nil {
- return nil, nil, errors.New("dsr querying: " + err.Error()),
http.StatusInternalServerError, &maxTime
+ sysErr = fmt.Errorf("dsr querying: %v", err)
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil,
sysErr)
+ return
}
defer rows.Close()
+ dsrs := []tc.DeliveryServiceRequestV30{}
+ needOriginals := map[int][]*tc.DeliveryServiceRequestV30{}
+ var originalIDs []int
for rows.Next() {
- var s TODeliveryServiceRequest
- if err = rows.StructScan(&s); err != nil {
- return nil, nil, errors.New("dsr scanning: " +
err.Error()), http.StatusInternalServerError, &maxTime
+ var dsr tc.DeliveryServiceRequestV30
+ if err = rows.StructScan(&dsr); err != nil {
+ api.HandleErr(w, r, tx, http.StatusInternalServerError,
nil, fmt.Errorf("dsr scanning: %v", err))
+ return
+ }
+ dsrs = append(dsrs, dsr)
+
+ if dsr.IsOpen() && dsr.ChangeType != tc.DSRChangeTypeCreate {
+ if dsr.ChangeType == tc.DSRChangeTypeUpdate &&
dsr.Requested != nil && dsr.Requested.ID != nil {
+ id := *dsr.Requested.ID
+ if _, ok := needOriginals[id]; !ok {
+ needOriginals[id] =
[]*tc.DeliveryServiceRequestV30{&dsrs[len(dsrs)-1]}
+ } else {
+ needOriginals[id] =
append(needOriginals[id], &dsrs[len(dsrs)-1])
+ }
+ originalIDs = append(originalIDs, id)
+ } else if dsr.ChangeType == tc.DSRChangeTypeDelete &&
dsr.Original != nil && dsr.Original.ID != nil {
+ id := *dsr.Original.ID
+ if _, ok := needOriginals[id]; !ok {
+ needOriginals[id] =
[]*tc.DeliveryServiceRequestV30{&dsrs[len(dsrs)-1]}
+ } else {
+ needOriginals[id] =
append(needOriginals[id], &dsrs[len(dsrs)-1])
+ }
+ originalIDs = append(originalIDs, id)
+ }
}
- deliveryServiceRequests = append(deliveryServiceRequests, s)
}
- return deliveryServiceRequests, nil, nil, http.StatusOK, &maxTime
-}
+ if maxTime != nil {
+ w.Header().Set(rfc.LastModified,
maxTime.Format(rfc.LastModifiedFormat))
+ }
-func selectMaxLastUpdatedQuery(where string) string {
- return `SELECT max(t) from (
- SELECT max(r.last_updated) as t FROM deliveryservice_request r
- JOIN tm_user a ON r.author_id = a.id
- LEFT OUTER JOIN tm_user s ON r.assignee_id = s.id
- LEFT OUTER JOIN tm_user e ON r.last_edited_by_id = e.id ` + where +
- ` UNION ALL
- select max(last_updated) as t from last_deleted l where
l.table_name='deliveryservice_request') as res`
-}
+ if version.Major >= 3 {
+ errCode, userErr, sysErr = getOriginals(originalIDs, inf.Tx,
needOriginals)
+ if userErr != nil || sysErr != nil {
+ api.HandleErr(w, r, tx, errCode, userErr, sysErr)
+ } else {
+ api.WriteResp(w, r, dsrs)
+ }
+ return
+ }
-func selectDeliveryServiceRequestsQuery() string {
-
- query := `SELECT
-a.username AS author,
-e.username AS lastEditedBy,
-s.username AS assignee,
-r.assignee_id,
-r.author_id,
-r.change_type,
-r.created_at,
-r.id,
-r.last_edited_by_id,
-r.last_updated,
-r.deliveryservice,
-r.status,
-r.deliveryservice->>'xmlId' as xml_id
+ downgraded := make([]tc.DeliveryServiceRequestV15, 0, len(dsrs))
+ for _, dsr := range dsrs {
+ downgraded = append(downgraded, dsr.Downgrade())
+ }
-FROM deliveryservice_request r
-JOIN tm_user a ON r.author_id = a.id
-LEFT OUTER JOIN tm_user s ON r.assignee_id = s.id
-LEFT OUTER JOIN tm_user e ON r.last_edited_by_id = e.id
-`
- return query
+ api.WriteResp(w, r, downgraded)
}
-// IsTenantAuthorized implements the Tenantable interface to ensure the user
is authorized on the deliveryservice tenant
-func (req TODeliveryServiceRequest) IsTenantAuthorized(user *auth.CurrentUser)
(bool, error) {
+// isTenantAuthorized ensures the user is authorized on the DSR's Requested
+// and/or Original Delivery Service's Tenant, as appropriate to the change
type.
+func isTenantAuthorized(dsr tc.DeliveryServiceRequestV30, inf *api.APIInfo)
(bool, error) {
+ if dsr.Requested != nil && (dsr.ChangeType == tc.DSRChangeTypeUpdate ||
dsr.ChangeType == tc.DSRChangeTypeCreate) {
+ if dsr.Requested.TenantID == nil {
+ log.Debugf("requested.tenantID is nil")
+ return false, errors.New("requested.tenantID is nil")
+ }
+ ok, err :=
tenant.IsResourceAuthorizedToUserTx(*dsr.Requested.TenantID, inf.User,
inf.Tx.Tx)
+ if err != nil {
+ err = fmt.Errorf("requested: %v", err)
+ }
+ if !ok || err != nil {
+ return ok, err
+ }
+ }
- ds := req.DeliveryService
- if ds == nil {
- // No deliveryservice applied yet -- wide open
+ ds := dsr.Original
+ if ds == nil || dsr.ChangeType == tc.DSRChangeTypeCreate {
+ // No deliveryservice applied yet or change type doesn't
require an original
return true, nil
}
+
if ds.TenantID == nil {
- log.Debugf("tenantID is nil")
- return false, errors.New("tenantID is nil")
+ log.Debugf("original.tenantID is nil")
+ return false, errors.New("original.tenantID is nil")
+ }
+ ok, err := tenant.IsResourceAuthorizedToUserTx(*ds.TenantID, inf.User,
inf.Tx.Tx)
+ if err != nil {
+ err = fmt.Errorf("original: %v", err)
}
- return tenant.IsResourceAuthorizedToUserTx(*ds.TenantID, user,
req.APIInfo().Tx.Tx)
+ return ok, err
}
-// Update implements the tc.Updater interface.
-//all implementations of Updater should use transactions and return the proper
errorType
-//ParsePQUniqueConstraintError is used to determine if a request with
conflicting values exists
-//if so, it will return an errorType of DataConflict and the type should be
appended to the
-//generic error message returned
-func (req *TODeliveryServiceRequest) Update() (error, error, int) {
- if req.ID == nil {
- return errors.New("missing id"), nil, http.StatusBadRequest
+// Warning: this assumes inf isn't nil, and neither is dsr, inf.Tx or inf.User
or inf.Tx.Tx.
+func insert(dsr *tc.DeliveryServiceRequestV30, inf *api.APIInfo) (int, error,
error) {
Review comment:
IMO `(error, error, int)` should be exempt from that rule because an
HTTP status code is effectively an error code. I can see why you would choose
the order that you did though,̵ ̵~so go with what you think is best~.
It would be nice to enforce linting at some point. In order for
`golangci-lint` to ignore that particular `golant` rule, we should exclude it
in `issues.exclude-rules` in our `.golangci.yml`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
