ocket8888 commented on code in PR #6996: URL: https://github.com/apache/trafficcontrol/pull/6996#discussion_r937986723
########## docs/source/api/v4/multiple_server_capabilities.rst: ########## @@ -0,0 +1,83 @@ +.. +.. +.. Licensed under the Apache License, Version 2.0 (the "License"); +.. you may not use this file except in compliance with the License. +.. You may obtain a copy of the License at +.. +.. http://www.apache.org/licenses/LICENSE-2.0 +.. +.. Unless required by applicable law or agreed to in writing, software +.. distributed under the License is distributed on an "AS IS" BASIS, +.. WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +.. See the License for the specific language governing permissions and +.. limitations under the License. +.. + +.. _to-api-multiple_server_capabilities: + +******************************** +``multiple_server_capabilities`` +******************************** + +.. versionadded:: 4.1 + +``PUT`` +======== +Associates a list of :term:`Server Capability` to a server. + +:Auth. Required: Yes +:Roles Required: "admin" or "operations" +:Permissions Required: SERVER:UPDATE, SERVER:CREATE, SERVER:READ, SERVER-CAPABILITY:READ +:Response Type: Object + +Request Structure +----------------- +:serverId: The integral, unique identifier of a server to be associated with a :term:`Server Capability` +:serverCapability: List of :term:`Server Capability`'s name to associate + +.. code-block:: http + :caption: Request Example + + PUT /api/4.1/multiple_server_capabilities/1 HTTP/1.1 + Host: trafficops.infra.ciab.test + User-Agent: curl/7.47.0 + Accept: */* + Cookie: mojolicious=... + Content-Length: 84 + Content-Type: application/json + + { + "serverId": 1, + "serverCapability": ["test", "disk"] + } Review Comment: This block is indented with spaces instead of indentation ########## docs/source/api/v4/multiple_server_capabilities.rst: ########## @@ -0,0 +1,83 @@ +.. +.. +.. Licensed under the Apache License, Version 2.0 (the "License"); +.. you may not use this file except in compliance with the License. +.. You may obtain a copy of the License at +.. +.. http://www.apache.org/licenses/LICENSE-2.0 +.. +.. Unless required by applicable law or agreed to in writing, software +.. distributed under the License is distributed on an "AS IS" BASIS, +.. WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +.. See the License for the specific language governing permissions and +.. limitations under the License. +.. + +.. _to-api-multiple_server_capabilities: + +******************************** +``multiple_server_capabilities`` +******************************** + +.. versionadded:: 4.1 + +``PUT`` +======== +Associates a list of :term:`Server Capability` to a server. + +:Auth. Required: Yes +:Roles Required: "admin" or "operations" +:Permissions Required: SERVER:UPDATE, SERVER:CREATE, SERVER:READ, SERVER-CAPABILITY:READ +:Response Type: Object + +Request Structure +----------------- +:serverId: The integral, unique identifier of a server to be associated with a :term:`Server Capability` +:serverCapability: List of :term:`Server Capability`'s name to associate + +.. code-block:: http + :caption: Request Example + + PUT /api/4.1/multiple_server_capabilities/1 HTTP/1.1 + Host: trafficops.infra.ciab.test + User-Agent: curl/7.47.0 + Accept: */* + Cookie: mojolicious=... + Content-Length: 84 + Content-Type: application/json + + { + "serverId": 1, + "serverCapability": ["test", "disk"] + } + +Response Structure +------------------ +:serverId: The integral, unique identifier of the newly associated server +:serverCapability: List of :term:`Server Capability`'s name + +.. code-block:: http + :caption: Response Example + + HTTP/1.1 200 OK + Access-Control-Allow-Credentials: true + Access-Control-Allow-Headers: Origin, X-Requested-With, Content-Type, Accept, Set-Cookie, Cookie + Access-Control-Allow-Methods: POST,GET,OPTIONS,PUT,DELETE + Access-Control-Allow-Origin: * + Content-Type: application/json + Set-Cookie: mojolicious=...; Path=/; Expires=Mon, 8 Aug 2022 17:40:54 GMT; Max-Age=3600; HttpOnly + Whole-Content-Sha512: eQrl48zWids0kDpfCYmmtYMpegjnFxfOVvlBYxxLSfp7P7p6oWX4uiC+/Cfh2X9i3G+MQ36eH95gukJqOBOGbQ== + X-Server-Name: traffic_ops_golang/ + Date: Mon, 02 Aug 2022 22:15:11 GMT + Content-Length: 157 + + { + "alerts": [{ + "text": "Multiple Server Capabilities assigned to a server:1", + "level": "success" + }], + "response": { + "serverId": 1, + "serverCapability": ["test", "disk"] + } + } Review Comment: This block is indented with spaces instead of indentation ########## lib/go-tc/server_server_capability.go: ########## @@ -27,6 +27,12 @@ type ServerServerCapability struct { ServerCapability *string `json:"serverCapability" db:"server_capability"` } +// MultipleServerCapabilities represents an association between a server and list of server capabilities. +type MultipleServerCapabilities struct { + ServerID int `json:"serverId" db:"server"` + ServerCapability []string `json:"serverCapability" db:"server_capability"` Review Comment: This should probably be plural since it's multiple; `ServerCapabilities`/`serverCapabilities` ########## traffic_ops/traffic_ops_golang/server/servers_server_capability.go: ########## @@ -441,3 +442,87 @@ func getDSTenantIDsByIDs(tx *sqlx.Tx, dsIDs []int64) ([]DSTenant, error) { return dsTenantIDs, nil } + +// AssignMultipleServerCapabilities helps assign multiple server capabilities to a given server. +func AssignMultipleServerCapabilities(w http.ResponseWriter, r *http.Request) { + inf, userErr, sysErr, errCode := api.NewInfo(r, []string{"id"}, []string{"id"}) + tx := inf.Tx.Tx + if userErr != nil || sysErr != nil { + api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr) + return + } + defer inf.Close() + + var msc tc.MultipleServerCapabilities + if err := json.NewDecoder(r.Body).Decode(&msc); err != nil { + api.HandleErr(w, r, tx, http.StatusBadRequest, err, nil) + return + } + + // Check existence prior to checking type + _, exists, err := dbhelpers.GetServerNameFromID(tx, int64(msc.ServerID)) + if err != nil { + api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, err) + } + if !exists { + userErr := fmt.Errorf("server %d does not exist", msc.ServerID) + api.HandleErr(w, r, tx, http.StatusNotFound, userErr, nil) + return + } + + // Ensure type is correct + correctType := true + if err := tx.QueryRow(scCheckServerTypeQuery(), msc.ServerID).Scan(&correctType); err != nil { + api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, fmt.Errorf("checking server type: %w", err)) + return + } + if !correctType { + userErr := fmt.Errorf("server %d has an incorrect server type. Server capabilities can only be assigned to EDGE or MID servers", msc.ServerID) + api.HandleErr(w, r, tx, http.StatusBadRequest, userErr, nil) + return + } + + cdnName, err := dbhelpers.GetCDNNameFromServerID(tx, int64(msc.ServerID)) + if err != nil { + api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, err) + return + } + + userErr, sysErr, errCode = dbhelpers.CheckIfCurrentUserCanModifyCDN(tx, string(cdnName), inf.User.UserName) + if userErr != nil || sysErr != nil { + api.HandleErr(w, r, tx, errCode, userErr, sysErr) + return + } + + //Delete existing rows from server_server_capability for a given server + _, err = tx.Exec("DELETE FROM server_server_capability ssc WHERE ssc.server=$1", msc.ServerID) + if err != nil { + useErr, sysErr, statusCode := api.ParseDBError(err) + api.HandleErr(w, r, tx, statusCode, useErr, sysErr) + return + } + + multipleServerCapabilities := make([]string, 0, len(msc.ServerCapability)) Review Comment: It doesn't look like this is used for anything, just scanned into but then the value is never read; did you mean to scan back into `msc.ServerCapability` instead? Or assign that property to this slice after the scan? ########## traffic_ops/traffic_ops_golang/server/servers_server_capability.go: ########## @@ -441,3 +442,87 @@ func getDSTenantIDsByIDs(tx *sqlx.Tx, dsIDs []int64) ([]DSTenant, error) { return dsTenantIDs, nil } + +// AssignMultipleServerCapabilities helps assign multiple server capabilities to a given server. +func AssignMultipleServerCapabilities(w http.ResponseWriter, r *http.Request) { + inf, userErr, sysErr, errCode := api.NewInfo(r, []string{"id"}, []string{"id"}) Review Comment: This required `id` route parameter is undocumented - and it doesn't appear to be used. The payload includes a server ID, and that appears to be what's used throughout the handler to identify the server being acted upon. So I think this parameter can be removed (and also removed from the pattern in the route definition in `routes.go`) -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
