ocket8888 commented on code in PR #6544: URL: https://github.com/apache/trafficcontrol/pull/6544#discussion_r843103615
########## traffic_ops/app/db/migrations/2022032914235900_add_server_profile_table.up.sql: ########## @@ -0,0 +1,30 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with this + * work for additional information regarding copyright ownership. The ASF + * licenses this file to you 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 undera + * the License. + */ + +CREATE TABLE IF NOT EXISTS public.server_profile ( + server bigint NOT NULL, + profile_name text NOT NULL, + priority bigint NOT NULL, Review Comment: Could be a nice safety to check that priority isn't negative. Something's wrong with the API code if that happens, of course, but you never know, that might help catch a bug. ########## traffic_ops/testing/api/v4/cdn_queue_updates_by_type_profile_test.go: ########## @@ -140,7 +140,7 @@ func QueueUpdatesByProfile(t *testing.T) { } server := testData.Servers[0] opts := client.NewRequestOptions() - if server.CDNName == nil || server.Profile == nil { + if server.CDNName == nil || server.ProfileNames[0] == "" { Review Comment: I think to replicate the segfault-prevention logic of the check this should be checking the length of `server.ProfileNames` instead of the value of its first entry. You can certainly check both, but the length is more important for that reason. ########## lib/go-atscfg/parentdotconfig.go: ########## @@ -1922,8 +1918,8 @@ func getProfileParentConfigParams(tcParentConfigParams []tc.Parameter) (map[stri func getServerParentConfigParams(server *Server, profileParentConfigParams map[string]map[string]string) map[string]string { // We only need parent.config params, don't need all the params on the server serverParams := map[string]string{} - if server.Profile == nil || *server.Profile != "" { // TODO warn/error if false? Servers requires profiles - for name, val := range profileParentConfigParams[*server.Profile] { + if len(server.ProfileNames) == 0 || server.ProfileNames[0] != "" { // TODO warn/error if false? Servers requires profiles + for name, val := range profileParentConfigParams[server.ProfileNames[0]] { Review Comment: This condition confuses me. I'm not sure that the original logic was correct. It seems like it should've been checking `!= nil` not `== nil`. Otherwise it was making sure that it's possible for the next line to segfault, which doesn't make sense. I think while what you've done recreates the original logic, it should instead check `!= 0`. @rob05c Care to weigh in? ########## traffic_ops/testing/api/v4/tc-fixtures.json: ########## @@ -3352,8 +3352,8 @@ { "ipAddresses": [ { - "address": "127.0.0.1/30", - "gateway": "127.0.0.1", + "address": "128.0.0.1/30", + "gateway": "128.0.0.1", Review Comment: same question as above ########## traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go: ########## @@ -1788,3 +1788,102 @@ func GetRegionNameFromID(tx *sql.Tx, regionID int) (string, bool, error) { } return regionName, true, nil } + +// GetCommonServerPropertiesFromV4 converts CommonServerPropertiesV40 to CommonServerProperties struct +func GetCommonServerPropertiesFromV4(s tc.ServerV40, tx *sql.Tx) (tc.CommonServerProperties, error) { + var id int + var desc string + rows, err := tx.Query("SELECT id, description from profile WHERE name=$1", (s.ProfileNames)[0]) + if err != nil { + return tc.CommonServerProperties{}, fmt.Errorf("querying profile id and description by profile_name: " + err.Error()) Review Comment: returning nested errors should wrap using the `%w` formatting parameter of `fmt.Errorf` instead of doing string concatenation ########## traffic_ops/testing/api/v4/servers_test.go: ########## @@ -606,24 +617,24 @@ func CreateTestServerWithoutProfileID(t *testing.T) { } server := resp.Response[0] - if server.Profile == nil || server.ID == nil || server.HostName == nil { + if server.ProfileNames[0] == "" || server.ID == nil || server.HostName == nil { t.Fatal("Traffic Ops returned a representation of a server with null or undefined ID and/or Profile and/or Host Name") } - originalProfile := *server.Profile + originalProfile := server.ProfileNames delResp, _, err := TOSession.DeleteServer(*server.ID, client.RequestOptions{}) if err != nil { t.Fatalf("cannot delete Server by ID %d: %v - %v", *server.ID, err, delResp) } - *server.Profile = "" - server.ProfileID = nil + server.ProfileNames = []string{""} + //server.ProfileID = nil Review Comment: This line should be removed instead of just commented out ########## traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go: ########## @@ -1788,3 +1788,102 @@ func GetRegionNameFromID(tx *sql.Tx, regionID int) (string, bool, error) { } return regionName, true, nil } + +// GetCommonServerPropertiesFromV4 converts CommonServerPropertiesV40 to CommonServerProperties struct +func GetCommonServerPropertiesFromV4(s tc.ServerV40, tx *sql.Tx) (tc.CommonServerProperties, error) { + var id int + var desc string + rows, err := tx.Query("SELECT id, description from profile WHERE name=$1", (s.ProfileNames)[0]) + if err != nil { + return tc.CommonServerProperties{}, fmt.Errorf("querying profile id and description by profile_name: " + err.Error()) + } + defer log.Close(rows, "closing rows in GetCommonServerPropertiesFromV4") + + for rows.Next() { + if err := rows.Scan(&id, &desc); err != nil { + return tc.CommonServerProperties{}, fmt.Errorf("scanning profile: " + err.Error()) + } + } + + return tc.CommonServerProperties{ + Cachegroup: s.Cachegroup, + CachegroupID: s.CachegroupID, + CDNID: s.CDNID, + CDNName: s.CDNName, + DeliveryServices: s.DeliveryServices, + DomainName: s.DomainName, + FQDN: s.FQDN, + FqdnTime: s.FqdnTime, + GUID: s.GUID, + HostName: s.HostName, + HTTPSPort: s.HTTPSPort, + ID: s.ID, + ILOIPAddress: s.ILOIPAddress, + ILOIPGateway: s.ILOIPGateway, + ILOIPNetmask: s.ILOIPNetmask, + ILOPassword: s.ILOPassword, + ILOUsername: s.ILOUsername, + LastUpdated: s.LastUpdated, + MgmtIPAddress: s.MgmtIPAddress, + MgmtIPGateway: s.MgmtIPGateway, + MgmtIPNetmask: s.MgmtIPNetmask, + OfflineReason: s.OfflineReason, + Profile: &(s.ProfileNames)[0], + ProfileDesc: &desc, + ProfileID: &id, + PhysLocation: s.PhysLocation, + PhysLocationID: s.PhysLocationID, + Rack: s.Rack, + RevalPending: s.RevalPending, + Status: s.Status, + StatusID: s.StatusID, + TCPPort: s.TCPPort, + Type: s.Type, + TypeID: s.TypeID, + UpdPending: s.UpdPending, + XMPPID: s.XMPPID, + XMPPPasswd: s.XMPPPasswd, + }, nil +} + +// UpdateServerProfilesForV4 updates server_profile table via update function for APIv4 +func UpdateServerProfilesForV4(id *int, profile []string, tx *sql.Tx) error { Review Comment: every time `id` is used in this function it's dereferenced, it'd probably be simpler to just make it an `int` and have callers dereference as necessary. ########## traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go: ########## @@ -1788,3 +1788,102 @@ func GetRegionNameFromID(tx *sql.Tx, regionID int) (string, bool, error) { } return regionName, true, nil } + +// GetCommonServerPropertiesFromV4 converts CommonServerPropertiesV40 to CommonServerProperties struct +func GetCommonServerPropertiesFromV4(s tc.ServerV40, tx *sql.Tx) (tc.CommonServerProperties, error) { + var id int + var desc string + rows, err := tx.Query("SELECT id, description from profile WHERE name=$1", (s.ProfileNames)[0]) + if err != nil { + return tc.CommonServerProperties{}, fmt.Errorf("querying profile id and description by profile_name: " + err.Error()) + } + defer log.Close(rows, "closing rows in GetCommonServerPropertiesFromV4") + + for rows.Next() { + if err := rows.Scan(&id, &desc); err != nil { + return tc.CommonServerProperties{}, fmt.Errorf("scanning profile: " + err.Error()) + } + } + + return tc.CommonServerProperties{ + Cachegroup: s.Cachegroup, + CachegroupID: s.CachegroupID, + CDNID: s.CDNID, + CDNName: s.CDNName, + DeliveryServices: s.DeliveryServices, + DomainName: s.DomainName, + FQDN: s.FQDN, + FqdnTime: s.FqdnTime, + GUID: s.GUID, + HostName: s.HostName, + HTTPSPort: s.HTTPSPort, + ID: s.ID, + ILOIPAddress: s.ILOIPAddress, + ILOIPGateway: s.ILOIPGateway, + ILOIPNetmask: s.ILOIPNetmask, + ILOPassword: s.ILOPassword, + ILOUsername: s.ILOUsername, + LastUpdated: s.LastUpdated, + MgmtIPAddress: s.MgmtIPAddress, + MgmtIPGateway: s.MgmtIPGateway, + MgmtIPNetmask: s.MgmtIPNetmask, + OfflineReason: s.OfflineReason, + Profile: &(s.ProfileNames)[0], + ProfileDesc: &desc, + ProfileID: &id, + PhysLocation: s.PhysLocation, + PhysLocationID: s.PhysLocationID, + Rack: s.Rack, + RevalPending: s.RevalPending, + Status: s.Status, + StatusID: s.StatusID, + TCPPort: s.TCPPort, + Type: s.Type, + TypeID: s.TypeID, + UpdPending: s.UpdPending, + XMPPID: s.XMPPID, + XMPPPasswd: s.XMPPPasswd, + }, nil +} + +// UpdateServerProfilesForV4 updates server_profile table via update function for APIv4 +func UpdateServerProfilesForV4(id *int, profile []string, tx *sql.Tx) error { + var profileNames []string + + //Delete existing rows from server_profile to get the priority correct for profile_name changes + _, err := tx.Exec("DELETE FROM server_profile WHERE server=$1", *id) + if err != nil { + return fmt.Errorf("updating server_profile by server id: %v" + strconv.Itoa(*id) + ", error: " + err.Error()) + } + + for i, pName := range profile { + query := `INSERT INTO server_profile (server, profile_name, priority) VALUES ($1, $2, $3)` + _, err := tx.Exec(query, *id, pName, i) + if err != nil { + return fmt.Errorf("error inserting into server_profile table, %v", err) + } + } + + err = tx.QueryRow("SELECT ARRAY_AGG(profile_name) FROM server_profile WHERE server=$1", *id).Scan(pq.Array(&profileNames)) + if err == sql.ErrNoRows { + return fmt.Errorf("selecting server_profile by profile_name: " + err.Error()) + } + return nil +} + +// UpdateServerProfileTableForV2V3 updates CommonServerPropertiesV40 struct and server_profile table via Update (server) function for API v2/v3 +func UpdateServerProfileTableForV2V3(id *int, newProfile *string, origProfile string, tx *sql.Tx) ([]string, error) { + var profileName []string + query := `UPDATE server_profile SET profile_name=$1 WHERE server=$2 AND profile_name=$3` Review Comment: I think this may have been discussed somewhere before but I can't remember - if I PUT at APIv3, does the current array go away and it should be length 1 with only the PUT-ed Profile? Or is it supposed to do this where it leaves extra entries intact? I could see an argument either way. ########## lib/go-tc/servers.go: ########## @@ -700,6 +700,44 @@ type CommonServerProperties struct { XMPPPasswd *string `json:"xmppPasswd" db:"xmpp_passwd"` } +type CommonServerPropertiesV40 struct { Review Comment: I don't know how to explain this without launching into a long spiel, so please bear with me. There are basically two schools of thought regarding how to handle creating new structures for new API versions. a. Create a new structure with all of the fields necessary to fully represent the object. (This is what I like to do/think is best) b. Create a new structure with _only_ the fields that differ from the previous version, by way of addition or data type change, and use structure nesting to prevent field duplication to the greatest extent possible. (This is what I think generally most other people like to do/think is best). This usually means that new structures embed the old ones like ```go type FooV40 struct { FooV3 NewField string `json:"newField" db:"new_field"` } ``` However, option b really only works cleanly when a new field is being added or an existing one is changing types. Handling the _removal_ of a property (e.g. `profileDesc`) is a bit harder. Since we don't support backward compatibility on sub-structure member access (for example, you can technically access `DeliveryServiceV4.DeliveryServiceNullableFieldsV11.GeoLimitCountries` which is a `*string` and different than `DeliveryServiceV4.GeoLimitCountries` which is a `[]string` but we don't support people doing the `DeliveryServiceV4.DeliveryServiceNullableFieldsV11` part) what we've done with servers is extract the parts that are duplicated between versions into their own struct, which we call `CommonServerProperties`, and embed that within each specific version's struct and only create the fields on each separate version that differ between the versions. That was introduced with the multi-interfaces change which removed, among other things, `ipAddress`. Both options a and b have been used to different degrees throughout the API. But this solution appears to straddle the line. You've essentially duplicated the `CommonServerProperties` structure but with some differences. At that point, I don't much see the value in keeping these fields outside the `ServerV4` structure. That would be option a, where the ServerV4 has all these fields that haven't changed between versions re-defined with it. To embrace option b - which, I believe @rawlinp championed when I was removing the IP fields - you'd just pull the parts you're removing out of `CommonServerProperties` and define them on their own in the APIv2 and v3 structures, then put `ProfileNames` on the `ServerV4` structure, and all versions can continue to embed `CommonServerProperties` without us needing to create a `CommonServerPropertiesV4`. You could put the removed fields into their own structure, to be shared between API versions 2 and 3, but I wouldn't bother because it's such a small amount of duplication and APIv2 being deprecated will be going away some time in the near future anyway. I, personally, like option a, because it's much simpler, even if it does duplicate things. I think @rawlinp would appreciate if you instead did b (which is why I'm pinging you, Rawlin, since you (and I think maybe some others) like it better that second way I don't want to encourage anyone to break that pattern unless you've changed your mind or something) but either way, I don't think we should end up with any kind of `CommonServerPropertiesV40`. ########## traffic_ops/app/db/migrations/2022032914235900_add_server_profile_table.up.sql: ########## @@ -0,0 +1,30 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with this + * work for additional information regarding copyright ownership. The ASF + * licenses this file to you 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 undera + * the License. + */ + +CREATE TABLE IF NOT EXISTS public.server_profile ( + server bigint NOT NULL, + profile_name text NOT NULL, + priority bigint NOT NULL, Review Comment: This doesn't matter at all but `priority` probably doesn't need to be a `bigint`, just `int` would do fine unless we envision a single server having more than 2,147,483,647 Profiles. Frankly even a `smallint` would suffice, but I only bring it up because `int` is the more generic data type, so `smallint` doesn't really move toward that goal of simplicity. `server` ~~has to be~~ should be a `bigint` because that's the underlying type of the `bigserial` that servers use to store IDs. Again, that doesn't matter at all, don't bother changing it unless you really agree and really really want to. Just wanted to mention it. ########## traffic_ops/app/db/migrations/2022032914235900_add_server_profile_table.up.sql: ########## @@ -0,0 +1,30 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with this + * work for additional information regarding copyright ownership. The ASF + * licenses this file to you 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 undera + * the License. + */ + +CREATE TABLE IF NOT EXISTS public.server_profile ( + server bigint NOT NULL, + profile_name text NOT NULL, + priority bigint NOT NULL, + CONSTRAINT pk_server_profile PRIMARY KEY (profile_name, server), + CONSTRAINT fk_server_id FOREIGN KEY (server) REFERENCES public.server(id) ON DELETE CASCADE ON UPDATE CASCADE, + CONSTRAINT fk_server_profile_name_profile FOREIGN KEY (profile_name) REFERENCES public.profile(name) ON UPDATE CASCADE ON DELETE RESTRICT + ); + +INSERT into public.server_profile(server, profile_name, priority) +SELECT s.id, p.name, 0 +FROM public.server AS s + JOIN public.profile p ON p.id=s.profile; Review Comment: Okay, so, I know you told me this already in person, but I can't remember anything unless it's written down, so I'm sorry but could you please tell me one more time why we're not getting rid of the `public.server.profile` column? ########## traffic_ops/traffic_ops_golang/server/servers.go: ########## @@ -1448,6 +1542,10 @@ func Update(w http.ResponseWriter, r *http.Request) { api.HandleErr(w, r, tx, http.StatusBadRequest, err, nil) return } + if err := dbhelpers.UpdateServerProfilesForV4(server.ID, server.ProfileNames, tx); err != nil { + api.HandleErr(w, r, tx, http.StatusBadRequest, err, nil) Review Comment: currently, `dbhelpers.UpdateServerProfilesForV4` only returns errors that occurred if they're `sql.ErrNoRows` exactly. I already left a comment asking that all errors be returned - but in either case it's not safe to expose that general error information to the user. Even when it's only `database/sql.ErrNoRows`, that doesn't tell the user what they did wrong. To figure out what to tell the user, what to log, and what the response code should be based on a database error, use `api.ParseDBError`. ########## traffic_portal/app/src/common/modules/form/server/FormServerController.js: ########## @@ -60,6 +60,14 @@ var FormServerController = function(server, $scope, $location, $state, $uibModal }); }; + $scope.getProfileID = function(profileName) { + for (const profile of $scope.profiles) { + if (profile.name === profileName) { + return "/#!/profiles/"+profile.id Review Comment: nit: Javascript has a builtin for this: [the `find` method of the `Array` class](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/find). So this whole function can be written on one line: ```javascript $scope.getProfileID = profileName => `/#!/profiles/${$scope.profiles.find(profileName).id}`; ``` ########## traffic_ops/traffic_ops_golang/server/servers.go: ########## @@ -1634,6 +1787,36 @@ func Update(w http.ResponseWriter, r *http.Request) { api.CreateChangeLogRawTx(api.ApiChange, changeLogMsg, inf.User, tx) } +func insertServerProfile(id *int, pName []string, tx *sql.Tx) (error, error, int) { + insertSPQuery := ` + INSERT INTO server_profile ( + server, + profile_name, + priority + ) VALUES + ` + + //var priorityArray pq.Int64Array + //priorityArray = append(priorityArray, int64(i)) Review Comment: These two lines should be removed, not just commented out ########## traffic_portal/app/src/common/modules/form/server/form.server.tpl.html: ########## @@ -112,11 +112,11 @@ <label for="profile" ng-class="{'has-error': hasError(serverForm.profile)}">Profile *</label> <div ng-class="{'has-error': hasError(serverForm.profile), 'has-feedback': hasError(serverForm.profile)}"> - <select id="profile" name="profile" class="form-control" ng-model="server.profileId" ng-options="profile.id as profile.name for profile in profiles" required> + <select id="profile" name="profile" class="form-control" ng-model="server.profileNames" multiple ng-options="profile.name as profile.name for profile in profiles" required> Review Comment: I'm _pretty_ sure that you can bind this to `server.profileNames[0]` if that property is properly initialized to an array in `traffic_portal/app/src/modules/private/servers/new/index.js`, but I still have to test that. I'll report back here once I have and either confirm or resolve the conversation. ########## traffic_ops/testing/api/v4/servers_test.go: ########## @@ -218,7 +218,7 @@ func LastServerInTopologyCacheGroup(t *testing.T) { t.Fatalf("expected to get %d server from cdn %s from cachegroup %s in topology %s, got %d servers", expectedLength, cdnName, cacheGroupName, topologyName, len(servers.Response)) } server := servers.Response[0] - if server.ID == nil || server.CDNID == nil || server.ProfileID == nil || server.CachegroupID == nil || server.HostName == nil { + if server.ID == nil || server.CDNID == nil || server.ProfileNames[0] == "" || server.CachegroupID == nil || server.HostName == nil { Review Comment: same as above RE: checking length. You may also want to alter the error message below, since currently it mentions a Profile *ID*. ########## traffic_ops/testing/api/v4/tc-fixtures.json: ########## @@ -2817,8 +2817,8 @@ "serviceAddress": false }, { - "address": "127.0.0.13/30", - "gateway": "127.0.0.1", + "address": "128.0.0.13/30", + "gateway": "128.0.0.1", Review Comment: Why'd you change these IPs? ########## traffic_ops/testing/api/v4/tc-fixtures.json: ########## @@ -3014,8 +3014,8 @@ "serviceAddress": false }, { - "address": "127.0.0.17/30", - "gateway": "127.0.0.17", + "address": "128.0.0.17/30", + "gateway": "128.0.0.17", Review Comment: same question as above ########## traffic_ops/testing/api/v4/servers_test.go: ########## @@ -606,24 +617,24 @@ func CreateTestServerWithoutProfileID(t *testing.T) { } server := resp.Response[0] - if server.Profile == nil || server.ID == nil || server.HostName == nil { + if server.ProfileNames[0] == "" || server.ID == nil || server.HostName == nil { Review Comment: Same as above ########## traffic_ops/testing/api/v4/tc-fixtures.json: ########## @@ -4100,8 +4100,8 @@ "serviceAddress": false }, { - "address": "192.0.2.13/24", - "gateway": "192.0.2.1", + "address": "192.0.3.13/24", + "gateway": "192.0.3.1", Review Comment: same question as above. These are in TEST-NET-2 so nothing should be depending on them being real. ########## traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go: ########## @@ -1788,3 +1788,102 @@ func GetRegionNameFromID(tx *sql.Tx, regionID int) (string, bool, error) { } return regionName, true, nil } + +// GetCommonServerPropertiesFromV4 converts CommonServerPropertiesV40 to CommonServerProperties struct +func GetCommonServerPropertiesFromV4(s tc.ServerV40, tx *sql.Tx) (tc.CommonServerProperties, error) { + var id int + var desc string + rows, err := tx.Query("SELECT id, description from profile WHERE name=$1", (s.ProfileNames)[0]) + if err != nil { + return tc.CommonServerProperties{}, fmt.Errorf("querying profile id and description by profile_name: " + err.Error()) + } + defer log.Close(rows, "closing rows in GetCommonServerPropertiesFromV4") + + for rows.Next() { + if err := rows.Scan(&id, &desc); err != nil { + return tc.CommonServerProperties{}, fmt.Errorf("scanning profile: " + err.Error()) + } + } + + return tc.CommonServerProperties{ + Cachegroup: s.Cachegroup, + CachegroupID: s.CachegroupID, + CDNID: s.CDNID, + CDNName: s.CDNName, + DeliveryServices: s.DeliveryServices, + DomainName: s.DomainName, + FQDN: s.FQDN, + FqdnTime: s.FqdnTime, + GUID: s.GUID, + HostName: s.HostName, + HTTPSPort: s.HTTPSPort, + ID: s.ID, + ILOIPAddress: s.ILOIPAddress, + ILOIPGateway: s.ILOIPGateway, + ILOIPNetmask: s.ILOIPNetmask, + ILOPassword: s.ILOPassword, + ILOUsername: s.ILOUsername, + LastUpdated: s.LastUpdated, + MgmtIPAddress: s.MgmtIPAddress, + MgmtIPGateway: s.MgmtIPGateway, + MgmtIPNetmask: s.MgmtIPNetmask, + OfflineReason: s.OfflineReason, + Profile: &(s.ProfileNames)[0], + ProfileDesc: &desc, + ProfileID: &id, + PhysLocation: s.PhysLocation, + PhysLocationID: s.PhysLocationID, + Rack: s.Rack, + RevalPending: s.RevalPending, + Status: s.Status, + StatusID: s.StatusID, + TCPPort: s.TCPPort, + Type: s.Type, + TypeID: s.TypeID, + UpdPending: s.UpdPending, + XMPPID: s.XMPPID, + XMPPPasswd: s.XMPPPasswd, + }, nil +} + +// UpdateServerProfilesForV4 updates server_profile table via update function for APIv4 Review Comment: GoDoc comments should end with punctuation ########## traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go: ########## @@ -1788,3 +1788,102 @@ func GetRegionNameFromID(tx *sql.Tx, regionID int) (string, bool, error) { } return regionName, true, nil } + +// GetCommonServerPropertiesFromV4 converts CommonServerPropertiesV40 to CommonServerProperties struct +func GetCommonServerPropertiesFromV4(s tc.ServerV40, tx *sql.Tx) (tc.CommonServerProperties, error) { + var id int + var desc string + rows, err := tx.Query("SELECT id, description from profile WHERE name=$1", (s.ProfileNames)[0]) + if err != nil { + return tc.CommonServerProperties{}, fmt.Errorf("querying profile id and description by profile_name: " + err.Error()) + } + defer log.Close(rows, "closing rows in GetCommonServerPropertiesFromV4") + + for rows.Next() { + if err := rows.Scan(&id, &desc); err != nil { + return tc.CommonServerProperties{}, fmt.Errorf("scanning profile: " + err.Error()) Review Comment: same as above RE: error-wrapping ########## traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go: ########## @@ -1788,3 +1788,102 @@ func GetRegionNameFromID(tx *sql.Tx, regionID int) (string, bool, error) { } return regionName, true, nil } + +// GetCommonServerPropertiesFromV4 converts CommonServerPropertiesV40 to CommonServerProperties struct Review Comment: GoDoc comments should end with punctuation ########## traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go: ########## @@ -1788,3 +1788,102 @@ func GetRegionNameFromID(tx *sql.Tx, regionID int) (string, bool, error) { } return regionName, true, nil } + +// GetCommonServerPropertiesFromV4 converts CommonServerPropertiesV40 to CommonServerProperties struct +func GetCommonServerPropertiesFromV4(s tc.ServerV40, tx *sql.Tx) (tc.CommonServerProperties, error) { + var id int + var desc string + rows, err := tx.Query("SELECT id, description from profile WHERE name=$1", (s.ProfileNames)[0]) + if err != nil { + return tc.CommonServerProperties{}, fmt.Errorf("querying profile id and description by profile_name: " + err.Error()) + } + defer log.Close(rows, "closing rows in GetCommonServerPropertiesFromV4") + + for rows.Next() { + if err := rows.Scan(&id, &desc); err != nil { + return tc.CommonServerProperties{}, fmt.Errorf("scanning profile: " + err.Error()) + } + } + + return tc.CommonServerProperties{ + Cachegroup: s.Cachegroup, + CachegroupID: s.CachegroupID, + CDNID: s.CDNID, + CDNName: s.CDNName, + DeliveryServices: s.DeliveryServices, + DomainName: s.DomainName, + FQDN: s.FQDN, + FqdnTime: s.FqdnTime, + GUID: s.GUID, + HostName: s.HostName, + HTTPSPort: s.HTTPSPort, + ID: s.ID, + ILOIPAddress: s.ILOIPAddress, + ILOIPGateway: s.ILOIPGateway, + ILOIPNetmask: s.ILOIPNetmask, + ILOPassword: s.ILOPassword, + ILOUsername: s.ILOUsername, + LastUpdated: s.LastUpdated, + MgmtIPAddress: s.MgmtIPAddress, + MgmtIPGateway: s.MgmtIPGateway, + MgmtIPNetmask: s.MgmtIPNetmask, + OfflineReason: s.OfflineReason, + Profile: &(s.ProfileNames)[0], + ProfileDesc: &desc, + ProfileID: &id, + PhysLocation: s.PhysLocation, + PhysLocationID: s.PhysLocationID, + Rack: s.Rack, + RevalPending: s.RevalPending, + Status: s.Status, + StatusID: s.StatusID, + TCPPort: s.TCPPort, + Type: s.Type, + TypeID: s.TypeID, + UpdPending: s.UpdPending, + XMPPID: s.XMPPID, + XMPPPasswd: s.XMPPPasswd, + }, nil +} + +// UpdateServerProfilesForV4 updates server_profile table via update function for APIv4 +func UpdateServerProfilesForV4(id *int, profile []string, tx *sql.Tx) error { + var profileNames []string + + //Delete existing rows from server_profile to get the priority correct for profile_name changes + _, err := tx.Exec("DELETE FROM server_profile WHERE server=$1", *id) + if err != nil { + return fmt.Errorf("updating server_profile by server id: %v" + strconv.Itoa(*id) + ", error: " + err.Error()) + } + + for i, pName := range profile { + query := `INSERT INTO server_profile (server, profile_name, priority) VALUES ($1, $2, $3)` + _, err := tx.Exec(query, *id, pName, i) + if err != nil { + return fmt.Errorf("error inserting into server_profile table, %v", err) + } + } + + err = tx.QueryRow("SELECT ARRAY_AGG(profile_name) FROM server_profile WHERE server=$1", *id).Scan(pq.Array(&profileNames)) + if err == sql.ErrNoRows { + return fmt.Errorf("selecting server_profile by profile_name: " + err.Error()) + } + return nil +} + +// UpdateServerProfileTableForV2V3 updates CommonServerPropertiesV40 struct and server_profile table via Update (server) function for API v2/v3 Review Comment: GoDoc comments should end with punctuation ########## traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go: ########## @@ -1788,3 +1788,102 @@ func GetRegionNameFromID(tx *sql.Tx, regionID int) (string, bool, error) { } return regionName, true, nil } + +// GetCommonServerPropertiesFromV4 converts CommonServerPropertiesV40 to CommonServerProperties struct +func GetCommonServerPropertiesFromV4(s tc.ServerV40, tx *sql.Tx) (tc.CommonServerProperties, error) { + var id int + var desc string + rows, err := tx.Query("SELECT id, description from profile WHERE name=$1", (s.ProfileNames)[0]) + if err != nil { + return tc.CommonServerProperties{}, fmt.Errorf("querying profile id and description by profile_name: " + err.Error()) + } + defer log.Close(rows, "closing rows in GetCommonServerPropertiesFromV4") + + for rows.Next() { + if err := rows.Scan(&id, &desc); err != nil { + return tc.CommonServerProperties{}, fmt.Errorf("scanning profile: " + err.Error()) + } + } + + return tc.CommonServerProperties{ + Cachegroup: s.Cachegroup, + CachegroupID: s.CachegroupID, + CDNID: s.CDNID, + CDNName: s.CDNName, + DeliveryServices: s.DeliveryServices, + DomainName: s.DomainName, + FQDN: s.FQDN, + FqdnTime: s.FqdnTime, + GUID: s.GUID, + HostName: s.HostName, + HTTPSPort: s.HTTPSPort, + ID: s.ID, + ILOIPAddress: s.ILOIPAddress, + ILOIPGateway: s.ILOIPGateway, + ILOIPNetmask: s.ILOIPNetmask, + ILOPassword: s.ILOPassword, + ILOUsername: s.ILOUsername, + LastUpdated: s.LastUpdated, + MgmtIPAddress: s.MgmtIPAddress, + MgmtIPGateway: s.MgmtIPGateway, + MgmtIPNetmask: s.MgmtIPNetmask, + OfflineReason: s.OfflineReason, + Profile: &(s.ProfileNames)[0], + ProfileDesc: &desc, + ProfileID: &id, + PhysLocation: s.PhysLocation, + PhysLocationID: s.PhysLocationID, + Rack: s.Rack, + RevalPending: s.RevalPending, + Status: s.Status, + StatusID: s.StatusID, + TCPPort: s.TCPPort, + Type: s.Type, + TypeID: s.TypeID, + UpdPending: s.UpdPending, + XMPPID: s.XMPPID, + XMPPPasswd: s.XMPPPasswd, + }, nil +} + +// UpdateServerProfilesForV4 updates server_profile table via update function for APIv4 +func UpdateServerProfilesForV4(id *int, profile []string, tx *sql.Tx) error { + var profileNames []string + + //Delete existing rows from server_profile to get the priority correct for profile_name changes + _, err := tx.Exec("DELETE FROM server_profile WHERE server=$1", *id) + if err != nil { + return fmt.Errorf("updating server_profile by server id: %v" + strconv.Itoa(*id) + ", error: " + err.Error()) + } + + for i, pName := range profile { + query := `INSERT INTO server_profile (server, profile_name, priority) VALUES ($1, $2, $3)` + _, err := tx.Exec(query, *id, pName, i) + if err != nil { + return fmt.Errorf("error inserting into server_profile table, %v", err) + } + } + + err = tx.QueryRow("SELECT ARRAY_AGG(profile_name) FROM server_profile WHERE server=$1", *id).Scan(pq.Array(&profileNames)) + if err == sql.ErrNoRows { + return fmt.Errorf("selecting server_profile by profile_name: " + err.Error()) Review Comment: same as above RE: error-wrapping ########## traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go: ########## @@ -1788,3 +1788,102 @@ func GetRegionNameFromID(tx *sql.Tx, regionID int) (string, bool, error) { } return regionName, true, nil } + +// GetCommonServerPropertiesFromV4 converts CommonServerPropertiesV40 to CommonServerProperties struct +func GetCommonServerPropertiesFromV4(s tc.ServerV40, tx *sql.Tx) (tc.CommonServerProperties, error) { + var id int + var desc string + rows, err := tx.Query("SELECT id, description from profile WHERE name=$1", (s.ProfileNames)[0]) + if err != nil { + return tc.CommonServerProperties{}, fmt.Errorf("querying profile id and description by profile_name: " + err.Error()) + } + defer log.Close(rows, "closing rows in GetCommonServerPropertiesFromV4") + + for rows.Next() { + if err := rows.Scan(&id, &desc); err != nil { + return tc.CommonServerProperties{}, fmt.Errorf("scanning profile: " + err.Error()) + } + } + + return tc.CommonServerProperties{ + Cachegroup: s.Cachegroup, + CachegroupID: s.CachegroupID, + CDNID: s.CDNID, + CDNName: s.CDNName, + DeliveryServices: s.DeliveryServices, + DomainName: s.DomainName, + FQDN: s.FQDN, + FqdnTime: s.FqdnTime, + GUID: s.GUID, + HostName: s.HostName, + HTTPSPort: s.HTTPSPort, + ID: s.ID, + ILOIPAddress: s.ILOIPAddress, + ILOIPGateway: s.ILOIPGateway, + ILOIPNetmask: s.ILOIPNetmask, + ILOPassword: s.ILOPassword, + ILOUsername: s.ILOUsername, + LastUpdated: s.LastUpdated, + MgmtIPAddress: s.MgmtIPAddress, + MgmtIPGateway: s.MgmtIPGateway, + MgmtIPNetmask: s.MgmtIPNetmask, + OfflineReason: s.OfflineReason, + Profile: &(s.ProfileNames)[0], + ProfileDesc: &desc, + ProfileID: &id, + PhysLocation: s.PhysLocation, + PhysLocationID: s.PhysLocationID, + Rack: s.Rack, + RevalPending: s.RevalPending, + Status: s.Status, + StatusID: s.StatusID, + TCPPort: s.TCPPort, + Type: s.Type, + TypeID: s.TypeID, + UpdPending: s.UpdPending, + XMPPID: s.XMPPID, + XMPPPasswd: s.XMPPPasswd, + }, nil +} + +// UpdateServerProfilesForV4 updates server_profile table via update function for APIv4 +func UpdateServerProfilesForV4(id *int, profile []string, tx *sql.Tx) error { + var profileNames []string + + //Delete existing rows from server_profile to get the priority correct for profile_name changes + _, err := tx.Exec("DELETE FROM server_profile WHERE server=$1", *id) + if err != nil { + return fmt.Errorf("updating server_profile by server id: %v" + strconv.Itoa(*id) + ", error: " + err.Error()) + } + + for i, pName := range profile { + query := `INSERT INTO server_profile (server, profile_name, priority) VALUES ($1, $2, $3)` + _, err := tx.Exec(query, *id, pName, i) + if err != nil { + return fmt.Errorf("error inserting into server_profile table, %v", err) Review Comment: same as above RE: error-wrapping ########## traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go: ########## @@ -1788,3 +1788,102 @@ func GetRegionNameFromID(tx *sql.Tx, regionID int) (string, bool, error) { } return regionName, true, nil } + +// GetCommonServerPropertiesFromV4 converts CommonServerPropertiesV40 to CommonServerProperties struct +func GetCommonServerPropertiesFromV4(s tc.ServerV40, tx *sql.Tx) (tc.CommonServerProperties, error) { + var id int + var desc string + rows, err := tx.Query("SELECT id, description from profile WHERE name=$1", (s.ProfileNames)[0]) + if err != nil { + return tc.CommonServerProperties{}, fmt.Errorf("querying profile id and description by profile_name: " + err.Error()) + } + defer log.Close(rows, "closing rows in GetCommonServerPropertiesFromV4") + + for rows.Next() { + if err := rows.Scan(&id, &desc); err != nil { + return tc.CommonServerProperties{}, fmt.Errorf("scanning profile: " + err.Error()) + } + } + + return tc.CommonServerProperties{ + Cachegroup: s.Cachegroup, + CachegroupID: s.CachegroupID, + CDNID: s.CDNID, + CDNName: s.CDNName, + DeliveryServices: s.DeliveryServices, + DomainName: s.DomainName, + FQDN: s.FQDN, + FqdnTime: s.FqdnTime, + GUID: s.GUID, + HostName: s.HostName, + HTTPSPort: s.HTTPSPort, + ID: s.ID, + ILOIPAddress: s.ILOIPAddress, + ILOIPGateway: s.ILOIPGateway, + ILOIPNetmask: s.ILOIPNetmask, + ILOPassword: s.ILOPassword, + ILOUsername: s.ILOUsername, + LastUpdated: s.LastUpdated, + MgmtIPAddress: s.MgmtIPAddress, + MgmtIPGateway: s.MgmtIPGateway, + MgmtIPNetmask: s.MgmtIPNetmask, + OfflineReason: s.OfflineReason, + Profile: &(s.ProfileNames)[0], + ProfileDesc: &desc, + ProfileID: &id, + PhysLocation: s.PhysLocation, + PhysLocationID: s.PhysLocationID, + Rack: s.Rack, + RevalPending: s.RevalPending, + Status: s.Status, + StatusID: s.StatusID, + TCPPort: s.TCPPort, + Type: s.Type, + TypeID: s.TypeID, + UpdPending: s.UpdPending, + XMPPID: s.XMPPID, + XMPPPasswd: s.XMPPPasswd, + }, nil +} + +// UpdateServerProfilesForV4 updates server_profile table via update function for APIv4 +func UpdateServerProfilesForV4(id *int, profile []string, tx *sql.Tx) error { + var profileNames []string + + //Delete existing rows from server_profile to get the priority correct for profile_name changes + _, err := tx.Exec("DELETE FROM server_profile WHERE server=$1", *id) + if err != nil { + return fmt.Errorf("updating server_profile by server id: %v" + strconv.Itoa(*id) + ", error: " + err.Error()) Review Comment: Same as above RE: error-wrapping also, you're using `fmt.Errorf` so you don't need to be using `strconv.Itoa`, you can just use the `%d` formatting parameter. Also also, that `%v` in there isn't being consumed from what I can tell, so I'm puzzled how that got past `go vet`. ########## traffic_ops/traffic_ops_golang/server/servers.go: ########## @@ -468,9 +443,7 @@ RETURNING offline_reason, (SELECT name FROM phys_location WHERE phys_location.id=server.phys_location) AS phys_location, phys_location AS phys_location_id, - profile AS profile_id, - (SELECT description FROM profile WHERE profile.id=server.profile) AS profile_desc, - (SELECT name FROM profile WHERE profile.id=server.profile) AS profile, + (SELECT ARRAY_AGG(profile_name) FROM server_profile WHERE server_profile.server=server.id) AS profile_name, Review Comment: Do we need to bother returning this? The server's Profiles will be inserted afterwards, and that'll just overwrite whatever this returns. ########## traffic_ops/traffic_ops_golang/server/servers.go: ########## @@ -688,15 +708,14 @@ JOIN profile p on p.Id = s.Profile JOIN interface i on i.server = s.ID JOIN ip_address ip on ip.Server = s.ID and ip.interface = i.name WHERE ip.service_address = true -and p.id = $1 ` var rows *sql.Rows var err error //ProfileID already validated if s.ID != nil { - rows, err = tx.Query(query+" and s.id != $2", *s.ProfileID, *s.ID) + rows, err = tx.Query(query+" and s.id != $1", *s.ID) } else { - rows, err = tx.Query(query, *s.ProfileID) + rows, err = tx.Query(query) } Review Comment: This section is validating the constraint that no two servers share a Profile and a "service address". These changes (I _think_) are going to make it instead validate that no two servers share a "service address", which we explicitly want to allow. That's not correct, but I'm not quite certain what is. When we discussed it on the blueprint (a discussion that was never resolved, I think), @rob05c said > I think we really need some kind of constraint to prevent duplicate servers. Namely the real unique network identifier: the IP-port tuple and/or fqdn-port tuple. > >Alternatively, if we don't want to incur the dev cost of that right now, we could probably make a database or API constraint (ideally both) for profiles-in-same-order+IP. It's probably a pretty ugly db constraint, but I think it's possible. > >I don't have a strong opinion either way. I think if you want to go the "port" route, you'd want to check both the `tcpPort` and the `httpsPort` (wherever they're not respectively `NULL`) because which one is in use could depend on unknowable configuration, or it could simply use both. That seems like the easiest solution to me, but you'd probably want that in the database migration so it can be enforced on all levels. If you want to go the "Profiles-in-the-same-order" route instead, then I don't believe based on our implementation that's possible as an actual database **constraint**. It *could* be a trigger, but I don't think that's a good idea, should probably just be validated here in the Go API handler. ########## traffic_ops/traffic_ops_golang/server/servers.go: ########## @@ -1466,7 +1564,15 @@ func Update(w http.ResponseWriter, r *http.Request) { api.HandleErr(w, r, tx, http.StatusBadRequest, err, nil) return } - server, err = serverV3.UpgradeToV40() + profileName, err := dbhelpers.UpdateServerProfileTableForV2V3(serverV3.ID, serverV3.Profile, (original.ProfileNames)[0], tx) + if err != nil { + api.HandleErr(w, r, tx, http.StatusBadRequest, nil, fmt.Errorf("failed to update server_profile: %v", err)) Review Comment: same as above RE: response code and client vs server error ########## traffic_ops/traffic_ops_golang/server/servers.go: ########## @@ -882,7 +906,12 @@ func Read(w http.ResponseWriter, r *http.Request) { legacyServers := make([]tc.ServerNullableV2, 0, len(servers)) for _, server := range servers { - legacyServer, err := server.ToServerV2FromV4() + csp, err := dbhelpers.GetCommonServerPropertiesFromV4(server, tx) + if err != nil { + api.HandleErr(w, r, tx, http.StatusBadRequest, nil, fmt.Errorf("failed to query profile: %v", err)) Review Comment: You're returning a client error but not giving the client any information about what they did wrong. The error you're sending back is a server-side error (client-side error is `nil`). It seems like the response code here should be `http.StatusInternalServerError`. ########## traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go: ########## @@ -1788,3 +1788,102 @@ func GetRegionNameFromID(tx *sql.Tx, regionID int) (string, bool, error) { } return regionName, true, nil } + +// GetCommonServerPropertiesFromV4 converts CommonServerPropertiesV40 to CommonServerProperties struct +func GetCommonServerPropertiesFromV4(s tc.ServerV40, tx *sql.Tx) (tc.CommonServerProperties, error) { + var id int + var desc string + rows, err := tx.Query("SELECT id, description from profile WHERE name=$1", (s.ProfileNames)[0]) + if err != nil { + return tc.CommonServerProperties{}, fmt.Errorf("querying profile id and description by profile_name: " + err.Error()) + } + defer log.Close(rows, "closing rows in GetCommonServerPropertiesFromV4") + + for rows.Next() { + if err := rows.Scan(&id, &desc); err != nil { + return tc.CommonServerProperties{}, fmt.Errorf("scanning profile: " + err.Error()) + } + } + + return tc.CommonServerProperties{ + Cachegroup: s.Cachegroup, + CachegroupID: s.CachegroupID, + CDNID: s.CDNID, + CDNName: s.CDNName, + DeliveryServices: s.DeliveryServices, + DomainName: s.DomainName, + FQDN: s.FQDN, + FqdnTime: s.FqdnTime, + GUID: s.GUID, + HostName: s.HostName, + HTTPSPort: s.HTTPSPort, + ID: s.ID, + ILOIPAddress: s.ILOIPAddress, + ILOIPGateway: s.ILOIPGateway, + ILOIPNetmask: s.ILOIPNetmask, + ILOPassword: s.ILOPassword, + ILOUsername: s.ILOUsername, + LastUpdated: s.LastUpdated, + MgmtIPAddress: s.MgmtIPAddress, + MgmtIPGateway: s.MgmtIPGateway, + MgmtIPNetmask: s.MgmtIPNetmask, + OfflineReason: s.OfflineReason, + Profile: &(s.ProfileNames)[0], + ProfileDesc: &desc, + ProfileID: &id, + PhysLocation: s.PhysLocation, + PhysLocationID: s.PhysLocationID, + Rack: s.Rack, + RevalPending: s.RevalPending, + Status: s.Status, + StatusID: s.StatusID, + TCPPort: s.TCPPort, + Type: s.Type, + TypeID: s.TypeID, + UpdPending: s.UpdPending, + XMPPID: s.XMPPID, + XMPPPasswd: s.XMPPPasswd, + }, nil +} + +// UpdateServerProfilesForV4 updates server_profile table via update function for APIv4 +func UpdateServerProfilesForV4(id *int, profile []string, tx *sql.Tx) error { + var profileNames []string + + //Delete existing rows from server_profile to get the priority correct for profile_name changes + _, err := tx.Exec("DELETE FROM server_profile WHERE server=$1", *id) + if err != nil { + return fmt.Errorf("updating server_profile by server id: %v" + strconv.Itoa(*id) + ", error: " + err.Error()) + } + + for i, pName := range profile { + query := `INSERT INTO server_profile (server, profile_name, priority) VALUES ($1, $2, $3)` + _, err := tx.Exec(query, *id, pName, i) + if err != nil { + return fmt.Errorf("error inserting into server_profile table, %v", err) + } + } + + err = tx.QueryRow("SELECT ARRAY_AGG(profile_name) FROM server_profile WHERE server=$1", *id).Scan(pq.Array(&profileNames)) + if err == sql.ErrNoRows { + return fmt.Errorf("selecting server_profile by profile_name: " + err.Error()) + } + return nil Review Comment: returning a non-nil error only when the error is `sql.ErrNoRows` (which should be checked using `errors.Is` wherever that's being done) ignores all other possible errors e.g. dropped database connection. We should return the (wrapped) error whenever it's non-`nil`. ########## traffic_ops/traffic_ops_golang/server/servers.go: ########## @@ -869,7 +888,12 @@ func Read(w http.ResponseWriter, r *http.Request) { if version.Major >= 3 { v3Servers := make([]tc.ServerV30, 0) for _, server := range servers { - v3Server, err := server.ToServerV3FromV4() + csp, err := dbhelpers.GetCommonServerPropertiesFromV4(server, tx) + if err != nil { + api.HandleErr(w, r, tx, http.StatusBadRequest, nil, fmt.Errorf("failed to query profile: %v", err)) Review Comment: errors should wrap ########## traffic_ops/traffic_ops_golang/server/servers.go: ########## @@ -1483,8 +1589,15 @@ func Update(w http.ResponseWriter, r *http.Request) { api.HandleErr(w, r, tx, http.StatusBadRequest, err, nil) return } - - server, err = legacyServer.UpgradeToV40() + profileName, err := dbhelpers.UpdateServerProfileTableForV2V3(legacyServer.ID, legacyServer.Profile, (original.ProfileNames)[0], tx) + if err != nil { + api.HandleErr(w, r, tx, http.StatusBadRequest, nil, fmt.Errorf("failed to update server_profile: %v", err)) Review Comment: same as above RE: response code and client vs server error ########## traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go: ########## @@ -1788,3 +1788,102 @@ func GetRegionNameFromID(tx *sql.Tx, regionID int) (string, bool, error) { } return regionName, true, nil } + +// GetCommonServerPropertiesFromV4 converts CommonServerPropertiesV40 to CommonServerProperties struct +func GetCommonServerPropertiesFromV4(s tc.ServerV40, tx *sql.Tx) (tc.CommonServerProperties, error) { + var id int + var desc string + rows, err := tx.Query("SELECT id, description from profile WHERE name=$1", (s.ProfileNames)[0]) + if err != nil { + return tc.CommonServerProperties{}, fmt.Errorf("querying profile id and description by profile_name: " + err.Error()) + } + defer log.Close(rows, "closing rows in GetCommonServerPropertiesFromV4") + + for rows.Next() { + if err := rows.Scan(&id, &desc); err != nil { + return tc.CommonServerProperties{}, fmt.Errorf("scanning profile: " + err.Error()) + } + } + + return tc.CommonServerProperties{ + Cachegroup: s.Cachegroup, + CachegroupID: s.CachegroupID, + CDNID: s.CDNID, + CDNName: s.CDNName, + DeliveryServices: s.DeliveryServices, + DomainName: s.DomainName, + FQDN: s.FQDN, + FqdnTime: s.FqdnTime, + GUID: s.GUID, + HostName: s.HostName, + HTTPSPort: s.HTTPSPort, + ID: s.ID, + ILOIPAddress: s.ILOIPAddress, + ILOIPGateway: s.ILOIPGateway, + ILOIPNetmask: s.ILOIPNetmask, + ILOPassword: s.ILOPassword, + ILOUsername: s.ILOUsername, + LastUpdated: s.LastUpdated, + MgmtIPAddress: s.MgmtIPAddress, + MgmtIPGateway: s.MgmtIPGateway, + MgmtIPNetmask: s.MgmtIPNetmask, + OfflineReason: s.OfflineReason, + Profile: &(s.ProfileNames)[0], + ProfileDesc: &desc, + ProfileID: &id, + PhysLocation: s.PhysLocation, + PhysLocationID: s.PhysLocationID, + Rack: s.Rack, + RevalPending: s.RevalPending, + Status: s.Status, + StatusID: s.StatusID, + TCPPort: s.TCPPort, + Type: s.Type, + TypeID: s.TypeID, + UpdPending: s.UpdPending, + XMPPID: s.XMPPID, + XMPPPasswd: s.XMPPPasswd, + }, nil +} + +// UpdateServerProfilesForV4 updates server_profile table via update function for APIv4 +func UpdateServerProfilesForV4(id *int, profile []string, tx *sql.Tx) error { + var profileNames []string + + //Delete existing rows from server_profile to get the priority correct for profile_name changes + _, err := tx.Exec("DELETE FROM server_profile WHERE server=$1", *id) + if err != nil { + return fmt.Errorf("updating server_profile by server id: %v" + strconv.Itoa(*id) + ", error: " + err.Error()) + } + + for i, pName := range profile { + query := `INSERT INTO server_profile (server, profile_name, priority) VALUES ($1, $2, $3)` + _, err := tx.Exec(query, *id, pName, i) + if err != nil { + return fmt.Errorf("error inserting into server_profile table, %v", err) + } + } + + err = tx.QueryRow("SELECT ARRAY_AGG(profile_name) FROM server_profile WHERE server=$1", *id).Scan(pq.Array(&profileNames)) Review Comment: This query needs an `ORDER BY` clause for the priority, or the resulting array may be in the wrong order. Or the ordering can be done by also selecting the priority and then ordering in Go, but that seems like way more work to me. ########## traffic_ops/traffic_ops_golang/server/servers.go: ########## @@ -1634,6 +1787,36 @@ func Update(w http.ResponseWriter, r *http.Request) { api.CreateChangeLogRawTx(api.ApiChange, changeLogMsg, inf.User, tx) } +func insertServerProfile(id *int, pName []string, tx *sql.Tx) (error, error, int) { + insertSPQuery := ` + INSERT INTO server_profile ( + server, + profile_name, + priority + ) VALUES + ` + + //var priorityArray pq.Int64Array + //priorityArray = append(priorityArray, int64(i)) + + iSPQueryParts := make([]string, 0, len(pName)) + iSPQueryValues := make([]interface{}, 0, len(pName)) + for i, name := range pName { + valStart := i * 3 + iSPQueryParts = append(iSPQueryParts, fmt.Sprintf("($%d, $%d, $%d)", valStart+1, valStart+2, valStart+3)) + iSPQueryValues = append(iSPQueryValues, *id, name, i) + } + + insertSPQuery += strings.Join(iSPQueryParts, ",") Review Comment: Instead of doing this manipulation of the query to build a multiple insert, we can make use of `pq.Array` and `UNNEST` to build a set of records from an array of input like so: ```sql INSERT INTO server_profile ( server, profile_name, priority ) SELECT $1, profile, priority FROM UNNEST($2::text[]) WITH ORDINALITY AS tmp(profile, priority) ``` Which you can then use like ```go if _, err := tx.Exec(insertSPQuery, pq.Array(pName)); err != nil { return api.ParseDBError(err) } return nil, nil, http.StatusOK ``` Building queries manually with string concatenation is extremely dangerous and we should avoid it if at all possible ########## traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go: ########## @@ -1788,3 +1788,102 @@ func GetRegionNameFromID(tx *sql.Tx, regionID int) (string, bool, error) { } return regionName, true, nil } + +// GetCommonServerPropertiesFromV4 converts CommonServerPropertiesV40 to CommonServerProperties struct +func GetCommonServerPropertiesFromV4(s tc.ServerV40, tx *sql.Tx) (tc.CommonServerProperties, error) { + var id int + var desc string + rows, err := tx.Query("SELECT id, description from profile WHERE name=$1", (s.ProfileNames)[0]) + if err != nil { + return tc.CommonServerProperties{}, fmt.Errorf("querying profile id and description by profile_name: " + err.Error()) + } + defer log.Close(rows, "closing rows in GetCommonServerPropertiesFromV4") + + for rows.Next() { + if err := rows.Scan(&id, &desc); err != nil { + return tc.CommonServerProperties{}, fmt.Errorf("scanning profile: " + err.Error()) + } + } + + return tc.CommonServerProperties{ + Cachegroup: s.Cachegroup, + CachegroupID: s.CachegroupID, + CDNID: s.CDNID, + CDNName: s.CDNName, + DeliveryServices: s.DeliveryServices, + DomainName: s.DomainName, + FQDN: s.FQDN, + FqdnTime: s.FqdnTime, + GUID: s.GUID, + HostName: s.HostName, + HTTPSPort: s.HTTPSPort, + ID: s.ID, + ILOIPAddress: s.ILOIPAddress, + ILOIPGateway: s.ILOIPGateway, + ILOIPNetmask: s.ILOIPNetmask, + ILOPassword: s.ILOPassword, + ILOUsername: s.ILOUsername, + LastUpdated: s.LastUpdated, + MgmtIPAddress: s.MgmtIPAddress, + MgmtIPGateway: s.MgmtIPGateway, + MgmtIPNetmask: s.MgmtIPNetmask, + OfflineReason: s.OfflineReason, + Profile: &(s.ProfileNames)[0], + ProfileDesc: &desc, + ProfileID: &id, + PhysLocation: s.PhysLocation, + PhysLocationID: s.PhysLocationID, + Rack: s.Rack, + RevalPending: s.RevalPending, + Status: s.Status, + StatusID: s.StatusID, + TCPPort: s.TCPPort, + Type: s.Type, + TypeID: s.TypeID, + UpdPending: s.UpdPending, + XMPPID: s.XMPPID, + XMPPPasswd: s.XMPPPasswd, + }, nil +} + +// UpdateServerProfilesForV4 updates server_profile table via update function for APIv4 +func UpdateServerProfilesForV4(id *int, profile []string, tx *sql.Tx) error { + var profileNames []string + + //Delete existing rows from server_profile to get the priority correct for profile_name changes + _, err := tx.Exec("DELETE FROM server_profile WHERE server=$1", *id) + if err != nil { + return fmt.Errorf("updating server_profile by server id: %v" + strconv.Itoa(*id) + ", error: " + err.Error()) + } + + for i, pName := range profile { + query := `INSERT INTO server_profile (server, profile_name, priority) VALUES ($1, $2, $3)` + _, err := tx.Exec(query, *id, pName, i) + if err != nil { + return fmt.Errorf("error inserting into server_profile table, %v", err) + } + } + + err = tx.QueryRow("SELECT ARRAY_AGG(profile_name) FROM server_profile WHERE server=$1", *id).Scan(pq.Array(&profileNames)) + if err == sql.ErrNoRows { + return fmt.Errorf("selecting server_profile by profile_name: " + err.Error()) + } Review Comment: To insert <var>n</var> Profiles, this function will make <code><var>n</var>+2</code> (<code><var>n</var>+1</code> in the highlighted block, which doesn't include the `DELETE` above). You can instead make it exactly 2 queries by just doing a single insert, and reading back the inserted values rather than making a separate query to retrieve them. Arguably you don't need to read them back at all because, assuming the insertion and deletion queries were both successful, the input array is exactly the same as the output. But it is safer to read them back, to be sure the output is _exactly_ what was inserted, so as long as performance isn't a big deal it's not a big deal to do it. The query for that would look something like ```sql WITH inserted AS ( INSERT INTO public.server_profile (server, profile_name, priority) SELECT $1, profile, priority FROM UNNEST($2::text[], $3::int[]) AS tmp("profile", "priority") RETURNING profile, priority ) SELECT profile FROM inserted ORDER BY priority ASC ``` The `WITH` is necessary to ensure the returned rows are in order, otherwise you could just also return the priority to Go and sort there - but that seems like more work to me than just using the `WITH`. This way you'd have to do a scanning loop instead of being able to cleanly scan into a `pq.Array`-wrapped slice, but if you wanted to make the query a bit more complex to make the Go simpler you could scan from a `QueryRow` into a `pq.Array` using ```sql WITH inserted AS ( INSERT INTO public.server_profile(server, profile_name, priority) SELECT $1, profile, priority FROM UNNEST($2::text[], $3::int[]) AS tmp("profile", "priority") RETURNING profile, priority ) SELECT ARRAY_AGG(profile) FROM ( SELECT profile FROM inserted ORDER BY priority ASC ) AS returned(profile) ``` Either way is fine as far as I'm concerned. But I do think reducing the number of queries to a constant is a good improvement to make. I have some sample programs I wrote that connect to the development environment's TODB server and demonstrate each approach using a table they make for themselves. They're lengthy, so I've included them in collapsible boxes below. <details><summary>Scanning rows sequentially in a loop</summary> ```go package main import ( "database/sql" "fmt" "os" "strings" "github.com/lib/pq" ) const setupquery = ` CREATE TABLE IF NOT EXISTS testquest( server bigint, profile text, priority int, CONSTRAINT uniq_server_prof UNIQUE(server, profile), CONSTRAINT uniq_server_prio UNIQUE(server, priority) )` const query = ` WITH inserted AS ( INSERT INTO testquest SELECT $1, "profile", "priority" FROM UNNEST($2::text[], $3::int[]) AS tmp("profile", "priority") RETURNING profile, priority ) SELECT profile FROM inserted ` const teardownquery = ` DROP TABLE IF EXISTS testquest ` func main() { db, err := sql.Open("postgres", "postgresql://postgres:twelve12@localhost:5432/traffic_ops_development?sslmode=disable") if err != nil { fmt.Fprintf(os.Stderr, "failed to connect to database: %v\n", err) os.Exit(1) } _, err = db.Exec(setupquery) if err != nil { fmt.Fprintf(os.Stderr, "failed to setup schema: %v\n", err) os.Exit(1) } exitcode := 0 defer func() { if _, err := db.Exec(teardownquery); err != nil { fmt.Fprintf(os.Stderr, "failed to tear down schema: %v\n", err) exitcode++ } os.Exit(exitcode) }() data := []string{"a", "bunch", "of", "Profile", "names"} priorities := []int{0, 1, 2, 3, 4} rows, err := db.Query(query, 1, pq.Array(data), pq.Array(priorities)) if err != nil { fmt.Fprintf(os.Stderr, "Failed to run insert query: %v\n", err) exitcode++ return } if err = rows.Err(); err != nil { fmt.Fprintf(os.Stderr, "Rows have error: %v\n", err) exitcode++ return } defer func() { if err := rows.Close(); err != nil { fmt.Fprintf(os.Stderr, "Failed to close rows: %v\n", err) exitcode++ } }() profiles := make([]string, 0, len(data)) for rows.Next() { var profile string if err = rows.Scan(&profile); err != nil { fmt.Fprintf(os.Stderr, "failed to scan row: %v\n", err) } profiles = append(profiles, profile) } fmt.Println(strings.Join(profiles, ", ")) } ``` </details> <details><summary>Scanning a single returned row into a <code>pq.Array</code></summary> ```go package main import ( "database/sql" "fmt" "os" "strings" "github.com/lib/pq" ) const setupquery = ` CREATE TABLE IF NOT EXISTS testquest( server bigint, profile text, priority int, CONSTRAINT uniq_server_prof UNIQUE(server, profile), CONSTRAINT uniq_server_prio UNIQUE(server, priority) )` const query = ` WITH inserted AS ( INSERT INTO testquest SELECT $1, "profile", "priority" FROM UNNEST($2::text[], $3::int[]) AS tmp("profile", "priority") RETURNING profile, priority ) SELECT ARRAY_AGG(profile) FROM ( SELECT profile FROM inserted ORDER BY priority ASC ) AS returned(profile) ` const teardownquery = ` DROP TABLE IF EXISTS testquest ` func main() { db, err := sql.Open("postgres", "postgresql://postgres:twelve12@localhost:5432/traffic_ops_development?sslmode=disable") if err != nil { fmt.Fprintf(os.Stderr, "failed to connect to database: %v\n", err) os.Exit(1) } _, err = db.Exec(setupquery) if err != nil { fmt.Fprintf(os.Stderr, "failed to setup schema: %v\n", err) os.Exit(1) } exitcode := 0 defer func() { if _, err := db.Exec(teardownquery); err != nil { fmt.Fprintf(os.Stderr, "failed to tear down schema: %v\n", err) exitcode++ } os.Exit(exitcode) }() data := []string{"a", "bunch", "of", "Profile", "names"} priorities := []int{0, 1, 2, 3, 4} profiles := make([]string, 0, len(data)) err = db.QueryRow(query, 1, pq.Array(data), pq.Array(priorities)).Scan(pq.Array(&profiles)) if err != nil { fmt.Fprintf(os.Stderr, "Failed to run insert query/scan results: %v\n", err) exitcode++ return } fmt.Println(strings.Join(profiles, ", ")) } ``` This one is actually quite a bit shorter. Maybe this really is the way to go? Query is more complex, and the loop is more generic/familiar than `pq.Array`, though. </details> Both programs print the profiles they insert on success (which is a static array you can find in the source code, but for reference it's `["a", "bunch", "of", "Profile", "names"]`, in that order). ########## traffic_portal/app/src/common/modules/table/servers/TableServersController.js: ########## @@ -162,7 +162,7 @@ var TableServersController = function(tableName, servers, filter, $scope, $state }, { headerName: "Profile", - field: "profile", + field: "profileName", Review Comment: This is a fine way to do it, but I'm just curious: did you try to use a `valueGetter`? ########## traffic_ops/traffic_ops_golang/server/servers.go: ########## @@ -2063,8 +2294,12 @@ func Delete(w http.ResponseWriter, r *http.Request) { if inf.Version.Major >= 3 { api.WriteRespAlertObj(w, r, tc.SuccessLevel, "Server deleted", server) } else { - - serverV2, err := server.ToServerV2FromV4() + csp, err := dbhelpers.GetCommonServerPropertiesFromV4(server, tx) + if err != nil { + api.HandleErr(w, r, tx, http.StatusBadRequest, nil, fmt.Errorf("failed to query profile: %v", err)) Review Comment: Same as above RE: status code and client vs server errors also, errors should wrap using `%w` -- 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]
