From aca5f3485e93273cb437f4f42b278b23ff978001 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 13 Apr 2026 21:26:21 +0000 Subject: [PATCH] Address Copilot review feedback on PR #4 - example/basic userinfo: handle ParsePermission error explicitly with an "unknown" fallback instead of silently treating malformed permission strings as zero. - README permissions snippet: clarify that empty permission strings return (0, nil); only non-numeric input produces a real error. - websocket_test: drop the redundant TestMaxFramePayload_Value (the same constant is already pinned by TestMaxFramePayload_SanityCheck). - permissions.go godoc example: use ParsePermission so the snippet actually compiles against Member.Permissions (which is a string, not a numeric type). - rest_test: rewrite the BulkDeleteMessages / GetMessages / BanMember tests to drive the real RestClient methods through a recording http.RoundTripper, so the tests catch the case where a future refactor accidentally drops the clamping/validation logic. The "exactly at maximum" case now uses 100 IDs and asserts success instead of misleadingly using 101. - example/slash roll: convert the interaction ID's last byte to a decimal digit before applying %max so the result is the actual digit (0-9) rather than the ASCII code (e.g. 55 for '7'). --- README.md | 4 +- example/basic/main.go | 11 ++- example/slash/main.go | 14 +++- permissions.go | 8 +- rest_test.go | 172 ++++++++++++++++++++++++++++++------------ websocket_test.go | 7 -- 6 files changed, 152 insertions(+), 64 deletions(-) diff --git a/README.md b/README.md index 54f8fc4..558365c 100644 --- a/README.md +++ b/README.md @@ -228,9 +228,11 @@ components := []discord.Component{ ```go // Parse the decimal string Discord sends for members and roles. +// An empty member.Permissions returns (0, nil) - only a non-numeric +// or out-of-range string produces a non-nil error. perms, err := discord.ParsePermission(member.Permissions) if err != nil { - // member.Permissions was empty or malformed + // member.Permissions was malformed (not a base-10 uint64). } // Check whether a member has both KickMembers and BanMembers. diff --git a/example/basic/main.go b/example/basic/main.go index 86bcd7f..c3f875b 100644 --- a/example/basic/main.go +++ b/example/basic/main.go @@ -122,7 +122,14 @@ func main() { return } - perms, _ := discord.ParsePermission(member.Permissions) + // member.Permissions can be empty when Discord doesn't include a + // resolved permission bitfield (e.g. /guilds/{id}/members), so + // distinguish the empty case (no error, perms == 0) from the + // malformed case (error, fall back to "unknown"). + adminValue := "unknown" + if perms, perr := discord.ParsePermission(member.Permissions); perr == nil { + adminValue = fmt.Sprintf("%v", perms.IsAdmin()) + } embed := discord.Embed{ Title: member.User.Tag(), @@ -132,7 +139,7 @@ func main() { {Name: "Joined", Value: member.JoinedAt, Inline: true}, {Name: "Roles", Value: fmt.Sprintf("%d roles", len(member.Roles)), Inline: true}, {Name: "Bot", Value: fmt.Sprintf("%v", member.User.Bot), Inline: true}, - {Name: "Admin", Value: fmt.Sprintf("%v", perms.IsAdmin()), Inline: true}, + {Name: "Admin", Value: adminValue, Inline: true}, }, Footer: &discord.EmbedFooter{Text: "GoDiscord basic example"}, } diff --git a/example/slash/main.go b/example/slash/main.go index ddff2c1..b11fe48 100644 --- a/example/slash/main.go +++ b/example/slash/main.go @@ -130,10 +130,16 @@ func handleCommand(b *discord.Bot, i *discord.Interaction) { if max < 1 { max = 1 } - // Derive a deterministic demo result from the interaction ID's last digit. - // A real implementation should use math/rand or crypto/rand. - lastDigit := i.ID[len(i.ID)-1] - result := int64(lastDigit)%max + 1 + // Derive a deterministic demo result from the interaction ID's last + // decimal digit. We subtract '0' so we get 0-9 (the integer value) + // instead of the byte's ASCII code (e.g. '7' == 55), which would + // produce a skewed and unintuitive modulus result. A real + // implementation should use math/rand or crypto/rand. + var lastDigit int64 + if last := i.ID[len(i.ID)-1]; last >= '0' && last <= '9' { + lastDigit = int64(last - '0') + } + result := lastDigit%max + 1 b.Rest.CreateInteractionResponse(i.ID, i.Token, discord.InteractionResponse{ Type: discord.InteractionCallbackTypeChannelMessage, Data: &discord.InteractionResponseData{ diff --git a/permissions.go b/permissions.go index c1bf774..0988940 100644 --- a/permissions.go +++ b/permissions.go @@ -8,8 +8,12 @@ package discord // // Quick usage: // -// // Check whether a member can send messages and embed links. -// perms := discord.Permission(member.Permissions) +// // Discord delivers member permissions as a base-10 string. Use +// // ParsePermission to convert it to a Permission bitfield safely. +// perms, err := discord.ParsePermission(member.Permissions) +// if err != nil { +// // member.Permissions was malformed (not a base-10 uint64). +// } // if perms.Has(discord.PermSendMessages, discord.PermEmbedLinks) { // // ... // } diff --git a/rest_test.go b/rest_test.go index 9f8a60f..69f028b 100644 --- a/rest_test.go +++ b/rest_test.go @@ -1,16 +1,58 @@ package discord import ( + "encoding/json" + "io" + "net/http" + "strings" "testing" ) // --------------------------------------------------------------------------- -// Input validation helpers (BanMember days clamping, BulkDelete count check) +// Test helpers - custom RoundTripper that records the request and returns a +// canned response, so we can drive the real RestClient methods end-to-end +// without touching discord.com. +// --------------------------------------------------------------------------- + +type recordingTransport struct { + lastReq *http.Request + lastBody []byte + respBody string + respCode int +} + +func (t *recordingTransport) RoundTrip(req *http.Request) (*http.Response, error) { + t.lastReq = req + if req.Body != nil { + t.lastBody, _ = io.ReadAll(req.Body) + _ = req.Body.Close() + } + code := t.respCode + if code == 0 { + code = http.StatusOK + } + body := t.respBody + if body == "" { + body = "[]" + } + return &http.Response{ + StatusCode: code, + Body: io.NopCloser(strings.NewReader(body)), + Header: make(http.Header), + Request: req, + }, nil +} + +func newTestRest(rt *recordingTransport) *RestClient { + return &RestClient{token: "test", client: &http.Client{Transport: rt}} +} + +// --------------------------------------------------------------------------- +// Input validation - BulkDeleteMessages // --------------------------------------------------------------------------- // TestBulkDeleteMessages_Validation verifies the in-process guard before any -// HTTP call is made. We call the method with a nil RestClient to confirm the -// validation returns an error rather than panicking. +// HTTP call is made. func TestBulkDeleteMessages_Validation(t *testing.T) { r := &RestClient{token: "x", client: nil} @@ -24,66 +66,100 @@ func TestBulkDeleteMessages_Validation(t *testing.T) { t.Error("BulkDeleteMessages with 0 IDs should return an error") } - // Exactly at maximum — should NOT fail locally (would fail at HTTP layer, - // but we cannot test that without a live server). + // 101 IDs is above the documented Discord maximum (100), so the local + // validator must reject it before any HTTP call is made. ids := make([]string, 101) if err := r.BulkDeleteMessages("chan", ids); err == nil { t.Error("BulkDeleteMessages with 101 IDs should return an error") } -} - -// TestGetMessages_LimitClamping uses a nil HTTP client so the function panics -// only when it tries to actually perform the HTTP call, not during validation. -// We test only that the clamping math is correct by inspecting the formatted path. -func TestGetMessages_LimitClamping(t *testing.T) { - // Verify that a limit of 0 is clamped to 1 and a limit of 200 is clamped - // to 100. We do this by testing the internal clamping logic directly - // (since we can't make a real HTTP call in unit tests). - - clamp := func(limit int) int { - if limit < 1 { - limit = 1 - } - if limit > 100 { - limit = 100 - } - return limit - } - if got := clamp(0); got != 1 { - t.Errorf("clamp(0) = %d, want 1", got) + // Exactly 100 IDs is within the documented Discord maximum. The local + // validator must not reject it; the actual request goes through the + // recording transport so we don't touch discord.com. + rt := &recordingTransport{respCode: http.StatusNoContent} + rOK := newTestRest(rt) + hundred := make([]string, 100) + for i := range hundred { + hundred[i] = "id" } - if got := clamp(-5); got != 1 { - t.Errorf("clamp(-5) = %d, want 1", got) + if err := rOK.BulkDeleteMessages("chan", hundred); err != nil { + t.Errorf("BulkDeleteMessages with 100 IDs returned err: %v", err) } - if got := clamp(200); got != 100 { - t.Errorf("clamp(200) = %d, want 100", got) + if rt.lastReq == nil { + t.Fatal("expected an HTTP request for 100 IDs") } - if got := clamp(50); got != 50 { - t.Errorf("clamp(50) = %d, want 50", got) + if !strings.HasSuffix(rt.lastReq.URL.Path, "/channels/chan/messages/bulk-delete") { + t.Errorf("unexpected request path: %s", rt.lastReq.URL.Path) } } -// TestBanMember_DaysClamping verifies the deleteMessageDays clamping logic. -func TestBanMember_DaysClamping(t *testing.T) { - // Same approach: test the clamping logic in isolation. - clamp := func(days int) int { - if days < 0 { - days = 0 +// --------------------------------------------------------------------------- +// GetMessages clamping - exercises the real method, not a re-implementation +// --------------------------------------------------------------------------- + +func TestGetMessages_LimitClamping(t *testing.T) { + cases := []struct { + input int + want string // expected ?limit= value + }{ + {0, "limit=1"}, + {-5, "limit=1"}, + {50, "limit=50"}, + {100, "limit=100"}, + {200, "limit=100"}, + } + for _, tc := range cases { + rt := &recordingTransport{respBody: "[]"} + r := newTestRest(rt) + if _, err := r.GetMessages("chan", tc.input); err != nil { + t.Errorf("GetMessages(%d) error: %v", tc.input, err) + continue } - if days > 7 { - days = 7 + if rt.lastReq == nil { + t.Errorf("GetMessages(%d): no request observed", tc.input) + continue + } + query := rt.lastReq.URL.RawQuery + if query != tc.want { + t.Errorf("GetMessages(%d) sent query %q, want %q", tc.input, query, tc.want) } - return days } +} - if got := clamp(-1); got != 0 { - t.Errorf("clamp(-1) = %d, want 0", got) - } - if got := clamp(8); got != 7 { - t.Errorf("clamp(8) = %d, want 7", got) +// --------------------------------------------------------------------------- +// BanMember clamping - exercises the real method via the recorded body +// --------------------------------------------------------------------------- + +func TestBanMember_DaysClamping(t *testing.T) { + cases := []struct { + input int + want int + }{ + {-1, 0}, + {0, 0}, + {3, 3}, + {7, 7}, + {8, 7}, + {99, 7}, } - if got := clamp(3); got != 3 { - t.Errorf("clamp(3) = %d, want 3", got) + for _, tc := range cases { + rt := &recordingTransport{respCode: http.StatusNoContent} + r := newTestRest(rt) + if err := r.BanMember("guild", "user", tc.input); err != nil { + t.Errorf("BanMember(%d) error: %v", tc.input, err) + continue + } + if len(rt.lastBody) == 0 { + t.Errorf("BanMember(%d): no body observed", tc.input) + continue + } + var payload map[string]int + if err := json.Unmarshal(rt.lastBody, &payload); err != nil { + t.Errorf("BanMember(%d): body unmarshal: %v (body=%q)", tc.input, err, string(rt.lastBody)) + continue + } + if payload["delete_message_days"] != tc.want { + t.Errorf("BanMember(%d) sent delete_message_days=%d, want %d", tc.input, payload["delete_message_days"], tc.want) + } } } diff --git a/websocket_test.go b/websocket_test.go index 35ff6fd..9f75a5c 100644 --- a/websocket_test.go +++ b/websocket_test.go @@ -45,13 +45,6 @@ func buildFakeFrameHeader(length uint64) []byte { return hdr } -// TestMaxFramePayload_Constant ensures the constant is in fact 64 MiB. -func TestMaxFramePayload_Value(t *testing.T) { - if maxFramePayload != 64*1024*1024 { - t.Errorf("maxFramePayload should be 64 MiB, got %d", maxFramePayload) - } -} - // TestFrameHeaderBuilding verifies that our helper builds headers correctly, // which indirectly tests our understanding of the WebSocket frame format used // in readFrame.