From 56113994b2d599061e17da50ae94dddadf3f9c9c Mon Sep 17 00:00:00 2001 From: Jagan Parthiban Date: Tue, 11 Jul 2023 09:35:24 +0530 Subject: [PATCH 1/9] Fixes Phys_location RFC3339 issue https://github.com/apache/trafficcontrol/issues/7630 --- lib/go-tc/physlocations.go | 63 ++++ .../dbhelpers/db_helpers.go | 15 + .../physlocation/phys_locations.go | 308 ++++++++++++++++++ .../traffic_ops_golang/routing/routes.go | 8 +- 4 files changed, 390 insertions(+), 4 deletions(-) diff --git a/lib/go-tc/physlocations.go b/lib/go-tc/physlocations.go index a238067709..3dcd218cba 100644 --- a/lib/go-tc/physlocations.go +++ b/lib/go-tc/physlocations.go @@ -19,6 +19,8 @@ package tc * under the License. */ +import "time" + // PhysLocationsResponse is a list of PhysLocations as a response. type PhysLocationsResponse struct { Response []PhysLocation `json:"response"` @@ -201,3 +203,64 @@ type PhysLocationNullable struct { type PhysLocationTrimmed struct { Name string `json:"name"` } + +// PhysLocationV5 is an alias for the latest minor version for the major version 5. +type PhysLocationV5 PhysLocationV50 + +// PhysLocationV50 contains the physical location of a cache group. +type PhysLocationV50 struct { + Address string `json:"address" db:"address"` + City string `json:"city" db:"city"` + Comments string `json:"comments" db:"comments"` + Email string `json:"email" db:"email"` + ID int `json:"id" db:"id"` + LastUpdated time.Time `json:"lastUpdated" db:"last_updated"` + Name string `json:"name" db:"name"` + Phone string `json:"phone" db:"phone"` + POC string `json:"poc" db:"poc"` + RegionID int `json:"regionId" db:"region"` + RegionName string `json:"region" db:"region_name"` + ShortName string `json:"shortName" db:"short_name"` + State string `json:"state" db:"state"` + Zip string `json:"zip" db:"zip"` +} + +// PhysLocationsResponseV5 is an alias for the latest minor version for the major version 5. +type PhysLocationsResponseV5 PhysLocationsResponseV50 + +// PhysLocationsResponseV50 is a list of PhysLocations as a response. +type PhysLocationsResponseV50 struct { + Response []PhysLocationV5 `json:"response"` + Alerts +} + +// PhysLocationNullableV5 is an alias for the latest minor version for the major version 5. +type PhysLocationNullableV5 PhysLocationNullableV50 + +// PhysLocationNullableV50 contains the physical location of a cache group. It +// allows for all fields to be null. +type PhysLocationNullableV50 struct { + Address *string `json:"address" db:"address"` + City *string `json:"city" db:"city"` + Comments *string `json:"comments" db:"comments"` + Email *string `json:"email" db:"email"` + ID *int `json:"id" db:"id"` + LastUpdated time.Time `json:"lastUpdated" db:"last_updated"` + Name *string `json:"name" db:"name"` + Phone *string `json:"phone" db:"phone"` + POC *string `json:"poc" db:"poc"` + RegionID *int `json:"regionId" db:"region"` + RegionName *string `json:"region" db:"region_name"` + ShortName *string `json:"shortName" db:"short_name"` + State *string `json:"state" db:"state"` + Zip *string `json:"zip" db:"zip"` +} + +// PhysLocationResponseV5 is an alias for the latest minor version for the major version 5. +type PhysLocationResponseV5 PhysLocationResponseV50 + +// PhysLocationResponseV50 is a single PhysLocationNullable as a response. +type PhysLocationResponseV50 struct { + Response PhysLocationNullableV5 `json:"response"` + Alerts +} diff --git a/traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go b/traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go index dde6815757..94f026b800 100644 --- a/traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go +++ b/traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go @@ -2232,3 +2232,18 @@ func DivisionExists(tx *sql.Tx, id string) (bool, error) { } return true, nil } + +// PhysLocationExists confirms whether the PhysLocationExists exists, and an error (if one occurs). +func PhysLocationExists(tx *sql.Tx, id string) (bool, error) { + var count int + if err := tx.QueryRow("SELECT count(name) FROM phys_location WHERE id=$1", id).Scan(&count); err != nil { + return false, fmt.Errorf("error getting PhysLocation info: %w", err) + } + if count == 0 { + return false, nil + } + if count != 1 { + return false, fmt.Errorf("getting PhysLocation info - expected row count: 1, actual: %d", count) + } + return true, nil +} diff --git a/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go b/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go index 81e315931f..9f781a8779 100644 --- a/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go +++ b/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go @@ -20,8 +20,12 @@ package physlocation */ import ( + "database/sql" + "encoding/json" "errors" "fmt" + "github.com/apache/trafficcontrol/lib/go-log" + "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/util/ims" "net/http" "strconv" "time" @@ -215,3 +219,307 @@ func deleteQuery() string { WHERE id=:id` return query } + +func GetPhysLocation(w http.ResponseWriter, r *http.Request) { + var runSecond bool + var maxTime time.Time + inf, userErr, sysErr, errCode := api.NewInfo(r, nil, nil) + tx := inf.Tx + if userErr != nil || sysErr != nil { + api.HandleErr(w, r, tx.Tx, errCode, userErr, sysErr) + return + } + defer inf.Close() + + // Query Parameters to Database Query column mappings + queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{ + "name": {Column: "pl.name", Checker: nil}, + "id": {Column: "pl.id", Checker: api.IsInt}, + "region": {Column: "pl.region", Checker: api.IsInt}, + } + if _, ok := inf.Params["orderby"]; !ok { + inf.Params["orderby"] = "name" + } + where, orderBy, pagination, queryValues, errs := dbhelpers.BuildWhereAndOrderByAndPagination(inf.Params, queryParamsToQueryCols) + if len(errs) > 0 { + api.HandleErr(w, r, tx.Tx, http.StatusBadRequest, util.JoinErrs(errs), nil) + return + } + + if inf.Config.UseIMS { + runSecond, maxTime = ims.TryIfModifiedSinceQuery(tx, r.Header, queryValues, selectMaxLastUpdatedQuery(where)) + if !runSecond { + log.Debugln("IMS HIT") + api.AddLastModifiedHdr(w, maxTime) + w.WriteHeader(http.StatusNotModified) + return + } + log.Debugln("IMS MISS") + } else { + log.Debugln("Non IMS request") + } + + query := selectQuery() + where + orderBy + pagination + rows, err := tx.NamedQuery(query, queryValues) + if err != nil { + api.HandleErr(w, r, tx.Tx, http.StatusInternalServerError, nil, fmt.Errorf("Phy_Location read: error getting Phy_Location(s): %w", err)) + return + } + defer log.Close(rows, "unable to close DB connection") + + physLocation := tc.PhysLocationV5{} + physLocationList := []tc.PhysLocationV5{} + for rows.Next() { + if err = rows.Scan(&physLocation.Address, &physLocation.City, &physLocation.Comments, &physLocation.Email, &physLocation.ID, &physLocation.LastUpdated, &physLocation.Name, &physLocation.Phone, &physLocation.POC, &physLocation.RegionID, &physLocation.RegionName, &physLocation.ShortName, &physLocation.State, &physLocation.Zip); err != nil { + api.HandleErr(w, r, tx.Tx, http.StatusInternalServerError, nil, fmt.Errorf("error getting physLocation(s): %w", err)) + return + } + physLocationList = append(physLocationList, physLocation) + } + + api.WriteResp(w, r, physLocationList) + return +} + +func CreatePhysLocation(w http.ResponseWriter, r *http.Request) { + inf, userErr, sysErr, errCode := api.NewInfo(r, nil, nil) + if userErr != nil || sysErr != nil { + api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr) + return + } + defer inf.Close() + tx := inf.Tx.Tx + + physLocation, readValErr := readAndValidateJsonStruct(r) + if readValErr != nil { + api.HandleErr(w, r, tx, http.StatusBadRequest, readValErr, nil) + return + } + + // check if phys_location already exists + var count int + err := tx.QueryRow("SELECT count(*) from phys_location where name = $1", physLocation.Name).Scan(&count) + if err != nil { + api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, fmt.Errorf("error: %w, when checking if physLocation with name %s exists", err, physLocation.Name)) + return + } + if count == 1 { + api.HandleErr(w, r, tx, http.StatusBadRequest, fmt.Errorf("physLocation name '%s' already exists", physLocation.Name), nil) + return + } + + // create phys_location + query := ` + INSERT INTO phys_location ( + address, + city, + comments, + email, + name, + phone, + poc, + region, + short_name, + state, + zip + ) VALUES ( + $1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11 + ) RETURNING id, last_updated +` + err = tx.QueryRow( + query, + physLocation.Address, + physLocation.City, + physLocation.Comments, + physLocation.Email, + physLocation.Name, + physLocation.Phone, + physLocation.POC, + physLocation.RegionID, + physLocation.ShortName, + physLocation.State, + physLocation.Zip, + ).Scan( + &physLocation.ID, + &physLocation.LastUpdated, + ) + + if err != nil { + if errors.Is(err, sql.ErrNoRows) { + api.HandleErr(w, r, tx, http.StatusInternalServerError, fmt.Errorf("error: %w in physLocation with name: %s", err, physLocation.Name), nil) + return + } + usrErr, sysErr, code := api.ParseDBError(err) + api.HandleErr(w, r, tx, code, usrErr, sysErr) + return + } + alerts := tc.CreateAlerts(tc.SuccessLevel, "physLocation was created.") + w.Header().Set("Location", fmt.Sprintf("/api/%d.%d/phys_locations?name=%s", inf.Version.Major, inf.Version.Minor, physLocation.Name)) + api.WriteAlertsObj(w, r, http.StatusCreated, alerts, physLocation) + return +} + +func UpdatePhysLocation(w http.ResponseWriter, r *http.Request) { + inf, userErr, sysErr, errCode := api.NewInfo(r, nil, nil) + if userErr != nil || sysErr != nil { + api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr) + return + } + defer inf.Close() + + tx := inf.Tx.Tx + physLocation, readValErr := readAndValidateJsonStruct(r) + if readValErr != nil { + api.HandleErr(w, r, tx, http.StatusBadRequest, readValErr, nil) + return + } + + requestedID := inf.Params["id"] + + intRequestId, convErr := strconv.Atoi(requestedID) + if convErr != nil { + api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, fmt.Errorf("physLocation update error: %w, while converting from string to int", convErr), nil) + } + // check if the entity was already updated + userErr, sysErr, errCode = api.CheckIfUnModified(r.Header, inf.Tx, intRequestId, "phys_location") + if userErr != nil || sysErr != nil { + api.HandleErr(w, r, tx, errCode, userErr, sysErr) + return + } + + //update name and description of a phys_location + query := ` + UPDATE phys_location pl + SET + address = $1, + city = $2, + comments = $3, + email = $4, + name = $5, + phone = $6, + poc = $7, + region = $8, + short_name = $9, + state = $10, + zip = $11 + WHERE + pl.id = $12 + RETURNING + pl.id, pl.last_updated +` + + err := tx.QueryRow( + query, + physLocation.Address, + physLocation.City, + physLocation.Comments, + physLocation.Email, + physLocation.Name, + physLocation.Phone, + physLocation.POC, + physLocation.RegionID, + physLocation.ShortName, + physLocation.State, + physLocation.Zip, + requestedID, + ).Scan( + &physLocation.ID, + &physLocation.LastUpdated, + ) + if err != nil { + if errors.Is(err, sql.ErrNoRows) { + api.HandleErr(w, r, tx, http.StatusBadRequest, fmt.Errorf("physLocation with ID: %v not found", physLocation.ID), nil) + return + } + usrErr, sysErr, code := api.ParseDBError(err) + api.HandleErr(w, r, tx, code, usrErr, sysErr) + return + } + alerts := tc.CreateAlerts(tc.SuccessLevel, "physLocation was updated") + api.WriteAlertsObj(w, r, http.StatusOK, alerts, physLocation) + return +} + +func DeletePhysLocation(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() + + id := inf.Params["id"] + exists, err := dbhelpers.PhysLocationExists(tx, id) + if err != nil { + api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, err) + return + } + if !exists { + if id != "" { + api.HandleErr(w, r, tx, http.StatusNotFound, fmt.Errorf("no PhysLocation exists by id: %s", id), nil) + return + } else { + api.HandleErr(w, r, tx, http.StatusBadRequest, fmt.Errorf("no PhysLocation exists for empty id: %s", id), nil) + return + } + } + + assignedServer := 0 + if err := inf.Tx.Get(&assignedServer, "SELECT count(id) FROM server sv WHERE sv.phys_location=$1", id); err != nil { + api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, fmt.Errorf("phys_location delete error, could not count assigned servers: %w", err)) + return + } else if assignedServer != 0 { + api.HandleErr(w, r, tx, http.StatusBadRequest, fmt.Errorf("can not delete a phys_location with %d assigned servers", assignedServer), nil) + return + } + + res, err := tx.Exec("DELETE FROM phys_location AS pl WHERE pl.id=$1", id) + if err != nil { + api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, err) + return + } + rowsAffected, err := res.RowsAffected() + if err != nil { + api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, fmt.Errorf("determining rows affected for delete phys_location: %w", err)) + return + } + if rowsAffected == 0 { + api.HandleErr(w, r, tx, http.StatusInternalServerError, fmt.Errorf("no rows deleted for phys_location"), nil) + return + } + alerts := tc.CreateAlerts(tc.SuccessLevel, "phys_location was deleted.") + api.WriteAlertsObj(w, r, http.StatusOK, alerts, inf.Params) + return +} + +func readAndValidateJsonStruct(r *http.Request) (tc.PhysLocationV5, error) { + var physLocation tc.PhysLocationV5 + if err := json.NewDecoder(r.Body).Decode(&physLocation); err != nil { + userErr := fmt.Errorf("error decoding POST request body into PhysLocationV5 struct %w", err) + return physLocation, userErr + } + + // validate JSON body + errs := tovalidate.ToErrors(validation.Errors{ + "address": validation.Validate(physLocation.Address, validation.Required), + "city": validation.Validate(physLocation.City, validation.Required), + "name": validation.Validate(physLocation.Name, validation.Required), + "regionId": validation.Validate(physLocation.RegionID, validation.Required, validation.Min(0)), + "shortName": validation.Validate(physLocation.ShortName, validation.Required), + "state": validation.Validate(physLocation.State, validation.Required), + "zip": validation.Validate(physLocation.Zip, validation.Required)}) + if len(errs) > 0 { + userErr := util.JoinErrs(errs) + return physLocation, userErr + } + return physLocation, nil +} + +// selectMaxLastUpdatedQuery used for TryIfModifiedSinceQuery() +func selectMaxLastUpdatedQuery(where string) string { + return `SELECT max(t) from ( + SELECT max(a.last_updated) as t from phys_location a` + where + + ` UNION ALL + select max(last_updated) as t from last_deleted l where l.table_name='phys_location') as res` +} diff --git a/traffic_ops/traffic_ops_golang/routing/routes.go b/traffic_ops/traffic_ops_golang/routing/routes.go index ba2f12d167..306169ad94 100644 --- a/traffic_ops/traffic_ops_golang/routing/routes.go +++ b/traffic_ops/traffic_ops_golang/routing/routes.go @@ -256,10 +256,10 @@ func Routes(d ServerData) ([]Route, http.Handler, error) { {Version: api.Version{Major: 5, Minor: 0}, Method: http.MethodDelete, Path: `parameters/{id}$`, Handler: api.DeleteHandler(¶meter.TOParameter{}), RequiredPrivLevel: auth.PrivLevelOperations, RequiredPermissions: []string{"PARAMETER:DELETE", "PARAMETER:READ"}, Authenticated: Authenticated, Middlewares: nil, ID: 42627711831}, //Phys_Location: CRUD - {Version: api.Version{Major: 5, Minor: 0}, Method: http.MethodGet, Path: `phys_locations/?$`, Handler: api.ReadHandler(&physlocation.TOPhysLocation{}), RequiredPrivLevel: auth.PrivLevelReadOnly, RequiredPermissions: []string{"PHYSICAL-LOCATION:READ"}, Authenticated: Authenticated, Middlewares: nil, ID: 42040518231}, - {Version: api.Version{Major: 5, Minor: 0}, Method: http.MethodPut, Path: `phys_locations/{id}$`, Handler: api.UpdateHandler(&physlocation.TOPhysLocation{}), RequiredPrivLevel: auth.PrivLevelOperations, RequiredPermissions: []string{"PHYSICAL-LOCATION:UPDATE", "PHYSICAL-LOCATION:READ"}, Authenticated: Authenticated, Middlewares: nil, ID: 42279502131}, - {Version: api.Version{Major: 5, Minor: 0}, Method: http.MethodPost, Path: `phys_locations/?$`, Handler: api.CreateHandler(&physlocation.TOPhysLocation{}), RequiredPrivLevel: auth.PrivLevelOperations, RequiredPermissions: []string{"PHYSICAL-LOCATION:CREATE", "PHYSICAL-LOCATION:READ"}, Authenticated: Authenticated, Middlewares: nil, ID: 424645664831}, - {Version: api.Version{Major: 5, Minor: 0}, Method: http.MethodDelete, Path: `phys_locations/{id}$`, Handler: api.DeleteHandler(&physlocation.TOPhysLocation{}), RequiredPrivLevel: auth.PrivLevelOperations, RequiredPermissions: []string{"PHYSICAL-LOCATION:DELETE", "PHYSICAL-LOCATION:READ"}, Authenticated: Authenticated, Middlewares: nil, ID: 4561422131}, + {Version: api.Version{Major: 5, Minor: 0}, Method: http.MethodGet, Path: `phys_locations/?$`, Handler: physlocation.GetPhysLocation, RequiredPrivLevel: auth.PrivLevelReadOnly, RequiredPermissions: []string{"PHYSICAL-LOCATION:READ"}, Authenticated: Authenticated, Middlewares: nil, ID: 42040518231}, + {Version: api.Version{Major: 5, Minor: 0}, Method: http.MethodPut, Path: `phys_locations/{id}$`, Handler: physlocation.UpdatePhysLocation, RequiredPrivLevel: auth.PrivLevelOperations, RequiredPermissions: []string{"PHYSICAL-LOCATION:UPDATE", "PHYSICAL-LOCATION:READ"}, Authenticated: Authenticated, Middlewares: nil, ID: 42279502131}, + {Version: api.Version{Major: 5, Minor: 0}, Method: http.MethodPost, Path: `phys_locations/?$`, Handler: physlocation.CreatePhysLocation, RequiredPrivLevel: auth.PrivLevelOperations, RequiredPermissions: []string{"PHYSICAL-LOCATION:CREATE", "PHYSICAL-LOCATION:READ"}, Authenticated: Authenticated, Middlewares: nil, ID: 424645664831}, + {Version: api.Version{Major: 5, Minor: 0}, Method: http.MethodDelete, Path: `phys_locations/{id}$`, Handler: physlocation.DeletePhysLocation, RequiredPrivLevel: auth.PrivLevelOperations, RequiredPermissions: []string{"PHYSICAL-LOCATION:DELETE", "PHYSICAL-LOCATION:READ"}, Authenticated: Authenticated, Middlewares: nil, ID: 4561422131}, //Ping {Version: api.Version{Major: 5, Minor: 0}, Method: http.MethodGet, Path: `ping$`, Handler: ping.Handler, RequiredPrivLevel: auth.PrivLevelUnauthenticated, RequiredPermissions: nil, Authenticated: NoAuth, Middlewares: nil, ID: 455566159731}, From 1de3d333ebb62a20a8c3dae04531c2a8724ac070 Mon Sep 17 00:00:00 2001 From: Jagan Parthiban Date: Tue, 11 Jul 2023 09:36:00 +0530 Subject: [PATCH 2/9] Integration Test Case updates for https://github.com/apache/trafficcontrol/issues/7630 --- .../testing/api/v5/phys_locations_test.go | 22 +++++++++---------- .../testing/api/v5/traffic_control_test.go | 2 +- traffic_ops/v5-client/phys_location.go | 8 +++---- 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/traffic_ops/testing/api/v5/phys_locations_test.go b/traffic_ops/testing/api/v5/phys_locations_test.go index f1f70ddab7..ae6f3b03a8 100644 --- a/traffic_ops/testing/api/v5/phys_locations_test.go +++ b/traffic_ops/testing/api/v5/phys_locations_test.go @@ -38,7 +38,7 @@ func TestPhysLocations(t *testing.T) { currentTimeRFC := currentTime.Format(time.RFC1123) tomorrow := currentTime.AddDate(0, 0, 1).Format(time.RFC1123) - methodTests := utils.TestCase[client.Session, client.RequestOptions, tc.PhysLocation]{ + methodTests := utils.TestCase[client.Session, client.RequestOptions, tc.PhysLocationV5]{ "GET": { "NOT MODIFIED when NO CHANGES made": { ClientSession: TOSession, @@ -99,7 +99,7 @@ func TestPhysLocations(t *testing.T) { "POST": { "OK when VALID request": { ClientSession: TOSession, - RequestBody: tc.PhysLocation{ + RequestBody: tc.PhysLocationV5{ Address: "100 blah lane", City: "foo", Comments: "comment", @@ -112,12 +112,12 @@ func TestPhysLocations(t *testing.T) { State: "CO", Zip: "80602", }, - Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusOK), + Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusCreated), validatePhysicalLocationUpdateCreateFields("testPhysicalLocation", map[string]interface{}{"Name": "testPhysicalLocation"})), }, "BAD REQUEST when REGION ID does NOT MATCH REGION NAME": { ClientSession: TOSession, - RequestBody: tc.PhysLocation{ + RequestBody: tc.PhysLocationV5{ Address: "1234 southern way", City: "Atlanta", Name: "HotAtlanta", @@ -135,7 +135,7 @@ func TestPhysLocations(t *testing.T) { "OK when VALID request": { EndpointID: GetPhysicalLocationID(t, "HotAtlanta"), ClientSession: TOSession, - RequestBody: tc.PhysLocation{ + RequestBody: tc.PhysLocationV5{ Address: "1234 southern way", City: "NewCity", Name: "HotAtlanta", @@ -152,7 +152,7 @@ func TestPhysLocations(t *testing.T) { EndpointID: GetPhysicalLocationID(t, "HotAtlanta"), ClientSession: TOSession, RequestOpts: client.RequestOptions{Header: http.Header{rfc.IfUnmodifiedSince: {currentTimeRFC}}}, - RequestBody: tc.PhysLocation{ + RequestBody: tc.PhysLocationV5{ Address: "1234 southern way", City: "Atlanta", RegionID: GetRegionID(t, "region1")(), @@ -166,7 +166,7 @@ func TestPhysLocations(t *testing.T) { "PRECONDITION FAILED when updating with IFMATCH ETAG Header": { EndpointID: GetPhysicalLocationID(t, "HotAtlanta"), ClientSession: TOSession, - RequestBody: tc.PhysLocation{ + RequestBody: tc.PhysLocationV5{ Address: "1234 southern way", City: "Atlanta", RegionID: GetRegionID(t, "region1")(), @@ -230,7 +230,7 @@ func TestPhysLocations(t *testing.T) { func validatePhysicalLocationFields(expectedResp map[string]interface{}) utils.CkReqFunc { return func(t *testing.T, _ toclientlib.ReqInf, resp interface{}, _ tc.Alerts, _ error) { assert.RequireNotNil(t, resp, "Expected Physical Location response to not be nil.") - plResp := resp.([]tc.PhysLocation) + plResp := resp.([]tc.PhysLocationV5) for field, expected := range expectedResp { for _, pl := range plResp { switch field { @@ -259,7 +259,7 @@ func validatePhysicalLocationUpdateCreateFields(name string, expectedResp map[st func validatePhysicalLocationPagination(paginationParam string) utils.CkReqFunc { return func(t *testing.T, _ toclientlib.ReqInf, resp interface{}, _ tc.Alerts, _ error) { - paginationResp := resp.([]tc.PhysLocation) + paginationResp := resp.([]tc.PhysLocationV5) opts := client.NewRequestOptions() opts.QueryParameters.Set("orderby", "id") @@ -283,7 +283,7 @@ func validatePhysicalLocationSort() utils.CkReqFunc { return func(t *testing.T, _ toclientlib.ReqInf, resp interface{}, alerts tc.Alerts, _ error) { assert.RequireNotNil(t, resp, "Expected Physical Location response to not be nil.") var physLocNames []string - physLocResp := resp.([]tc.PhysLocation) + physLocResp := resp.([]tc.PhysLocationV5) for _, pl := range physLocResp { physLocNames = append(physLocNames, pl.Name) } @@ -295,7 +295,7 @@ func validatePhysicalLocationIDSort() utils.CkReqFunc { return func(t *testing.T, _ toclientlib.ReqInf, resp interface{}, alerts tc.Alerts, _ error) { assert.RequireNotNil(t, resp, "Expected Physical Location response to not be nil.") var physLocIDs []int - physLocResp := resp.([]tc.PhysLocation) + physLocResp := resp.([]tc.PhysLocationV5) for _, pl := range physLocResp { physLocIDs = append(physLocIDs, pl.ID) } diff --git a/traffic_ops/testing/api/v5/traffic_control_test.go b/traffic_ops/testing/api/v5/traffic_control_test.go index 789e475021..f15417ee73 100644 --- a/traffic_ops/testing/api/v5/traffic_control_test.go +++ b/traffic_ops/testing/api/v5/traffic_control_test.go @@ -42,7 +42,7 @@ type TrafficControl struct { Profiles []tc.Profile `json:"profiles"` Parameters []tc.Parameter `json:"parameters"` ProfileParameters []tc.ProfileParameter `json:"profileParameters"` - PhysLocations []tc.PhysLocation `json:"physLocations"` + PhysLocations []tc.PhysLocationV5 `json:"physLocations"` Regions []tc.Region `json:"regions"` Roles []tc.RoleV4 `json:"roles"` Servers []tc.ServerV4 `json:"servers"` diff --git a/traffic_ops/v5-client/phys_location.go b/traffic_ops/v5-client/phys_location.go index c9c94f3f76..ce50b45a63 100644 --- a/traffic_ops/v5-client/phys_location.go +++ b/traffic_ops/v5-client/phys_location.go @@ -26,7 +26,7 @@ import ( const apiPhysLocations = "/phys_locations" // CreatePhysLocation creates the passed Physical Location. -func (to *Session) CreatePhysLocation(pl tc.PhysLocation, opts RequestOptions) (tc.Alerts, toclientlib.ReqInf, error) { +func (to *Session) CreatePhysLocation(pl tc.PhysLocationV5, opts RequestOptions) (tc.Alerts, toclientlib.ReqInf, error) { if pl.RegionID == 0 && pl.RegionName != "" { regionOpts := NewRequestOptions() regionOpts.QueryParameters.Set("name", pl.RegionName) @@ -47,7 +47,7 @@ func (to *Session) CreatePhysLocation(pl tc.PhysLocation, opts RequestOptions) ( // UpdatePhysLocation replaces the Physical Location identified by 'id' with // the given Physical Location structure. -func (to *Session) UpdatePhysLocation(id int, pl tc.PhysLocation, opts RequestOptions) (tc.Alerts, toclientlib.ReqInf, error) { +func (to *Session) UpdatePhysLocation(id int, pl tc.PhysLocationV5, opts RequestOptions) (tc.Alerts, toclientlib.ReqInf, error) { route := fmt.Sprintf("%s/%d", apiPhysLocations, id) var alerts tc.Alerts reqInf, err := to.put(route, opts, pl, &alerts) @@ -55,8 +55,8 @@ func (to *Session) UpdatePhysLocation(id int, pl tc.PhysLocation, opts RequestOp } // GetPhysLocations retrieves Physical Locations from Traffic Ops. -func (to *Session) GetPhysLocations(opts RequestOptions) (tc.PhysLocationsResponse, toclientlib.ReqInf, error) { - var data tc.PhysLocationsResponse +func (to *Session) GetPhysLocations(opts RequestOptions) (tc.PhysLocationsResponseV5, toclientlib.ReqInf, error) { + var data tc.PhysLocationsResponseV5 reqInf, err := to.get(apiPhysLocations, opts, &data) return data, reqInf, err } From 00aae808fddcf24bd705cd003b73cc289c44c77b Mon Sep 17 00:00:00 2001 From: Jagan Parthiban Date: Tue, 11 Jul 2023 14:44:31 +0530 Subject: [PATCH 3/9] Documentation updates for https://github.com/apache/trafficcontrol/issues/7630 --- docs/source/api/v5/phys_locations.rst | 8 ++++---- docs/source/api/v5/phys_locations_id.rst | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/source/api/v5/phys_locations.rst b/docs/source/api/v5/phys_locations.rst index 9aae8be2b8..08cb486d04 100644 --- a/docs/source/api/v5/phys_locations.rst +++ b/docs/source/api/v5/phys_locations.rst @@ -71,7 +71,7 @@ Response Structure :comments: Any and all human-readable comments :email: The email address of the physical location's ``poc`` :id: An integral, unique identifier for the physical location -:lastUpdated: The date and time at which the physical location was last updated, in :ref:`non-rfc-datetime` +:lastUpdated: The date and time at which the physical location was last updated, in :rfc:`3339` Format :name: The name of the physical location :phone: A phone number where the the physical location's ``poc`` might be reached :poc: The name of a "point of contact" for the physical location @@ -103,7 +103,7 @@ Response Structure "comments": "", "email": "", "id": 2, - "lastUpdated": "2018-12-05 17:50:58+00", + "lastUpdated": "2018-12-05T18:56:27.057163+05:30", "name": "CDN_in_a_Box", "phone": "", "poc": "", @@ -173,7 +173,7 @@ Response Structure :comments: Any and all human-readable comments :email: The email address of the physical location's ``poc`` :id: An integral, unique identifier for the physical location -:lastUpdated: The date and time at which the physical location was last updated, in :ref:`non-rfc-datetime` +:lastUpdated: The date and time at which the physical location was last updated, in :rfc:`3339` :name: The name of the physical location :phone: A phone number where the the physical location's ``poc`` might be reached :poc: The name of a "point of contact" for the physical location @@ -210,7 +210,7 @@ Response Structure "comments": "Buckingham Palace", "email": "steve.kingstone@royal.gsx.gov.uk", "id": 3, - "lastUpdated": "2018-12-06 00:14:47+00", + "lastUpdated": "2018-12-06T18:56:27.057163+05:30", "name": "Great_Britain", "phone": "0-843-816-6276", "poc": "Her Majesty The Queen Elizabeth Alexandra Mary Windsor II", diff --git a/docs/source/api/v5/phys_locations_id.rst b/docs/source/api/v5/phys_locations_id.rst index b945bf2177..bdeda10bb0 100644 --- a/docs/source/api/v5/phys_locations_id.rst +++ b/docs/source/api/v5/phys_locations_id.rst @@ -85,7 +85,7 @@ Response Structure :comments: Any and all human-readable comments :email: The email address of the physical location's ``poc`` :id: An integral, unique identifier for the physical location -:lastUpdated: The date and time at which the physical location was last updated, in :ref:`non-rfc-datetime` +:lastUpdated: The date and time at which the physical location was last updated, in :rfc:`3339` :name: The name of the physical location :phone: A phone number where the the physical location's ``poc`` might be reached :poc: The name of a "point of contact" for the physical location @@ -122,7 +122,7 @@ Response Structure "comments": "The White House", "email": "the@white.house", "id": 2, - "lastUpdated": "2018-12-05 23:39:17+00", + "lastUpdated": "2018-12-05T18:56:27.057163+05:30", "name": "CDN_in_a_Box", "phone": "1-202-456-1414", "poc": "Donald J. Trump", From bdab8218f38dedff0f836bd1675d7f5c86319cc5 Mon Sep 17 00:00:00 2001 From: Jagan Parthiban Date: Tue, 11 Jul 2023 15:12:42 +0530 Subject: [PATCH 4/9] CHANGELOG.md updates for https://github.com/apache/trafficcontrol/issues/7630 --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 04c45b4d47..0d7fd15cbe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -67,6 +67,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). ### Fixed - [#4393](https://github.com/apache/trafficcontrol/issues/4393) *Traffic Ops* Fixed the error code and alert structure when TO is queried for a delivery service with no ssl keys. +- [#7631] (https://github.com/apache/trafficcontrol/pull/7631) *Traffic Ops* Fixes Phys_Location V5 apis to respond with RFC3339 date/time Format - [#7623] (https://github.com/apache/trafficcontrol/pull/7623) *Traffic Ops* Removed TryIfModifiedSinceQuery from servicecategories.go and reused from ims.go - [#7608](https://github.com/apache/trafficcontrol/pull/7608) *Traffic Monitor* Use stats_over_http(plugin.system_stats.timestamp_ms) timestamp field to calculate bandwidth for TM's caches. - [#7612](https://github.com/apache/trafficcontrol/pull/7612) *Traffic Ops* Fixes Divisions V5 apis to respond with RFC3339 date/time Format From 34ab8435795fe869486fba8a178b111a504037ac Mon Sep 17 00:00:00 2001 From: Jagan Parthiban Date: Fri, 14 Jul 2023 09:48:29 +0530 Subject: [PATCH 5/9] PR Review comment Updates for https://github.com/apache/trafficcontrol/issues/7630 --- .../physlocation/phys_locations.go | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go b/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go index 9f781a8779..af83531888 100644 --- a/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go +++ b/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go @@ -24,17 +24,17 @@ import ( "encoding/json" "errors" "fmt" - "github.com/apache/trafficcontrol/lib/go-log" - "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/util/ims" "net/http" "strconv" "time" + "github.com/apache/trafficcontrol/lib/go-log" "github.com/apache/trafficcontrol/lib/go-tc" "github.com/apache/trafficcontrol/lib/go-tc/tovalidate" "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/dbhelpers" + "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/util/ims" validation "github.com/go-ozzo/ozzo-validation" ) @@ -450,19 +450,18 @@ func DeletePhysLocation(w http.ResponseWriter, r *http.Request) { defer inf.Close() id := inf.Params["id"] + if id == "" { + api.HandleErr(w, r, tx, http.StatusBadRequest, fmt.Errorf("couldn't delete Phys_Location. Invalid ID. Id Cannot be empty for Delete Operation"), nil) + return + } exists, err := dbhelpers.PhysLocationExists(tx, id) if err != nil { api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, err) return } if !exists { - if id != "" { - api.HandleErr(w, r, tx, http.StatusNotFound, fmt.Errorf("no PhysLocation exists by id: %s", id), nil) - return - } else { - api.HandleErr(w, r, tx, http.StatusBadRequest, fmt.Errorf("no PhysLocation exists for empty id: %s", id), nil) - return - } + api.HandleErr(w, r, tx, http.StatusNotFound, fmt.Errorf("no PhysLocation exists by id: %s", id), nil) + return } assignedServer := 0 From 1998b1ea7587dec13460515983296bea9a7e13b0 Mon Sep 17 00:00:00 2001 From: Jagan Parthiban Date: Mon, 17 Jul 2023 15:48:46 +0530 Subject: [PATCH 6/9] PR Review comment Updates for https://github.com/apache/trafficcontrol/issues/7630 --- traffic_ops/traffic_ops_golang/physlocation/phys_locations.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go b/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go index af83531888..44c39bca09 100644 --- a/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go +++ b/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go @@ -267,8 +267,8 @@ func GetPhysLocation(w http.ResponseWriter, r *http.Request) { } defer log.Close(rows, "unable to close DB connection") - physLocation := tc.PhysLocationV5{} - physLocationList := []tc.PhysLocationV5{} + physLocation := tc.PhysLocationNullableV5{} + physLocationList := []tc.PhysLocationNullableV5{} for rows.Next() { if err = rows.Scan(&physLocation.Address, &physLocation.City, &physLocation.Comments, &physLocation.Email, &physLocation.ID, &physLocation.LastUpdated, &physLocation.Name, &physLocation.Phone, &physLocation.POC, &physLocation.RegionID, &physLocation.RegionName, &physLocation.ShortName, &physLocation.State, &physLocation.Zip); err != nil { api.HandleErr(w, r, tx.Tx, http.StatusInternalServerError, nil, fmt.Errorf("error getting physLocation(s): %w", err)) From 13ff9f51c36dfbdbd963e42dc42585f03026e0b1 Mon Sep 17 00:00:00 2001 From: Jagan Parthiban Date: Wed, 19 Jul 2023 11:56:53 +0530 Subject: [PATCH 7/9] PR Review comment Updates for https://github.com/apache/trafficcontrol/issues/7630 --- .../physlocation/phys_locations.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go b/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go index 44c39bca09..b24149fb03 100644 --- a/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go +++ b/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go @@ -296,6 +296,22 @@ func CreatePhysLocation(w http.ResponseWriter, r *http.Request) { return } + // checks to see if the supplied region name and ID in the phys_location body correspond to each other. + if physLocation.RegionName != "" { + regionName, ok, err := dbhelpers.GetRegionNameFromID(tx, physLocation.RegionID) + if err != nil { + api.HandleErr(w, r, tx, http.StatusInternalServerError, fmt.Errorf("error fetching name from region ID: %w", err), nil) + return + } else if !ok { + api.HandleErr(w, r, tx, http.StatusNotFound, errors.New("no such region"), nil) + return + } + if regionName != physLocation.RegionName { + api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("region name and ID do not match"), nil) + return + } + } + // check if phys_location already exists var count int err := tx.QueryRow("SELECT count(*) from phys_location where name = $1", physLocation.Name).Scan(&count) From c7a62eeb0f1816fb693f2f74af2157f2dab95f3c Mon Sep 17 00:00:00 2001 From: Jagan Parthiban Date: Thu, 20 Jul 2023 16:07:06 +0530 Subject: [PATCH 8/9] PR Review comment Updates for https://github.com/apache/trafficcontrol/issues/7630 --- .../physlocation/phys_locations.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go b/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go index b24149fb03..db5e744aef 100644 --- a/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go +++ b/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go @@ -390,6 +390,22 @@ func UpdatePhysLocation(w http.ResponseWriter, r *http.Request) { return } + // checks to see if the supplied region name and ID in the phys_location body correspond to each other. + if physLocation.RegionName != "" { + regionName, ok, err := dbhelpers.GetRegionNameFromID(tx, physLocation.RegionID) + if err != nil { + api.HandleErr(w, r, tx, http.StatusInternalServerError, fmt.Errorf("error fetching name from region ID: %w", err), nil) + return + } else if !ok { + api.HandleErr(w, r, tx, http.StatusNotFound, errors.New("no such region"), nil) + return + } + if regionName != physLocation.RegionName { + api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("region name and ID do not match"), nil) + return + } + } + requestedID := inf.Params["id"] intRequestId, convErr := strconv.Atoi(requestedID) From 72fc6fe610c7709568aab27afc43a4c630cf153b Mon Sep 17 00:00:00 2001 From: Jagan Parthiban Date: Fri, 21 Jul 2023 23:09:20 +0530 Subject: [PATCH 9/9] PR Review comment Updates for https://github.com/apache/trafficcontrol/issues/7630 --- traffic_ops/traffic_ops_golang/physlocation/phys_locations.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go b/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go index db5e744aef..08770fb52e 100644 --- a/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go +++ b/traffic_ops/traffic_ops_golang/physlocation/phys_locations.go @@ -520,7 +520,7 @@ func DeletePhysLocation(w http.ResponseWriter, r *http.Request) { return } alerts := tc.CreateAlerts(tc.SuccessLevel, "phys_location was deleted.") - api.WriteAlertsObj(w, r, http.StatusOK, alerts, inf.Params) + api.WriteAlerts(w, r, http.StatusOK, alerts) return }