ocket8888 commented on code in PR #7241:
URL: https://github.com/apache/trafficcontrol/pull/7241#discussion_r1048802698
##########
lib/go-tc/parameters.go:
##########
@@ -277,12 +277,15 @@ func ParamExists(id int64, tx *sql.Tx) (bool, error) {
// ParamsExist returns whether parameters exist for all the given ids, and any
error.
// TODO move to helper package.
-func ParamsExist(ids []int64, tx *sql.Tx) (bool, error) {
- count := 0
- if err := tx.QueryRow(`SELECT count(*) from parameter where id =
ANY($1)`, pq.Array(ids)).Scan(&count); err != nil {
- return false, errors.New("querying parameters existence from
id: " + err.Error())
+func ParamsExist(ids []int64, tx *sql.Tx) (bool, []int64, error) {
+ var nonExistingIDs []int64
+ if err := tx.QueryRow(`SELECT ARRAY_AGG(id) FROM UNNEST($1::INT[]) AS
id WHERE id NOT IN (SELECT id FROM parameter)`,
pq.Array(ids)).Scan(pq.Array(&nonExistingIDs)); err != nil {
+ return false, nil, errors.New("querying parameters existence
from id: " + err.Error())
Review Comment:
constructing errors from errors should use wrapping e.g. with
`fmt.Errorf``'s `%w` formatting parameter.
##########
lib/go-tc/parameters.go:
##########
@@ -277,12 +277,15 @@ func ParamExists(id int64, tx *sql.Tx) (bool, error) {
// ParamsExist returns whether parameters exist for all the given ids, and any
error.
// TODO move to helper package.
-func ParamsExist(ids []int64, tx *sql.Tx) (bool, error) {
- count := 0
- if err := tx.QueryRow(`SELECT count(*) from parameter where id =
ANY($1)`, pq.Array(ids)).Scan(&count); err != nil {
- return false, errors.New("querying parameters existence from
id: " + err.Error())
+func ParamsExist(ids []int64, tx *sql.Tx) (bool, []int64, error) {
+ var nonExistingIDs []int64
+ if err := tx.QueryRow(`SELECT ARRAY_AGG(id) FROM UNNEST($1::INT[]) AS
id WHERE id NOT IN (SELECT id FROM parameter)`,
pq.Array(ids)).Scan(pq.Array(&nonExistingIDs)); err != nil {
+ return false, nil, errors.New("querying parameters existence
from id: " + err.Error())
Review Comment:
Also, this error shouldn't be shown to the user, but the way this works
currently it would be. You don't need to fix that here, but you can if you want.
##########
lib/go-tc/parameters.go:
##########
@@ -277,12 +277,15 @@ func ParamExists(id int64, tx *sql.Tx) (bool, error) {
// ParamsExist returns whether parameters exist for all the given ids, and any
error.
// TODO move to helper package.
-func ParamsExist(ids []int64, tx *sql.Tx) (bool, error) {
- count := 0
- if err := tx.QueryRow(`SELECT count(*) from parameter where id =
ANY($1)`, pq.Array(ids)).Scan(&count); err != nil {
- return false, errors.New("querying parameters existence from
id: " + err.Error())
+func ParamsExist(ids []int64, tx *sql.Tx) (bool, []int64, error) {
Review Comment:
It looks like the first return value - the boolean - is only false if the
error is non-nil or there are non-existing IDs being returned. Which means it
doesn't serve much purpose because it can be trivially inferred from those
other return values.
##########
lib/go-tc/parameters.go:
##########
@@ -210,10 +210,10 @@ func (pp *PostProfileParam) Validate(tx *sql.Tx) error {
}
if pp.ParamIDs == nil {
errs = append(errs, errors.New("paramIds missing"))
- } else if ok, err := ParamsExist(*pp.ParamIDs, tx); err != nil {
+ } else if ok, nonExistingIDs, err := ParamsExist(*pp.ParamIDs, tx); err
!= nil {
errs = append(errs, errors.New(fmt.Sprintf("checking parameter
IDs %v existence: "+err.Error(), *pp.ParamIDs)))
} else if !ok {
- errs = append(errs, errors.New(fmt.Sprintf("parameters with IDs
%v don't all exist", *pp.ParamIDs)))
+ errs = append(errs, errors.New(fmt.Sprintf("parameters with IDs
%v don't all exist", nonExistingIDs)))
Review Comment:
Please use `fmt.Errorf(args)` instead of `errors.New(fmt.Sprintf(args))`. I
see it being done in other places but if you've gotta change the line anyway,
this heinous practice can be fixed in at least this one place.
also, the text "... don't all exist" implies that one or more of them may
actually exist, but it's more accurate to report "... all don't exist" or just
"... don't exist".
--
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]
