ocket8888 commented on code in PR #6544:
URL: https://github.com/apache/trafficcontrol/pull/6544#discussion_r851508553
##########
traffic_ops/traffic_ops_golang/server/servers.go:
##########
@@ -409,7 +442,53 @@ func validateCommon(s *tc.CommonServerProperties, tx
*sql.Tx) []error {
if cdnID != *s.CDNID {
errs = append(errs, fmt.Errorf("CDN id '%d' for profile '%d'
does not match Server CDN '%d'", cdnID, *s.ProfileID, *s.CDNID))
}
+ return errs
+}
+
+func validateCommonV40(s *tc.CommonServerPropertiesV40, tx *sql.Tx) []error {
+
+ noSpaces := validation.NewStringRule(tovalidate.NoSpaces, "cannot
contain spaces")
+
+ errs := tovalidate.ToErrors(validation.Errors{
+ "cachegroupId": validation.Validate(s.CachegroupID,
validation.NotNil),
+ "cdnId": validation.Validate(s.CDNID,
validation.NotNil),
+ "domainName": validation.Validate(s.DomainName,
validation.Required, noSpaces),
+ "hostName": validation.Validate(s.HostName,
validation.Required, noSpaces),
+ "physLocationId": validation.Validate(s.PhysLocationID,
validation.NotNil),
+ "profileNames": validation.Validate(s.ProfileNames,
validation.NotNil),
+ "statusId": validation.Validate(s.StatusID,
validation.NotNil),
+ "typeId": validation.Validate(s.TypeID,
validation.NotNil),
+ "httpsPort": validation.Validate(s.HTTPSPort,
validation.By(tovalidate.IsValidPortNumber)),
+ "tcpPort": validation.Validate(s.TCPPort,
validation.By(tovalidate.IsValidPortNumber)),
+ })
+
+ if len(errs) > 0 {
+ return errs
+ }
+
+ if _, err := tc.ValidateTypeID(tx, s.TypeID, "server"); err != nil {
+ errs = append(errs, err)
+ }
+
+ if len(s.ProfileNames) == 0 {
+ errs = append(errs, fmt.Errorf("no profiles exists"))
+ }
+
+ var cdnID int
+ if err := tx.QueryRow("SELECT cdn from profile WHERE name=$1",
s.ProfileNames[0]).Scan(&cdnID); err != nil {
+ log.Errorf("could not execute select cdnID from profile: %s\n",
err)
+ if err == sql.ErrNoRows {
Review Comment:
error comparisons should use `errors.Is` rather than `==`
##########
traffic_ops/traffic_ops_golang/server/servers.go:
##########
@@ -1490,7 +1709,16 @@ func Update(w http.ResponseWriter, r *http.Request) {
}
api.WriteRespAlertObj(w, r, tc.SuccessLevel, "Server updated",
srvr)
} else {
- v2Server, err := srvr.ToServerV2FromV4()
+ csp, err := dbhelpers.GetCommonServerPropertiesFromV4(server,
tx)
+ if err != nil {
+ api.HandleErr(w, r, tx, http.StatusInternalServerError,
nil, fmt.Errorf("failed to get common server properties from V4 server struct:
%v", err))
+ return
+ }
+ 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: error wrapping
##########
traffic_ops/traffic_ops_golang/server/servers.go:
##########
@@ -409,7 +442,53 @@ func validateCommon(s *tc.CommonServerProperties, tx
*sql.Tx) []error {
if cdnID != *s.CDNID {
errs = append(errs, fmt.Errorf("CDN id '%d' for profile '%d'
does not match Server CDN '%d'", cdnID, *s.ProfileID, *s.CDNID))
}
+ return errs
+}
+
+func validateCommonV40(s *tc.CommonServerPropertiesV40, tx *sql.Tx) []error {
+
+ noSpaces := validation.NewStringRule(tovalidate.NoSpaces, "cannot
contain spaces")
+
+ errs := tovalidate.ToErrors(validation.Errors{
+ "cachegroupId": validation.Validate(s.CachegroupID,
validation.NotNil),
+ "cdnId": validation.Validate(s.CDNID,
validation.NotNil),
+ "domainName": validation.Validate(s.DomainName,
validation.Required, noSpaces),
+ "hostName": validation.Validate(s.HostName,
validation.Required, noSpaces),
+ "physLocationId": validation.Validate(s.PhysLocationID,
validation.NotNil),
+ "profileNames": validation.Validate(s.ProfileNames,
validation.NotNil),
+ "statusId": validation.Validate(s.StatusID,
validation.NotNil),
+ "typeId": validation.Validate(s.TypeID,
validation.NotNil),
+ "httpsPort": validation.Validate(s.HTTPSPort,
validation.By(tovalidate.IsValidPortNumber)),
+ "tcpPort": validation.Validate(s.TCPPort,
validation.By(tovalidate.IsValidPortNumber)),
+ })
+
+ if len(errs) > 0 {
+ return errs
+ }
+
+ if _, err := tc.ValidateTypeID(tx, s.TypeID, "server"); err != nil {
+ errs = append(errs, err)
+ }
+
+ if len(s.ProfileNames) == 0 {
+ errs = append(errs, fmt.Errorf("no profiles exists"))
+ }
+
+ var cdnID int
+ if err := tx.QueryRow("SELECT cdn from profile WHERE name=$1",
s.ProfileNames[0]).Scan(&cdnID); err != nil {
+ log.Errorf("could not execute select cdnID from profile: %s\n",
err)
+ if err == sql.ErrNoRows {
+ errs = append(errs, fmt.Errorf("no such profileName:
'%v'", s.ProfileNames[0]))
Review Comment:
`%v` should be reserved for printing data structures and/or when the type of
the thing being formatted isn't known until runtime. Strings should format with
`%s`.
##########
traffic_ops/traffic_ops_golang/server/servers.go:
##########
@@ -743,9 +822,14 @@ 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.StatusInternalServerError, nil, fmt.Errorf("failed to convert servers to
V3 format: %v", err))
+ api.HandleErr(w, r, tx,
http.StatusInternalServerError, nil, fmt.Errorf("failed to get common server
properties from V4 server struct: %v", err))
Review Comment:
making an error from an error should wrap with `%w`
##########
traffic_ops/traffic_ops_golang/server/servers.go:
##########
@@ -756,7 +840,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.StatusInternalServerError,
nil, fmt.Errorf("failed to get common server properties from V4 server struct:
%v", err))
Review Comment:
same as above RE: error wrapping
##########
lib/go-tc/servers.go:
##########
@@ -700,6 +700,44 @@ type CommonServerProperties struct {
XMPPPasswd *string `json:"xmppPasswd"
db:"xmpp_passwd"`
}
+type CommonServerPropertiesV40 struct {
Review Comment:
This structure is superfluous and adds complexity we don't need
##########
traffic_ops/traffic_ops_golang/server/servers.go:
##########
@@ -409,7 +442,53 @@ func validateCommon(s *tc.CommonServerProperties, tx
*sql.Tx) []error {
if cdnID != *s.CDNID {
errs = append(errs, fmt.Errorf("CDN id '%d' for profile '%d'
does not match Server CDN '%d'", cdnID, *s.ProfileID, *s.CDNID))
}
+ return errs
+}
+
+func validateCommonV40(s *tc.CommonServerPropertiesV40, tx *sql.Tx) []error {
+
+ noSpaces := validation.NewStringRule(tovalidate.NoSpaces, "cannot
contain spaces")
+
+ errs := tovalidate.ToErrors(validation.Errors{
+ "cachegroupId": validation.Validate(s.CachegroupID,
validation.NotNil),
+ "cdnId": validation.Validate(s.CDNID,
validation.NotNil),
+ "domainName": validation.Validate(s.DomainName,
validation.Required, noSpaces),
+ "hostName": validation.Validate(s.HostName,
validation.Required, noSpaces),
+ "physLocationId": validation.Validate(s.PhysLocationID,
validation.NotNil),
+ "profileNames": validation.Validate(s.ProfileNames,
validation.NotNil),
+ "statusId": validation.Validate(s.StatusID,
validation.NotNil),
+ "typeId": validation.Validate(s.TypeID,
validation.NotNil),
+ "httpsPort": validation.Validate(s.HTTPSPort,
validation.By(tovalidate.IsValidPortNumber)),
+ "tcpPort": validation.Validate(s.TCPPort,
validation.By(tovalidate.IsValidPortNumber)),
+ })
+
+ if len(errs) > 0 {
+ return errs
+ }
+
+ if _, err := tc.ValidateTypeID(tx, s.TypeID, "server"); err != nil {
+ errs = append(errs, err)
+ }
+
+ if len(s.ProfileNames) == 0 {
+ errs = append(errs, fmt.Errorf("no profiles exists"))
+ }
+
+ var cdnID int
+ if err := tx.QueryRow("SELECT cdn from profile WHERE name=$1",
s.ProfileNames[0]).Scan(&cdnID); err != nil {
+ log.Errorf("could not execute select cdnID from profile: %s\n",
err)
+ if err == sql.ErrNoRows {
+ errs = append(errs, fmt.Errorf("no such profileName:
'%v'", s.ProfileNames[0]))
+ } else {
+ errs = append(errs, tc.DBError)
Review Comment:
From the GoDoc for `tc.DBError`:
> Deprecated: Since internal errors are not returned to users, there's no
reason not to include more detail in an error message than this.
When something like this happens, the right response code is a `500 Internal
Server Error`, but currently it would be a `400 Bad Request` - which means that
the solution isn't to just append a different error. The only way I can think
of to fix it is to change the return signature from `[]error` to `([]error,
error)` so it can return a server-side error. It looks like that wouldn't be
too big of a refactor, just add some `, nil`s and change the handling statement
a little bit.
##########
traffic_ops/traffic_ops_golang/server/servers.go:
##########
@@ -1490,7 +1709,16 @@ func Update(w http.ResponseWriter, r *http.Request) {
}
api.WriteRespAlertObj(w, r, tc.SuccessLevel, "Server updated",
srvr)
} else {
- v2Server, err := srvr.ToServerV2FromV4()
+ csp, err := dbhelpers.GetCommonServerPropertiesFromV4(server,
tx)
+ if err != nil {
+ api.HandleErr(w, r, tx, http.StatusInternalServerError,
nil, fmt.Errorf("failed to get common server properties from V4 server struct:
%v", err))
Review Comment:
same as above RE: error wrapping
--
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]
