ocket8888 commented on code in PR #7718:
URL: https://github.com/apache/trafficcontrol/pull/7718#discussion_r1306052909
##########
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:
You're equating `>=` and `<=` to `.GreaterThanOrEqualTo` and
`LessThanOrEqualTo`, respectively. But those are literally different. These
aren't operators, and they're just as equivalent to `.GreaterThan || .EqualTo`
and `.LessThan || .EqualTo`. I don't care if the `OrEqualTo` versions exist,
but it's crazy to me to say that, to extend your metaphor, having the pair of
operators `>=` and `<` is somehow more sensible than just choosing the pair
`>=` and `<=`, or `>` and `<`. There's no law that says we can't do all four,
but if we're only going to have two I think it makes way more sense for them to
be symmetric operations.
> Any code that forces everyone to think about it in the same way as the
author is not good code and should be refactored
I couldn't agree less. Any code that leaves you with an inaccurate picture
of the way the author thought about it is bad code and should be refactored. If
I was stating these restrictions in English, "... an API version no less than
4.0..." seems more natural to me than "... an API version that is greater than
or equal to 4.0...". If you think that's an uncommon way to phrase things,
that's fair, but it's definitely because it's actually important to get the
information across.
--
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]
