[GitHub] rob05c commented on a change in pull request #2282: Deliveryservice_Server API conversion to Go - formerly #2269
rob05c commented on a change in pull request #2282: Deliveryservice_Server API
conversion to Go - formerly #2269
URL:
https://github.com/apache/incubator-trafficcontrol/pull/2282#discussion_r190616966
##
File path: traffic_ops/traffic_ops_golang/routes.go
##
@@ -156,7 +157,13 @@ func Routes(d ServerData) ([]Route, []RawRoute,
http.Handler, error) {
{1.1, http.MethodDelete, `regions/{id}$`,
api.DeleteHandler(region.GetRefType(), d.DB), auth.PrivLevelOperations,
Authenticated, nil},
// get all edge servers associated with a delivery service
(from deliveryservice_server table)
- {1.1, http.MethodGet, `deliveryservices/{id}/servers$`,
api.ReadHandler(dsserver.GetRefType(), d.DB), auth.PrivLevelReadOnly,
Authenticated, nil},
+ {1.1, http.MethodGet, `deliveryserviceserver$`,
dsserver.ReadDSSHandler(d.DB),auth.PrivLevelReadOnly, Authenticated, nil},
+ {1.1, http.MethodPost,`deliveryserviceserver$`,
dsserver.GetReplaceHandler(d.DB),auth.PrivLevelOperations, Authenticated, nil},
+ {1.1, http.MethodPost,`deliveryservices/{xml_id}/servers$`,
dsserver.GetCreateHandler( d.DB ) ,auth.PrivLevelOperations, Authenticated,
nil},
+ {1.1, http.MethodGet, `servers/{id}/deliveryservices$`,
api.ReadHandler(dsserver.GetDServiceRef(), d.DB),auth.PrivLevelReadOnly,
Authenticated, nil},
+ {1.1, http.MethodGet, `deliveryservices/{id}/servers$`,
dsserver.GetReadHandler(d.DB, tc.Assigned),auth.PrivLevelReadOnly,
Authenticated, nil},
+ {1.1, http.MethodGet,
`deliveryservices/{id}/unassigned_servers$`, dsserver.GetReadHandler(d.DB,
tc.Unassigned),auth.PrivLevelReadOnly, Authenticated, nil},
Review comment:
Same as above, needs wrapped in `{"response": [`.
You can do this with a single line with a helper func that was recently
merged into master, `api.WriteResp(obj)`, which will serialize the JSON, wrap
it in `"response"`, write the `Content-Type` header, and log and return any
error appropriately.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rob05c commented on a change in pull request #2282: Deliveryservice_Server API conversion to Go - formerly #2269
rob05c commented on a change in pull request #2282: Deliveryservice_Server API
conversion to Go - formerly #2269
URL:
https://github.com/apache/incubator-trafficcontrol/pull/2282#discussion_r190616966
##
File path: traffic_ops/traffic_ops_golang/routes.go
##
@@ -156,7 +157,13 @@ func Routes(d ServerData) ([]Route, []RawRoute,
http.Handler, error) {
{1.1, http.MethodDelete, `regions/{id}$`,
api.DeleteHandler(region.GetRefType(), d.DB), auth.PrivLevelOperations,
Authenticated, nil},
// get all edge servers associated with a delivery service
(from deliveryservice_server table)
- {1.1, http.MethodGet, `deliveryservices/{id}/servers$`,
api.ReadHandler(dsserver.GetRefType(), d.DB), auth.PrivLevelReadOnly,
Authenticated, nil},
+ {1.1, http.MethodGet, `deliveryserviceserver$`,
dsserver.ReadDSSHandler(d.DB),auth.PrivLevelReadOnly, Authenticated, nil},
+ {1.1, http.MethodPost,`deliveryserviceserver$`,
dsserver.GetReplaceHandler(d.DB),auth.PrivLevelOperations, Authenticated, nil},
+ {1.1, http.MethodPost,`deliveryservices/{xml_id}/servers$`,
dsserver.GetCreateHandler( d.DB ) ,auth.PrivLevelOperations, Authenticated,
nil},
+ {1.1, http.MethodGet, `servers/{id}/deliveryservices$`,
api.ReadHandler(dsserver.GetDServiceRef(), d.DB),auth.PrivLevelReadOnly,
Authenticated, nil},
+ {1.1, http.MethodGet, `deliveryservices/{id}/servers$`,
dsserver.GetReadHandler(d.DB, tc.Assigned),auth.PrivLevelReadOnly,
Authenticated, nil},
+ {1.1, http.MethodGet,
`deliveryservices/{id}/unassigned_servers$`, dsserver.GetReadHandler(d.DB,
tc.Unassigned),auth.PrivLevelReadOnly, Authenticated, nil},
Review comment:
Same as above, needs wrapped in `{"response"[`.
You can do this with a single line with a helper func that was recently
merged into master, `api.WriteResp(obj)`, which will serialize the JSON, wrap
it in `"response"`, write the `Content-Type` header, and log and return any
error appropriately.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rob05c commented on a change in pull request #2282: Deliveryservice_Server API conversion to Go - formerly #2269
rob05c commented on a change in pull request #2282: Deliveryservice_Server API
conversion to Go - formerly #2269
URL:
https://github.com/apache/incubator-trafficcontrol/pull/2282#discussion_r190610400
##
File path: traffic_ops/traffic_ops_golang/routes.go
##
@@ -156,7 +157,13 @@ func Routes(d ServerData) ([]Route, []RawRoute,
http.Handler, error) {
{1.1, http.MethodDelete, `regions/{id}$`,
api.DeleteHandler(region.GetRefType(), d.DB), auth.PrivLevelOperations,
Authenticated, nil},
// get all edge servers associated with a delivery service
(from deliveryservice_server table)
- {1.1, http.MethodGet, `deliveryservices/{id}/servers$`,
api.ReadHandler(dsserver.GetRefType(), d.DB), auth.PrivLevelReadOnly,
Authenticated, nil},
+ {1.1, http.MethodGet, `deliveryserviceserver$`,
dsserver.ReadDSSHandler(d.DB),auth.PrivLevelReadOnly, Authenticated, nil},
+ {1.1, http.MethodPost,`deliveryserviceserver$`,
dsserver.GetReplaceHandler(d.DB),auth.PrivLevelOperations, Authenticated, nil},
+ {1.1, http.MethodPost,`deliveryservices/{xml_id}/servers$`,
dsserver.GetCreateHandler( d.DB ) ,auth.PrivLevelOperations, Authenticated,
nil},
+ {1.1, http.MethodGet, `servers/{id}/deliveryservices$`,
api.ReadHandler(dsserver.GetDServiceRef(), d.DB),auth.PrivLevelReadOnly,
Authenticated, nil},
Review comment:
The `/api/1.2/deliveryservices/42/servers` route isn't correct, it returns
```
[
{
"cachegroup": "
```
The existing Perl (and the docs) returns
```
{
"response": [
{
"cachegroup": "
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rob05c commented on a change in pull request #2282: Deliveryservice_Server API conversion to Go - formerly #2269
rob05c commented on a change in pull request #2282: Deliveryservice_Server API
conversion to Go - formerly #2269
URL:
https://github.com/apache/incubator-trafficcontrol/pull/2282#discussion_r190612184
##
File path: traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
##
@@ -263,7 +243,447 @@ func (dss *TODeliveryServiceServer) Delete(db *sqlx.DB,
user auth.CurrentUser) (
rollbackTransaction = false
return nil, tc.NoError
}
-func selectQuery() string {
+
+func selectQuery(orderBy string, limit string, offset string) (string, error) {
+
+ selectStmt := `SELECT
+ s.deliveryService,
+ s.server,
+ s.last_updated
+ FROM deliveryservice_server s`
+
+ allowedOrderByCols := map[string]string{
+ "":"",
+ "deliveryservice": "s.deliveryService",
+ "server": "s.server",
+ "lastupdated": "s.last_updated",
+ "deliveryService": "s.deliveryService",
+ "lastUpdated": "s.last_updated",
+ "last_updated":"s.last_updated",
+ }
+ orderBy, ok := allowedOrderByCols[orderBy]
+ if !ok {
+ return "", errors.New("orderBy '" + orderBy + "' not permitted")
+ }
+
+ if orderBy != "" {
+ selectStmt += ` ORDER BY ` + orderBy
+ }
+
+ selectStmt += ` LIMIT ` + limit + ` OFFSET ` + offset + ` ROWS`
+ return selectStmt, nil
+}
+
+func deleteQuery() string {
+ query := `DELETE FROM deliveryservice_server
+ WHERE deliveryservice=:deliveryservice and server=:server`
+ return query
+}
+
+type DSServers struct {
+ DsId*int `json:"dsId" db:"deliveryservice"`
+ Servers []int `json:"servers"`
+ Replace *bool `json:"replace"`
+}
+
+type TODSServers DSServers
+
+
+func createServersForDsIdRef() *TODSServers {
+ var dsserversRef = TODSServers(DSServers{})
+ return &dsserversRef
+}
+
+func GetReplaceHandler(db *sqlx.DB) http.HandlerFunc {
+ return func(w http.ResponseWriter, r *http.Request) {
+ defer r.Body.Close()
+ handleErrs := tc.GetHandleErrorsFunc(w, r)
+ ctx := r.Context()
+ user, err := auth.GetCurrentUser(ctx)
+ if err != nil {
+ log.Errorf("unable to retrieve current user from
context: %s", err)
+ handleErrs(http.StatusInternalServerError, err)
+ return
+ }
+
+ // get list of server Ids to insert
+ payload := createServersForDsIdRef()
+
+ if err := json.NewDecoder(r.Body).Decode(payload); err != nil {
+ log.Errorf("Error trying to decode the request body:
%s", err)
+ handleErrs(http.StatusInternalServerError, err)
+ return
+ }
+
+ servers := payload.Servers
+ dsId := payload.DsId
+
+ if servers == nil {
+ log.Error.Printf("no servers sent in POST; could not
begin transaction")
Review comment:
These 3 log lines need removed, `api.HandleErr` will log if there's a system
error (there isn't here, but we don't need to log bad user request errors
anyway, beyond an access log).
Further, accessing the log objects directly via `log.Error` isn't safe, this
will panic if the error log is configured to be null. Always use the helper
funcs, `log.Errorf`, unless there's a compelling reason to use the raw logger,
and then it must be checked `if log.Error != nil`
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rob05c commented on a change in pull request #2282: Deliveryservice_Server API conversion to Go - formerly #2269
rob05c commented on a change in pull request #2282: Deliveryservice_Server API
conversion to Go - formerly #2269
URL:
https://github.com/apache/incubator-trafficcontrol/pull/2282#discussion_r190610400
##
File path: traffic_ops/traffic_ops_golang/routes.go
##
@@ -156,7 +157,13 @@ func Routes(d ServerData) ([]Route, []RawRoute,
http.Handler, error) {
{1.1, http.MethodDelete, `regions/{id}$`,
api.DeleteHandler(region.GetRefType(), d.DB), auth.PrivLevelOperations,
Authenticated, nil},
// get all edge servers associated with a delivery service
(from deliveryservice_server table)
- {1.1, http.MethodGet, `deliveryservices/{id}/servers$`,
api.ReadHandler(dsserver.GetRefType(), d.DB), auth.PrivLevelReadOnly,
Authenticated, nil},
+ {1.1, http.MethodGet, `deliveryserviceserver$`,
dsserver.ReadDSSHandler(d.DB),auth.PrivLevelReadOnly, Authenticated, nil},
+ {1.1, http.MethodPost,`deliveryserviceserver$`,
dsserver.GetReplaceHandler(d.DB),auth.PrivLevelOperations, Authenticated, nil},
+ {1.1, http.MethodPost,`deliveryservices/{xml_id}/servers$`,
dsserver.GetCreateHandler( d.DB ) ,auth.PrivLevelOperations, Authenticated,
nil},
+ {1.1, http.MethodGet, `servers/{id}/deliveryservices$`,
api.ReadHandler(dsserver.GetDServiceRef(), d.DB),auth.PrivLevelReadOnly,
Authenticated, nil},
Review comment:
This route isn't correct. Going to `/api/1.2/deliveryservices/42/servers`
returns
```
[
{
"cachegroup": "
```
The existing Perl (and the docs) returns
```
{
"response": [
{
"cachegroup": "
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rob05c commented on a change in pull request #2282: Deliveryservice_Server API conversion to Go - formerly #2269
rob05c commented on a change in pull request #2282: Deliveryservice_Server API
conversion to Go - formerly #2269
URL:
https://github.com/apache/incubator-trafficcontrol/pull/2282#discussion_r190276662
##
File path: traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
##
@@ -263,7 +243,447 @@ func (dss *TODeliveryServiceServer) Delete(db *sqlx.DB,
user auth.CurrentUser) (
rollbackTransaction = false
return nil, tc.NoError
}
-func selectQuery() string {
+
+func selectQuery(orderBy string, limit string, offset string) (string, error) {
+
+ selectStmt := `SELECT
+ s.deliveryService,
+ s.server,
+ s.last_updated
+ FROM deliveryservice_server s`
+
+ allowedOrderByCols := map[string]string{
+ "":"",
+ "deliveryservice": "s.deliveryService",
+ "server": "s.server",
+ "lastupdated": "s.last_updated",
+ "deliveryService": "s.deliveryService",
+ "lastUpdated": "s.last_updated",
+ "last_updated":"s.last_updated",
+ }
+ orderBy, ok := allowedOrderByCols[orderBy]
+ if !ok {
+ return "", errors.New("orderBy '" + orderBy + "' not permitted")
+ }
+
+ if orderBy != "" {
+ selectStmt += ` ORDER BY ` + orderBy
+ }
+
+ selectStmt += ` LIMIT ` + limit + ` OFFSET ` + offset + ` ROWS`
+ return selectStmt, nil
+}
+
+func deleteQuery() string {
+ query := `DELETE FROM deliveryservice_server
+ WHERE deliveryservice=:deliveryservice and server=:server`
+ return query
+}
+
+type DSServers struct {
+ DsId*int `json:"dsId" db:"deliveryservice"`
+ Servers []int `json:"servers"`
+ Replace *bool `json:"replace"`
+}
+
+type TODSServers DSServers
+
+
+func createServersForDsIdRef() *TODSServers {
+ var dsserversRef = TODSServers(DSServers{})
+ return &dsserversRef
+}
+
+func GetReplaceHandler(db *sqlx.DB) http.HandlerFunc {
+ return func(w http.ResponseWriter, r *http.Request) {
+ defer r.Body.Close()
+ handleErrs := tc.GetHandleErrorsFunc(w, r)
+ ctx := r.Context()
+ user, err := auth.GetCurrentUser(ctx)
+ if err != nil {
+ log.Errorf("unable to retrieve current user from
context: %s", err)
+ handleErrs(http.StatusInternalServerError, err)
+ return
+ }
+
+ // get list of server Ids to insert
+ payload := createServersForDsIdRef()
+
+ if err := json.NewDecoder(r.Body).Decode(payload); err != nil {
+ log.Errorf("Error trying to decode the request body:
%s", err)
+ handleErrs(http.StatusInternalServerError, err)
+ return
+ }
+
+ servers := payload.Servers
+ dsId := payload.DsId
+
+ if servers == nil {
+ log.Error.Printf("no servers sent in POST; could not
begin transaction: %v", err)
+ handleErrs(http.StatusBadRequest, err)
Review comment:
This `err` is coming from above, `user, err := auth.GetCurrentUser(ctx)`,
and will always be `nil`, but only by coincidence.
Can you change all 3 of these to `api.HandleErr(w, r, http.StatusBadRequest,
errors.New("servers must exist"), nil)`, `"dsId must exist"`, `"replace must
exist"`?
Incidentally, `handleErrs := tc.GetHandleErrorsFunc(w, r)` is a poor system
(that's my fault), because it doesn't allow you to distinguish between server
and user errors, and reply versus log appropriately. We need to move to
`api.HandleErr`, which lets you distinguish and specify both in a single line.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rob05c commented on a change in pull request #2282: Deliveryservice_Server API conversion to Go - formerly #2269
rob05c commented on a change in pull request #2282: Deliveryservice_Server API
conversion to Go - formerly #2269
URL:
https://github.com/apache/incubator-trafficcontrol/pull/2282#discussion_r190064897
##
File path: traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
##
@@ -263,7 +243,428 @@ func (dss *TODeliveryServiceServer) Delete(db *sqlx.DB,
user auth.CurrentUser) (
rollbackTransaction = false
return nil, tc.NoError
}
-func selectQuery() string {
+
+func selectQuery(orderBy string, limit string, offset string) (string, error) {
+
+ selectStmt := `SELECT
+ s.deliveryService,
+ s.server,
+ s.last_updated
+ FROM deliveryservice_server s`
+
+ allowedOrderByCols := map[string]string{
+ "":"",
+ "deliveryservice": "s.deliveryService",
+ "server": "s.server",
+ "lastupdated": "s.last_updated",
+ "deliveryService": "s.deliveryService",
+ "lastUpdated": "s.last_updated",
+ "last_updated":"s.last_updated",
+ }
+ orderBy, ok := allowedOrderByCols[orderBy]
+ if !ok {
+ return "", errors.New("orderBy '" + orderBy + "' not permitted")
+ }
+
+ if orderBy != "" {
+ selectStmt += ` ORDER BY ` + orderBy
+ }
+
+ selectStmt += ` LIMIT ` + limit + ` OFFSET ` + offset + ` ROWS`
+ return selectStmt, nil
+}
+
+func deleteQuery() string {
+ query := `DELETE FROM deliveryservice_server
+ WHERE deliveryservice=:deliveryservice and server=:server`
+ return query
+}
+
+type DSServers struct {
+ DsId*int `json:"dsId" db:"deliveryservice"`
+ Servers []int `json:"servers"`
+ Replace *bool `json:"replace"`
+}
+
+type TODSServers DSServers
+
+
+func createServersForDsIdRef() *TODSServers {
+ var dsserversRef = TODSServers(DSServers{})
+ return &dsserversRef
+}
+
+func GetReplaceHandler(db *sqlx.DB) http.HandlerFunc {
+ return func(w http.ResponseWriter, r *http.Request) {
+ defer r.Body.Close()
+ handleErrs := tc.GetHandleErrorsFunc(w, r)
+ ctx := r.Context()
+ user, err := auth.GetCurrentUser(ctx)
+ if err != nil {
+ log.Errorf("unable to retrieve current user from
context: %s", err)
+ handleErrs(http.StatusInternalServerError, err)
+ return
+ }
+
+ // get list of server Ids to insert
+ payload := createServersForDsIdRef()
+ servers := payload.Servers
+ dsId := payload.DsId
+
+ if err := json.NewDecoder(r.Body).Decode(payload); err != nil {
+ log.Errorf("Error trying to decode the request body:
%s", err)
+ handleErrs(http.StatusInternalServerError, err)
+ return
+ }
+
+ // if the object has tenancy enabled, check that user is able
to access the tenant
+ // check user tenancy access to this resource.
+ row := db.QueryRow("SELECT xml_id FROM deliveryservice WHERE id
= $1", *dsId)
Review comment:
This is panicing. It looks like it's because You're assigning `dsId` above
before decoding the JSON, so it's always `nil`.
But, even if the JSON has been decoded, you still need to check for nil,
because the user POST could be missing that field. In which case, we should
return a `http.StatusBadRequest` to the user, with a message explaining that
`dsId` is required.
The `Replace` field also needs checked that it's not `nil` before
dereferencing.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rob05c commented on a change in pull request #2282: Deliveryservice_Server API conversion to Go - formerly #2269
rob05c commented on a change in pull request #2282: Deliveryservice_Server API
conversion to Go - formerly #2269
URL:
https://github.com/apache/incubator-trafficcontrol/pull/2282#discussion_r189707585
##
File path: traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
##
@@ -97,130 +100,107 @@ func (dss *TODeliveryServiceServer) Validate(db
*sqlx.DB) []error {
return tovalidate.ToErrors(errs)
}
-//The TODeliveryServiceServer implementation of the Creator interface
-//all implementations of Creator should use transactions and return the proper
errorType
-//ParsePQUniqueConstraintError is used to determine if a profileparameter with
conflicting values exists
-//if so, it will return an errorType of DataConflict and the type should be
appended to the
-//generic error message returned
-//The insert sql returns the profile and lastUpdated values of the newly
inserted profileparameter and have
-//to be added to the struct
-func (dss *TODeliveryServiceServer) Create(db *sqlx.DB, user auth.CurrentUser)
(error, tc.ApiErrorType) {
- rollbackTransaction := true
- tx, err := db.Beginx()
- defer func() {
- if tx == nil || !rollbackTransaction {
- return
+// api/1.1/deliveryserviceserver$
Review comment:
This breaks GoDoc parsing, a comment above a function needs to start with
the name of the function, i.e. `// ReadDSSHandler is served at
api/1.1/deliveryserviceserver`. Alternatively, you can put the comment
somewhere else, like inside the function, to avoid breaking GoDoc.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rob05c commented on a change in pull request #2282: Deliveryservice_Server API conversion to Go - formerly #2269
rob05c commented on a change in pull request #2282: Deliveryservice_Server API
conversion to Go - formerly #2269
URL:
https://github.com/apache/incubator-trafficcontrol/pull/2282#discussion_r189707285
##
File path: lib/go-tc/deliveryservice_servers.go
##
@@ -78,3 +106,63 @@ type DssServer struct {
TypeID *int `json:"typeId"
db:"server_type_id"`
UpdPending *bool`json:"updPending"
db:"upd_pending"`
}
+
+// DSSDeliveryService - a version of the deliveryservice that allows for all
fields to be null
Review comment:
GoDoc comments should be complete sentences, i.e. "DSSDeliveryService is a
version of..."
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rob05c commented on a change in pull request #2282: Deliveryservice_Server API conversion to Go - formerly #2269
rob05c commented on a change in pull request #2282: Deliveryservice_Server API
conversion to Go - formerly #2269
URL:
https://github.com/apache/incubator-trafficcontrol/pull/2282#discussion_r189707065
##
File path: lib/go-tc/deliveryservice_servers.go
##
@@ -19,21 +21,47 @@ import "time"
// DeliveryServiceServerResponse ...
type DeliveryServiceServerResponse struct {
+ Orderby string `json:"orderby"`
Response []DeliveryServiceServer `json:"response"`
Size int `json:"size"`
- OrderBy string `json:"orderby"`
Limitint `json:"limit"`
}
+type DSSMapResponse struct {
+ DsIdint `json:"dsId"`
+ Replace bool `json:"replace"`
+ Servers []int `json:"servers"`
+}
+
+type DSSReplaceResponse struct {
+ Alerts []Alert`json:"alerts"`
+ Response DSSMapResponse `json:"response"`
+}
+
+type DSServersResponse struct {
+ Response DeliveryServiceServers `json:"response"`
+}
+
+type DeliveryServiceServers struct {
+ ServerNames []string `json:"serverNames"`
+ XmlId string `json:"xmlId"`
+}
+
// DeliveryServiceServer ...
type DeliveryServiceServer struct {
- Server *int `json:"server"`
- DeliveryService *int `json:"deliveryService"`
- LastUpdated *TimeNoMod `json:"lastUpdated" db:"last_updated"`
+ Server *int `json:"server" db:"server"`
+ DeliveryService *int `json:"deliveryService" db:"deliveryservice"`
+ LastUpdated *TimeNoMod `json:"lastUpdated" db:"last_updated"`
}
+type Filter int
+const (
+ ASSIGNED Filter = iota
Review comment:
Go enums and constants are CamelCased, not ALLCAPS. These should be
`Assigned`, `Unassigned`, `Eligible`
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rob05c commented on a change in pull request #2282: Deliveryservice_Server API conversion to Go - formerly #2269
rob05c commented on a change in pull request #2282: Deliveryservice_Server API
conversion to Go - formerly #2269
URL:
https://github.com/apache/incubator-trafficcontrol/pull/2282#discussion_r189706518
##
File path: traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
##
@@ -263,7 +243,428 @@ func (dss *TODeliveryServiceServer) Delete(db *sqlx.DB,
user auth.CurrentUser) (
rollbackTransaction = false
return nil, tc.NoError
}
-func selectQuery() string {
+
+func selectQuery(orderBy string, limit string, offset string) (string, error) {
+
+ selectStmt := `SELECT
+ s.deliveryService,
+ s.server,
+ s.last_updated
+ FROM deliveryservice_server s`
+
+ allowedOrderByCols := map[string]string{
+ "":"",
+ "deliveryservice": "s.deliveryService",
+ "server": "s.server",
+ "lastupdated": "s.last_updated",
+ "deliveryService": "s.deliveryService",
+ "lastUpdated": "s.last_updated",
+ "last_updated":"s.last_updated",
+ }
+ orderBy, ok := allowedOrderByCols[orderBy]
+ if !ok {
+ return "", errors.New("orderBy '" + orderBy + "' not permitted")
+ }
+
+ if orderBy != "" {
+ selectStmt += ` ORDER BY ` + orderBy
+ }
+
+ selectStmt += ` LIMIT ` + limit + ` OFFSET ` + offset + ` ROWS`
+ return selectStmt, nil
+}
+
+func deleteQuery() string {
+ query := `DELETE FROM deliveryservice_server
+ WHERE deliveryservice=:deliveryservice and server=:server`
+ return query
+}
+
+type DSServers struct {
+ DsId*int `json:"dsId" db:"deliveryservice"`
+ Servers []int `json:"servers"`
+ Replace *bool `json:"replace"`
+}
+
+type TODSServers DSServers
+
+var dsserversRef = TODSServers(DSServers{})
+
+func GetServersForDsIdRef() *TODSServers {
+ return &dsserversRef
+}
+
+func GetReplaceHandler(db *sqlx.DB) http.HandlerFunc {
+ return func(w http.ResponseWriter, r *http.Request) {
+ defer r.Body.Close()
+ handleErrs := tc.GetHandleErrorsFunc(w, r)
+ ctx := r.Context()
+ user, err := auth.GetCurrentUser(ctx)
+ if err != nil {
+ log.Errorf("unable to retrieve current user from
context: %s", err)
+ handleErrs(http.StatusInternalServerError, err)
+ return
+ }
+
+ // get list of server Ids to insert
+ payload := GetServersForDsIdRef()
+ servers := payload.Servers
+ dsId := payload.DsId
+
+ if err := json.NewDecoder(r.Body).Decode(payload); err != nil {
+ log.Errorf("Error trying to decode the request body:
%s", err)
+ handleErrs(http.StatusInternalServerError, err)
+ return
+ }
+
+ // if the object has tenancy enabled, check that user is able
to access the tenant
+ // check user tenancy access to this resource.
+ row := db.QueryRow("SELECT xml_id FROM deliveryservice WHERE id
= $1", *dsId)
+ var xmlId string
+ row.Scan(&xmlId)
+ hasAccess, err, apiStatus := tenant.HasTenant(*user, xmlId, db)
+ if !hasAccess {
+ switch apiStatus {
+ case tc.SystemError:
+ handleErrs(http.StatusInternalServerError, err)
+ return
+ case tc.DataMissingError:
+ handleErrs(http.StatusBadRequest, err)
+ return
+ case tc.ForbiddenError:
+ handleErrs(http.StatusForbidden, err)
+ return
+ }
+ }
+
+ // perform the insert transaction
+ rollbackTransaction := true
+ tx, err := db.Beginx()
+ defer func() {
+ if tx == nil || !rollbackTransaction {
+ return
+ }
+ err := tx.Rollback()
+ if err != nil {
+ log.Errorln(errors.New("rolling back
transaction: " + err.Error()))
+ }
+ }()
+
+ if err != nil {
+ log.Error.Printf("could not begin transaction: %v", err)
+ handleErrs(http.StatusInternalServerError, err)
+ return
+ }
+
+ if *payload.Replace {
+ // delete existing
+ rows, err := db.Queryx("DELETE FROM
deliveryservice_server WHERE deliveryservice = $1", *
[GitHub] rob05c commented on a change in pull request #2282: Deliveryservice_Server API conversion to Go - formerly #2269
rob05c commented on a change in pull request #2282: Deliveryservice_Server API
conversion to Go - formerly #2269
URL:
https://github.com/apache/incubator-trafficcontrol/pull/2282#discussion_r189707974
##
File path: traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
##
@@ -263,7 +243,428 @@ func (dss *TODeliveryServiceServer) Delete(db *sqlx.DB,
user auth.CurrentUser) (
rollbackTransaction = false
return nil, tc.NoError
}
-func selectQuery() string {
+
+func selectQuery(orderBy string, limit string, offset string) (string, error) {
+
+ selectStmt := `SELECT
+ s.deliveryService,
+ s.server,
+ s.last_updated
+ FROM deliveryservice_server s`
+
+ allowedOrderByCols := map[string]string{
+ "":"",
+ "deliveryservice": "s.deliveryService",
+ "server": "s.server",
+ "lastupdated": "s.last_updated",
+ "deliveryService": "s.deliveryService",
+ "lastUpdated": "s.last_updated",
+ "last_updated":"s.last_updated",
+ }
+ orderBy, ok := allowedOrderByCols[orderBy]
+ if !ok {
+ return "", errors.New("orderBy '" + orderBy + "' not permitted")
+ }
+
+ if orderBy != "" {
+ selectStmt += ` ORDER BY ` + orderBy
+ }
+
+ selectStmt += ` LIMIT ` + limit + ` OFFSET ` + offset + ` ROWS`
+ return selectStmt, nil
+}
+
+func deleteQuery() string {
+ query := `DELETE FROM deliveryservice_server
+ WHERE deliveryservice=:deliveryservice and server=:server`
+ return query
+}
+
+type DSServers struct {
+ DsId*int `json:"dsId" db:"deliveryservice"`
+ Servers []int `json:"servers"`
+ Replace *bool `json:"replace"`
+}
+
+type TODSServers DSServers
+
+var dsserversRef = TODSServers(DSServers{})
+
+func GetServersForDsIdRef() *TODSServers {
+ return &dsserversRef
+}
+
+func GetReplaceHandler(db *sqlx.DB) http.HandlerFunc {
+ return func(w http.ResponseWriter, r *http.Request) {
+ defer r.Body.Close()
+ handleErrs := tc.GetHandleErrorsFunc(w, r)
+ ctx := r.Context()
+ user, err := auth.GetCurrentUser(ctx)
+ if err != nil {
+ log.Errorf("unable to retrieve current user from
context: %s", err)
+ handleErrs(http.StatusInternalServerError, err)
+ return
+ }
+
+ // get list of server Ids to insert
+ payload := GetServersForDsIdRef()
Review comment:
Ad above, `GetServersForDsIdRef` is unsafe for concurrent use, and needs to
return a new object, i.e. `s := TODSServers(DSServers{}); return &s`
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rob05c commented on a change in pull request #2282: Deliveryservice_Server API conversion to Go - formerly #2269
rob05c commented on a change in pull request #2282: Deliveryservice_Server API
conversion to Go - formerly #2269
URL:
https://github.com/apache/incubator-trafficcontrol/pull/2282#discussion_r189708268
##
File path: traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
##
@@ -263,7 +243,428 @@ func (dss *TODeliveryServiceServer) Delete(db *sqlx.DB,
user auth.CurrentUser) (
rollbackTransaction = false
return nil, tc.NoError
}
-func selectQuery() string {
+
+func selectQuery(orderBy string, limit string, offset string) (string, error) {
+
+ selectStmt := `SELECT
+ s.deliveryService,
+ s.server,
+ s.last_updated
+ FROM deliveryservice_server s`
+
+ allowedOrderByCols := map[string]string{
+ "":"",
+ "deliveryservice": "s.deliveryService",
+ "server": "s.server",
+ "lastupdated": "s.last_updated",
+ "deliveryService": "s.deliveryService",
+ "lastUpdated": "s.last_updated",
+ "last_updated":"s.last_updated",
+ }
+ orderBy, ok := allowedOrderByCols[orderBy]
+ if !ok {
+ return "", errors.New("orderBy '" + orderBy + "' not permitted")
+ }
+
+ if orderBy != "" {
+ selectStmt += ` ORDER BY ` + orderBy
+ }
+
+ selectStmt += ` LIMIT ` + limit + ` OFFSET ` + offset + ` ROWS`
+ return selectStmt, nil
+}
+
+func deleteQuery() string {
+ query := `DELETE FROM deliveryservice_server
+ WHERE deliveryservice=:deliveryservice and server=:server`
+ return query
+}
+
+type DSServers struct {
+ DsId*int `json:"dsId" db:"deliveryservice"`
+ Servers []int `json:"servers"`
+ Replace *bool `json:"replace"`
+}
+
+type TODSServers DSServers
+
+var dsserversRef = TODSServers(DSServers{})
+
+func GetServersForDsIdRef() *TODSServers {
+ return &dsserversRef
+}
+
+func GetReplaceHandler(db *sqlx.DB) http.HandlerFunc {
+ return func(w http.ResponseWriter, r *http.Request) {
+ defer r.Body.Close()
+ handleErrs := tc.GetHandleErrorsFunc(w, r)
+ ctx := r.Context()
+ user, err := auth.GetCurrentUser(ctx)
+ if err != nil {
+ log.Errorf("unable to retrieve current user from
context: %s", err)
+ handleErrs(http.StatusInternalServerError, err)
+ return
+ }
+
+ // get list of server Ids to insert
+ payload := GetServersForDsIdRef()
+ servers := payload.Servers
+ dsId := payload.DsId
+
+ if err := json.NewDecoder(r.Body).Decode(payload); err != nil {
+ log.Errorf("Error trying to decode the request body:
%s", err)
+ handleErrs(http.StatusInternalServerError, err)
+ return
+ }
+
+ // if the object has tenancy enabled, check that user is able
to access the tenant
+ // check user tenancy access to this resource.
+ row := db.QueryRow("SELECT xml_id FROM deliveryservice WHERE id
= $1", *dsId)
+ var xmlId string
+ row.Scan(&xmlId)
+ hasAccess, err, apiStatus := tenant.HasTenant(*user, xmlId, db)
+ if !hasAccess {
+ switch apiStatus {
+ case tc.SystemError:
+ handleErrs(http.StatusInternalServerError, err)
+ return
+ case tc.DataMissingError:
+ handleErrs(http.StatusBadRequest, err)
+ return
+ case tc.ForbiddenError:
+ handleErrs(http.StatusForbidden, err)
+ return
+ }
+ }
+
+ // perform the insert transaction
+ rollbackTransaction := true
+ tx, err := db.Beginx()
+ defer func() {
+ if tx == nil || !rollbackTransaction {
+ return
+ }
+ err := tx.Rollback()
+ if err != nil {
+ log.Errorln(errors.New("rolling back
transaction: " + err.Error()))
+ }
+ }()
+
+ if err != nil {
+ log.Error.Printf("could not begin transaction: %v", err)
+ handleErrs(http.StatusInternalServerError, err)
+ return
+ }
+
+ if *payload.Replace {
+ // delete existing
+ rows, err := db.Queryx("DELETE FROM
deliveryservice_server WHERE deliveryservice = $1", *
