ocket8888 commented on code in PR #7224:
URL: https://github.com/apache/trafficcontrol/pull/7224#discussion_r1048646070
##########
traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go:
##########
@@ -1584,12 +1591,53 @@ func Validate(tx *sql.Tx, ds *tc.DeliveryServiceV5)
error {
if err := validateTypeFields(tx, ds); err != nil {
errs = append(errs, fmt.Errorf("type fields: %w", err))
}
+ userErr, sysErr := validateRequiredCapabilities(tx, ds)
+ if userErr != nil {
+ errs = append(errs, errors.New("required capabilities:
"+userErr.Error()))
+ }
+ if sysErr != nil {
+ errs = append(errs, errors.New("reading/ scanning required
capabilities: "+sysErr.Error()))
Review Comment:
constructing errors from errors should wrap them e.g. using `fmt.Errorf`
with the `%w` formatting parameter.
Also, this is returning a system-only error to the user, by appending it to
a list of user-facing errors. If a system error occurs, a `500 Internal Server
Error` must be written back to the client, and no information about server-side
problems or configuration should be made visible to the client. The best way to
do this would be to change this function to return two errors, one system and
one user-facing, so that you can handle it appropriately e.g. with
`api.HandleErr`, but you could also conceivably handle it by returning a "dummy
error" from a package-internal constant and then the caller has to check that
the returned error is that error using `errors.Is` and do something special in
that case. I'm not convinced that this approach is less work than just changing
the function's call signature, but however you want to accomplish that without
ignoring the server-side error is fine.
##########
traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go:
##########
@@ -1584,12 +1591,53 @@ func Validate(tx *sql.Tx, ds *tc.DeliveryServiceV5)
error {
if err := validateTypeFields(tx, ds); err != nil {
errs = append(errs, fmt.Errorf("type fields: %w", err))
}
+ userErr, sysErr := validateRequiredCapabilities(tx, ds)
+ if userErr != nil {
+ errs = append(errs, errors.New("required capabilities:
"+userErr.Error()))
+ }
+ if sysErr != nil {
+ errs = append(errs, errors.New("reading/ scanning required
capabilities: "+sysErr.Error()))
+ }
if len(errs) == 0 {
return nil
}
return util.JoinErrs(errs)
}
+func validateRequiredCapabilities(tx *sql.Tx, ds *tc.DeliveryServiceV5)
(error, error) {
+ missing := make([]string, 0)
+ var missingCap string
+ query := `SELECT missing
+FROM (
+ SELECT UNNEST($1::TEXT[])
+ EXCEPT
+ SELECT UNNEST(ARRAY_AGG(name)) FROM server_capability) t(missing)
Review Comment:
I think creating this anonymous table is entirely unnecessary. It seems that
`SELECT col FROM (*table-valued expression with one column*) t(col)` would
always be the same as just the `*table-valued expression with one column*`,
because the expression only has one column and you are selecting that column
from every row in the expression. Similarly, `UNNEST` converts an array into a
set of elements and `ARRAY_AGG` aggregates a set of elements into an array, so
they are more or less inverse functions. That is, `UNNEST(ARRAY_AGG(x))` is
just the same as `x`, so there's no reason to wrap it those operations.
##########
traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go:
##########
@@ -1584,12 +1591,53 @@ func Validate(tx *sql.Tx, ds *tc.DeliveryServiceV5)
error {
if err := validateTypeFields(tx, ds); err != nil {
errs = append(errs, fmt.Errorf("type fields: %w", err))
}
+ userErr, sysErr := validateRequiredCapabilities(tx, ds)
+ if userErr != nil {
+ errs = append(errs, errors.New("required capabilities:
"+userErr.Error()))
+ }
+ if sysErr != nil {
+ errs = append(errs, errors.New("reading/ scanning required
capabilities: "+sysErr.Error()))
+ }
if len(errs) == 0 {
return nil
}
return util.JoinErrs(errs)
}
+func validateRequiredCapabilities(tx *sql.Tx, ds *tc.DeliveryServiceV5)
(error, error) {
+ missing := make([]string, 0)
+ var missingCap string
+ query := `SELECT missing
+FROM (
+ SELECT UNNEST($1::TEXT[])
+ EXCEPT
+ SELECT UNNEST(ARRAY_AGG(name)) FROM server_capability) t(missing)
+`
+ if len(ds.RequiredCapabilities) > 0 {
+ rows, err := tx.Query(query, pq.Array(ds.RequiredCapabilities))
+ if err != nil {
+ return nil, err
+ }
+ defer rows.Close()
+ for rows.Next() {
+ err = rows.Scan(&missingCap)
+ if err != nil {
+ return nil, err
+ }
+ missing = append(missing, missingCap)
+ }
+ if len(missing) > 0 {
+ var msg string
+ for _, m := range missing {
+ msg = msg + m + " "
+ }
+ userErr := fmt.Errorf("the following capabilities do
not exist: %s", msg)
Review Comment:
there's a standard library function for this:
[`strings.Join`](https://pkg.go.dev/strings#Join)
##########
traffic_ops/traffic_ops_golang/server/servers.go:
##########
@@ -825,10 +826,20 @@ func getServers(h http.Header, params map[string]string,
tx *sqlx.Tx, user *auth
}
var joinSubQuery string
- if err =
tx.QueryRow(deliveryservice.HasRequiredCapabilitiesQuery,
dsID).Scan(&dsHasRequiredCapabilities); err != nil {
+ rows, err :=
tx.Query(deliveryservice.GetRequiredCapabilitiesQuery, dsID)
Review Comment:
Does the `HasRequiredCapabilitiesQuery` not work here anymore? I would
assume that's because of the structural changes to the table(s). So should the
old query just be deleted, since we know it doesn't work?
It looks like this still returns a single row, so you could keep it shorter
by continuing to use `QueryRow`.
--
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]
