fix: corrrectly return 4XX errors instead of 5XX errors #24519
Conversation
davidby-influx
added
kind/bug
area/api
area/2.x
davidby-influx
changed the title
authorization/storage.go
Outdated
| @@ -43,7 +43,7 @@ func (s *Store) Update(ctx context.Context, fn func(kv.Tx) error) error { | |||
| } | |||
|
|
|||
| func (s *Store) setup() error { | |||
| return s.Update(context.Background(), func(tx kv.Tx) error { | |||
| err := s.Update(context.Background(), func(tx kv.Tx) error { | |||
There was a problem hiding this comment.
Did you make this change intending to add further changes in this code?
There was a problem hiding this comment.
I had changes there and backed them out. I will revert this.
| @@ -35,7 +35,8 @@ func (h ErrorHandler) HandleHTTPError(ctx context.Context, err error, w http.Res | |||
|
|
|||
| code := errors2.ErrorCode(err) | |||
| var msg string | |||
| if _, ok := err.(*errors2.Error); ok { | |||
| var eTmp *errors2.Error | |||
| if ok := errors.As(err, &eTmp); ok { | |||
There was a problem hiding this comment.
Why As with a tmp var instead of Is? I noticed this pattern in a few places, so I assume its intentional but I'm not sure why.
There was a problem hiding this comment.
For errors.Is you have to provide a concrete object that is matched against for equality. In errors.As you are trying to find something that is assignable to the target. Since the influx Error struct actually can represent tons of errors, assignable is all we really want. I don't want field-by-field comparison.
tenant/storage_org.go
Outdated
| @@ -217,13 +218,13 @@ func (s *Store) UpdateOrg(ctx context.Context, tx kv.Tx, id platform.ID, upd inf | |||
| } | |||
|
|
|||
| if err := idx.Delete(organizationIndexKey(u.Name)); err != nil { | |||
| return nil, ErrInternalServiceError(err) | |||
| return nil, errors.ErrInternalServiceError(err) | |||
There was a problem hiding this comment.
In this file, you have some ErrInternalServiceError where you add the errors.WithErrorOp and some that you don't. Should they be consistent?
There was a problem hiding this comment.
Apparently not, judging by the existing code. Some errors in, for example, GetOrgByName, return op codes for that call but some of the actions within CreateOrg have their own private op codes, like kv/MarshalOrg. And some OS errors bubble up without op codes.
I need to consider how much time we want to spend on rationalizing this...
| @@ -57,7 +57,7 @@ func (s *Store) uniqueBucketName(ctx context.Context, tx kv.Tx, oid platform.ID, | |||
| } | |||
|
|
|||
| // any other error is some sort of internal server error | |||
| return ErrInternalServiceError(err) | |||
| return errors.ErrInternalServiceError(err) | |||
There was a problem hiding this comment.
Similar to my question in storage_org.go, do you want the errors.WithErrorOp on the errors here?
There was a problem hiding this comment.
uniqueBucketName is called from CreateBucket and UpdateBucket so there is no single operation to use. It might be better to set the operation in the caller.
tenant/storage_user.go
Outdated
| @@ -208,11 +209,11 @@ func (s *Store) CreateUser(ctx context.Context, tx kv.Tx, u *influxdb.User) erro | |||
| } | |||
|
|
|||
| if err := idx.Put([]byte(u.Name), encodedID); err != nil { | |||
| return ErrInternalServiceError(err) | |||
| return errors.ErrInternalServiceError(err, errors.WithErrorOp(influxdb.OpCreateUser)) | |||
There was a problem hiding this comment.
Same question as storage_org.go
davidby-influx
force-pushed
the
DSB_5_to_4
branch
from
a198c97 to
0591cfc
Compare
HTTP 5XX errors were being returned incorrectly from BoltDB errors that were actually bad requests, e.g., names that were too long for buckets, users, and organizations. Map BoltDB errors to correct Influx errors and return 4XX errors where appropriate. Also add op codes to more errors (cherry picked from commit a3fd489)
) HTTP 5XX errors were being returned incorrectly from BoltDB errors that were actually bad requests, e.g., names that were too long for buckets, users, and organizations. Map BoltDB errors to correct Influx errors and return 4XX errors where appropriate. Also add op codes to more errors (cherry picked from commit a3fd489)
HTTP 5XX errors were being returned incorrectly from
BoltDB errors that were really bad requests like names
that were too long for buckets, users, and organizations.
Map BoltDB errors to correct Influx errors and return
4XX errors where appropriate.