rob05c commented on a change in pull request #4942:
URL: https://github.com/apache/trafficcontrol/pull/4942#discussion_r485139802
##########
File path: traffic_ops/traffic_ops_golang/api/api.go
##########
@@ -994,3 +994,36 @@ func CreateDeprecationAlerts(alternative *string)
tc.Alerts {
return tc.CreateAlerts(tc.WarnLevel, "This endpoint is
deprecated, and will be removed in the future")
}
}
+
+// 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) (*tc.TimeNoMod,
error) {
+ lastUpdated := tc.TimeNoMod{}
+ rows, err := tx.Query(`select last_updated from `+tableName+` where
id=$1`, ID)
+ if err != nil {
+ return nil, err
Review comment:
Nitpick: adding the context that this was a query would make it easier
to debug, e.g. `return nil, errors.New("querying: " + err.Error())`
##########
File path: traffic_ops/traffic_ops_golang/api/generic_crud.go
##########
@@ -56,6 +57,7 @@ type GenericUpdater interface {
APIInfo() *APIInfo
SetLastUpdated(tc.TimeNoMod)
UpdateQuery() string
+ GetLastUpdated() (*tc.TimeNoMod, error)
Review comment:
As above `TimeNoMod` is a JSON thing. Is there a reason not to use
`time.Time` here?
##########
File path: traffic_ops/traffic_ops_golang/api/shared_handlers.go
##########
@@ -167,8 +168,9 @@ func ReadHandler(reader Reader) http.HandlerFunc {
return
}
if maxTime != nil {
+ truncatedTime :=
maxTime.Truncate(time.Second).Add(time.Second)
// RFC1123
- date := maxTime.Format("Mon, 02 Jan 2006 15:04:05 MST")
+ date := truncatedTime.Format("Mon, 02 Jan 2006 15:04:05
MST")
Review comment:
`rfc.FormatHTTPDate` ?
##########
File path: traffic_ops/traffic_ops_golang/api/api.go
##########
@@ -994,3 +994,36 @@ func CreateDeprecationAlerts(alternative *string)
tc.Alerts {
return tc.CreateAlerts(tc.WarnLevel, "This endpoint is
deprecated, and will be removed in the future")
}
}
+
+// 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) (*tc.TimeNoMod,
error) {
+ lastUpdated := tc.TimeNoMod{}
+ rows, err := tx.Query(`select last_updated from `+tableName+` where
id=$1`, ID)
+ if err != nil {
+ return nil, err
+ }
+ defer rows.Close()
+ if !rows.Next() {
+ return nil, errors.New("no resource found with this id")
+ }
+ if err := rows.Scan(&lastUpdated); err != nil {
+ return nil, err
Review comment:
Nitpick: context for debugging, `return nil, errors.New("scanning: " +
err.Error())`
##########
File path: traffic_ops/traffic_ops_golang/servicecategory/servicecategories.go
##########
@@ -39,6 +39,22 @@ type TOServiceCategory struct {
tc.ServiceCategory
}
+func (v *TOServiceCategory) GetLastUpdated() (*tc.TimeNoMod, error) {
+ lastUpdated := tc.TimeNoMod{}
+ rows, err := v.APIInfo().Tx.Query(`select last_updated from
service_category where name=$1`, v.Name)
+ if err != nil {
+ return nil, err
+ }
+ defer rows.Close()
+ if !rows.Next() {
+ return nil, errors.New("no resource found with this id")
Review comment:
distinguish db err, `(time.time, bool, error)`
##########
File path: traffic_ops/traffic_ops_golang/steeringtargets/steeringtargets.go
##########
@@ -268,6 +276,22 @@ func (st *TOSteeringTargetV11) Update() (error, error,
int) {
return nil, nil, http.StatusOK
}
+func CheckIfExistsBeforeUpdate(tx *sqlx.Tx, st *TOSteeringTargetV11) (error,
*tc.TimeNoMod) {
+ lastUpdated := tc.TimeNoMod{}
+ rows, err := tx.NamedQuery(`select last_updated from steering_target
where deliveryservice=:deliveryservice and target=:target`, st)
+ if err != nil {
+ return err, nil
+ }
+ defer rows.Close()
+ if !rows.Next() {
+ return errors.New("no server found with this id"), nil
+ }
+ if err := rows.Scan(&lastUpdated); err != nil {
+ return err, nil
Review comment:
Debugging context, return nil, errors.New("scanning: " + err.Error())
##########
File path: traffic_ops/traffic_ops_golang/user/user.go
##########
@@ -265,6 +266,13 @@ func (user *TOUser) Update() (error, error, int) {
return nil, err, http.StatusInternalServerError
}
}
+ existingLastUpdated, err := api.GetLastUpdated(user.ReqInfo.Tx,
*user.ID, "tm_user")
+ if err != nil {
+ return errors.New("no server found with this id"), nil,
http.StatusNotFound
Review comment:
distinguish db err, `(time.Time, bool, error)`
##########
File path: traffic_ops/traffic_ops_golang/api/api.go
##########
@@ -994,3 +994,36 @@ func CreateDeprecationAlerts(alternative *string)
tc.Alerts {
return tc.CreateAlerts(tc.WarnLevel, "This endpoint is
deprecated, and will be removed in the future")
}
}
+
+// 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) (*tc.TimeNoMod,
error) {
Review comment:
The `tc.TimeNoMod` struct exists for JSON reasons. Can this just return
a `time.Time`?
##########
File path: traffic_ops/traffic_ops_golang/api/shared_handlers.go
##########
@@ -167,8 +168,9 @@ func ReadHandler(reader Reader) http.HandlerFunc {
return
}
if maxTime != nil {
+ truncatedTime :=
maxTime.Truncate(time.Second).Add(time.Second)
// RFC1123
- date := maxTime.Format("Mon, 02 Jan 2006 15:04:05 MST")
+ date := truncatedTime.Format("Mon, 02 Jan 2006 15:04:05
MST")
Review comment:
This could be `date := rfc.FormatHTTPDate(truncatedTime)` to avoid the
magic string
##########
File path: traffic_ops/traffic_ops_golang/api/api.go
##########
@@ -994,3 +994,36 @@ func CreateDeprecationAlerts(alternative *string)
tc.Alerts {
return tc.CreateAlerts(tc.WarnLevel, "This endpoint is
deprecated, and will be removed in the future")
}
}
+
+// 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) (*tc.TimeNoMod,
error) {
+ lastUpdated := tc.TimeNoMod{}
+ rows, err := tx.Query(`select last_updated from `+tableName+` where
id=$1`, ID)
+ if err != nil {
+ return nil, err
+ }
+ defer rows.Close()
+ if !rows.Next() {
+ return nil, errors.New("no resource found with this id")
Review comment:
If anyone ever needs to check this, e.g. to determine whether to return
a 4xx or 5xx response, the check will be fragile and expensive.
Is it worth making this func return a `(time.Time, bool, error)`, where the
boolean is whether it existed or not?
##########
File path: traffic_ops/traffic_ops_golang/cachegroup/cachegroups.go
##########
@@ -531,6 +532,14 @@ func (cg *TOCacheGroup) Update() (error, error, int) {
cg.FallbackToClosest = &fbc
}
+ existingLastUpdated, e := api.GetLastUpdated(cg.ReqInfo.Tx, *cg.ID,
"cachegroup")
+ if e != nil {
+ return errors.New("no cachegroup found with this id"), nil,
http.StatusNotFound
Review comment:
As above, the error might not be a not-found, `api.GetLastUpdated`
should return a boolean, so this can return the proper user or system error.
##########
File path: traffic_ops/traffic_ops_golang/api/generic_crud.go
##########
@@ -223,7 +225,15 @@ func GenericRead(h http.Header, val GenericReader, useIMS
bool) ([]interface{},
}
// GenericUpdate handles the common update case, where the update returns the
new last_modified time.
-func GenericUpdate(val GenericUpdater) (error, error, int) {
+func GenericUpdate(h http.Header, val GenericUpdater) (error, error, int) {
+ existingLastUpdated, err := val.GetLastUpdated()
+ if err != nil {
+ return errors.New("no " + val.GetType() + " found with this
id"), err, http.StatusNotFound
Review comment:
This is assuming any error was the doesn't-exist error. This will lead
to some very confusing debugging, if a real database error occurs.
Maybe `GetLastUpdated` should return a `(time.Time, bool, error)` where the
boolean is whether it existed? Then this can return the appropriate user and
system errors
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -779,6 +780,14 @@ 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, err := api.GetLastUpdated(inf.Tx, *ds.ID,
"deliveryservice")
+ if err != nil {
+ return nil, http.StatusNotFound, errors.New("no deliveryservice
found with this id"), nil
Review comment:
As above, the error might not be a not-found, `api.GetLastUpdated`
should return a boolean, so this can return the proper user or system error
##########
File path: lib/go-rfc/cachecontrol.go
##########
@@ -0,0 +1,113 @@
+package rfc
+
+import (
+ "errors"
+ "net/http"
+ "strconv"
+ "strings"
+ "time"
+)
+
+/*
+ * 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.
+ */
+
+const (
+ IfModifiedSince = "If-Modified-Since" // RFC7232§3.3
+ LastModified = "Last-Modified" // RFC7232§2.2
+ ETagHeader = "ETag"
+ IfMatch = "If-Match"
+ IfUnmodifiedSince = "If-Unmodified-Since"
+ ETagVersion = 1
+)
+
+// ETag takes the last time the object was modified, and returns an ETag
string. Note the string is the complete header value, including quotes. ETags
must be quoted strings.
+func ETag(t time.Time) string {
+ return `"v` + strconv.Itoa(ETagVersion) + `-` +
strconv.FormatInt(t.UnixNano(), 36) + `"`
+}
+
+// GetETagOrIfUnmodifiedSinceTime parses the http header and returns a list of
Etags/ an "if-unmodified-since" time to compare to, in that order.
+func GetETagOrIfUnmodifiedSinceTime(h http.Header) ([]string, *time.Time) {
Review comment:
I'm not sure I understand the purpose of this, or its helper
`ParseEtagsList`.
It seems like we're parsing the ETag format, then re-serializing it in a
very particular format, and returning that? What's the purpose of that?
It looks like this PR never uses that. And it seems like any code would
always need the time, and it could easily format it if necessary.
For now, should this just be changed to `GetIfUnmodifiedSince` and remove
the unused ETag stuff?
##########
File path: traffic_ops/traffic_ops_golang/api/api.go
##########
@@ -994,3 +994,36 @@ func CreateDeprecationAlerts(alternative *string)
tc.Alerts {
return tc.CreateAlerts(tc.WarnLevel, "This endpoint is
deprecated, and will be removed in the future")
}
}
+
+// 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) (*tc.TimeNoMod,
error) {
+ lastUpdated := tc.TimeNoMod{}
+ rows, err := tx.Query(`select last_updated from `+tableName+` where
id=$1`, ID)
+ if err != nil {
+ return nil, err
+ }
+ defer rows.Close()
+ if !rows.Next() {
+ return nil, errors.New("no resource found with this id")
+ }
+ if err := rows.Scan(&lastUpdated); err != nil {
+ return nil, err
+ }
+ return &lastUpdated, 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, existingLastUpdated *tc.TimeNoMod) bool {
+ _, iumsTime := rfc.GetETagOrIfUnmodifiedSinceTime(h)
+ existingEtag := rfc.ETag(existingLastUpdated.Time)
+
+ if h != nil {
+ if h.Get(rfc.IfMatch) != "" &&
!strings.Contains(h.Get(rfc.IfMatch), existingEtag) {
Review comment:
This exact string match could lead to unexpected errors. For example,
around rounding, or if the time precision of the query changed, or the ETag
version changed.
It would be safer to have a `lib/go-rfc` helper func to parse all the ETags
in the `If-Match` header, return the latest, and compare that to
`existingLastUpdated` here.
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
##########
@@ -185,8 +186,9 @@ func ReadDSSHandlerV14(w http.ResponseWriter, r
*http.Request) {
results, err, maxTime := dss.readDSS(r.Header, inf.Tx, inf.User,
inf.Params, inf.IntParams, dsIDs, serverIDs, useIMS)
if maxTime != nil {
+ truncatedTime := maxTime.Truncate(time.Second).Add(time.Second)
// RFC1123
- date := maxTime.Format("Mon, 02 Jan 2006 15:04:05 MST")
+ date := truncatedTime.Format("Mon, 02 Jan 2006 15:04:05 MST")
Review comment:
`rfc.FormatHTTPDate` ?
##########
File path: traffic_ops/traffic_ops_golang/cachegroup/cachegroups.go
##########
@@ -531,6 +532,14 @@ func (cg *TOCacheGroup) Update() (error, error, int) {
cg.FallbackToClosest = &fbc
}
+ existingLastUpdated, e := api.GetLastUpdated(cg.ReqInfo.Tx, *cg.ID,
"cachegroup")
+ if e != nil {
Review comment:
As above, db error, `(time.Time, bool, error)`
##########
File path: traffic_ops/traffic_ops_golang/federations/federations.go
##########
@@ -115,8 +115,9 @@ func Get(w http.ResponseWriter, r *http.Request) {
}
allFederations := addResolvers([]tc.IAllFederation{}, feds,
fedsResolvers)
if maxTime != nil {
+ truncatedTime := maxTime.Truncate(time.Second).Add(time.Second)
// RFC1123
- date := maxTime.Format("Mon, 02 Jan 2006 15:04:05 MST")
+ date := truncatedTime.Format("Mon, 02 Jan 2006 15:04:05 MST")
Review comment:
`rfc.FormatHTTPDate` ?
##########
File path: traffic_ops/traffic_ops_golang/federations/allfederations.go
##########
@@ -92,8 +94,9 @@ func GetAll(w http.ResponseWriter, r *http.Request) {
fedsResolvers, err, code, maxTime := getFederationResolvers(inf.Tx.Tx,
fedInfoIDs(feds), useIMS, r.Header)
if code == http.StatusNotModified {
if maxTime != nil {
+ truncatedTime :=
maxTime.Truncate(time.Second).Add(time.Second)
// RFC1123
- date := maxTime.Format("Mon, 02 Jan 2006 15:04:05 MST")
+ date := truncatedTime.Format("Mon, 02 Jan 2006 15:04:05
MST")
Review comment:
`rfc.FormatHTTPDate` ?
##########
File path:
traffic_ops/traffic_ops_golang/federation_resolvers/federation_resolvers.go
##########
@@ -185,8 +186,9 @@ func Read(w http.ResponseWriter, r *http.Request) {
resolvers = append(resolvers, resolver)
}
+ truncatedTime := maxTime.Truncate(time.Second).Add(time.Second)
// RFC1123
- date := maxTime.Format("Mon, 02 Jan 2006 15:04:05 MST")
+ date := truncatedTime.Format("Mon, 02 Jan 2006 15:04:05 MST")
Review comment:
`rfc.FormatHTTPDate` ?
##########
File path: traffic_ops/traffic_ops_golang/api/api.go
##########
@@ -994,3 +994,36 @@ func CreateDeprecationAlerts(alternative *string)
tc.Alerts {
return tc.CreateAlerts(tc.WarnLevel, "This endpoint is
deprecated, and will be removed in the future")
}
}
+
+// 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) (*tc.TimeNoMod,
error) {
+ lastUpdated := tc.TimeNoMod{}
+ rows, err := tx.Query(`select last_updated from `+tableName+` where
id=$1`, ID)
+ if err != nil {
+ return nil, err
+ }
+ defer rows.Close()
+ if !rows.Next() {
+ return nil, errors.New("no resource found with this id")
Review comment:
This makes it difficult and expensive to tell the difference in a
missing ID and a real database error.
Can we make `GetLastUpdated` return `(time.time, bool, error)` where the
bool is whether it existed? Then, callers can check that and return an
appropriate 404 or 503.
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
##########
@@ -123,8 +123,9 @@ func ReadDSSHandler(w http.ResponseWriter, r *http.Request)
{
return
}
if maxTime != nil {
+ truncatedTime := maxTime.Truncate(time.Second).Add(time.Second)
// RFC1123
- date := maxTime.Format("Mon, 02 Jan 2006 15:04:05 MST")
+ date := truncatedTime.Format("Mon, 02 Jan 2006 15:04:05 MST")
Review comment:
`rfc.FormatHTTPDate` ?
##########
File path: traffic_ops/traffic_ops_golang/federations/allfederations.go
##########
@@ -75,8 +76,9 @@ func GetAll(w http.ResponseWriter, r *http.Request) {
feds, err, code, maxTime = getAllFederations(inf.Tx.Tx, useIMS,
r.Header)
if code == http.StatusNotModified {
if maxTime != nil {
+ truncatedTime :=
maxTime.Truncate(time.Second).Add(time.Second)
// RFC1123
- date := maxTime.Format("Mon, 02 Jan 2006
15:04:05 MST")
+ date := truncatedTime.Format("Mon, 02 Jan 2006
15:04:05 MST")
Review comment:
`rfc.FormatHTTPDate` ?
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -649,8 +649,9 @@ func Read(w http.ResponseWriter, r *http.Request) {
servers, serverCount, userErr, sysErr, errCode, maxTime =
getServers(r.Header, inf.Params, inf.Tx, inf.User, useIMS, *version)
if maxTime != nil {
+ truncatedTime := maxTime.Truncate(time.Second).Add(time.Second)
// RFC1123
- date := maxTime.Format("Mon, 02 Jan 2006 15:04:05 MST")
+ date := truncatedTime.Format("Mon, 02 Jan 2006 15:04:05 MST")
Review comment:
`rfc.FormatHTTPDate` ?
##########
File path:
traffic_ops/traffic_ops_golang/federation_resolvers/federation_resolvers.go
##########
@@ -146,8 +146,9 @@ func Read(w http.ResponseWriter, r *http.Request) {
runSecond, maxTime = TryIfModifiedSinceQuery(r.Header, inf.Tx,
where, orderBy, pagination, queryValues)
if !runSecond {
log.Debugln("IMS HIT")
+ truncatedTime :=
maxTime.Truncate(time.Second).Add(time.Second)
// RFC1123
- date := maxTime.Format("Mon, 02 Jan 2006 15:04:05 MST")
+ date := truncatedTime.Format("Mon, 02 Jan 2006 15:04:05
MST")
Review comment:
`rfc.FormatHTTPDate` ?
##########
File path: traffic_ops/traffic_ops_golang/federations/allfederations.go
##########
@@ -58,8 +58,9 @@ func GetAll(w http.ResponseWriter, r *http.Request) {
feds, err, code, maxTime = getAllFederationsForCDN(inf.Tx.Tx,
cdnName, useIMS, r.Header)
if code == http.StatusNotModified {
if maxTime != nil {
+ truncatedTime :=
maxTime.Truncate(time.Second).Add(time.Second)
// RFC1123
- date := maxTime.Format("Mon, 02 Jan 2006
15:04:05 MST")
+ date := truncatedTime.Format("Mon, 02 Jan 2006
15:04:05 MST")
Review comment:
`rfc.FormatHTTPDate` ?
##########
File path: traffic_ops/traffic_ops_golang/steeringtargets/steeringtargets.go
##########
@@ -243,6 +244,13 @@ func (st *TOSteeringTargetV11) Update() (error, error,
int) {
if userErr, sysErr, errCode := tenant.CheckID(st.ReqInfo.Tx.Tx,
st.ReqInfo.User, int(*st.DeliveryServiceID)); userErr != nil || sysErr != nil {
return userErr, sysErr, errCode
}
+ err, existingLastUpdated := CheckIfExistsBeforeUpdate(st.ReqInfo.Tx, st)
+ if err != nil {
+ return errors.New("no steering target found with this id"),
nil, http.StatusNotFound
Review comment:
distinguish db err, `(time.time, bool, error)`
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -1267,6 +1268,16 @@ func Update(w http.ResponseWriter, r *http.Request) {
return
}
+ existingLastUpdated, err := api.GetLastUpdated(inf.Tx, *server.ID,
"server")
+ if err != nil {
+ api.HandleErr(w, r, tx, http.StatusNotFound, errors.New("no
server found with this id"), nil)
Review comment:
db error, `(time.Time, bool, error)`
##########
File path: traffic_ops/traffic_ops_golang/api/generic_crud.go
##########
@@ -223,7 +225,15 @@ func GenericRead(h http.Header, val GenericReader, useIMS
bool) ([]interface{},
}
// GenericUpdate handles the common update case, where the update returns the
new last_modified time.
-func GenericUpdate(val GenericUpdater) (error, error, int) {
+func GenericUpdate(h http.Header, val GenericUpdater) (error, error, int) {
+ existingLastUpdated, err := val.GetLastUpdated()
+ if err != nil {
+ return errors.New("no " + val.GetType() + " found with this
id"), err, http.StatusNotFound
Review comment:
This could be a real database error. As above, I think we should change
it to `func GetLastUpdated(tx *sqlx.Tx, ID int, tableName string) (time.Time,
bool, error)` and handle both here,
```
existingLastUpdated, ok, err := val.GetLastUpdated()
if !ok {
return errors.New("no " + val.GetType() + " found with this
id"), err, http.StatusNotFound
} else if err != nil {
return errors.New(nil, errors.New("getting " + val.GetType() +
" last updated: " + err.Error()), err, http.StatusInternalServerError
```
##########
File path: traffic_ops/traffic_ops_golang/servicecategory/servicecategories.go
##########
@@ -39,6 +39,22 @@ type TOServiceCategory struct {
tc.ServiceCategory
}
+func (v *TOServiceCategory) GetLastUpdated() (*tc.TimeNoMod, error) {
+ lastUpdated := tc.TimeNoMod{}
+ rows, err := v.APIInfo().Tx.Query(`select last_updated from
service_category where name=$1`, v.Name)
+ if err != nil {
+ return nil, err
+ }
+ defer rows.Close()
+ if !rows.Next() {
+ return nil, errors.New("no resource found with this id")
+ }
+ if err := rows.Scan(&lastUpdated); err != nil {
+ return nil, err
Review comment:
Nitpick: context, `return nil, errors.New("scanning: " + err.Error())`
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -779,6 +780,14 @@ 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, err := api.GetLastUpdated(inf.Tx, *ds.ID,
"deliveryservice")
+ if err != nil {
+ return nil, http.StatusNotFound, errors.New("no deliveryservice
found with this id"), nil
Review comment:
As above, db error, `(time.Time, bool, error)`
##########
File path: traffic_ops/traffic_ops_golang/api/api.go
##########
@@ -994,3 +994,36 @@ func CreateDeprecationAlerts(alternative *string)
tc.Alerts {
return tc.CreateAlerts(tc.WarnLevel, "This endpoint is
deprecated, and will be removed in the future")
}
}
+
+// 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) (*tc.TimeNoMod,
error) {
+ lastUpdated := tc.TimeNoMod{}
+ rows, err := tx.Query(`select last_updated from `+tableName+` where
id=$1`, ID)
+ if err != nil {
+ return nil, err
Review comment:
It'll be easier to debug if we include the context of what was happening
when we got an error here, `return nil, errors.New("querying: " + err.Error())`
##########
File path: traffic_ops/traffic_ops_golang/api/api.go
##########
@@ -994,3 +994,36 @@ func CreateDeprecationAlerts(alternative *string)
tc.Alerts {
return tc.CreateAlerts(tc.WarnLevel, "This endpoint is
deprecated, and will be removed in the future")
}
}
+
+// 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) (*tc.TimeNoMod,
error) {
+ lastUpdated := tc.TimeNoMod{}
+ rows, err := tx.Query(`select last_updated from `+tableName+` where
id=$1`, ID)
+ if err != nil {
+ return nil, err
+ }
+ defer rows.Close()
+ if !rows.Next() {
+ return nil, errors.New("no resource found with this id")
+ }
+ if err := rows.Scan(&lastUpdated); err != nil {
+ return nil, err
Review comment:
Debugging context, `return nil, errors.New("scanning: " + err.Error())`
##########
File path: traffic_ops/traffic_ops_golang/servicecategory/servicecategories.go
##########
@@ -39,6 +39,22 @@ type TOServiceCategory struct {
tc.ServiceCategory
}
+func (v *TOServiceCategory) GetLastUpdated() (*tc.TimeNoMod, error) {
+ lastUpdated := tc.TimeNoMod{}
+ rows, err := v.APIInfo().Tx.Query(`select last_updated from
service_category where name=$1`, v.Name)
+ if err != nil {
+ return nil, err
Review comment:
Nitpick: context, `return nil, errors.New("querying: " + err.Error())`
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/request/requests.go
##########
@@ -383,6 +388,14 @@ func (req *deliveryServiceRequestAssignment) Update()
(error, error, int) {
req.DeliveryServiceRequestNullable =
current.DeliveryServiceRequestNullable
req.AssigneeID = assigneeID
+ existingLastUpdated, err := api.GetLastUpdated(req.ReqInfo.Tx, *req.ID,
"deliveryservice_request")
+ if err != nil {
+ return errors.New("no delivery service request found with this
id"), nil, http.StatusNotFound
Review comment:
As above, db error, `(time.Time, bool, error)`
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
##########
@@ -185,8 +186,9 @@ func ReadDSSHandlerV14(w http.ResponseWriter, r
*http.Request) {
results, err, maxTime := dss.readDSS(r.Header, inf.Tx, inf.User,
inf.Params, inf.IntParams, dsIDs, serverIDs, useIMS)
if maxTime != nil {
+ truncatedTime := maxTime.Truncate(time.Second).Add(time.Second)
// RFC1123
- date := maxTime.Format("Mon, 02 Jan 2006 15:04:05 MST")
+ date := truncatedTime.Format("Mon, 02 Jan 2006 15:04:05 MST")
Review comment:
This is only 2 lines, but it's repeated everywhere. What would you think
about making an `api` helper for this, `w.Header().Add(rfc.LastModified,
api.FormatLastModified(maxTime))`
```
func FormatLastModified(ti time.Time) string {
return rfc.FormatHTTPDate(ti.Truncate(time.Second).Add(time.Second))
}
```
Maybe also:
```
func AddLastModifiedHdr(w http.ResponseWriter, t time.Time) {
w.Header().Add(rfc.LastModified, FormatLastModified(t)
}
```
?
##########
File path: traffic_ops/traffic_ops_golang/steeringtargets/steeringtargets.go
##########
@@ -268,6 +276,22 @@ func (st *TOSteeringTargetV11) Update() (error, error,
int) {
return nil, nil, http.StatusOK
}
+func CheckIfExistsBeforeUpdate(tx *sqlx.Tx, st *TOSteeringTargetV11) (error,
*tc.TimeNoMod) {
Review comment:
Like the other func, I think this also needs to distinguish db errors
from not-found, `(time.Time, bool, error)`.
Also can be `time.Time`
##########
File path: lib/go-rfc/cachecontrol.go
##########
@@ -0,0 +1,113 @@
+package rfc
+
+import (
+ "errors"
+ "net/http"
+ "strconv"
+ "strings"
+ "time"
+)
+
+/*
+ * 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.
+ */
+
+const (
+ IfModifiedSince = "If-Modified-Since" // RFC7232§3.3
+ LastModified = "Last-Modified" // RFC7232§2.2
+ ETagHeader = "ETag"
+ IfMatch = "If-Match"
+ IfUnmodifiedSince = "If-Unmodified-Since"
+ ETagVersion = 1
+)
+
+// ETag takes the last time the object was modified, and returns an ETag
string. Note the string is the complete header value, including quotes. ETags
must be quoted strings.
+func ETag(t time.Time) string {
+ return `"v` + strconv.Itoa(ETagVersion) + `-` +
strconv.FormatInt(t.UnixNano(), 36) + `"`
+}
+
+// GetETagOrIfUnmodifiedSinceTime parses the http header and returns a list of
Etags/ an "if-unmodified-since" time to compare to, in that order.
+func GetETagOrIfUnmodifiedSinceTime(h http.Header) ([]string, *time.Time) {
+ if h == nil {
+ return nil, nil
+ }
+ valIUMS := h.Get(IfUnmodifiedSince)
+ valIfMatch := h.Get(IfMatch)
+ // Check the If-Match header first, if that exists, go off of that. If
not, check for If-Unmodified-Since header.
+ if valIfMatch != "" {
+ s := strings.Split(valIfMatch, ",")
+ eTagsTimeList := ParseEtagsList(s)
+ return eTagsTimeList, nil
+ }
+ if valIUMS != "" {
+ t, ok := ParseHTTPDate(valIUMS)
+ if ok {
+ return nil, &t
+ } else {
+ return nil, nil
+ }
+ }
+ return nil, nil
+}
+
+// ParseETag takes a complete ETag header string, including the quotes (if the
client correctly set them), and returns the last modified time encoded in the
ETag.
+func ParseETag(e string) (time.Time, error) {
+ if len(e) < 2 || e[0] != '"' || e[len(e)-1] != '"' {
+ return time.Time{}, errors.New("unquoted string, value must be
quoted")
+ }
+ e = e[1 : len(e)-1] // strip quotes
+
+ prefix := `v` + strconv.Itoa(ETagVersion) + `-`
+ if len(e) < len(prefix) || !strings.HasPrefix(e, prefix) {
+ return time.Time{}, errors.New("malformed, no version prefix")
+ }
+
+ timeStr := e[len(prefix):]
+
+ i, err := strconv.ParseInt(timeStr, 36, 64)
+ if err != nil {
+ return time.Time{}, errors.New("malformed")
+ }
+
+ t := time.Unix(0, i)
+
+ const year = time.Hour * 24 * 365
+
+ // sanity check - if the time isn't +/- 20 years, error. This catches
overflows and near-zero errors
+ if t.After(time.Now().Add(20*year)) ||
t.Before(time.Now().Add(-20*year)) {
+ return time.Time{}, errors.New("malformed, out of range")
+ }
+
+ return t, nil
+}
+
+// ParseEtagsList parses a list of etags and returns the time equivalent
string for each of the etags.
+func ParseEtagsList(eTags []string) []string {
+ tagTimes := make([]string, 0, len(eTags))
+ for _, tag := range eTags {
+ tag = strings.TrimSpace(tag)
+ t, err := ParseETag(`"` + tag + `"`)
Review comment:
These will already be quoted, If-Match must be quoted, e.g. `If-Match:
"tag0", "tag1"`.
So this should be `t, err := ParseETag(tag)`
##########
File path: traffic_ops/traffic_ops_golang/steeringtargets/steeringtargets.go
##########
@@ -268,6 +276,22 @@ func (st *TOSteeringTargetV11) Update() (error, error,
int) {
return nil, nil, http.StatusOK
}
+func CheckIfExistsBeforeUpdate(tx *sqlx.Tx, st *TOSteeringTargetV11) (error,
*tc.TimeNoMod) {
+ lastUpdated := tc.TimeNoMod{}
+ rows, err := tx.NamedQuery(`select last_updated from steering_target
where deliveryservice=:deliveryservice and target=:target`, st)
+ if err != nil {
+ return err, nil
Review comment:
Debugging context, return nil, errors.New("querying: " + err.Error())
##########
File path: traffic_ops/traffic_ops_golang/api/api.go
##########
@@ -994,3 +994,36 @@ func CreateDeprecationAlerts(alternative *string)
tc.Alerts {
return tc.CreateAlerts(tc.WarnLevel, "This endpoint is
deprecated, and will be removed in the future")
}
}
+
+// 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) (*tc.TimeNoMod,
error) {
+ lastUpdated := tc.TimeNoMod{}
+ rows, err := tx.Query(`select last_updated from `+tableName+` where
id=$1`, ID)
+ if err != nil {
+ return nil, err
+ }
+ defer rows.Close()
+ if !rows.Next() {
+ return nil, errors.New("no resource found with this id")
+ }
+ if err := rows.Scan(&lastUpdated); err != nil {
+ return nil, err
+ }
+ return &lastUpdated, 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, existingLastUpdated *tc.TimeNoMod) bool {
+ _, iumsTime := rfc.GetETagOrIfUnmodifiedSinceTime(h)
+ existingEtag := rfc.ETag(existingLastUpdated.Time)
+
+ if h != nil {
+ if h.Get(rfc.IfMatch) != "" &&
!strings.Contains(h.Get(rfc.IfMatch), existingEtag) {
Review comment:
Maybe something like
```
func GetUnmodifiedTime(h http.Header) (time.Time, bool) {
if h == nil {
return time.Time{}, false
}
if im := h.Get(IfMatch); im != "" {
if et, ok := ParseETags(strings.Split(im, ",")); ok {
return et, true
}
}
if ius := h.Get(IfUnmodifiedSince); ius != "" {
if tm, ok := ParseHTTPDate(ius); ok {
return tm, true
}
}
return time.Time{}, false
}
// ParseETags the latest time of any valid ETag, and whether a valid ETag
was found.
func ParseETags(eTags []string) (time.Time, bool) {
latestTime := time.Time{}
for _, tag := range eTags {
tag = strings.TrimSpace(tag)
et, err := ParseETag(tag)
// errors are recoverable, keep going through the list of etags
if err != nil {
continue
}
if et.After(latestTime) {
latestTime = et
}
}
if latestTime == (time.Time{}) {
return time.Time{}, false
}
return latestTime, true
}
// 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: consider unmodified
}
return !lastUpdated.UTC().After(unmodifiedTime.UTC())
}
```
?
That would protect us against edge cases and future ETag changes, which will
be contained in the `ParseETag` func
----------------------------------------------------------------
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]
