Skip to content

Commit a8fef75

Browse files
henrybarretogustavosbarreto
henrybarreto
authored and
gustavosbarreto
committed
feat(api,ssh,pkg): standardize error handling in internal http client
It refactors the internal client to standardize error handling and improve its structure. A new error type is introduced to encapsulate HTTP response details, including the status code, which was previously returned as a separate value. This simplifies the client's function signatures and makes error handling more consistent. The configuration has been extracted into a dedicated file with default values, and the now-redundant WithConfig option has been removed. All client methods have been updated to use the new error handling and configuration, and the corresponding mocks and usages have been adjusted accordingly.
1 parent 0d5db74 commit a8fef75

File tree

16 files changed

+227
-340
lines changed

16 files changed

+227
-340
lines changed

‎api/services/billing.go‎

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package services
22

33
import (
44
"context"
5+
"errors"
56

67
req "github.com/shellhub-io/shellhub/pkg/api/internalclient"
78
)
@@ -13,7 +14,7 @@ type BillingInterface interface {
1314

1415
// BillingEvaluate evaluate in the billing service if the namespace can create accept more devices.
1516
func (s *service) BillingEvaluate(ctx context.Context, client req.Client, tenant string) (bool, error) {
16-
evaluation, _, err := client.BillingEvaluate(ctx, tenant)
17+
evaluation, err := client.BillingEvaluate(ctx, tenant)
1718
if err != nil {
1819
return false, ErrEvaluate
1920
}
@@ -27,17 +28,19 @@ const (
2728
)
2829

2930
func (s *service) BillingReport(ctx context.Context, client req.Client, tenant string, action string) error {
30-
status, err := client.BillingReport(ctx, tenant, action)
31-
if err != nil {
32-
return err
31+
if err := client.BillingReport(ctx, tenant, action); err != nil {
32+
var e *req.Error
33+
if ok := errors.As(err, &e); !ok {
34+
return ErrReport
35+
}
36+
37+
switch e.Code {
38+
case 402:
39+
return ErrPaymentRequired
40+
default:
41+
return ErrReport
42+
}
3343
}
3444

35-
switch status {
36-
case 200:
37-
return nil
38-
case 402:
39-
return ErrPaymentRequired
40-
default:
41-
return ErrReport
42-
}
45+
return nil
4346
}

‎api/services/billing_test.go‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77

88
"github.com/shellhub-io/shellhub/api/store"
99
"github.com/shellhub-io/shellhub/api/store/mocks"
10+
req "github.com/shellhub-io/shellhub/pkg/api/internalclient"
1011
"github.com/shellhub-io/shellhub/pkg/cache"
1112
"github.com/shellhub-io/shellhub/pkg/models"
1213
"github.com/stretchr/testify/assert"
@@ -31,15 +32,15 @@ func TestBillingEvaluate(t *testing.T) {
3132
description: "succeeds when client method succeeds",
3233
tenant: "00000000-0000-0000-0000-000000000000",
3334
requiredMocks: func() {
34-
clientMock.On("BillingEvaluate", mock.Anything, "00000000-0000-0000-0000-000000000000").Return(&models.BillingEvaluation{CanAccept: true, CanConnect: true}, 0, nil).Once()
35+
clientMock.On("BillingEvaluate", mock.Anything, "00000000-0000-0000-0000-000000000000").Return(&models.BillingEvaluation{CanAccept: true, CanConnect: true}, nil).Once()
3536
},
3637
expected: Expected{canAccept: true, err: nil},
3738
},
3839
{
3940
description: "fails when client method fails",
4041
tenant: "00000000-0000-0000-0000-000000000000",
4142
requiredMocks: func() {
42-
clientMock.On("BillingEvaluate", mock.Anything, "00000000-0000-0000-0000-000000000000").Return(&models.BillingEvaluation{CanAccept: true, CanConnect: true}, 0, ErrEvaluate).Once()
43+
clientMock.On("BillingEvaluate", mock.Anything, "00000000-0000-0000-0000-000000000000").Return(&models.BillingEvaluation{CanAccept: true, CanConnect: true}, ErrEvaluate).Once()
4344
},
4445
expected: Expected{canAccept: false, err: ErrEvaluate},
4546
},
@@ -73,7 +74,7 @@ func TestBillingReport(t *testing.T) {
7374
tenant: "00000000-0000-0000-0000-000000000000",
7475
action: "device_accept",
7576
requiredMocks: func() {
76-
clientMock.On("BillingReport", mock.Anything, "00000000-0000-0000-0000-000000000000", "device_accept").Return(200, nil).Once()
77+
clientMock.On("BillingReport", mock.Anything, "00000000-0000-0000-0000-000000000000", "device_accept").Return(nil).Once()
7778
},
7879
expected: nil,
7980
},
@@ -82,7 +83,7 @@ func TestBillingReport(t *testing.T) {
8283
tenant: "00000000-0000-0000-0000-000000000000",
8384
action: "device_accept",
8485
requiredMocks: func() {
85-
clientMock.On("BillingReport", mock.Anything, "00000000-0000-0000-0000-000000000000", "device_accept").Return(402, nil).Once()
86+
clientMock.On("BillingReport", mock.Anything, "00000000-0000-0000-0000-000000000000", "device_accept").Return(&req.Error{Code: 402, Message: ""}).Once()
8687
},
8788
expected: ErrPaymentRequired,
8889
},
@@ -91,7 +92,7 @@ func TestBillingReport(t *testing.T) {
9192
tenant: "00000000-0000-0000-0000-000000000000",
9293
action: "device_accept",
9394
requiredMocks: func() {
94-
clientMock.On("BillingReport", mock.Anything, "00000000-0000-0000-0000-000000000000", "device_accept").Return(500, nil).Once()
95+
clientMock.On("BillingReport", mock.Anything, "00000000-0000-0000-0000-000000000000", "device_accept").Return(&req.Error{Code: 500, Message: ""}).Once()
9596
},
9697
expected: ErrReport,
9798
},
@@ -100,9 +101,9 @@ func TestBillingReport(t *testing.T) {
100101
tenant: "00000000-0000-0000-0000-000000000000",
101102
action: "device_accept",
102103
requiredMocks: func() {
103-
clientMock.On("BillingReport", mock.Anything, "00000000-0000-0000-0000-000000000000", "device_accept").Return(0, errors.New("error")).Once()
104+
clientMock.On("BillingReport", mock.Anything, "00000000-0000-0000-0000-000000000000", "device_accept").Return(errors.New("error")).Once()
104105
},
105-
expected: errors.New("error"),
106+
expected: ErrReport,
106107
},
107108
}
108109

‎api/services/device_test.go‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"github.com/shellhub-io/shellhub/api/store"
99
storemock "github.com/shellhub-io/shellhub/api/store/mocks"
1010
"github.com/shellhub-io/shellhub/pkg/api/authorizer"
11+
req "github.com/shellhub-io/shellhub/pkg/api/internalclient"
1112
"github.com/shellhub-io/shellhub/pkg/api/query"
1213
"github.com/shellhub-io/shellhub/pkg/api/requests"
1314
storecache "github.com/shellhub-io/shellhub/pkg/cache"
@@ -2416,7 +2417,7 @@ func TestUpdateDeviceStatus(t *testing.T) {
24162417
Once()
24172418
clientMock.
24182419
On("BillingEvaluate", mock.Anything, "00000000-0000-0000-0000-000000000000").
2419-
Return(&models.BillingEvaluation{CanAccept: false}, 0, errors.New("error", "store", 0)).
2420+
Return(&models.BillingEvaluation{CanAccept: false}, errors.New("error", "store", 0)).
24202421
Once()
24212422
},
24222423
expectedError: NewErrBillingEvaluate(errors.New("evaluate error", "service", 4)),
@@ -2487,7 +2488,7 @@ func TestUpdateDeviceStatus(t *testing.T) {
24872488
Once()
24882489
clientMock.
24892490
On("BillingEvaluate", mock.Anything, "00000000-0000-0000-0000-000000000000").
2490-
Return(&models.BillingEvaluation{CanAccept: false}, 0, nil).
2491+
Return(&models.BillingEvaluation{CanAccept: false}, nil).
24912492
Once()
24922493
},
24932494
expectedError: ErrDeviceLimit,
@@ -2558,7 +2559,7 @@ func TestUpdateDeviceStatus(t *testing.T) {
25582559
Once()
25592560
clientMock.
25602561
On("BillingEvaluate", mock.Anything, "00000000-0000-0000-0000-000000000000").
2561-
Return(&models.BillingEvaluation{CanAccept: true}, 0, nil).
2562+
Return(&models.BillingEvaluation{CanAccept: true}, nil).
25622563
Once()
25632564
storeMock.
25642565
On("DeviceUpdate", ctx, "00000000-0000-0000-0000-000000000000", "removed-device", mock.MatchedBy(func(changes *models.DeviceChanges) bool {
@@ -2642,10 +2643,10 @@ func TestUpdateDeviceStatus(t *testing.T) {
26422643
Once()
26432644
clientMock.
26442645
On("BillingReport", mock.Anything, "00000000-0000-0000-0000-000000000000", ReportDeviceAccept).
2645-
Return(0, errors.New("billing error", "", 0)).
2646+
Return(errors.New("billing error", "", 0)).
26462647
Once()
26472648
},
2648-
expectedError: NewErrBillingReportNamespaceDelete(errors.New("billing error", "", 0)),
2649+
expectedError: NewErrBillingReportNamespaceDelete(ErrReport),
26492650
},
26502651
{
26512652
description: "failure (accepted) (different MAC) (billing active) - payment required [cloud]",
@@ -2712,7 +2713,7 @@ func TestUpdateDeviceStatus(t *testing.T) {
27122713
Once()
27132714
clientMock.
27142715
On("BillingReport", mock.Anything, "00000000-0000-0000-0000-000000000000", ReportDeviceAccept).
2715-
Return(402, nil).
2716+
Return(&req.Error{Code: 402, Message: ""}).
27162717
Once()
27172718
},
27182719
expectedError: NewErrBillingReportNamespaceDelete(ErrPaymentRequired),
@@ -2782,7 +2783,7 @@ func TestUpdateDeviceStatus(t *testing.T) {
27822783
Once()
27832784
clientMock.
27842785
On("BillingReport", mock.Anything, "00000000-0000-0000-0000-000000000000", ReportDeviceAccept).
2785-
Return(200, nil).
2786+
Return(nil).
27862787
Once()
27872788
storeMock.
27882789
On("DeviceUpdate", ctx, "00000000-0000-0000-0000-000000000000", "cloud-device", mock.MatchedBy(func(changes *models.DeviceChanges) bool {

‎pkg/api/internalclient/billing.go‎

Lines changed: 12 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -2,51 +2,44 @@ package internalclient
22

33
import (
44
"context"
5-
"errors"
65

76
"github.com/shellhub-io/shellhub/pkg/models"
87
)
98

109
// billingAPI defines methods for interacting with billing-related functionality.
1110
type billingAPI interface {
1211
// BillingReport sends a billing report for the specified tenant and action.
13-
// It returns the HTTP status code of the response and an error, if any.
14-
BillingReport(ctx context.Context, tenant string, action string) (int, error)
12+
// It returns an error, if any.
13+
BillingReport(ctx context.Context, tenant string, action string) error
1514

1615
// BillingEvaluate evaluates the billing status for the specified tenant.
17-
// It returns the billing evaluation result, the HTTP status code of the response, and an error, if any.
18-
BillingEvaluate(ctx context.Context, tenantID string) (*models.BillingEvaluation, int, error)
16+
// It returns the billing evaluation result and an error, if any.
17+
BillingEvaluate(ctx context.Context, tenantID string) (*models.BillingEvaluation, error)
1918
}
2019

21-
var ErrBillingRequestFailed = errors.New("billing request failed")
22-
23-
func (c *client) BillingReport(ctx context.Context, tenant string, action string) (int, error) {
20+
func (c *client) BillingReport(ctx context.Context, tenant string, action string) error {
2421
res, err := c.http.
2522
R().
2623
SetContext(ctx).
2724
SetHeader("X-Tenant-ID", tenant).
2825
SetQueryParam("action", action).
29-
Post(c.Config.EnterpriseBaseURL + "/internal/billing/report")
30-
if err != nil {
31-
// TODO: It shouldn't return the status code.
32-
return res.StatusCode(), errors.Join(ErrBillingRequestFailed, err)
33-
}
26+
Post(c.config.EnterpriseBaseURL + "/internal/billing/report")
3427

35-
return res.StatusCode(), nil
28+
return NewError(res, err)
3629
}
3730

38-
func (c *client) BillingEvaluate(ctx context.Context, tenantID string) (*models.BillingEvaluation, int, error) {
31+
func (c *client) BillingEvaluate(ctx context.Context, tenantID string) (*models.BillingEvaluation, error) {
3932
eval := new(models.BillingEvaluation)
4033

4134
resp, err := c.http.
4235
R().
4336
SetContext(ctx).
4437
SetHeader("X-Tenant-ID", tenantID).
4538
SetResult(&eval).
46-
Post(c.Config.EnterpriseBaseURL + "/internal/billing/evaluate")
47-
if err != nil {
48-
return nil, resp.StatusCode(), errors.Join(ErrBillingRequestFailed, err)
39+
Post(c.config.EnterpriseBaseURL + "/internal/billing/evaluate")
40+
if HasError(resp, err) {
41+
return nil, NewError(resp, err)
4942
}
5043

51-
return eval, resp.StatusCode(), nil
44+
return eval, nil
5245
}

‎pkg/api/internalclient/client.go‎

Lines changed: 12 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
"errors"
55
"net"
66
"net/http"
7-
"time"
87

98
resty "github.com/go-resty/resty/v2"
109
"github.com/shellhub-io/shellhub/pkg/worker"
@@ -21,28 +20,12 @@ type Client interface {
2120
firewallAPI
2221
}
2322

24-
// Config holds configuration options for the client.
25-
type Config struct {
26-
// RetryCount defines how many times the client should retry a request in case of failure.
27-
RetryCount int
28-
// RetryWaitTime defines the wait time between retries.
29-
RetryWaitTime time.Duration
30-
// RetryMaxWaitTime defines the maximum wait time between retries.
31-
RetryMaxWaitTime time.Duration
32-
33-
// APIBaseURL defines the base URL for the API service.
34-
APIBaseURL string
35-
36-
// EnterpriseBaseURL defines the base URL for enterprise endpoints (cloud component).
37-
EnterpriseBaseURL string
38-
}
39-
4023
type client struct {
4124
http *resty.Client
4225
logger *log.Logger
4326
worker worker.Client
4427

45-
Config *Config
28+
config *Config
4629
}
4730

4831
const (
@@ -57,18 +40,16 @@ var (
5740
)
5841

5942
func NewClient(opts ...clientOption) (Client, error) {
43+
cfg, err := DefaultConfig()
44+
if err != nil {
45+
return nil, err
46+
}
47+
6048
httpClient := resty.New()
6149

6250
c := &client{
63-
http: httpClient,
64-
Config: &Config{
65-
// NOTE: Default values can be overridden using the WithConfig option.
66-
RetryCount: 3,
67-
RetryWaitTime: 5 * time.Second,
68-
RetryMaxWaitTime: 20 * time.Second,
69-
APIBaseURL: "http://api:8080",
70-
EnterpriseBaseURL: "http://cloud:8080",
71-
},
51+
http: httpClient,
52+
config: cfg,
7253
}
7354

7455
for _, opt := range opts {
@@ -82,10 +63,10 @@ func NewClient(opts ...clientOption) (Client, error) {
8263
}
8364

8465
// NOTE: Avoid setting a global base URL on the Resty client. Calls to enterprise endpoints
85-
// will use c.Config.EnterpriseBaseURL explicitly when needed.
86-
httpClient.SetRetryCount(c.Config.RetryCount)
87-
httpClient.SetRetryWaitTime(c.Config.RetryWaitTime)
88-
httpClient.SetRetryMaxWaitTime(c.Config.RetryMaxWaitTime)
66+
// will use c.config.EnterpriseBaseURL explicitly when needed.
67+
httpClient.SetRetryCount(c.config.RetryCount)
68+
httpClient.SetRetryWaitTime(c.config.RetryWaitTime)
69+
httpClient.SetRetryMaxWaitTime(c.config.RetryMaxWaitTime)
8970
httpClient.AddRetryCondition(func(r *resty.Response, err error) bool {
9071
if _, ok := err.(net.Error); ok { // if the error is a network error, retry.
9172
return true

‎pkg/api/internalclient/config.go‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
package internalclient
2+
3+
import "time"
4+
5+
// Config holds configuration options for the client.
6+
type Config struct {
7+
// RetryCount defines how many times the client should retry a request in case of failure.
8+
RetryCount int
9+
// RetryWaitTime defines the wait time between retries.
10+
RetryWaitTime time.Duration
11+
// RetryMaxWaitTime defines the maximum wait time between retries.
12+
RetryMaxWaitTime time.Duration
13+
14+
// APIBaseURL defines the base URL for the API service.
15+
APIBaseURL string
16+
17+
// EnterpriseBaseURL defines the base URL for enterprise endpoints (cloud component).
18+
EnterpriseBaseURL string
19+
}
20+
21+
// DefaultConfig returns a Config struct with default values.
22+
func DefaultConfig() (*Config, error) {
23+
return &Config{
24+
RetryCount: 3,
25+
RetryWaitTime: 5 * time.Second,
26+
RetryMaxWaitTime: 20 * time.Second,
27+
APIBaseURL: "http://api:8080",
28+
EnterpriseBaseURL: "http://cloud:8080",
29+
}, nil
30+
}

0 commit comments

Comments
 (0)