ocket8888 commented on code in PR #7733:
URL: https://github.com/apache/trafficcontrol/pull/7733#discussion_r1303462079


##########
docs/source/api/v5/origins.rst:
##########
@@ -89,7 +89,7 @@ Response Structure
 :ip6Address:        The IPv6 address of the :term:`Origin`
 :ipAddress:         The IPv4 address of the :term:`Origin`
 :isPrimary:         A boolean value which, when ``true`` specifies this 
:term:`Origin` as the 'primary' :term:`Origin` served by ``deliveryService``
-:lastUpdated:       The date and time at which this :term:`Origin` was last 
modified
+:lastUpdated:       The date and time at which this :term:`Origin` was last 
modified in :rfc:`3339` Format

Review Comment:
   "Format" should not be capitalized



##########
lib/go-tc/origins.go:
##########
@@ -54,3 +56,74 @@ type Origin struct {
        Tenant            *string    `json:"tenant" db:"tenant"`
        TenantID          *int       `json:"tenantId" db:"tenant_id"`
 }
+
+// OriginsResponseV5 is an alias for the latest minor version of the major 
version 5.
+type OriginsResponseV5 = OriginsResponseV50
+
+// OriginsResponseV50 is a list of Origins as a response for APIv5.
+type OriginsResponseV50 struct {
+       Response []OriginV5 `json:"response"`
+       Alerts
+}
+
+// OriginDetailResponseV5 is an alias for the latest minor version of the 
major version 5.
+type OriginDetailResponseV5 = OriginDetailResponseV50
+
+// OriginDetailResponseV50 is the JSON object returned for a single origin in 
APIv5.
+type OriginDetailResponseV50 struct {
+       Response OriginV5 `json:"response"`
+       Alerts
+}
+
+// OriginV5 is an alias for the latest minor version of the major version 5.
+type OriginV5 = OriginV50
+
+// OriginV50 contains information relating to an Origin, in the latest minor 
version APIv50.
+type OriginV50 struct {
+       Cachegroup        *string    `json:"cachegroup" db:"cachegroup"`
+       CachegroupID      *int       `json:"cachegroupId" db:"cachegroup_id"`
+       Coordinate        *string    `json:"coordinate" db:"coordinate"`
+       CoordinateID      *int       `json:"coordinateId" db:"coordinate_id"`
+       DeliveryService   *string    `json:"deliveryService" 
db:"deliveryservice"`
+       DeliveryServiceID *int       `json:"deliveryServiceId" 
db:"deliveryservice_id"`
+       FQDN              *string    `json:"fqdn" db:"fqdn"`
+       ID                *int       `json:"id" db:"id"`
+       IP6Address        *string    `json:"ip6Address" db:"ip6_address"`
+       IPAddress         *string    `json:"ipAddress" db:"ip_address"`
+       IsPrimary         *bool      `json:"isPrimary" db:"is_primary"`
+       LastUpdated       *time.Time `json:"lastUpdated" db:"last_updated"`
+       Name              *string    `json:"name" db:"name"`
+       Port              *int       `json:"port" db:"port"`
+       Profile           *string    `json:"profile" db:"profile"`
+       ProfileID         *int       `json:"profileId" db:"profile_id"`
+       Protocol          *string    `json:"protocol" db:"protocol"`
+       Tenant            *string    `json:"tenant" db:"tenant"`
+       TenantID          *int       `json:"tenantId" db:"tenant_id"`

Review Comment:
   These values are never allowed to be `nil`, so they shouldn't be reference 
types



##########
lib/go-tc/origins.go:
##########
@@ -54,3 +56,74 @@ type Origin struct {
        Tenant            *string    `json:"tenant" db:"tenant"`
        TenantID          *int       `json:"tenantId" db:"tenant_id"`
 }
+
+// OriginsResponseV5 is an alias for the latest minor version of the major 
version 5.
+type OriginsResponseV5 = OriginsResponseV50
+
+// OriginsResponseV50 is a list of Origins as a response for APIv5.
+type OriginsResponseV50 struct {
+       Response []OriginV5 `json:"response"`
+       Alerts
+}
+
+// OriginDetailResponseV5 is an alias for the latest minor version of the 
major version 5.
+type OriginDetailResponseV5 = OriginDetailResponseV50
+
+// OriginDetailResponseV50 is the JSON object returned for a single origin in 
APIv5.
+type OriginDetailResponseV50 struct {
+       Response OriginV5 `json:"response"`
+       Alerts
+}
+
+// OriginV5 is an alias for the latest minor version of the major version 5.
+type OriginV5 = OriginV50
+
+// OriginV50 contains information relating to an Origin, in the latest minor 
version APIv50.
+type OriginV50 struct {
+       Cachegroup        *string    `json:"cachegroup" db:"cachegroup"`
+       CachegroupID      *int       `json:"cachegroupId" db:"cachegroup_id"`
+       Coordinate        *string    `json:"coordinate" db:"coordinate"`
+       CoordinateID      *int       `json:"coordinateId" db:"coordinate_id"`
+       DeliveryService   *string    `json:"deliveryService" 
db:"deliveryservice"`
+       DeliveryServiceID *int       `json:"deliveryServiceId" 
db:"deliveryservice_id"`
+       FQDN              *string    `json:"fqdn" db:"fqdn"`
+       ID                *int       `json:"id" db:"id"`

Review Comment:
   These values are never allowed to be `nil`, so they shouldn't be reference 
types



##########
traffic_ops/traffic_ops_golang/origin/origins.go:
##########
@@ -487,3 +488,217 @@ func deleteQuery() string {
 WHERE id=:id`
        return query
 }
+
+// Get is the handler for GET requests to Origins of APIv5

Review Comment:
   GoDoc comment should end with punctuation.



##########
traffic_ops/traffic_ops_golang/origin/origins.go:
##########
@@ -487,3 +488,217 @@ func deleteQuery() string {
 WHERE id=:id`
        return query
 }
+
+// Get is the handler for GET requests to Origins of APIv5
+func Get(w http.ResponseWriter, r *http.Request) {
+
+       var useIMS bool
+
+       inf, sysErr, userErr, errCode := api.NewInfo(r, nil, nil)
+       if sysErr != nil || userErr != nil {
+               api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+               return
+       }
+       defer inf.Close()
+
+       origins, userErr, sysErr, errCode, maxTime := getOrigins(w.Header(), 
inf.Params, inf.Tx, inf.User, useIMS)
+       var returnable []tc.OriginV5
+       if userErr != nil || sysErr != nil {
+               api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+               return
+       }
+
+       for _, origin := range origins {
+               returnable = append(returnable, origin.ToOriginV5())
+       }
+       _ = maxTime // consider removing maxTime if it's not used in getOrigins 
in future
+
+       api.WriteResp(w, r, returnable)
+       return
+}
+
+// Create Origin with the passed data for APIv5.
+func Create(w http.ResponseWriter, r *http.Request) {
+       inf, sysError, userError, errorCode := api.NewInfo(r, nil, nil)
+       tx := inf.Tx
+       if sysError != nil || userError != nil {
+               api.HandleErr(w, r, inf.Tx.Tx, errorCode, userError, sysError)
+               return
+       }
+       defer inf.Close()
+       org, readValErr := readAndValidateJsonStruct(r)
+       if readValErr != nil {
+               api.HandleErr(w, r, tx.Tx, http.StatusBadRequest, readValErr, 
nil)
+               return
+       }
+
+       userErr, sysErr, errCode := checkTenancy(org.TenantID, 
org.DeliveryServiceID, tx, inf.User)
+       if userErr != nil || sysErr != nil {
+               api.HandleErr(w, r, tx.Tx, errCode, userErr, sysErr)
+               return
+       }
+
+       _, cdnName, _, err := dbhelpers.GetDSNameAndCDNFromID(inf.Tx.Tx, 
*org.DeliveryServiceID)
+       if err != nil {
+               api.HandleErr(w, r, tx.Tx, http.StatusInternalServerError, 
fmt.Errorf("Error checking the database for delivery service name and cdn, 
whether it existed, and any error"), nil)
+               return
+       }
+       userErr, sysErr, errCode = 
dbhelpers.CheckIfCurrentUserCanModifyCDN(tx.Tx, string(cdnName), 
inf.User.UserName)
+       if userErr != nil || sysErr != nil {
+               api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+               return
+       }
+
+       resultRows, err := tx.NamedQuery(insertQuery(), org)
+       if err != nil {
+               usrErr, sysErr, code := api.ParseDBError(err)
+               api.HandleErr(w, r, tx.Tx, code, usrErr, sysErr)
+               return
+       }
+       defer resultRows.Close()
+
+       rowsAffected := 0
+       for resultRows.Next() {
+               rowsAffected++
+               if err := resultRows.Scan(&org.ID, &org.LastUpdated); err != 
nil {
+                       api.HandleErr(w, r, tx.Tx, 
http.StatusInternalServerError, fmt.Errorf("origin create: scanning: 
"+err.Error()), nil)

Review Comment:
   constructing an error from another error should use a wrapping construction



##########
traffic_ops/traffic_ops_golang/origin/origins.go:
##########
@@ -487,3 +488,217 @@ func deleteQuery() string {
 WHERE id=:id`
        return query
 }
+
+// Get is the handler for GET requests to Origins of APIv5
+func Get(w http.ResponseWriter, r *http.Request) {
+
+       var useIMS bool
+
+       inf, sysErr, userErr, errCode := api.NewInfo(r, nil, nil)
+       if sysErr != nil || userErr != nil {
+               api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+               return
+       }
+       defer inf.Close()
+
+       origins, userErr, sysErr, errCode, maxTime := getOrigins(w.Header(), 
inf.Params, inf.Tx, inf.User, useIMS)
+       var returnable []tc.OriginV5
+       if userErr != nil || sysErr != nil {
+               api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+               return
+       }
+
+       for _, origin := range origins {
+               returnable = append(returnable, origin.ToOriginV5())
+       }
+       _ = maxTime // consider removing maxTime if it's not used in getOrigins 
in future
+
+       api.WriteResp(w, r, returnable)
+       return
+}
+
+// Create Origin with the passed data for APIv5.
+func Create(w http.ResponseWriter, r *http.Request) {
+       inf, sysError, userError, errorCode := api.NewInfo(r, nil, nil)
+       tx := inf.Tx
+       if sysError != nil || userError != nil {
+               api.HandleErr(w, r, inf.Tx.Tx, errorCode, userError, sysError)
+               return
+       }
+       defer inf.Close()
+       org, readValErr := readAndValidateJsonStruct(r)
+       if readValErr != nil {
+               api.HandleErr(w, r, tx.Tx, http.StatusBadRequest, readValErr, 
nil)
+               return
+       }
+
+       userErr, sysErr, errCode := checkTenancy(org.TenantID, 
org.DeliveryServiceID, tx, inf.User)
+       if userErr != nil || sysErr != nil {
+               api.HandleErr(w, r, tx.Tx, errCode, userErr, sysErr)
+               return
+       }
+
+       _, cdnName, _, err := dbhelpers.GetDSNameAndCDNFromID(inf.Tx.Tx, 
*org.DeliveryServiceID)
+       if err != nil {
+               api.HandleErr(w, r, tx.Tx, http.StatusInternalServerError, 
fmt.Errorf("Error checking the database for delivery service name and cdn, 
whether it existed, and any error"), nil)
+               return
+       }
+       userErr, sysErr, errCode = 
dbhelpers.CheckIfCurrentUserCanModifyCDN(tx.Tx, string(cdnName), 
inf.User.UserName)
+       if userErr != nil || sysErr != nil {
+               api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+               return
+       }
+
+       resultRows, err := tx.NamedQuery(insertQuery(), org)
+       if err != nil {
+               usrErr, sysErr, code := api.ParseDBError(err)
+               api.HandleErr(w, r, tx.Tx, code, usrErr, sysErr)
+               return
+       }
+       defer resultRows.Close()
+
+       rowsAffected := 0
+       for resultRows.Next() {
+               rowsAffected++
+               if err := resultRows.Scan(&org.ID, &org.LastUpdated); err != 
nil {
+                       api.HandleErr(w, r, tx.Tx, 
http.StatusInternalServerError, fmt.Errorf("origin create: scanning: 
"+err.Error()), nil)
+                       return
+               }
+       }
+       if rowsAffected == 0 {
+               api.HandleErr(w, r, tx.Tx, http.StatusInternalServerError, 
fmt.Errorf("origin create: no rows returned"), nil)
+               return
+       } else if rowsAffected > 1 {
+               api.HandleErr(w, r, tx.Tx, http.StatusInternalServerError, 
fmt.Errorf("origin create: multiple rows returned"), nil)
+               return
+       }
+
+       alerts := tc.CreateAlerts(tc.SuccessLevel, "origin was created.")
+       api.WriteAlertsObj(w, r, http.StatusCreated, alerts, org)
+}
+
+// Update a Origin for APIv5.
+func Update(w http.ResponseWriter, r *http.Request) {
+       inf, sysError, userError, errorCode := api.NewInfo(r, []string{"id"}, 
[]string{"id"})
+       tx := inf.Tx
+       if sysError != nil || userError != nil {
+               api.HandleErr(w, r, inf.Tx.Tx, errorCode, userError, sysError)
+               return
+       }
+       defer inf.Close()
+
+       origin, readValErr := readAndValidateJsonStruct(r)
+       if readValErr != nil {
+               api.HandleErr(w, r, tx.Tx, http.StatusBadRequest, readValErr, 
nil)
+               return
+       }
+       userErr, sysErr, errCode := checkTenancy(origin.TenantID, 
origin.DeliveryServiceID, tx, inf.User)
+       if userErr != nil || sysErr != nil {
+               api.HandleErr(w, r, tx.Tx, errCode, userErr, sysErr)
+               return
+       }
+
+       isPrimary := false
+       ds := 0
+       var existingLastUpdated time.Time
+
+       requestedOriginId := inf.IntParams["id"]
+
+       q := `SELECT is_primary, deliveryservice, last_updated FROM origin 
WHERE id = $1`
+       if err := tx.QueryRow(q, requestedOriginId).Scan(&isPrimary, &ds, 
&existingLastUpdated); err != nil {
+               if err == sql.ErrNoRows {

Review Comment:
   Error identity checking should use [`errors.Is`](pkg.go.dev/errors#Is)



##########
traffic_ops/traffic_ops_golang/origin/origins.go:
##########
@@ -487,3 +488,217 @@ func deleteQuery() string {
 WHERE id=:id`
        return query
 }
+
+// Get is the handler for GET requests to Origins of APIv5
+func Get(w http.ResponseWriter, r *http.Request) {
+
+       var useIMS bool
+
+       inf, sysErr, userErr, errCode := api.NewInfo(r, nil, nil)
+       if sysErr != nil || userErr != nil {
+               api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+               return
+       }
+       defer inf.Close()
+
+       origins, userErr, sysErr, errCode, maxTime := getOrigins(w.Header(), 
inf.Params, inf.Tx, inf.User, useIMS)
+       var returnable []tc.OriginV5
+       if userErr != nil || sysErr != nil {
+               api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+               return
+       }
+
+       for _, origin := range origins {
+               returnable = append(returnable, origin.ToOriginV5())
+       }
+       _ = maxTime // consider removing maxTime if it's not used in getOrigins 
in future
+
+       api.WriteResp(w, r, returnable)
+       return
+}
+
+// Create Origin with the passed data for APIv5.
+func Create(w http.ResponseWriter, r *http.Request) {
+       inf, sysError, userError, errorCode := api.NewInfo(r, nil, nil)
+       tx := inf.Tx
+       if sysError != nil || userError != nil {
+               api.HandleErr(w, r, inf.Tx.Tx, errorCode, userError, sysError)
+               return
+       }
+       defer inf.Close()
+       org, readValErr := readAndValidateJsonStruct(r)
+       if readValErr != nil {
+               api.HandleErr(w, r, tx.Tx, http.StatusBadRequest, readValErr, 
nil)
+               return
+       }
+
+       userErr, sysErr, errCode := checkTenancy(org.TenantID, 
org.DeliveryServiceID, tx, inf.User)
+       if userErr != nil || sysErr != nil {
+               api.HandleErr(w, r, tx.Tx, errCode, userErr, sysErr)
+               return
+       }
+
+       _, cdnName, _, err := dbhelpers.GetDSNameAndCDNFromID(inf.Tx.Tx, 
*org.DeliveryServiceID)
+       if err != nil {
+               api.HandleErr(w, r, tx.Tx, http.StatusInternalServerError, 
fmt.Errorf("Error checking the database for delivery service name and cdn, 
whether it existed, and any error"), nil)
+               return
+       }
+       userErr, sysErr, errCode = 
dbhelpers.CheckIfCurrentUserCanModifyCDN(tx.Tx, string(cdnName), 
inf.User.UserName)
+       if userErr != nil || sysErr != nil {
+               api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+               return
+       }
+
+       resultRows, err := tx.NamedQuery(insertQuery(), org)
+       if err != nil {
+               usrErr, sysErr, code := api.ParseDBError(err)
+               api.HandleErr(w, r, tx.Tx, code, usrErr, sysErr)
+               return
+       }
+       defer resultRows.Close()
+
+       rowsAffected := 0
+       for resultRows.Next() {
+               rowsAffected++
+               if err := resultRows.Scan(&org.ID, &org.LastUpdated); err != 
nil {
+                       api.HandleErr(w, r, tx.Tx, 
http.StatusInternalServerError, fmt.Errorf("origin create: scanning: 
"+err.Error()), nil)
+                       return
+               }
+       }
+       if rowsAffected == 0 {
+               api.HandleErr(w, r, tx.Tx, http.StatusInternalServerError, 
fmt.Errorf("origin create: no rows returned"), nil)
+               return
+       } else if rowsAffected > 1 {
+               api.HandleErr(w, r, tx.Tx, http.StatusInternalServerError, 
fmt.Errorf("origin create: multiple rows returned"), nil)
+               return
+       }
+
+       alerts := tc.CreateAlerts(tc.SuccessLevel, "origin was created.")
+       api.WriteAlertsObj(w, r, http.StatusCreated, alerts, org)
+}
+
+// Update a Origin for APIv5.
+func Update(w http.ResponseWriter, r *http.Request) {
+       inf, sysError, userError, errorCode := api.NewInfo(r, []string{"id"}, 
[]string{"id"})
+       tx := inf.Tx
+       if sysError != nil || userError != nil {
+               api.HandleErr(w, r, inf.Tx.Tx, errorCode, userError, sysError)
+               return
+       }
+       defer inf.Close()
+
+       origin, readValErr := readAndValidateJsonStruct(r)
+       if readValErr != nil {
+               api.HandleErr(w, r, tx.Tx, http.StatusBadRequest, readValErr, 
nil)
+               return
+       }
+       userErr, sysErr, errCode := checkTenancy(origin.TenantID, 
origin.DeliveryServiceID, tx, inf.User)
+       if userErr != nil || sysErr != nil {
+               api.HandleErr(w, r, tx.Tx, errCode, userErr, sysErr)
+               return
+       }
+
+       isPrimary := false
+       ds := 0
+       var existingLastUpdated time.Time
+
+       requestedOriginId := inf.IntParams["id"]
+
+       q := `SELECT is_primary, deliveryservice, last_updated FROM origin 
WHERE id = $1`
+       if err := tx.QueryRow(q, requestedOriginId).Scan(&isPrimary, &ds, 
&existingLastUpdated); err != nil {
+               if err == sql.ErrNoRows {
+                       api.HandleErr(w, r, tx.Tx, http.StatusNotFound, 
fmt.Errorf("origin not found"), nil)
+                       return
+               }
+               api.HandleErr(w, r, tx.Tx, http.StatusInternalServerError, nil, 
fmt.Errorf("error: %s, when checking if origin with id %s exists", err.Error(), 
requestedOriginId))

Review Comment:
   Constructing an error from an error should use a wrapping construction.



##########
traffic_ops/traffic_ops_golang/origin/origins.go:
##########
@@ -487,3 +488,217 @@ func deleteQuery() string {
 WHERE id=:id`
        return query
 }
+
+// Get is the handler for GET requests to Origins of APIv5
+func Get(w http.ResponseWriter, r *http.Request) {
+
+       var useIMS bool
+
+       inf, sysErr, userErr, errCode := api.NewInfo(r, nil, nil)
+       if sysErr != nil || userErr != nil {
+               api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+               return
+       }
+       defer inf.Close()
+
+       origins, userErr, sysErr, errCode, maxTime := getOrigins(w.Header(), 
inf.Params, inf.Tx, inf.User, useIMS)
+       var returnable []tc.OriginV5
+       if userErr != nil || sysErr != nil {
+               api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+               return
+       }
+
+       for _, origin := range origins {
+               returnable = append(returnable, origin.ToOriginV5())
+       }
+       _ = maxTime // consider removing maxTime if it's not used in getOrigins 
in future

Review Comment:
   can we just do that right now?



##########
traffic_ops/traffic_ops_golang/origin/origins.go:
##########
@@ -487,3 +488,217 @@ func deleteQuery() string {
 WHERE id=:id`
        return query
 }
+
+// Get is the handler for GET requests to Origins of APIv5
+func Get(w http.ResponseWriter, r *http.Request) {
+
+       var useIMS bool
+
+       inf, sysErr, userErr, errCode := api.NewInfo(r, nil, nil)
+       if sysErr != nil || userErr != nil {
+               api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+               return
+       }
+       defer inf.Close()
+
+       origins, userErr, sysErr, errCode, maxTime := getOrigins(w.Header(), 
inf.Params, inf.Tx, inf.User, useIMS)
+       var returnable []tc.OriginV5

Review Comment:
   If zero origins match the filtering parameters sent by the client, this will 
cause it to return a `null` `response` property, when the proper representation 
for an empty list is the empty array, i.e. `[]`.



##########
lib/go-tc/origins.go:
##########
@@ -54,3 +56,74 @@ type Origin struct {
        Tenant            *string    `json:"tenant" db:"tenant"`
        TenantID          *int       `json:"tenantId" db:"tenant_id"`
 }
+
+// OriginsResponseV5 is an alias for the latest minor version of the major 
version 5.
+type OriginsResponseV5 = OriginsResponseV50
+
+// OriginsResponseV50 is a list of Origins as a response for APIv5.
+type OriginsResponseV50 struct {
+       Response []OriginV5 `json:"response"`
+       Alerts
+}
+
+// OriginDetailResponseV5 is an alias for the latest minor version of the 
major version 5.
+type OriginDetailResponseV5 = OriginDetailResponseV50
+
+// OriginDetailResponseV50 is the JSON object returned for a single origin in 
APIv5.
+type OriginDetailResponseV50 struct {
+       Response OriginV5 `json:"response"`
+       Alerts
+}
+
+// OriginV5 is an alias for the latest minor version of the major version 5.
+type OriginV5 = OriginV50
+
+// OriginV50 contains information relating to an Origin, in the latest minor 
version APIv50.
+type OriginV50 struct {
+       Cachegroup        *string    `json:"cachegroup" db:"cachegroup"`
+       CachegroupID      *int       `json:"cachegroupId" db:"cachegroup_id"`
+       Coordinate        *string    `json:"coordinate" db:"coordinate"`
+       CoordinateID      *int       `json:"coordinateId" db:"coordinate_id"`
+       DeliveryService   *string    `json:"deliveryService" 
db:"deliveryservice"`
+       DeliveryServiceID *int       `json:"deliveryServiceId" 
db:"deliveryservice_id"`
+       FQDN              *string    `json:"fqdn" db:"fqdn"`
+       ID                *int       `json:"id" db:"id"`
+       IP6Address        *string    `json:"ip6Address" db:"ip6_address"`
+       IPAddress         *string    `json:"ipAddress" db:"ip_address"`
+       IsPrimary         *bool      `json:"isPrimary" db:"is_primary"`
+       LastUpdated       *time.Time `json:"lastUpdated" db:"last_updated"`
+       Name              *string    `json:"name" db:"name"`

Review Comment:
   These values are never allowed to be `nil`, so they shouldn't be reference 
types



##########
traffic_ops/traffic_ops_golang/origin/origins.go:
##########
@@ -487,3 +488,217 @@ func deleteQuery() string {
 WHERE id=:id`
        return query
 }
+
+// Get is the handler for GET requests to Origins of APIv5
+func Get(w http.ResponseWriter, r *http.Request) {
+
+       var useIMS bool
+
+       inf, sysErr, userErr, errCode := api.NewInfo(r, nil, nil)
+       if sysErr != nil || userErr != nil {
+               api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+               return
+       }
+       defer inf.Close()
+
+       origins, userErr, sysErr, errCode, maxTime := getOrigins(w.Header(), 
inf.Params, inf.Tx, inf.User, useIMS)
+       var returnable []tc.OriginV5
+       if userErr != nil || sysErr != nil {
+               api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+               return
+       }
+
+       for _, origin := range origins {
+               returnable = append(returnable, origin.ToOriginV5())
+       }
+       _ = maxTime // consider removing maxTime if it's not used in getOrigins 
in future
+
+       api.WriteResp(w, r, returnable)
+       return
+}
+
+// Create Origin with the passed data for APIv5.
+func Create(w http.ResponseWriter, r *http.Request) {
+       inf, sysError, userError, errorCode := api.NewInfo(r, nil, nil)
+       tx := inf.Tx
+       if sysError != nil || userError != nil {
+               api.HandleErr(w, r, inf.Tx.Tx, errorCode, userError, sysError)
+               return
+       }
+       defer inf.Close()
+       org, readValErr := readAndValidateJsonStruct(r)
+       if readValErr != nil {
+               api.HandleErr(w, r, tx.Tx, http.StatusBadRequest, readValErr, 
nil)
+               return
+       }
+
+       userErr, sysErr, errCode := checkTenancy(org.TenantID, 
org.DeliveryServiceID, tx, inf.User)
+       if userErr != nil || sysErr != nil {
+               api.HandleErr(w, r, tx.Tx, errCode, userErr, sysErr)
+               return
+       }
+
+       _, cdnName, _, err := dbhelpers.GetDSNameAndCDNFromID(inf.Tx.Tx, 
*org.DeliveryServiceID)
+       if err != nil {
+               api.HandleErr(w, r, tx.Tx, http.StatusInternalServerError, 
fmt.Errorf("Error checking the database for delivery service name and cdn, 
whether it existed, and any error"), nil)

Review Comment:
   error messages should not begin with a capitalized word as though they were 
the beginning of a sentence, and they really shouldn't start with the word 
"error" at all, since that's implied by the fact that it's literally an 
`error`. Also, this message tells me nothing about what actually happened; all 
of that information is carried by `err`, which you aren't using.



##########
traffic_ops/traffic_ops_golang/origin/origins.go:
##########
@@ -487,3 +488,217 @@ func deleteQuery() string {
 WHERE id=:id`
        return query
 }
+
+// Get is the handler for GET requests to Origins of APIv5
+func Get(w http.ResponseWriter, r *http.Request) {
+
+       var useIMS bool
+
+       inf, sysErr, userErr, errCode := api.NewInfo(r, nil, nil)
+       if sysErr != nil || userErr != nil {
+               api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+               return
+       }
+       defer inf.Close()
+
+       origins, userErr, sysErr, errCode, maxTime := getOrigins(w.Header(), 
inf.Params, inf.Tx, inf.User, useIMS)
+       var returnable []tc.OriginV5
+       if userErr != nil || sysErr != nil {
+               api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+               return
+       }
+
+       for _, origin := range origins {
+               returnable = append(returnable, origin.ToOriginV5())
+       }
+       _ = maxTime // consider removing maxTime if it's not used in getOrigins 
in future
+
+       api.WriteResp(w, r, returnable)
+       return
+}
+
+// Create Origin with the passed data for APIv5.
+func Create(w http.ResponseWriter, r *http.Request) {
+       inf, sysError, userError, errorCode := api.NewInfo(r, nil, nil)
+       tx := inf.Tx
+       if sysError != nil || userError != nil {
+               api.HandleErr(w, r, inf.Tx.Tx, errorCode, userError, sysError)
+               return
+       }
+       defer inf.Close()
+       org, readValErr := readAndValidateJsonStruct(r)
+       if readValErr != nil {
+               api.HandleErr(w, r, tx.Tx, http.StatusBadRequest, readValErr, 
nil)
+               return
+       }
+
+       userErr, sysErr, errCode := checkTenancy(org.TenantID, 
org.DeliveryServiceID, tx, inf.User)
+       if userErr != nil || sysErr != nil {
+               api.HandleErr(w, r, tx.Tx, errCode, userErr, sysErr)
+               return
+       }
+
+       _, cdnName, _, err := dbhelpers.GetDSNameAndCDNFromID(inf.Tx.Tx, 
*org.DeliveryServiceID)
+       if err != nil {
+               api.HandleErr(w, r, tx.Tx, http.StatusInternalServerError, 
fmt.Errorf("Error checking the database for delivery service name and cdn, 
whether it existed, and any error"), nil)
+               return
+       }
+       userErr, sysErr, errCode = 
dbhelpers.CheckIfCurrentUserCanModifyCDN(tx.Tx, string(cdnName), 
inf.User.UserName)
+       if userErr != nil || sysErr != nil {
+               api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+               return
+       }
+
+       resultRows, err := tx.NamedQuery(insertQuery(), org)
+       if err != nil {
+               usrErr, sysErr, code := api.ParseDBError(err)
+               api.HandleErr(w, r, tx.Tx, code, usrErr, sysErr)
+               return
+       }
+       defer resultRows.Close()
+
+       rowsAffected := 0
+       for resultRows.Next() {
+               rowsAffected++
+               if err := resultRows.Scan(&org.ID, &org.LastUpdated); err != 
nil {
+                       api.HandleErr(w, r, tx.Tx, 
http.StatusInternalServerError, fmt.Errorf("origin create: scanning: 
"+err.Error()), nil)
+                       return
+               }
+       }
+       if rowsAffected == 0 {
+               api.HandleErr(w, r, tx.Tx, http.StatusInternalServerError, 
fmt.Errorf("origin create: no rows returned"), nil)
+               return
+       } else if rowsAffected > 1 {
+               api.HandleErr(w, r, tx.Tx, http.StatusInternalServerError, 
fmt.Errorf("origin create: multiple rows returned"), nil)
+               return
+       }
+
+       alerts := tc.CreateAlerts(tc.SuccessLevel, "origin was created.")
+       api.WriteAlertsObj(w, r, http.StatusCreated, alerts, org)

Review Comment:
   A `201 Created` response MUST be accompanied by a `Location` header which 
gives the URL to which the client may send a GET request to receive a 
representation of the created object.



-- 
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.

To unsubscribe, e-mail: issues-unsubscr...@trafficcontrol.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to