From 5b35b6434cd5a296d7f8fb626c942161acbaed0f Mon Sep 17 00:00:00 2001 From: Simo Kinnunen Date: Mon, 3 Feb 2025 21:57:45 +0900 Subject: [PATCH] fix: pass empty Locations and PrivateLocations to backend [sc-23129] (#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/), 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 --- checkly.go | 115 ++++++++++++++++++++---------------------------- checkly_test.go | 42 ++++++++++-------- types.go | 8 ++-- 3 files changed, 75 insertions(+), 90 deletions(-) diff --git a/checkly.go b/checkly.go index 3dceef2..11a1ef3 100644 --- a/checkly.go +++ b/checkly.go @@ -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/. 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 @@ -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. @@ -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 @@ -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 { @@ -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 @@ -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 { @@ -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 diff --git a/checkly_test.go b/checkly_test.go index e0cdcb4..0eda68f 100644 --- a/checkly_test.go +++ b/checkly_test.go @@ -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", @@ -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") } @@ -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{ @@ -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)) } @@ -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)) } @@ -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)) } diff --git a/types.go b/types.go index 98849f8..565fddd 100644 --- a/types.go +++ b/types.go @@ -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"` @@ -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"` @@ -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"` @@ -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"`