Skip to content

Commit

Permalink
fix: pass empty Locations and PrivateLocations to backend [sc-23129] (#…
Browse files Browse the repository at this point in the history 
…125)

* fix: remove omitempty from Locations and PrivateLocations

When omitempty, the property is not sent to the server. Our PUT endpoints
however act like PATCH would, meaning that partial updates are supported.
When a property is not set, it is kept intact, even though the intention
most likely was to remove it.

* fix: always send empty [] for locations (public/private) even when not set

If the backend does not receive a value, or receives a null value, it will
not update the database. This is an issue for many other values too, but
as an API level fix will be needed, for now this patch only addresses the
fields we've received reports about.

To make the patch a little easier, Create now calls CreateCheck, Update
UpdateCheck and Delete DeleteCheck. The latter two were already equivalent.
Create and CreateCheck had some differences (mainly that that former would
send the request to /v1/checks instead of /v1/checks/<type>), but given
how CreateCheck covers all check types, there should be no difference.

* chore: update tests, these need a complete overhaul though

* fix: keep using /v1/checks for Create after all as it behaves differently
  • Loading branch information
@sorccu
sorccu authored Feb 3, 2025
1 parent dac9fad commit 5b35b64
Show file tree
Hide file tree
Showing 3 changed files with 75 additions and 90 deletions.
115 changes: 48 additions & 67 deletions checkly.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,27 +66,9 @@ func (c *client) Create(
ctx context.Context,
check Check,
) (*Check, error) {
data, err := json.Marshal(check)
if err != nil {
return nil, err
}
status, res, err := c.apiCall(
ctx,
http.MethodPost,
withAutoAssignAlertsFlag("checks"),
data,
)
if err != nil {
return nil, err
}
if status != http.StatusCreated {
return nil, fmt.Errorf("unexpected response status %d: %q", status, res)
}
var result Check
if err = json.NewDecoder(strings.NewReader(res)).Decode(&result); err != nil {
return nil, fmt.Errorf("decoding error for data %s: %v", res, err)
}
return &result, nil
// There are differences between /v1/checks and /v1/checks/<type>. Keep
// using /v1/checks here for backwards compatibility reasons.
return c.createCheck(ctx, check, "checks")
}

// Update updates an existing check with the specified details. It returns the
Expand All @@ -98,28 +80,7 @@ func (c *client) Update(
ctx context.Context,
ID string, check Check,
) (*Check, error) {
data, err := json.Marshal(check)
if err != nil {
return nil, err
}
status, res, err := c.apiCall(
ctx,
http.MethodPut,
withAutoAssignAlertsFlag(fmt.Sprintf("checks/%s", ID)),
data,
)
if err != nil {
return nil, err
}
if status != http.StatusOK {
return nil, fmt.Errorf("unexpected response status %d: %q", status, res)
}
var result Check
err = json.NewDecoder(strings.NewReader(res)).Decode(&result)
if err != nil {
return nil, fmt.Errorf("decoding error for data %s: %v", res, err)
}
return &result, nil
return c.UpdateCheck(ctx, ID, check)
}

// Delete deletes the check with the specified ID.
Expand All @@ -130,19 +91,7 @@ func (c *client) Delete(
ctx context.Context,
ID string,
) error {
status, res, err := c.apiCall(
ctx,
http.MethodDelete,
fmt.Sprintf("checks/%s", ID),
nil,
)
if err != nil {
return err
}
if status != http.StatusNoContent {
return fmt.Errorf("unexpected response status %d: %q", status, res)
}
return nil
return c.DeleteCheck(ctx, ID)
}

// Get takes the ID of an existing check, and returns the check parameters, or
Expand Down Expand Up @@ -180,29 +129,37 @@ func (c *client) CreateCheck(
ctx context.Context,
check Check,
) (*Check, error) {
data, err := json.Marshal(check)
if err != nil {
return nil, err
}
var checkType string
var endpoint string
switch check.Type {
case "BROWSER":
checkType = "checks/browser"
endpoint = "checks/browser"
case "API":
checkType = "checks/api"
endpoint = "checks/api"
case "HEARTBEAT":
checkType = "checks/heartbeat"
endpoint = "checks/heartbeat"
case "MULTI_STEP":
checkType = "checks/multistep"
endpoint = "checks/multistep"
case "TCP":
return nil, fmt.Errorf("user error: use CreateTCPCheck to create TCP checks")
default:
return nil, fmt.Errorf("unknown check type: %s", checkType)
return nil, fmt.Errorf("unknown check type: %s", endpoint)
}
return c.createCheck(ctx, check, endpoint)
}

