rob05c commented on a change in pull request #4942:
URL: https://github.com/apache/trafficcontrol/pull/4942#discussion_r504978133
##########
File path: traffic_ops/traffic_ops_golang/api/api.go
##########
@@ -999,6 +999,44 @@ func CreateDeprecationAlerts(alternative *string)
tc.Alerts {
}
}
+// GetLastUpdated checks for the resource in the database, and returns its
last_updated timestamp, if available.
+func GetLastUpdated(tx *sqlx.Tx, ID int, tableName string) (*time.Time, bool,
error) {
+ lastUpdated := time.Time{}
+ found := false
+ rows, err := tx.Query(`select last_updated from `+tableName+` where
id=$1`, ID)
Review comment:
Would you mind adding `pq.QuoteIdentifier(tableName)` here?
This makes me really nervous. It could very easily become a SQL Injection
vulnerability. I know the code here only ever passes constant strings, but it'd
be very easy for someone to add a function or endpoint in the future that
passed user input like a query parameter.
Again, I know it isn't technically necessary, but it's a tiny computational
cost, to prevent future SQL Injection.
##########
File path: traffic_ops/traffic_ops_golang/api/api.go
##########
@@ -999,6 +999,44 @@ func CreateDeprecationAlerts(alternative *string)
tc.Alerts {
}
}
+// GetLastUpdated checks for the resource in the database, and returns its
last_updated timestamp, if available.
+func GetLastUpdated(tx *sqlx.Tx, ID int, tableName string) (*time.Time, bool,
error) {
+ lastUpdated := time.Time{}
+ found := false
+ rows, err := tx.Query(`select last_updated from `+tableName+` where
id=$1`, ID)
+ if err != nil {
+ return nil, found, errors.New("querying last_updated: " +
err.Error())
+ }
+ defer rows.Close()
+ if !rows.Next() {
+ return nil, found, nil
+ }
+ found = true
+ if err := rows.Scan(&lastUpdated); err != nil {
+ return nil, found, errors.New("scanning last_updated: " +
err.Error())
+ }
+ return &lastUpdated, found, nil
+}
+
+// IsUnmodified returns a boolean, saying whether or not the resource in
question was modified since the time specified in the headers.
+func IsUnmodified(h http.Header, lastUpdated time.Time) bool {
+ unmodifiedTime, ok := rfc.GetUnmodifiedTime(h)
+ if !ok {
+ return true // no IUS/IM header: unmodified, proceed with
normal update
+ }
+ return !lastUpdated.After(unmodifiedTime)
+}
+
+// FormatLastModified trims the time string and formats it according to RFC1123
+func FormatLastModified(t time.Time) string {
+ return rfc.FormatHTTPDate(t.Truncate(time.Second).Add(time.Second))
+}
+
+// AddLastModifiedHdr adds the "last modified" header to the response
Review comment:
Nitpick: GoDoc, `.`
##########
File path: traffic_ops/traffic_ops_golang/api/api.go
##########
@@ -999,6 +999,44 @@ func CreateDeprecationAlerts(alternative *string)
tc.Alerts {
}
}
+// GetLastUpdated checks for the resource in the database, and returns its
last_updated timestamp, if available.
+func GetLastUpdated(tx *sqlx.Tx, ID int, tableName string) (*time.Time, bool,
error) {
+ lastUpdated := time.Time{}
+ found := false
+ rows, err := tx.Query(`select last_updated from `+tableName+` where
id=$1`, ID)
+ if err != nil {
+ return nil, found, errors.New("querying last_updated: " +
err.Error())
+ }
+ defer rows.Close()
+ if !rows.Next() {
+ return nil, found, nil
+ }
+ found = true
+ if err := rows.Scan(&lastUpdated); err != nil {
+ return nil, found, errors.New("scanning last_updated: " +
err.Error())
+ }
+ return &lastUpdated, found, nil
+}
+
+// IsUnmodified returns a boolean, saying whether or not the resource in
question was modified since the time specified in the headers.
+func IsUnmodified(h http.Header, lastUpdated time.Time) bool {
+ unmodifiedTime, ok := rfc.GetUnmodifiedTime(h)
+ if !ok {
+ return true // no IUS/IM header: unmodified, proceed with
normal update
+ }
+ return !lastUpdated.After(unmodifiedTime)
+}
+
+// FormatLastModified trims the time string and formats it according to RFC1123
Review comment:
Nitpick: GoDoc comments need to be full sentences, needs a trailing `.`.
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -789,6 +789,18 @@ func updateV30(w http.ResponseWriter, r *http.Request, inf
*api.APIInfo, ds *tc.
deepCachingType = ds.DeepCachingType.String() // necessary,
because DeepCachingType's default needs to insert the string, not "", and Query
doesn't call .String().
}
+ existingLastUpdated, found, err := api.GetLastUpdated(inf.Tx, *ds.ID,
"deliveryservice")
+ if err == nil && found == false {
+ return nil, http.StatusNotFound, errors.New("no deliveryservice
found with this id"), nil
+ }
+ if err != nil {
+ return nil, http.StatusInternalServerError, nil, err
Review comment:
Nitpick: Error context would make this easier to debug, e.g. `return
nil, http.StatusInternalServerError, nil, errors.New("getting last updated: " +
err.Error())`
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/request/requests.go
##########
@@ -383,6 +387,17 @@ func (req *deliveryServiceRequestAssignment) Update()
(error, error, int) {
req.DeliveryServiceRequestNullable =
current.DeliveryServiceRequestNullable
req.AssigneeID = assigneeID
+ existingLastUpdated, found, err := api.GetLastUpdated(req.ReqInfo.Tx,
*req.ID, "deliveryservice_request")
+ if err == nil && found == false {
+ return errors.New("no delivery service request found with this
id"), nil, http.StatusNotFound
+ }
+ if err != nil {
+ return nil, err, http.StatusInternalServerError
Review comment:
Nitpick: error context, `return nil, http.StatusInternalServerError,
nil, errors.New("getting last updated: " + err.Error())`
##########
File path: lib/go-rfc/cachecontrol_test.go
##########
@@ -0,0 +1,44 @@
+package rfc
+
+import (
+ "testing"
+ "time"
+)
+
+/*
Review comment:
Nitpick: the license header should go immediately after the `package`,
but before the `import`.
##########
File path: traffic_ops/traffic_ops_golang/cachegroup/cachegroups.go
##########
@@ -531,6 +532,17 @@ func (cg *TOCacheGroup) Update() (error, error, int) {
cg.FallbackToClosest = &fbc
}
+ existingLastUpdated, found, e := api.GetLastUpdated(cg.ReqInfo.Tx,
*cg.ID, "cachegroup")
Review comment:
Nitpick: `err`. The Go idiom is for errors to be named `err`, and for
variable name length to match scope, i.e. one-letter variables should only be
used in extremely small scopes, like a 2-line if-statement.
----------------------------------------------------------------
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]
