rob05c commented on a change in pull request #4942:
URL: https://github.com/apache/trafficcontrol/pull/4942#discussion_r468289446
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/request/requests.go
##########
@@ -383,6 +401,20 @@ func (req *deliveryServiceRequestAssignment) Update()
(error, error, int) {
req.DeliveryServiceRequestNullable =
current.DeliveryServiceRequestNullable
req.AssigneeID = assigneeID
+ err, existingLastUpdated := CheckIfExistsBeforeUpdate(req.ReqInfo.Tx,
*req.ID)
Review comment:
This whole block seems to be identical in a lot of places. Could it be
abstracted into something like `func IsUnmodified(tx *sql.Tx, id int, tableName
string, hdr http.Header) bool` ?
##########
File path: traffic_ops/traffic_ops_golang/user/user.go
##########
@@ -266,6 +286,20 @@ func (user *TOUser) Update() (error, error, int) {
}
}
+ err, existingLastUpdated := CheckIfExistsBeforeUpdate(user.ReqInfo.Tx,
*user.ID)
+ if err != nil {
+ return errors.New("no server found with this id"), nil,
http.StatusNotFound
+ }
+ _, iumsTime := rfc.GetETagOrIfUnmodifiedSinceTime(h)
+ existingEtag := rfc.ETag(existingLastUpdated.Time)
+
+ if h.Get(rfc.IfMatch) != "" && !strings.Contains(h.Get(rfc.IfMatch),
existingEtag) {
+ return errors.New("header preconditions dont match"), nil,
http.StatusPreconditionFailed
+ }
+ if iumsTime != nil && existingLastUpdated.UTC().After(iumsTime.UTC()) {
Review comment:
If the header was an `If-Unmodified-Since`, the `HTTP-Date` only has
second-precison. Which means, if we simply round or truncate when setting
`Last-Modified`, `If-Modified-Since` will always be true, and
`If-Unmodified-Since` will always be false.
But ETag has as much precision as we want to give it, and we definitely want
to keep that precision in the check.
It could be changed in your code here, to add a second if it's exactly
rounded to the second (and therefore an `IUS` not `IM`).
But I think the better solution is to leave your code here as it is, and fix
the place(s) that set `LastModified` to use
`lastModified.Truncate(time.Second).Add(time.Second)`
##########
File path: traffic_ops/traffic_ops_golang/origin/origins.go
##########
@@ -309,6 +309,7 @@ func (origin *TOOrigin) Update() (error, error, int) {
}
log.Debugf("about to run exec query: %s with origin: %++v",
updateQuery(), origin)
+ // ToDo : Srijeet change here
Review comment:
I assume an Unmodified check needs to go here?
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/request/requests.go
##########
@@ -226,7 +244,7 @@ func (req *TODeliveryServiceRequest) Update() (error,
error, int) {
userID := tc.IDNoMod(req.APIInfo().User.ID)
req.LastEditedByID = &userID
- return api.GenericUpdate(req)
+ return api.GenericUpdate(nil, req)
Review comment:
Why is this passing `nil`, and not the `http.Header`?
##########
File path: traffic_ops/traffic_ops_golang/cachegroup/cachegroups.go
##########
@@ -567,6 +582,22 @@ func (cg *TOCacheGroup) Update() (error, error, int) {
return nil, nil, http.StatusOK
}
+func CheckIfExistsBeforeUpdate(tx *sqlx.Tx, cgID int) (error, *tc.TimeNoMod) {
Review comment:
These functions look identical, except for the table in the select query.
Could they be refactored to use a common func, maybe something like
```
// GetLastUpdated returns the last updated time of the given ID in the given
table, whether the ID existed, and any error.
func GetLastUpdated(tx *sqlx.Tx, dbTable string, id int) (time.Time, bool,
error)
````
In the `api` package?
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/safe.go
##########
@@ -23,11 +23,10 @@ import (
"database/sql"
"fmt"
"github.com/apache/trafficcontrol/lib/go-log"
- "net/http"
-
"github.com/apache/trafficcontrol/lib/go-tc"
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/api"
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/tenant"
+ "net/http"
Review comment:
We try to separate imports with newlines for "standard library," "third
party", and "our own," just to make it a little easier to read. Would you mind
moving this up with the other standard library imports? Mind also moving
`lib/go-log` down with the other TC ones? (that one wasn't yours, but while
you're improving it, might as well do that one too).
##########
File path: traffic_ops/traffic_ops_golang/steeringtargets/steeringtargets.go
##########
@@ -244,6 +244,7 @@ func (st *TOSteeringTargetV11) Update() (error, error, int)
{
return userErr, sysErr, errCode
}
+ // ToDo : Srijeet change here
Review comment:
Need an Unmodified check here?
##########
File path: traffic_ops/traffic_ops_golang/server/servers_server_capability.go
##########
@@ -121,7 +121,7 @@ func (ssc TOServerServerCapability) Validate() error {
return util.JoinErrs(tovalidate.ToErrors(errs))
}
-func (ssc *TOServerServerCapability) Update() (error, error, int) {
+func (ssc *TOServerServerCapability) Update(h http.Header) (error, error, int)
{
Review comment:
I know you didn't create this function, but it can be deleted.
This is a legacy from when the "CRUDer" stuff required every func be
implemented. This endpoint doesn't have a PUT method, and the "CRUDer"
framework no longer requires all funcs, so it can just be deleted now.
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/request/requests.go
##########
@@ -383,6 +401,20 @@ func (req *deliveryServiceRequestAssignment) Update()
(error, error, int) {
req.DeliveryServiceRequestNullable =
current.DeliveryServiceRequestNullable
req.AssigneeID = assigneeID
+ err, existingLastUpdated := CheckIfExistsBeforeUpdate(req.ReqInfo.Tx,
*req.ID)
+ if err != nil {
+ return errors.New("no delivery service request found with this
id"), nil, http.StatusNotFound
+ }
+ _, iumsTime := rfc.GetETagOrIfUnmodifiedSinceTime(h)
+ existingEtag := rfc.ETag(existingLastUpdated.Time)
+
+ if h.Get(rfc.IfMatch) != "" && !strings.Contains(h.Get(rfc.IfMatch),
existingEtag) {
+ return errors.New("header preconditions dont match"), nil,
http.StatusPreconditionFailed
Review comment:
The text here doesn't have to match the code, and since it's a user
error, I think it's safe (from a security perspective) to let them know why
they're getting a 412. Could we make the message something more informative
like "resource was modified" ?
----------------------------------------------------------------
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]
