ocket8888 commented on a change in pull request #4942: URL: https://github.com/apache/trafficcontrol/pull/4942#discussion_r469600814
########## File path: lib/go-rfc/cachecontrol.go ########## @@ -0,0 +1,115 @@ +package rfc + +import ( + "errors" + "github.com/apache/trafficcontrol/lib/go-log" + "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 +} + +func ParseEtagsList(eTags []string) []string { + var tagTimes []string + if eTags != nil { + for _, tag := range eTags { + tag = strings.TrimSpace(tag) + t, err := ParseETag(`"` + tag + `"`) + if err != nil { + log.Warnf("Couldn't parse etag %v. err: %v", tag, err.Error()) Review comment: I don't think we should import the logger into library code. If an error occurs, it should be returned to the handler. If errors are recoverable (as it appears in this case), it should be documented that errors are recoverable. ########## File path: lib/go-rfc/cachecontrol.go ########## @@ -0,0 +1,115 @@ +package rfc + +import ( + "errors" + "github.com/apache/trafficcontrol/lib/go-log" + "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 +} + +func ParseEtagsList(eTags []string) []string { + var tagTimes []string Review comment: Since you're using string concatenation instead of `fmt.Sprintf`, I assume the intention is for the code to be pretty performant. In that case, you can build this list a little bit faster by pre-allocating all of the memory you already know you'll need: ```go tagTimes := make([]string, 0, len(eTags)) ``` ########## File path: lib/go-rfc/cachecontrol.go ########## @@ -0,0 +1,115 @@ +package rfc + +import ( + "errors" + "github.com/apache/trafficcontrol/lib/go-log" + "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 +} + +func ParseEtagsList(eTags []string) []string { + var tagTimes []string + if eTags != nil { Review comment: I don't think you need to wrap the loop in this. If `eTags` is `nil`, `len(eTags)` is `0`, so `for _, tag := eTags` will just iterate zero times; it doesn't panic or anything. ```go package main import ( "fmt" ) func printIt(strs []string) { for _, str := range strs { fmt.Println(str) } } func main() { printIt(nil) // just doesn't print anything } ``` ([playground](https://play.golang.org/p/caw8V4Uu6Xx)) ########## File path: traffic_ops/client/cachegroup.go ########## @@ -19,13 +19,12 @@ import ( "encoding/json" "errors" "fmt" + "github.com/apache/trafficcontrol/lib/go-log" + "github.com/apache/trafficcontrol/lib/go-tc" "net" "net/http" "net/url" "strconv" - - "github.com/apache/trafficcontrol/lib/go-log" - "github.com/apache/trafficcontrol/lib/go-tc" Review comment: Why'd you re-order these? It's pretty standard - at least for us - to put imports in groups in the order: 1. standard libraries 2. golang.org/x/ imports 3. imports from within the project 4. third-party libraries although I think 1 and 2 get mixed together pretty frequently. Which is fine, IMO. ########## File path: traffic_ops/testing/api/v3/federation_resolvers_test.go ########## @@ -17,11 +17,11 @@ package v3 import ( "github.com/apache/trafficcontrol/lib/go-rfc" + "github.com/apache/trafficcontrol/lib/go-tc" + "github.com/apache/trafficcontrol/lib/go-util" "net/http" "testing" "time" - "github.com/apache/trafficcontrol/lib/go-tc" - "github.com/apache/trafficcontrol/lib/go-util" Review comment: Same as above RE: re-ordering imports ########## File path: traffic_ops/testing/api/v3/crconfig_test.go ########## @@ -17,10 +17,9 @@ package v3 import ( "encoding/json" + "github.com/apache/trafficcontrol/lib/go-tc" "strings" "testing" - - "github.com/apache/trafficcontrol/lib/go-tc" Review comment: Same as above RE: re-ordering imports. ########## File path: traffic_ops/testing/api/v3/tenants_test.go ########## @@ -16,6 +16,8 @@ package v3 */ import ( + "github.com/apache/trafficcontrol/lib/go-rfc" Review comment: I think this should go after the standard imports. ########## File path: traffic_ops/testing/api/v3/origins_test.go ########## @@ -16,6 +16,8 @@ package v3 */ import ( + "github.com/apache/trafficcontrol/lib/go-rfc" Review comment: I think this should go beneath the standard imports ########## File path: traffic_ops/traffic_ops_golang/api/api.go ########## @@ -986,3 +986,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) (error, *tc.TimeNoMod) { + lastUpdated := tc.TimeNoMod{} + rows, err := tx.Query(`select last_updated from `+tableName+` where id=$1`, ID) + if err != nil { + return err, nil + } + defer rows.Close() + if !rows.Next() { + return errors.New("no resource found with this id"), nil + } + if err := rows.Scan(&lastUpdated); err != nil { + return err, nil + } + return nil, &lastUpdated +} + +// IsUnmodified returns a boolean, saying whether or not the resource in question was modified since the time specified in the headers Review comment: Same as above RE: punctuation. ########## File path: traffic_ops/traffic_ops_golang/api/api.go ########## @@ -986,3 +986,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) (error, *tc.TimeNoMod) { Review comment: I don't think this is a rule or anything, but generally errors are the last returned value of functions ########## File path: traffic_ops/traffic_ops_golang/api/api.go ########## @@ -986,3 +986,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 Review comment: nit: GoDocs should be complete sentences that end with proper punctuation. ########## File path: traffic_ops/testing/api/v3/cachegroups_test.go ########## @@ -42,11 +42,43 @@ func TestCacheGroups(t *testing.T) { var header http.Header header = make(map[string][]string) header.Set(rfc.IfModifiedSince, time) + header.Set(rfc.IfUnmodifiedSince, time) Review comment: Wait... what's the expected behavior if you send `If-Modified-Since` and `If-Unmodified-Since` in the same request set to the same time? ########## File path: traffic_ops/testing/api/v3/cdnfederations_test.go ########## @@ -18,8 +18,11 @@ package v3 import ( "encoding/json" "github.com/apache/trafficcontrol/lib/go-log" + "github.com/apache/trafficcontrol/lib/go-rfc" + "net/http" "strings" "testing" + "time" Review comment: I think the project-internal imports should be grouped after the standard library imports ########## File path: traffic_ops/traffic_ops_golang/api/generic_crud.go ########## @@ -213,15 +215,23 @@ 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) { + err, existingLastUpdated := val.GetLastUpdated() + if err != nil { + return errors.New("no " + val.GetType() + " found with this id"), err, http.StatusNotFound + } + if !IsUnmodified(h, existingLastUpdated) { + return errors.New("resource was modified"), nil, http.StatusPreconditionFailed + } + rows, err := val.APIInfo().Tx.NamedQuery(val.UpdateQuery(), val) if err != nil { return ParseDBError(err) } defer rows.Close() if !rows.Next() { - return errors.New("no " + val.GetType() + " found with this id"), nil, http.StatusNotFound + return errors.New("resource was modified"), nil, http.StatusPreconditionFailed Review comment: I don't think this should've changed, should it? ########## File path: traffic_ops/testing/api/v3/roles_test.go ########## @@ -40,13 +40,38 @@ func TestRoles(t *testing.T) { var header http.Header header = make(map[string][]string) header.Set(rfc.IfModifiedSince, time) + header.Set(rfc.IfUnmodifiedSince, time) UpdateTestRoles(t) GetTestRoles(t) + UpdateTestRolesWithHeaders(t, header) GetTestRolesIMSAfterChange(t, header) VerifyGetRolesOrder(t) + header = make(map[string][]string) + etag := rfc.ETag(currentTime) + header.Set(rfc.IfMatch, etag) + UpdateTestRolesWithHeaders(t, header) }) } +func UpdateTestRolesWithHeaders(t *testing.T, header http.Header) { + t.Logf("testData.Roles contains: %+v\n", testData.Roles) + firstRole := testData.Roles[0] + // Retrieve the Role by role so we can get the id for the Update + resp, _, status, err := TOSession.GetRoleByName(*firstRole.Name, header) + t.Log("Status Code: ", status) + if err != nil { + t.Errorf("cannot GET Role by role: %v - %v", firstRole.Name, err) + } + t.Logf("got response: %+v\n", resp) Review comment: nit but you don't need `\n` in `t.Logf` - or other logging/error/fatal message methods of `testing.T` types. ---------------------------------------------------------------- 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]
