ocket8888 commented on a change in pull request #4942: URL: https://github.com/apache/trafficcontrol/pull/4942#discussion_r485102737
########## 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") Review comment: Did you want to pass up the error from the conversion? ########## File path: traffic_ops/testing/api/v3/cachegroups_test.go ########## @@ -42,11 +42,45 @@ func TestCacheGroups(t *testing.T) { var header http.Header header = make(map[string][]string) header.Set(rfc.IfModifiedSince, time) + header.Set(rfc.IfUnmodifiedSince, time) UpdateTestCacheGroups(t) + UpdateTestCacheGroupsWithHeaders(t, header) GetTestCacheGroupsAfterChangeIMS(t, header) + header = make(map[string][]string) + etag := rfc.ETag(currentTime) + header.Set(rfc.IfMatch, etag) + UpdateTestCacheGroupsWithHeaders(t, header) }) } +func UpdateTestCacheGroupsWithHeaders(t *testing.T, h http.Header) { + firstCG := testData.CacheGroups[0] + resp, _, err := TOSession.GetCacheGroupNullableByNameWithHdr(*firstCG.Name, h) + if err != nil { + t.Errorf("cannot GET CACHEGROUP by name: %v - %v", *firstCG.Name, err) + } + if len(resp) > 0 { + cg := resp[0] + expectedShortName := "blah" + cg.ShortName = &expectedShortName + + // fix the type id for test + typeResp, _, err := TOSession.GetTypeByIDWithHdr(*cg.TypeID, h) + if err != nil { + t.Error("could not lookup a typeID for this cachegroup") Review comment: This should probably be fatal, since typeResp could be `nil` if an error occurred. ########## 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 Review comment: `strconv.Unquote` might do this for you. ########## File path: traffic_ops/testing/api/v3/deliveryservices_test.go ########## @@ -53,9 +55,51 @@ func TestDeliveryServices(t *testing.T) { DeliveryServiceMinorVersionsTest(t) DeliveryServiceTenancyTest(t) PostDeliveryServiceTest(t) + header = make(map[string][]string) + etag := rfc.ETag(currentTime) + header.Set(rfc.IfMatch, etag) + UpdateTestDeliveryServicesWithHeaders(t, header) }) } +func UpdateTestDeliveryServicesWithHeaders(t *testing.T, header http.Header) { + firstDS := testData.DeliveryServices[0] Review comment: should check length > 0 to avoid segfaulting. Also, you could check that the XMLID isn't `nil` right here to save a little time if the test can't proceed. ########## File path: traffic_ops/testing/api/v3/phys_locations_test.go ########## @@ -35,12 +35,39 @@ func TestPhysLocations(t *testing.T) { var header http.Header header = make(map[string][]string) header.Set(rfc.IfModifiedSince, time) + header.Set(rfc.IfUnmodifiedSince, time) UpdateTestPhysLocations(t) + UpdateTestPhysLocationsWithHeaders(t, header) GetTestPhysLocations(t) GetTestPhysLocationsIMSAfterChange(t, header) + header = make(map[string][]string) + etag := rfc.ETag(currentTime) + header.Set(rfc.IfMatch, etag) + UpdateTestPhysLocationsWithHeaders(t, header) }) } +func UpdateTestPhysLocationsWithHeaders(t *testing.T, header http.Header) { + firstPhysLocation := testData.PhysLocations[0] Review comment: Should check that length > 0 to avoid segfaults ########## File path: traffic_ops/testing/api/v3/origins_test.go ########## @@ -16,26 +16,63 @@ package v3 */ import ( + "net/http" "reflect" "strings" "testing" "time" + "github.com/apache/trafficcontrol/lib/go-rfc" "github.com/apache/trafficcontrol/lib/go-tc" "github.com/apache/trafficcontrol/lib/go-util" toclient "github.com/apache/trafficcontrol/traffic_ops/client" ) func TestOrigins(t *testing.T) { WithObjs(t, []TCObj{CDNs, Types, Tenants, Parameters, Profiles, Statuses, Divisions, Regions, PhysLocations, CacheGroups, Servers, Users, Topologies, DeliveryServices, Coordinates, Origins}, func() { + currentTime := time.Now().UTC().Add(-5 * time.Second) + time := currentTime.Format(time.RFC1123) + var header http.Header + header = make(map[string][]string) + header.Set(rfc.IfUnmodifiedSince, time) UpdateTestOrigins(t) + UpdateTestOriginsWithHeaders(t, header) GetTestOrigins(t) NotFoundDeleteTest(t) OriginTenancyTest(t) VerifyPaginationSupport(t) + header = make(map[string][]string) + etag := rfc.ETag(currentTime) + header.Set(rfc.IfMatch, etag) + UpdateTestOriginsWithHeaders(t, header) }) } +func UpdateTestOriginsWithHeaders(t *testing.T, header http.Header) { + firstOrigin := testData.Origins[0] Review comment: Should check if length is > 0 and that neither `firstOrigin.Name` nor `firstOrigin.ID` is `nil` to avoid segfaults. ########## File path: traffic_ops/testing/api/v3/deliveryservices_test.go ########## @@ -53,9 +55,51 @@ func TestDeliveryServices(t *testing.T) { DeliveryServiceMinorVersionsTest(t) DeliveryServiceTenancyTest(t) PostDeliveryServiceTest(t) + header = make(map[string][]string) + etag := rfc.ETag(currentTime) + header.Set(rfc.IfMatch, etag) + UpdateTestDeliveryServicesWithHeaders(t, header) }) } +func UpdateTestDeliveryServicesWithHeaders(t *testing.T, header http.Header) { + firstDS := testData.DeliveryServices[0] + + dses, _, err := TOSession.GetDeliveryServicesNullableWithHdr(header) + if err != nil { + t.Errorf("cannot GET Delivery Services: %v", err) + } + + remoteDS := tc.DeliveryServiceNullable{} + found := false + for _, ds := range dses { + if *ds.XMLID == *firstDS.XMLID { + found = true + remoteDS = ds + break + } + } + if !found { + t.Errorf("GET Delivery Services missing: %v", firstDS.XMLID) Review comment: If you don't find the XMLID you probably don't want to do the rest of the test, so this should probably be fatal. ########## File path: traffic_ops/testing/api/v3/cdnfederations_test.go ########## @@ -17,9 +17,13 @@ package v3 import ( "encoding/json" - "github.com/apache/trafficcontrol/lib/go-log" + "net/http" "strings" "testing" + "time" + + "github.com/apache/trafficcontrol/lib/go-log" Review comment: This isn't your fault or your problem, but I'd appreciate it if you could get rid of this. We shouldn't be importing the logger into tests; they have their own logger. The only place it's being used looks like line 126, where `log.Errorln` should probably just be `t.Error`. ########## File path: traffic_ops/testing/api/v3/cachegroups_test.go ########## @@ -42,11 +42,45 @@ func TestCacheGroups(t *testing.T) { var header http.Header header = make(map[string][]string) header.Set(rfc.IfModifiedSince, time) + header.Set(rfc.IfUnmodifiedSince, time) UpdateTestCacheGroups(t) + UpdateTestCacheGroupsWithHeaders(t, header) GetTestCacheGroupsAfterChangeIMS(t, header) + header = make(map[string][]string) + etag := rfc.ETag(currentTime) + header.Set(rfc.IfMatch, etag) + UpdateTestCacheGroupsWithHeaders(t, header) }) } +func UpdateTestCacheGroupsWithHeaders(t *testing.T, h http.Header) { + firstCG := testData.CacheGroups[0] + resp, _, err := TOSession.GetCacheGroupNullableByNameWithHdr(*firstCG.Name, h) + if err != nil { + t.Errorf("cannot GET CACHEGROUP by name: %v - %v", *firstCG.Name, err) + } + if len(resp) > 0 { + cg := resp[0] + expectedShortName := "blah" + cg.ShortName = &expectedShortName + + // fix the type id for test + typeResp, _, err := TOSession.GetTypeByIDWithHdr(*cg.TypeID, h) + if err != nil { + t.Error("could not lookup a typeID for this cachegroup") + } + cg.TypeID = &typeResp[0].ID Review comment: Should check if `typeResp` has length of at least 1, or this will segfault. ########## File path: traffic_ops/testing/api/v3/deliveryservices_test.go ########## @@ -53,9 +55,51 @@ func TestDeliveryServices(t *testing.T) { DeliveryServiceMinorVersionsTest(t) DeliveryServiceTenancyTest(t) PostDeliveryServiceTest(t) + header = make(map[string][]string) + etag := rfc.ETag(currentTime) + header.Set(rfc.IfMatch, etag) + UpdateTestDeliveryServicesWithHeaders(t, header) }) } +func UpdateTestDeliveryServicesWithHeaders(t *testing.T, header http.Header) { + firstDS := testData.DeliveryServices[0] + + dses, _, err := TOSession.GetDeliveryServicesNullableWithHdr(header) + if err != nil { + t.Errorf("cannot GET Delivery Services: %v", err) + } + + remoteDS := tc.DeliveryServiceNullable{} + found := false + for _, ds := range dses { + if *ds.XMLID == *firstDS.XMLID { Review comment: should check that these aren't `nil` to avoid segfaulting - can skip checking if `firstDS.XMLID` is `nil` if you already did above. Also, the test will segfault later if `ds.ID` is `nil`, so this might be a good time to check that, too. ########## File path: traffic_ops/testing/api/v3/parameters_test.go ########## @@ -37,12 +37,39 @@ func TestParameters(t *testing.T) { var header http.Header header = make(map[string][]string) header.Set(rfc.IfModifiedSince, time) + header.Set(rfc.IfUnmodifiedSince, time) UpdateTestParameters(t) + UpdateTestParametersWithHeaders(t, header) GetTestParameters(t) GetTestParametersIMSAfterChange(t, header) + header = make(map[string][]string) + etag := rfc.ETag(currentTime) + header.Set(rfc.IfMatch, etag) + UpdateTestParametersWithHeaders(t, header) }) } +func UpdateTestParametersWithHeaders(t *testing.T, header http.Header) { + firstParameter := testData.Parameters[0] Review comment: Should check that length > 0 to avoid segfaults ########## File path: traffic_ops/testing/api/v3/deliveryservices_test.go ########## @@ -53,9 +55,51 @@ func TestDeliveryServices(t *testing.T) { DeliveryServiceMinorVersionsTest(t) DeliveryServiceTenancyTest(t) PostDeliveryServiceTest(t) + header = make(map[string][]string) + etag := rfc.ETag(currentTime) + header.Set(rfc.IfMatch, etag) + UpdateTestDeliveryServicesWithHeaders(t, header) }) } +func UpdateTestDeliveryServicesWithHeaders(t *testing.T, header http.Header) { + firstDS := testData.DeliveryServices[0] + + dses, _, err := TOSession.GetDeliveryServicesNullableWithHdr(header) + if err != nil { + t.Errorf("cannot GET Delivery Services: %v", err) + } + + remoteDS := tc.DeliveryServiceNullable{} + found := false + for _, ds := range dses { + if *ds.XMLID == *firstDS.XMLID { + found = true + remoteDS = ds + break + } + } + if !found { + t.Errorf("GET Delivery Services missing: %v", firstDS.XMLID) Review comment: oh, also, this will print out the pointer value. I think that argument should dereference like: `*firstDS.XMLID`. ---------------------------------------------------------------- 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]
