srijeet0406 commented on code in PR #7079: URL: https://github.com/apache/trafficcontrol/pull/7079#discussion_r990344181
########## docs/source/api/v4/multiple_servers_capabilities.rst: ########## @@ -0,0 +1,190 @@ +.. +.. +.. 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-v4-multiple_servers_capabilities: + +********************************* +``multiple_servers_capabilities`` +********************************* + +.. versionadded:: 4.1 + +``POST`` +======== +Associates a list of :term:`Server Capability` to a server. The API call replaces all the server capabilities assigned to a server with the ones specified in the serverCapabilities field. +And also Associates a list of :term:`Servers` to a server capability. The API call replaces all the servers assigned to a server capability with the ones specified in the servers field. Review Comment: Instead of repitition, could we reframe this sentence as: Associates a list of server capabilities to a server, and vice versa. This API call replaces all the ..., and vice versa. ########## docs/source/api/v4/multiple_servers_capabilities.rst: ########## @@ -0,0 +1,190 @@ +.. +.. +.. 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-v4-multiple_servers_capabilities: + +********************************* +``multiple_servers_capabilities`` +********************************* + +.. versionadded:: 4.1 + +``POST`` +======== +Associates a list of :term:`Server Capability` to a server. The API call replaces all the server capabilities assigned to a server with the ones specified in the serverCapabilities field. +And also Associates a list of :term:`Servers` to a server capability. The API call replaces all the servers assigned to a server capability with the ones specified in the servers field. + +:Auth. Required: Yes +:Roles Required: "admin" or "operations" +:Permissions Required: SERVER:READ, SERVER:CREATE, SERVER-CAPABILITY:READ, SERVER-CAPABILITY:CREATE +:Response Type: Object + +Request Structure +----------------- +:serverIds: List of :term:`Server` ids ((integral, unique identifier) associated with a :term:`Server Capability` +:serverCapabilities: List of :term:`Server Capability`'s name to associate with a :term:`Server` id + + +.. code-block:: http + :caption: Request Example1 + + POST /api/4.1/multiple_servers_capabilities/ HTTP/1.1 + Host: trafficops.infra.ciab.test + User-Agent: curl/7.47.0 + Accept: */* + Cookie: mojolicious=... + Content-Length: 84 + Content-Type: application/json + + { + "serverIds": [1], + "serverCapabilities": ["test", "disk"] + } + +.. code-block:: http + :caption: Request Example2 + + POST /api/4.1/multiple_servers_capabilities/ HTTP/1.1 + Host: trafficops.infra.ciab.test + User-Agent: curl/7.47.0 + Accept: */* + Cookie: mojolicious=... + Content-Length: 84 + Content-Type: application/json + + { + "serverIds": [2, 3] + "serverCapabilities": ["eas"], + } + +Response Structure +------------------ +:serverId: List of :term:`Server` ids ((integral, unique identifier) associated with a server capability. +:serverCapabilities: List of :term:`Server Capability`'s name to be associated with a :term:`Server` id. Review Comment: same as above re: `names` ########## docs/source/api/v4/multiple_servers_capabilities.rst: ########## @@ -0,0 +1,190 @@ +.. +.. +.. 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-v4-multiple_servers_capabilities: + +********************************* +``multiple_servers_capabilities`` +********************************* + +.. versionadded:: 4.1 + +``POST`` +======== +Associates a list of :term:`Server Capability` to a server. The API call replaces all the server capabilities assigned to a server with the ones specified in the serverCapabilities field. +And also Associates a list of :term:`Servers` to a server capability. The API call replaces all the servers assigned to a server capability with the ones specified in the servers field. + +:Auth. Required: Yes +:Roles Required: "admin" or "operations" +:Permissions Required: SERVER:READ, SERVER:CREATE, SERVER-CAPABILITY:READ, SERVER-CAPABILITY:CREATE +:Response Type: Object + +Request Structure +----------------- +:serverIds: List of :term:`Server` ids ((integral, unique identifier) associated with a :term:`Server Capability` +:serverCapabilities: List of :term:`Server Capability`'s name to associate with a :term:`Server` id Review Comment: nit: should be `names` ########## docs/source/api/v5/multiple_servers_capabilities.rst: ########## @@ -0,0 +1,187 @@ +.. Review Comment: The same comments for the v4 docs apply here, so I won't repeat myself. ########## docs/source/api/v4/multiple_servers_capabilities.rst: ########## @@ -0,0 +1,190 @@ +.. +.. +.. 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-v4-multiple_servers_capabilities: + +********************************* +``multiple_servers_capabilities`` +********************************* + +.. versionadded:: 4.1 + +``POST`` +======== +Associates a list of :term:`Server Capability` to a server. The API call replaces all the server capabilities assigned to a server with the ones specified in the serverCapabilities field. +And also Associates a list of :term:`Servers` to a server capability. The API call replaces all the servers assigned to a server capability with the ones specified in the servers field. + +:Auth. Required: Yes +:Roles Required: "admin" or "operations" +:Permissions Required: SERVER:READ, SERVER:CREATE, SERVER-CAPABILITY:READ, SERVER-CAPABILITY:CREATE +:Response Type: Object + +Request Structure +----------------- +:serverIds: List of :term:`Server` ids ((integral, unique identifier) associated with a :term:`Server Capability` +:serverCapabilities: List of :term:`Server Capability`'s name to associate with a :term:`Server` id + + +.. code-block:: http + :caption: Request Example1 + + POST /api/4.1/multiple_servers_capabilities/ HTTP/1.1 + Host: trafficops.infra.ciab.test + User-Agent: curl/7.47.0 + Accept: */* + Cookie: mojolicious=... + Content-Length: 84 + Content-Type: application/json + + { + "serverIds": [1], + "serverCapabilities": ["test", "disk"] + } + +.. code-block:: http + :caption: Request Example2 + + POST /api/4.1/multiple_servers_capabilities/ HTTP/1.1 + Host: trafficops.infra.ciab.test + User-Agent: curl/7.47.0 + Accept: */* + Cookie: mojolicious=... + Content-Length: 84 + Content-Type: application/json + + { + "serverIds": [2, 3] + "serverCapabilities": ["eas"], + } + +Response Structure +------------------ +:serverId: List of :term:`Server` ids ((integral, unique identifier) associated with a server capability. +:serverCapabilities: List of :term:`Server Capability`'s name to be associated with a :term:`Server` id. + +.. code-block:: http Review Comment: Do you think it would be easier for a reader if the requests and corresponding responses were grouped together? Like this: request1 response1 request2 response2 ########## docs/source/api/v4/multiple_servers_capabilities.rst: ########## @@ -0,0 +1,190 @@ +.. +.. +.. 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-v4-multiple_servers_capabilities: + +********************************* +``multiple_servers_capabilities`` +********************************* + +.. versionadded:: 4.1 + +``POST`` +======== +Associates a list of :term:`Server Capability` to a server. The API call replaces all the server capabilities assigned to a server with the ones specified in the serverCapabilities field. +And also Associates a list of :term:`Servers` to a server capability. The API call replaces all the servers assigned to a server capability with the ones specified in the servers field. + +:Auth. Required: Yes +:Roles Required: "admin" or "operations" +:Permissions Required: SERVER:READ, SERVER:CREATE, SERVER-CAPABILITY:READ, SERVER-CAPABILITY:CREATE +:Response Type: Object + +Request Structure +----------------- +:serverIds: List of :term:`Server` ids ((integral, unique identifier) associated with a :term:`Server Capability` +:serverCapabilities: List of :term:`Server Capability`'s name to associate with a :term:`Server` id + + +.. code-block:: http + :caption: Request Example1 + + POST /api/4.1/multiple_servers_capabilities/ HTTP/1.1 + Host: trafficops.infra.ciab.test + User-Agent: curl/7.47.0 + Accept: */* + Cookie: mojolicious=... + Content-Length: 84 + Content-Type: application/json + + { + "serverIds": [1], + "serverCapabilities": ["test", "disk"] + } + +.. code-block:: http + :caption: Request Example2 + + POST /api/4.1/multiple_servers_capabilities/ HTTP/1.1 + Host: trafficops.infra.ciab.test + User-Agent: curl/7.47.0 + Accept: */* + Cookie: mojolicious=... + Content-Length: 84 + Content-Type: application/json + + { + "serverIds": [2, 3] + "serverCapabilities": ["eas"], + } + +Response Structure +------------------ +:serverId: List of :term:`Server` ids ((integral, unique identifier) associated with a server capability. Review Comment: same as above ########## docs/source/api/v4/multiple_servers_capabilities.rst: ########## @@ -0,0 +1,190 @@ +.. +.. +.. 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-v4-multiple_servers_capabilities: + +********************************* +``multiple_servers_capabilities`` +********************************* + +.. versionadded:: 4.1 + +``POST`` +======== +Associates a list of :term:`Server Capability` to a server. The API call replaces all the server capabilities assigned to a server with the ones specified in the serverCapabilities field. +And also Associates a list of :term:`Servers` to a server capability. The API call replaces all the servers assigned to a server capability with the ones specified in the servers field. + +:Auth. Required: Yes +:Roles Required: "admin" or "operations" +:Permissions Required: SERVER:READ, SERVER:CREATE, SERVER-CAPABILITY:READ, SERVER-CAPABILITY:CREATE +:Response Type: Object + +Request Structure +----------------- +:serverIds: List of :term:`Server` ids ((integral, unique identifier) associated with a :term:`Server Capability` +:serverCapabilities: List of :term:`Server Capability`'s name to associate with a :term:`Server` id + + +.. code-block:: http + :caption: Request Example1 + + POST /api/4.1/multiple_servers_capabilities/ HTTP/1.1 + Host: trafficops.infra.ciab.test + User-Agent: curl/7.47.0 + Accept: */* + Cookie: mojolicious=... + Content-Length: 84 + Content-Type: application/json + + { + "serverIds": [1], + "serverCapabilities": ["test", "disk"] + } + +.. code-block:: http + :caption: Request Example2 + + POST /api/4.1/multiple_servers_capabilities/ HTTP/1.1 + Host: trafficops.infra.ciab.test + User-Agent: curl/7.47.0 + Accept: */* + Cookie: mojolicious=... + Content-Length: 84 + Content-Type: application/json + + { + "serverIds": [2, 3] + "serverCapabilities": ["eas"], + } + +Response Structure +------------------ +:serverId: List of :term:`Server` ids ((integral, unique identifier) associated with a server capability. +:serverCapabilities: List of :term:`Server Capability`'s name to be associated with a :term:`Server` id. + +.. code-block:: http + :caption: Response Example1 + + 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 22:40:54 GMT; Max-Age=3600; HttpOnly + Whole-Content-Sha512: eQrl48zWids0kDpfCYmmtYMpegjnFxfOVvlBYxxLSfp7P7p6oWX4uiC+/Cfh2X9i3G+MQ36eH95gukJqOBOGbQ== + X-Server-Name: traffic_ops_golang/ + Date: Mon, 08 Aug 2022 16:15:11 GMT + Content-Length: 157 + + { + "alerts": [{ + "text": "Multiple Server Capabilities assigned to a server", + "level": "success" + }], + "response": { + "serverIds": [1], + "serverCapabilities": ["test", "disk"] + } + } + +.. code-block:: http + :caption: Response Example2 + + 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 22:40:54 GMT; Max-Age=3600; HttpOnly + Whole-Content-Sha512: eQrl48zWids0kDpfCYmmtYMpegjnFxfOVvlBYxxLSfp7P7p6oWX4uiC+/Cfh2X9i3G+MQ36eH95gukJqOBOGbQ== + X-Server-Name: traffic_ops_golang/ + Date: Mon, 08 Aug 2022 16:15:11 GMT + Content-Length: 157 + + { + "alerts": [{ + "text": "Multiple Servers assigned to a capability", + "level": "success" + }], + "response": { + "serverIds": [2, 3] + "serverCapabilities": ["eas"], + } + } + +``DELETE`` +========== +Deletes a list of :term:`Server Capability` associated to a server. The API call deletes all the server capabilities assigned to a server with the ones specified in the serverCapabilities field. Review Comment: Same comment about reframing the sentences. ########## traffic_ops/traffic_ops_golang/server/servers_server_capability.go: ########## @@ -453,77 +454,166 @@ func AssignMultipleServerCapabilities(w http.ResponseWriter, r *http.Request) { } 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) + var mssc tc.MultipleServersCapabilities + if err := json.NewDecoder(r.Body).Decode(&mssc); err != nil { + api.HandleErr(w, r, tx, http.StatusBadRequest, fmt.Errorf("error decoding POST request body into MultipleServersCapabilities struct %w", 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 + if len(mssc.ServerIDs) == 1 { + errCode, userErr, sysErr = checkExistingServer(tx, mssc.ServerIDs[0], inf.User.UserName) + if userErr != nil || sysErr != nil { + api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr) + return + } } - // Ensure type is correct - correctType := true - if err := tx.QueryRow(scCheckServerTypeQuery(), msc.ServerID).Scan(&correctType); err != nil { + //Check if the server type is MID and/or EDGE + var servArray []int64 + queryType := `SELECT array_agg(s.id) + FROM server s + JOIN type t ON s.type = t.id + WHERE s.id = any ($1) + AND t.use_in_table = 'server' + AND (t.name LIKE 'MID%' OR t.name LIKE 'EDGE%')` + if err := tx.QueryRow(queryType, pq.Array(mssc.ServerIDs)).Scan(pq.Array(&servArray)); 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 + cmp := make(map[int64]bool) + for _, item := range servArray { + cmp[item] = true + } + for _, sid := range mssc.ServerIDs { + if _, ok := cmp[sid]; !ok { + userErr := fmt.Errorf("server id: %d has an incorrect server type. Server capability can only be assigned to EDGE or MID servers", sid) + api.HandleErr(w, r, tx, http.StatusBadRequest, userErr, nil) + return + } } - cdnName, err := dbhelpers.GetCDNNameFromServerID(tx, int64(msc.ServerID)) + // Insert rows in DB + sid := make([]int64, len(mssc.ServerCapabilities)) + scs := make([]string, len(mssc.ServerIDs)) + if len(mssc.ServerIDs) == 1 { + if len(mssc.ServerCapabilities) >= 1 { + for i := range mssc.ServerCapabilities { + sid[i] = mssc.ServerIDs[0] + } + scs = mssc.ServerCapabilities + } + } else if len(mssc.ServerCapabilities) == 1 { + if len(mssc.ServerIDs) >= 1 { + for i := range mssc.ServerIDs { + scs[i] = mssc.ServerCapabilities[0] + } + sid = mssc.ServerIDs + } + } else { + scs = mssc.ServerCapabilities + sid = mssc.ServerIDs + } + + msscQuery := `INSERT INTO server_server_capability + select "server_capability", "server" + FROM UNNEST($1::text[], $2::int[]) AS tmp("server_capability", "server")` + _, err := tx.Query(msscQuery, pq.Array(scs), pq.Array(sid)) if err != nil { - api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, err) + useErr, sysErr, statusCode := api.ParseDBError(err) + api.HandleErr(w, r, tx, statusCode, useErr, sysErr) return } - userErr, sysErr, errCode = dbhelpers.CheckIfCurrentUserCanModifyCDN(tx, string(cdnName), inf.User.UserName) + var alerts tc.Alerts + if len(mssc.ServerCapabilities) == 1 && len(mssc.ServerIDs) == 1 { Review Comment: I was thinking some more about this last night after we spoke. Do you think it would make sense to have another query parameter added to the request that says whether the assignment is from a server to capability or the other way around. Then based on that query param, you could form your database queries. That way, we could get rid of all the extra `if..else` conditions. ########## docs/source/api/v4/multiple_servers_capabilities.rst: ########## @@ -0,0 +1,190 @@ +.. +.. +.. 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-v4-multiple_servers_capabilities: + +********************************* +``multiple_servers_capabilities`` +********************************* + +.. versionadded:: 4.1 + +``POST`` +======== +Associates a list of :term:`Server Capability` to a server. The API call replaces all the server capabilities assigned to a server with the ones specified in the serverCapabilities field. +And also Associates a list of :term:`Servers` to a server capability. The API call replaces all the servers assigned to a server capability with the ones specified in the servers field. + +:Auth. Required: Yes +:Roles Required: "admin" or "operations" +:Permissions Required: SERVER:READ, SERVER:CREATE, SERVER-CAPABILITY:READ, SERVER-CAPABILITY:CREATE +:Response Type: Object + +Request Structure +----------------- +:serverIds: List of :term:`Server` ids ((integral, unique identifier) associated with a :term:`Server Capability` Review Comment: extra opening `(` here ########## traffic_ops/traffic_ops_golang/server/servers_server_capability.go: ########## @@ -443,8 +444,8 @@ 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) { +// AssignMultipleServersCapabilities assigns multiple servers to a capability or multiple server capabilities to a server +func AssignMultipleServersCapabilities(w http.ResponseWriter, r *http.Request) { Review Comment: This function needs to check the locking mechanism, as followed by the other endpoints. ########## traffic_ops/testing/api/v4/server_server_capabilities_test.go: ########## @@ -223,7 +233,14 @@ func TestServerServerCapabilities(t *testing.T) { }) case "PUT": t.Run(name, func(t *testing.T) { - alerts, reqInf, err := testCase.ClientSession.AssignMultipleServerCapability(msc, testCase.RequestOpts, serverId) + var alerts tc.Alerts Review Comment: Sorry just seeing this, where are those checks? I usually do a POST followed by a GET to make sure whether or not the operation actually went through to the database or not. ########## traffic_ops/traffic_ops_golang/server/servers_server_capability.go: ########## @@ -453,77 +454,166 @@ func AssignMultipleServerCapabilities(w http.ResponseWriter, r *http.Request) { } 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) + var mssc tc.MultipleServersCapabilities + if err := json.NewDecoder(r.Body).Decode(&mssc); err != nil { + api.HandleErr(w, r, tx, http.StatusBadRequest, fmt.Errorf("error decoding POST request body into MultipleServersCapabilities struct %w", 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 + if len(mssc.ServerIDs) == 1 { + errCode, userErr, sysErr = checkExistingServer(tx, mssc.ServerIDs[0], inf.User.UserName) + if userErr != nil || sysErr != nil { + api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr) + return + } } - // Ensure type is correct - correctType := true - if err := tx.QueryRow(scCheckServerTypeQuery(), msc.ServerID).Scan(&correctType); err != nil { + //Check if the server type is MID and/or EDGE + var servArray []int64 + queryType := `SELECT array_agg(s.id) + FROM server s + JOIN type t ON s.type = t.id + WHERE s.id = any ($1) + AND t.use_in_table = 'server' + AND (t.name LIKE 'MID%' OR t.name LIKE 'EDGE%')` + if err := tx.QueryRow(queryType, pq.Array(mssc.ServerIDs)).Scan(pq.Array(&servArray)); 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 + cmp := make(map[int64]bool) + for _, item := range servArray { + cmp[item] = true + } + for _, sid := range mssc.ServerIDs { + if _, ok := cmp[sid]; !ok { + userErr := fmt.Errorf("server id: %d has an incorrect server type. Server capability can only be assigned to EDGE or MID servers", sid) + api.HandleErr(w, r, tx, http.StatusBadRequest, userErr, nil) + return + } } - cdnName, err := dbhelpers.GetCDNNameFromServerID(tx, int64(msc.ServerID)) + // Insert rows in DB + sid := make([]int64, len(mssc.ServerCapabilities)) + scs := make([]string, len(mssc.ServerIDs)) + if len(mssc.ServerIDs) == 1 { + if len(mssc.ServerCapabilities) >= 1 { + for i := range mssc.ServerCapabilities { + sid[i] = mssc.ServerIDs[0] + } + scs = mssc.ServerCapabilities + } + } else if len(mssc.ServerCapabilities) == 1 { + if len(mssc.ServerIDs) >= 1 { + for i := range mssc.ServerIDs { + scs[i] = mssc.ServerCapabilities[0] + } + sid = mssc.ServerIDs + } + } else { + scs = mssc.ServerCapabilities + sid = mssc.ServerIDs + } + + msscQuery := `INSERT INTO server_server_capability + select "server_capability", "server" + FROM UNNEST($1::text[], $2::int[]) AS tmp("server_capability", "server")` + _, err := tx.Query(msscQuery, pq.Array(scs), pq.Array(sid)) if err != nil { - api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, err) + useErr, sysErr, statusCode := api.ParseDBError(err) + api.HandleErr(w, r, tx, statusCode, useErr, sysErr) return } - userErr, sysErr, errCode = dbhelpers.CheckIfCurrentUserCanModifyCDN(tx, string(cdnName), inf.User.UserName) + var alerts tc.Alerts + if len(mssc.ServerCapabilities) == 1 && len(mssc.ServerIDs) == 1 { + alerts = tc.CreateAlerts(tc.SuccessLevel, "Assigned either a Server Capability to a server or a Server to a capability") + } else if len(mssc.ServerCapabilities) > 1 && len(mssc.ServerIDs) == 1 { + alerts = tc.CreateAlerts(tc.SuccessLevel, "Multiple Server Capabilities assigned to a server") + } else if len(mssc.ServerCapabilities) == 1 && len(mssc.ServerIDs) > 1 { + alerts = tc.CreateAlerts(tc.SuccessLevel, "Multiple Servers assigned to a capability") + } else { + alerts = tc.CreateAlerts(tc.SuccessLevel, "Multiple Servers assigned to multiple capabilities") + } + api.WriteAlertsObj(w, r, http.StatusOK, alerts, mssc) + return +} + +// DeleteMultipleServersCapabilities deletes multiple servers to a capability or multiple server capabilities to a server +func DeleteMultipleServersCapabilities(w http.ResponseWriter, r *http.Request) { + inf, userErr, sysErr, errCode := api.NewInfo(r, nil, nil) + tx := inf.Tx.Tx if userErr != nil || sysErr != nil { - api.HandleErr(w, r, tx, errCode, userErr, sysErr) + api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr) return } + defer inf.Close() - //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) + var mssc tc.MultipleServersCapabilities + if err := json.NewDecoder(r.Body).Decode(&mssc); err != nil { + api.HandleErr(w, r, tx, http.StatusBadRequest, fmt.Errorf("error decoding DELETE request body into MultipleServersCapabilities struct %w", err), nil) return } - multipleServerCapabilities := make([]string, 0, len(msc.ServerCapabilities)) + if len(mssc.ServerIDs) == 1 { + errCode, userErr, sysErr = checkExistingServer(tx, mssc.ServerIDs[0], inf.User.UserName) + if userErr != nil || sysErr != nil { + api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr) + return + } + } - mscQuery := `WITH inserted AS ( - INSERT INTO server_server_capability - SELECT "server_capability", $2 - FROM UNNEST($1::text[]) AS tmp("server_capability") - RETURNING server_capability - ) - SELECT ARRAY_AGG(server_capability) - FROM ( - SELECT server_capability - FROM inserted - ) AS returned(server_capability)` + //Delete existing rows from server_server_capability for a given server or for a given capability + const delQuery = `DELETE FROM server_server_capability ssc WHERE ` + var err error + var result sql.Result + var alerts tc.Alerts + if len(mssc.ServerCapabilities) == 1 && len(mssc.ServerIDs) == 1 { + result, err = tx.Exec(delQuery+`ssc.server_capability=$1 AND ssc.server=$2`, mssc.ServerCapabilities[0], mssc.ServerIDs[0]) + } else if len(mssc.ServerCapabilities) == 1 { + result, err = tx.Exec(delQuery+`ssc.server_capability=$1`, mssc.ServerCapabilities[0]) + } else if len(mssc.ServerIDs) == 1 { + result, err = tx.Exec(delQuery+`ssc.server=$1`, mssc.ServerIDs[0]) + } - err = tx.QueryRow(mscQuery, pq.Array(msc.ServerCapabilities), msc.ServerID).Scan(pq.Array(&multipleServerCapabilities)) if err != nil { useErr, sysErr, statusCode := api.ParseDBError(err) api.HandleErr(w, r, tx, statusCode, useErr, sysErr) return } - msc.ServerCapabilities = multipleServerCapabilities - alerts := tc.CreateAlerts(tc.SuccessLevel, "Multiple Server Capabilities assigned to a server") - api.WriteAlertsObj(w, r, http.StatusOK, alerts, msc) + rowsAffected, err := result.RowsAffected() + if err != nil { + api.HandleErr(w, r, tx, http.StatusInternalServerError, fmt.Errorf("no rows were deleted from server_server_capability table: %w", err), sysErr) + return + } + if rowsAffected == 1 { + alerts = tc.CreateAlerts(tc.SuccessLevel, "Removed either a Server Capability to a server or a Server to a capability") Review Comment: If you were to adopt the above suggestion, you could base your alerts off of that query param. ########## traffic_ops/traffic_ops_golang/server/servers_server_capability.go: ########## @@ -453,77 +454,166 @@ func AssignMultipleServerCapabilities(w http.ResponseWriter, r *http.Request) { } 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) + var mssc tc.MultipleServersCapabilities + if err := json.NewDecoder(r.Body).Decode(&mssc); err != nil { + api.HandleErr(w, r, tx, http.StatusBadRequest, fmt.Errorf("error decoding POST request body into MultipleServersCapabilities struct %w", 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 + if len(mssc.ServerIDs) == 1 { + errCode, userErr, sysErr = checkExistingServer(tx, mssc.ServerIDs[0], inf.User.UserName) + if userErr != nil || sysErr != nil { + api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr) + return + } } - // Ensure type is correct - correctType := true - if err := tx.QueryRow(scCheckServerTypeQuery(), msc.ServerID).Scan(&correctType); err != nil { + //Check if the server type is MID and/or EDGE + var servArray []int64 Review Comment: Is there a way that you could reuse/ tweak the `scCheckServerTypeQuery` to achieve this functionality? -- 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]
