diff --git a/doc/changes/label-update-pagination.md b/doc/changes/label-update-pagination.md new file mode 100644 index 0000000..42d64c4 --- /dev/null +++ b/doc/changes/label-update-pagination.md @@ -0,0 +1,12 @@ +# label +update 分页取值修复 + +修复 `label +update` 在标签数量超过一页时静默覆盖服务端数据的问题。 + +`+update` 需要先取标签当前的 `name`/`description`/`color`（更新接口要求三者同时提交），此前 `fetchLabel` 只对列表接口做单次 `GET`，没有翻页；虽然注释声称“分页匹配 id”，实际当目标标签落在第二页及以后时返回“未找到”。随后 `+update` 用空描述与缺省颜色 `#1E90FF` 回填，PATCH 便把服务端真实的描述与颜色抹掉，造成数据丢失。 + +变更： + +- `fetchLabel` 改为真正翻页（按 `page`/`limit=50` 循环，页内条数少于一页即视为末页），与仓库其他列表接口的翻页方式一致。 +- 只有用户显式传入的字段才会覆盖，未传字段一律保留服务端现值，不再用缺省值静默重置。 +- 全量翻页后仍找不到该 id 时直接返回明确错误，不再带缺省值发起 PATCH。 +- 单元测试新增“目标标签在第二页”用例，断言 PATCH 报文保留原描述与颜色；并补充“标签不存在时报错、不 PATCH”用例。 diff --git a/shortcuts/label/label.go b/shortcuts/label/label.go index 2b6a298..0e8db4e 100644 --- a/shortcuts/label/label.go +++ b/shortcuts/label/label.go @@ -14,6 +14,10 @@ import ( // defaultLabelColor is used when the caller does not provide a color. const defaultLabelColor = "#1E90FF" +// labelListPageSize is the page size fetchLabel requests while paging the list +// endpoint. A short page that returns fewer rows than this marks the last page. +const labelListPageSize = 50 + // hexColorPattern matches #RGB and #RRGGBB hex color values. var hexColorPattern = regexp.MustCompile(`^#(?:[0-9a-fA-F]{3}|[0-9a-fA-F]{6})$`) @@ -142,7 +146,7 @@ func runUpdate(ctx *common.RuntimeContext) error { if name == "" { return fmt.Errorf("could not resolve label name for id %s; pass --name explicitly", id) } - color := firstNonEmpty(ctx.Arg("color"), stringFromMap(current, "color"), defaultLabelColor) + color := firstNonEmpty(ctx.Arg("color"), stringFromMap(current, "color")) if err := validateColor(color); err != nil { return err } @@ -164,32 +168,41 @@ func runUpdate(ctx *common.RuntimeContext) error { } // fetchLabel looks up a single label by id from the list endpoint. GitLink does -// not expose a single-label GET, so we page through the list and match by id. -// A nil result (label not found) is not an error: the caller falls back to the -// flags it was given. +// not expose a single-label GET, so we page through the list and match by id. A +// label beyond the first page must still be found, otherwise update would PATCH +// the server's real name/description/color away with defaults, so an id that is +// absent after the whole list is exhausted is reported as an error. func fetchLabel(ctx *common.RuntimeContext, id string) (map[string]interface{}, error) { - env, err := ctx.CallAPI("GET", labelPath(ctx), nil) - if err != nil { - return nil, err - } - data, ok := env.Data.(map[string]interface{}) - if !ok { - return nil, nil - } - rawTags, ok := data["issue_tags"].([]interface{}) - if !ok { - return nil, nil - } - for _, raw := range rawTags { - tag, ok := raw.(map[string]interface{}) + for page := 1; ; page++ { + q := url.Values{} + q.Set("page", strconv.Itoa(page)) + q.Set("limit", strconv.Itoa(labelListPageSize)) + env, err := ctx.CallAPIWithQuery("GET", labelPath(ctx), q) + if err != nil { + return nil, err + } + data, ok := env.Data.(map[string]interface{}) if !ok { - continue + break } - if labelIDString(tag["id"]) == id { - return tag, nil + rawTags, ok := data["issue_tags"].([]interface{}) + if !ok { + break + } + for _, raw := range rawTags { + tag, ok := raw.(map[string]interface{}) + if !ok { + continue + } + if labelIDString(tag["id"]) == id { + return tag, nil + } + } + if len(rawTags) < labelListPageSize { + break } } - return nil, nil + return nil, fmt.Errorf("label id %s not found in this repository's issue labels", id) } func labelPath(ctx *common.RuntimeContext) string { diff --git a/shortcuts/label/label_test.go b/shortcuts/label/label_test.go index 66bd5dc..ff1141e 100644 --- a/shortcuts/label/label_test.go +++ b/shortcuts/label/label_test.go @@ -2,6 +2,7 @@ package label import ( "encoding/json" + "fmt" "net/http" "net/http/httptest" "testing" @@ -124,6 +125,92 @@ func TestLabelUpdatePreservesCurrentFields(t *testing.T) { assertEqual(t, payload["color"], "#00FF00") } +func TestLabelUpdatePreservesFieldsWhenLabelOnSecondPage(t *testing.T) { + var payload map[string]interface{} + var pagesFetched []string + server := newLabelTestServer(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == "GET" && r.URL.Path == "/v1/owner/repo/issue_tags.json": + page := r.URL.Query().Get("page") + pagesFetched = append(pagesFetched, page) + switch page { + case "1": + writeJSON(t, w, map[string]interface{}{ + "total_count": labelListPageSize + 1, + "issue_tags": fillerLabels(labelListPageSize), + }) + case "2": + writeJSON(t, w, map[string]interface{}{ + "total_count": labelListPageSize + 1, + "issue_tags": []interface{}{ + map[string]interface{}{ + "id": float64(7), + "name": "bug", + "description": "old description", + "color": "#FF0000", + }, + }, + }) + default: + t.Fatalf("unexpected page %q", page) + } + case r.Method == "PATCH" && r.URL.Path == "/v1/owner/repo/issue_tags/7.json": + payload = decodeJSON(t, r) + writeJSON(t, w, map[string]interface{}{"status": 0, "message": "success"}) + default: + t.Fatalf("unexpected request: %s %s", r.Method, r.URL.Path) + } + }) + defer server.Close() + + err := runLabelShortcut(t, server, "update", map[string]string{ + "id": "7", + "name": "renamed", + }) + if err != nil { + t.Fatalf("update shortcut failed: %v", err) + } + + if len(pagesFetched) != 2 || pagesFetched[0] != "1" || pagesFetched[1] != "2" { + t.Fatalf("expected pages [1 2] to be fetched, got %v", pagesFetched) + } + // The label lives on page 2; only --name was passed, so the description and + // color the server already holds must survive the PATCH untouched. + assertEqual(t, payload["name"], "renamed") + assertEqual(t, payload["description"], "old description") + assertEqual(t, payload["color"], "#FF0000") +} + +func TestLabelUpdateErrorsWhenLabelNotFound(t *testing.T) { + server := newLabelTestServer(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == "GET" && r.URL.Path == "/v1/owner/repo/issue_tags.json": + writeJSON(t, w, map[string]interface{}{ + "total_count": 1, + "issue_tags": []interface{}{ + map[string]interface{}{ + "id": float64(3), + "name": "docs", + "description": "documentation", + "color": "#00FF00", + }, + }, + }) + default: + t.Fatalf("missing label must not PATCH, got: %s %s", r.Method, r.URL.Path) + } + }) + defer server.Close() + + err := runLabelShortcut(t, server, "update", map[string]string{ + "id": "7", + "color": "#123456", + }) + if err == nil { + t.Fatal("expected update of a missing label id to return an error") + } +} + func TestLabelUpdateRequiresAtLeastOneField(t *testing.T) { server := newLabelTestServer(t, func(w http.ResponseWriter, r *http.Request) { t.Fatalf("update with no fields should not call API, got: %s %s", r.Method, r.URL.Path) @@ -205,6 +292,21 @@ func newLabelTestServer(t *testing.T, handler http.HandlerFunc) *httptest.Server return httptest.NewServer(handler) } +// fillerLabels builds n distinct labels whose ids never collide with the target +// ids used in the update tests, so a full page forces fetchLabel onto the next. +func fillerLabels(n int) []interface{} { + labels := make([]interface{}, 0, n) + for i := 0; i < n; i++ { + labels = append(labels, map[string]interface{}{ + "id": float64(1000 + i), + "name": fmt.Sprintf("filler-%d", i), + "description": "filler", + "color": "#123456", + }) + } + return labels +} + func assertRequest(t *testing.T, r *http.Request, method, path string) { t.Helper() if r.Method != method || r.URL.Path != path {