You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2020/11/03 13:31:45 UTC
[GitHub] [trafficcontrol] rawlinp opened a new pull request #5231: Fix service categories if-not-modified/if-match functionality
rawlinp opened a new pull request #5231:
URL: https://github.com/apache/trafficcontrol/pull/5231
## What does this PR (Pull Request) do?
This functionality was accidentally lost due to a recent change. Also,
add the missing last_deleted functionality in order for deletions to
count as modifications.
## Which Traffic Control components are affected by this PR?
- Traffic Ops
## What is the best way to verify this PR?
Verify the checks pass. Run the DB upgrade and downgrade, verify they're successful. Make `if-not-modified` and `if-match` `PUT /3.0/service_categories/{name}` with given timestamps and ETags, verify either 200 or 412 status codes as expected (this is what the added test does).
## If this is a bug fix, what versions of Traffic Control are affected?
- master
- 5.0.x
## The following criteria are ALL met by this PR
- [x] This PR includes tests
- [x] bug fix, no docs necessary
- [x] bug not yet released, no changelog necessary
- [x] This PR includes any and all required license headers
- [x] This PR ensures that database migration sequence is correct
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://www.apache.org/security/) for details)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #5231: Fix service categories if-not-modified/if-match functionality
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5231:
URL: https://github.com/apache/trafficcontrol/pull/5231#discussion_r517468476
##########
File path: traffic_ops/traffic_ops_golang/servicecategory/servicecategories.go
##########
@@ -149,14 +135,18 @@ func Update(w http.ResponseWriter, r *http.Request) {
}
var origSC TOServiceCategory
- if err := inf.Tx.QueryRow(`SELECT name FROM service_category WHERE name = $1`, name).Scan(&origSC.Name); err != nil {
+ if err := inf.Tx.QueryRow(`SELECT name, last_updated FROM service_category WHERE name = $1`, name).Scan(&origSC.Name, &origSC.LastUpdated); err != nil {
if err == sql.ErrNoRows {
api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("no service category found with name "+name), nil)
return
}
api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, err)
return
}
+ if !api.IsUnmodified(r.Header, origSC.LastUpdated.Time) {
+ api.HandleErr(w, r, inf.Tx.Tx, http.StatusPreconditionFailed, errors.New("service category has been modified"), nil)
Review comment:
Fixed in https://github.com/apache/trafficcontrol/pull/5231/commits/7230f480a8a75cdf9c14c8ceb2f95f88ee02b26d
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #5231: Fix service categories if-not-modified/if-match functionality
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5231:
URL: https://github.com/apache/trafficcontrol/pull/5231#discussion_r517707548
##########
File path: traffic_ops/v3-client/serviceCategory.go
##########
@@ -51,20 +51,22 @@ func (to *Session) CreateServiceCategory(serviceCategory tc.ServiceCategory) (tc
}
// UpdateServiceCategoryByName updates a service category by its unique name.
-func (to *Session) UpdateServiceCategoryByName(name string, serviceCategory tc.ServiceCategory) (tc.Alerts, ReqInf, error) {
-
+func (to *Session) UpdateServiceCategoryByName(name string, serviceCategory tc.ServiceCategory, header http.Header) (tc.Alerts, ReqInf, error) {
Review comment:
I might have been away when that discussion took place, but it has been discussed in the past before with most devs seemingly okay with changing the client method signatures without a deprecation period.
That said, this method is being introduced in the 5.0.0 release, which means we can still change the signature without breaking any clients (assuming this PR gets accepted into 5.0.0 which it should).
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman merged pull request #5231: Fix service categories if-not-modified/if-match functionality
Posted by GitBox <gi...@apache.org>.
zrhoffman merged pull request #5231:
URL: https://github.com/apache/trafficcontrol/pull/5231
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #5231: Fix service categories if-not-modified/if-match functionality
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #5231:
URL: https://github.com/apache/trafficcontrol/pull/5231#discussion_r517705199
##########
File path: traffic_ops/v3-client/serviceCategory.go
##########
@@ -51,20 +51,22 @@ func (to *Session) CreateServiceCategory(serviceCategory tc.ServiceCategory) (tc
}
// UpdateServiceCategoryByName updates a service category by its unique name.
-func (to *Session) UpdateServiceCategoryByName(name string, serviceCategory tc.ServiceCategory) (tc.Alerts, ReqInf, error) {
-
+func (to *Session) UpdateServiceCategoryByName(name string, serviceCategory tc.ServiceCategory, header http.Header) (tc.Alerts, ReqInf, error) {
Review comment:
Is changing the function signature acceptable? I know we had this discussion a while back about the consistency of function signatures and structs, for external clients' use cases not breaking.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #5231: Fix service categories if-not-modified/if-match functionality
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #5231:
URL: https://github.com/apache/trafficcontrol/pull/5231#discussion_r516973306
##########
File path: traffic_ops/traffic_ops_golang/servicecategory/servicecategories.go
##########
@@ -149,14 +135,18 @@ func Update(w http.ResponseWriter, r *http.Request) {
}
var origSC TOServiceCategory
- if err := inf.Tx.QueryRow(`SELECT name FROM service_category WHERE name = $1`, name).Scan(&origSC.Name); err != nil {
+ if err := inf.Tx.QueryRow(`SELECT name, last_updated FROM service_category WHERE name = $1`, name).Scan(&origSC.Name, &origSC.LastUpdated); err != nil {
if err == sql.ErrNoRows {
api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("no service category found with name "+name), nil)
return
}
api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, err)
return
}
+ if !api.IsUnmodified(r.Header, origSC.LastUpdated.Time) {
+ api.HandleErr(w, r, inf.Tx.Tx, http.StatusPreconditionFailed, errors.New("service category has been modified"), nil)
Review comment:
The API error should not be "service category has been modified", rather it should be something like "service category could not be modified because the precondition failed", or something to that effect.
----------------------------------------------------------------
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:
users@infra.apache.org