zrhoffman commented on code in PR #7718: URL: https://github.com/apache/trafficcontrol/pull/7718#discussion_r1304603210
########## traffic_ops/traffic_ops_golang/server/servers.go: ########## @@ -1421,230 +1365,200 @@ func Update(w http.ResponseWriter, r *http.Request) { } _, userErr, sysErr := validateV3(&serverV3, tx) if userErr != nil || sysErr != nil { - code := http.StatusBadRequest if sysErr != nil { - code = http.StatusInternalServerError + return http.StatusInternalServerError, userErr, sysErr } - api.HandleErr(w, r, tx, code, userErr, sysErr) - return + return http.StatusBadRequest, userErr, sysErr } profileName, exists, err := dbhelpers.GetProfileNameFromID(*serverV3.ProfileID, tx) if err != nil { - api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, err) - return + return http.StatusInternalServerError, nil, err } if !exists { - api.HandleErr(w, r, tx, http.StatusNotFound, errors.New("profile does not exist"), nil) - return + return http.StatusNotFound, errors.New("profile does not exist"), nil } profileNames := []string{profileName} - server, err = serverV3.UpgradeToV40(profileNames) + upgraded, err := serverV3.UpgradeToV40(profileNames) if err != nil { - sysErr = fmt.Errorf("error upgrading valid V3 server to V4 structure: %v", err) - api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, sysErr) - return + return http.StatusInternalServerError, nil, fmt.Errorf("error upgrading valid V3 server to V4 structure: %w", err) } + server = upgraded.Upgrade() } - if *original.CachegroupID != *server.CachegroupID || *original.CDNID != *server.CDNID { - hasDSOnCDN, err := dbhelpers.CachegroupHasTopologyBasedDeliveryServicesOnCDN(tx, *original.CachegroupID, *original.CDNID) + if original.CacheGroupID != server.CacheGroupID || original.CDNID != server.CDNID { + hasDSOnCDN, err := dbhelpers.CachegroupHasTopologyBasedDeliveryServicesOnCDN(tx, original.CacheGroupID, original.CDNID) if err != nil { - api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, err) - return + return http.StatusInternalServerError, nil, err } CDNIDs := []int{} if hasDSOnCDN { - CDNIDs = append(CDNIDs, *original.CDNID) + CDNIDs = append(CDNIDs, original.CDNID) } - cacheGroupIds := []int{*original.CachegroupID} - serverIds := []int{*original.ID} - if err = topology_validation.CheckForEmptyCacheGroups(inf.Tx, cacheGroupIds, CDNIDs, true, serverIds); err != nil { - api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("server is the last one in its cachegroup, which is used by a topology, so it cannot be moved to another cachegroup: "+err.Error()), nil) - return + if err = topology_validation.CheckForEmptyCacheGroups(inf.Tx, []int{original.CacheGroupID}, CDNIDs, true, []int{original.ID}); err != nil { + return http.StatusBadRequest, fmt.Errorf("server is the last one in its Cache Group, which is used by a Topology, so it cannot be moved to another Cache Group: %w", err), nil } } - status, ok, err := dbhelpers.GetStatusByID(*server.StatusID, tx) + status, ok, err := dbhelpers.GetStatusByID(server.StatusID, tx) if err != nil { - sysErr = fmt.Errorf("getting server #%d status (#%d): %v", id, *server.StatusID, err) - api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, sysErr) - return + return http.StatusInternalServerError, nil, fmt.Errorf("getting server #%d status (#%d): %v", id, server.StatusID, err) } if !ok { - log.Warnf("previously existent status #%d not found when fetching later", *server.StatusID) - userErr = fmt.Errorf("no such Status: #%d", *server.StatusID) - api.HandleErr(w, r, tx, http.StatusBadRequest, userErr, nil) - return + log.Warnf("previously existent status #%d not found when fetching later", server.StatusID) + return http.StatusBadRequest, fmt.Errorf("no such Status: #%d", server.StatusID), nil } if status.Name == nil { - sysErr = fmt.Errorf("status #%d had no name", *server.StatusID) - api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, sysErr) - return + return http.StatusInternalServerError, nil, fmt.Errorf("status #%d had no name", server.StatusID) } if *status.Name != string(tc.CacheStatusOnline) && *status.Name != string(tc.CacheStatusReported) { dsIDs, err := getActiveDeliveryServicesThatOnlyHaveThisServerAssigned(id, original.Type, tx) if err != nil { - sysErr = fmt.Errorf("getting Delivery Services to which server #%d is assigned that have no other servers: %v", id, err) - api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, sysErr) - return + return http.StatusInternalServerError, + nil, + fmt.Errorf("getting Delivery Services to which server #%d is assigned that have no other servers: %w", id, err) } if len(dsIDs) > 0 { prefix := fmt.Sprintf("setting server status to '%s' would leave Active Delivery Service", *status.Name) alertText := InvalidStatusForDeliveryServicesAlertText(prefix, original.Type, dsIDs) - api.WriteAlerts(w, r, http.StatusConflict, tc.CreateAlerts(tc.ErrorLevel, alertText)) - return + return http.StatusConflict, errors.New(alertText), nil } } if userErr, sysErr, errCode = checkTypeChangeSafety(server, inf.Tx); userErr != nil || sysErr != nil { - api.HandleErr(w, r, tx, errCode, userErr, sysErr) - return + return errCode, userErr, sysErr } if server.XMPPID != nil && *server.XMPPID != "" && originalXMPPID != "" && *server.XMPPID != originalXMPPID { - api.WriteAlerts(w, r, http.StatusBadRequest, tc.CreateAlerts(tc.ErrorLevel, fmt.Sprintf("server cannot be updated due to requested XMPPID change. XMPIDD is immutable"))) - return + return http.StatusBadRequest, errors.New("server cannot be updated due to requested XMPPID change. XMPIDD is immutable"), nil } - userErr, sysErr, statusCode := api.CheckIfUnModified(r.Header, inf.Tx, *server.ID, "server") + userErr, sysErr, statusCode := api.CheckIfUnModified(inf.RequestHeaders(), inf.Tx, server.ID, "server") if userErr != nil || sysErr != nil { - api.HandleErr(w, r, tx, statusCode, userErr, sysErr) - return + return statusCode, userErr, sysErr } - if server.CDNName != nil { - userErr, sysErr, statusCode = dbhelpers.CheckIfCurrentUserCanModifyCDN(inf.Tx.Tx, *server.CDNName, inf.User.UserName) + if server.CDN != "" { + userErr, sysErr, statusCode = dbhelpers.CheckIfCurrentUserCanModifyCDN(inf.Tx.Tx, server.CDN, inf.User.UserName) if userErr != nil || sysErr != nil { - api.HandleErr(w, r, inf.Tx.Tx, statusCode, userErr, sysErr) - return + return statusCode, userErr, sysErr } - } else if server.CDNID != nil { - userErr, sysErr, statusCode = dbhelpers.CheckIfCurrentUserCanModifyCDNWithID(inf.Tx.Tx, int64(*server.CDNID), inf.User.UserName) + } else { + userErr, sysErr, statusCode = dbhelpers.CheckIfCurrentUserCanModifyCDNWithID(inf.Tx.Tx, int64(server.CDNID), inf.User.UserName) if userErr != nil || sysErr != nil { - api.HandleErr(w, r, inf.Tx.Tx, statusCode, userErr, sysErr) - return + return statusCode, userErr, sysErr } } if inf.Version.Major >= 4 { - if err = dbhelpers.UpdateServerProfilesForV4(*server.ID, server.ProfileNames, tx); err != nil { + if err = dbhelpers.UpdateServerProfilesForV4(server.ID, server.Profiles, tx); err != nil { userErr, sysErr, errCode := api.ParseDBError(err) - api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr) - return + return errCode, userErr, sysErr } } else { - if err = dbhelpers.UpdateServerProfileTableForV3(serverV3.ID, serverV3.ProfileID, (original.ProfileNames)[0], tx); err != nil { - api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, fmt.Errorf("failed to update server_profile: %w", err)) - return + if err = dbhelpers.UpdateServerProfileTableForV3(serverV3.ID, serverV3.ProfileID, (original.Profiles)[0], tx); err != nil { + return http.StatusInternalServerError, nil, fmt.Errorf("failed to update server_profile: %w", err) } } serverID, errCode, userErr, sysErr := updateServer(inf.Tx, server) if userErr != nil || sysErr != nil { - api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr) - return + return errCode, userErr, sysErr } if userErr, sysErr, errCode = deleteInterfaces(id, tx); userErr != nil || sysErr != nil { - api.HandleErr(w, r, tx, errCode, userErr, sysErr) - return + return errCode, userErr, sysErr } if userErr, sysErr, errCode = createInterfaces(id, server.Interfaces, tx); userErr != nil || sysErr != nil { - api.HandleErr(w, r, tx, errCode, userErr, sysErr) - return + return errCode, userErr, sysErr } where := `WHERE s.id = $1` var selquery string - if version.Major <= 4 { + if inf.Version.Major <= 4 { Review Comment: > It's not a constraint, just a suggestion. You could just as easily use `v.GreaterThan(other) || v.Equal(other)`, or even `v.GreaterThan(other) || v == other`. The idea that the `<=` and `>=` operators are unnecessary is very silly. Some things are just more readable using a specific operator, and if that operator doesn't exist, the code is automatically less readable as a result. > That will be confusing only if you force yourself to attempt to read it as "v is greater than or equal to other" rather than "v is not less than other". Any code that forces everyone to think about it in the same way as the author is not good code and should be refactored -- 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: issues-unsubscr...@trafficcontrol.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org