func (c *client) createCheck(
ctx context.Context,
check Check,
endpoint string,
) (*Check, error) {
data, err := json.Marshal(check)
if err != nil {
return nil, err
}
status, res, err := c.apiCall(
ctx,
http.MethodPost,
withAutoAssignAlertsFlag(checkType),
withAutoAssignAlertsFlag(endpoint),
data,
)
if err != nil {
Expand Down Expand Up @@ -278,6 +235,14 @@ func (c *client) UpdateCheck(
ctx context.Context,
ID string, check Check,
) (*Check, error) {
// A nil value for a list will cause the backend to not update the value.
// We must send empty lists instead.
if check.Locations == nil {
check.Locations = []string{}
}
if check.PrivateLocations == nil {
check.PrivateLocations = &[]string{}
}
data, err := json.Marshal(check)
if err != nil {
return nil, err
Expand Down Expand Up @@ -337,6 +302,14 @@ func (c *client) UpdateTCPCheck(
ID string,
check TCPCheck,
) (*TCPCheck, error) {
// A nil value for a list will cause the backend to not update the value.
// We must send empty lists instead.
if check.Locations == nil {
check.Locations = []string{}
}
if check.PrivateLocations == nil {
check.PrivateLocations = &[]string{}
}
// Unfortunately `checkType` is required for this endpoint, so sneak it in
// using an anonymous struct.
payload := struct {
Expand Down Expand Up @@ -532,6 +505,14 @@ func (c *client) UpdateGroup(
ID int64,
group Group,
) (*Group, error) {
// A nil value for a list will cause the backend to not update the value.
// We must send empty lists instead.
if group.Locations == nil {
group.Locations = []string{}
}
if group.PrivateLocations == nil {
group.PrivateLocations = &[]string{}
}
data, err := json.Marshal(group)
if err != nil {
return nil, err
Expand Down
42 changes: 23 additions & 19 deletions checkly_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,15 @@ import (
var wantCheckID = "73d29e72-6540-4bb5-967e-e07fa2c9465e"

var wantCheck = checkly.Check{
Name: "test",
Type: checkly.TypeAPI,
Frequency: 10,
Activated: true,
Muted: false,
DoubleCheck: true,
ShouldFail: false,
Locations: []string{"eu-west-1"},
Name: "test",
Type: checkly.TypeAPI,
Frequency: 10,
Activated: true,
Muted: false,
DoubleCheck: true,
ShouldFail: false,
Locations: []string{"eu-west-1"},
PrivateLocations: &[]string{},
Request: checkly.Request{
Method: http.MethodGet,
URL: "https://example.com",
Expand Down Expand Up @@ -161,14 +162,16 @@ func TestAPIError(t *testing.T) {
t.Parallel()
ts := cannedResponseServer(t,
http.MethodPost,
"/v1/checks?autoAssignAlerts=false",
"/v1/checks/api?autoAssignAlerts=false",
validateAnything,
http.StatusBadRequest,
"BadRequest.json",
)
defer ts.Close()
client := checkly.NewClient(ts.URL, "dummy-key", ts.Client(), nil)
_, err := client.Create(context.Background(), checkly.Check{})
_, err := client.CreateCheck(context.Background(), checkly.Check{
Type: checkly.TypeAPI,
})
if err == nil {
t.Error("want error when API returns 'bad request' status, got nil")
}
Expand Down Expand Up @@ -334,12 +337,13 @@ func TestDeleteCheck(t *testing.T) {
var wantGroupID int64 = 135

var wantGroup = checkly.Group{
Name: "test",
Activated: true,
Muted: false,
Tags: []string{"auto"},
Locations: []string{"eu-west-1"},
Concurrency: 3,
Name: "test",
Activated: true,
Muted: false,
Tags: []string{"auto"},
Locations: []string{"eu-west-1"},
PrivateLocations: &[]string{},
Concurrency: 3,
APICheckDefaults: checkly.APICheckDefaults{
BaseURL: "example.com/api/test",
Headers: []checkly.KeyValue{
Expand Down Expand Up @@ -429,7 +433,7 @@ func TestCreateGroup(t *testing.T) {
if err != nil {
t.Error(err)
}
ignored := cmpopts.IgnoreFields(checkly.Group{}, "ID", "AlertChannelSubscriptions")
ignored := cmpopts.IgnoreFields(checkly.Group{}, "ID", "AlertChannelSubscriptions", "PrivateLocations")
if !cmp.Equal(wantGroup, *gotGroup, ignored) {
t.Error(cmp.Diff(wantGroup, *gotGroup, ignoreGroupFields))
}
Expand All @@ -450,7 +454,7 @@ func TestGetGroup(t *testing.T) {
if err != nil {
t.Error(err)
}
ignored := cmpopts.IgnoreFields(checkly.Group{}, "ID", "AlertChannelSubscriptions")
ignored := cmpopts.IgnoreFields(checkly.Group{}, "ID", "AlertChannelSubscriptions", "PrivateLocations")
if !cmp.Equal(wantGroup, *gotGroup, ignored) {
t.Error(cmp.Diff(wantGroup, *gotGroup, ignored))
}
Expand All @@ -471,7 +475,7 @@ func TestUpdateGroup(t *testing.T) {
if err != nil {
t.Error(err)
}
ignored := cmpopts.IgnoreFields(checkly.Group{}, "ID", "AlertChannelSubscriptions")
ignored := cmpopts.IgnoreFields(checkly.Group{}, "ID", "AlertChannelSubscriptions", "PrivateLocations")
if !cmp.Equal(wantGroup, *gotGroup, ignored) {
t.Error(cmp.Diff(wantGroup, *gotGroup, ignoreGroupFields))
}
Expand Down
8 changes: 4 additions & 4 deletions types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ type Check struct {
Muted bool `json:"muted"`
ShouldFail bool `json:"shouldFail"`
RunParallel bool `json:"runParallel"`
Locations []string `json:"locations,omitempty"`
Locations []string `json:"locations"`
DegradedResponseTime int `json:"degradedResponseTime"`
MaxResponseTime int `json:"maxResponseTime"`
Script string `json:"script,omitempty"`
Expand Down Expand Up @@ -517,7 +517,7 @@ type MultiStepCheck struct {
Muted bool `json:"muted"`
ShouldFail bool `json:"shouldFail"`
RunParallel bool `json:"runParallel"`
Locations []string `json:"locations,omitempty"`
Locations []string `json:"locations"`
Script string `json:"script,omitempty"`
EnvironmentVariables []EnvironmentVariable `json:"environmentVariables"`
Tags []string `json:"tags,omitempty"`
Expand Down Expand Up @@ -559,7 +559,7 @@ type TCPCheck struct {
Muted bool `json:"muted"`
ShouldFail bool `json:"shouldFail"`
RunParallel bool `json:"runParallel"`
Locations []string `json:"locations,omitempty"`
Locations []string `json:"locations"`
DegradedResponseTime int `json:"degradedResponseTime,omitempty"`
MaxResponseTime int `json:"maxResponseTime,omitempty"`
Tags []string `json:"tags,omitempty"`
Expand All @@ -569,7 +569,7 @@ type TCPCheck struct {
GroupID int64 `json:"groupId,omitempty"`
GroupOrder int `json:"groupOrder,omitempty"`
AlertChannelSubscriptions []AlertChannelSubscription `json:"alertChannelSubscriptions,omitempty"`
PrivateLocations *[]string `json:"privateLocations,omitempty"`
PrivateLocations *[]string `json:"privateLocations"`
RuntimeID *string `json:"runtimeId"`
RetryStrategy *RetryStrategy `json:"retryStrategy,omitempty"`
CreatedAt time.Time `json:"created_at,omitempty"`
Expand Down

0 comments on commit 5b35b64

Please sign in to comment.