Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DeleteGraphDef to graph-defs API #157

Merged
merged 1 commit into from
May 29, 2024
Merged

Add DeleteGraphDef to graph-defs API #157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions graph_defs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package mackerel

import "net/http"

// GraphDefsParam parameters for posting graph definitions
type GraphDefsParam struct {
Name string `json:"name"`
Expand All @@ -20,3 +22,9 @@ func (c *Client) CreateGraphDefs(graphDefs []*GraphDefsParam) error {
_, err := requestPost[any](c, "/api/v0/graph-defs/create", graphDefs)
return err
}

// DeleteGraphDef deletes a graph definition.
func (c *Client) DeleteGraphDef(name string) error {
_, err := requestJSON[any](c, http.MethodDelete, "/api/v0/graph-defs/delete", map[string]string{"name": name})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, this is the first DELETE API requiring the request body :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes :) We did so, although it is somewhat unusual, because while it is common to DELETE a resource by including an ID or similar in the URL path, I was hesitant to include a part of the metric name in the path as an ID.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could use the percent encoded name in path like /api/v0/hosts-by-custom-identifier/{custom-identifier}, but I'm not strongly against using the request body. My another concern is the verb in the paths is unusual in Mackerel APIs. For example POST /api/v0/graph-defs and DELETE /api/v0/graph-defs (or DELETE /api/v0/graph-defs/{metric-name}) are more aligned to other APIs? I remember some (Google?) API design guides recommend POST /{resource-name}/:{verb}, but consistency in the API set is more important anyway. The POST /api/v0/graph-defs/create is an old(est?) API so you'll need to keep the alias forever (GET /api/v0/hosts.json is still there I believe).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We considered the idea of putting the name parameter in the URL path, but we thought it was unnatural as our internal data model (sorry for the internal talk). We are still puzzled, but we decided to put it in the request body. I hope you will forgive me.

Regarding the second topic, you are absolutely right about the /delete in the URL path. It should be DELETE /api/v0/graph-defs. We will fix it, so please wait.

return err
}
38 changes: 38 additions & 0 deletions graph_defs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,3 +99,41 @@ func TestGraphDefsOmitJSON(t *testing.T) {
t.Errorf("json.Marshal(%#v) = %q; want %q", g, s, want)
}
}

func TestDeleteGraphDef(t *testing.T) {
ts := httptest.NewServer(http.HandlerFunc(func(res http.ResponseWriter, req *http.Request) {
if req.URL.Path != "/api/v0/graph-defs/delete" {
t.Error("request URL should be /api/v0/graph-defs/delete but:", req.URL.Path)
}

if req.Method != "DELETE" {
t.Error("request method should be DELETE but: ", req.Method)
}
body, _ := io.ReadAll(req.Body)

var data struct {
Name string `json:"name"`
}

err := json.Unmarshal(body, &data)
if err != nil {
t.Fatal("request body should be decoded as json", string(body))
}

if data.Name != "mackerel" {
t.Errorf("request sends json including name but: %s", data.Name)
}

respJSON, _ := json.Marshal((map[string]bool{"success": true}))
res.Header()["Content-Type"] = []string{"application/json"}
fmt.Fprint(res, string(respJSON))
}))
defer ts.Close()

client, _ := NewClientWithOptions("dummy-key", ts.URL, false)
err := client.DeleteGraphDef("mackerel")

if err != nil {
t.Error("err should be nil but: ", err)
}
}