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
