diff --git a/internal/format/json_dtos.go b/internal/format/json_dtos.go index 288c60c..e1f0772 100644 --- a/internal/format/json_dtos.go +++ b/internal/format/json_dtos.go @@ -271,6 +271,20 @@ func IssueToCompactDTO(issue *core.Issue) IssueCompactDTO { } // populateIssueBase populates the shared base fields from a core.Issue. +// +// Empty collections are rendered as `[]`, never `null` and never an omitted key. +// Why: a consumer reading `.labels` gets an iterable array in every case, instead +// of having to handle missing-key / null / list as three separate shapes. A +// collection rendered as `null` is ambiguous between "this issue has none" and +// "this renderer does not report them" — that ambiguity is exactly what made a +// label-less `issues create --output json` response indistinguishable from a +// broken one. +// +// Delegate deliberately differs: it keeps `omitempty` and disappears when nil. +// That is correct and must not be "harmonised" with the collections. An absent +// scalar/object field unambiguously means "no delegate", whereas an absent or +// null *collection* is ambiguous. Flipping either one to match the other +// reintroduces the confusion. func populateIssueBase(issue *core.Issue) issueBaseFields { base := issueBaseFields{ Identifier: issue.Identifier, @@ -312,13 +326,13 @@ func populateIssueBase(issue *core.Issue) issueBaseFields { } } - if issue.Labels != nil && len(issue.Labels.Nodes) > 0 { - base.Labels = make([]LabelDTO, len(issue.Labels.Nodes)) - for i, label := range issue.Labels.Nodes { - base.Labels[i] = LabelDTO{ + base.Labels = []LabelDTO{} + if issue.Labels != nil { + for _, label := range issue.Labels.Nodes { + base.Labels = append(base.Labels, LabelDTO{ ID: label.ID, Name: label.Name, - } + }) } } @@ -345,21 +359,19 @@ func populateIssueBase(issue *core.Issue) issueBaseFields { } } - if issue.Children.Nodes != nil && len(issue.Children.Nodes) > 0 { - base.Children = make([]IssueRefDTO, len(issue.Children.Nodes)) - for i, child := range issue.Children.Nodes { - base.Children[i] = IssueRefDTO{ - Identifier: child.Identifier, - Title: child.Title, - State: child.State.Name, - } - } + base.Children = []IssueRefDTO{} + for _, child := range issue.Children.Nodes { + base.Children = append(base.Children, IssueRefDTO{ + Identifier: child.Identifier, + Title: child.Title, + State: child.State.Name, + }) } - if issue.Attachments != nil && len(issue.Attachments.Nodes) > 0 { - base.Attachments = make([]AttachmentDTO, len(issue.Attachments.Nodes)) - for i, att := range issue.Attachments.Nodes { - base.Attachments[i] = AttachmentToDTO(&att) + base.Attachments = []AttachmentDTO{} + if issue.Attachments != nil { + for _, att := range issue.Attachments.Nodes { + base.Attachments = append(base.Attachments, AttachmentToDTO(&att)) } } @@ -370,10 +382,13 @@ func populateIssueBase(issue *core.Issue) issueBaseFields { func IssueToFullDTO(issue *core.Issue) IssueFullDTO { dto := IssueFullDTO{issueBaseFields: populateIssueBase(issue)} - if issue.Comments != nil && len(issue.Comments.Nodes) > 0 { - dto.Comments = make([]CommentDTO, len(issue.Comments.Nodes)) - for i, comment := range issue.Comments.Nodes { - dto.Comments[i] = CommentDTO{ + // Empty renders as [], for the reasons documented on populateIssueBase. + // Emitting `"labels": []` next to `"comments": null` in the same object would + // reproduce the very ambiguity that fix removes. + dto.Comments = []CommentDTO{} + if issue.Comments != nil { + for _, comment := range issue.Comments.Nodes { + dto.Comments = append(dto.Comments, CommentDTO{ ID: comment.ID, Body: comment.Body, User: &UserDTO{ @@ -381,7 +396,7 @@ func IssueToFullDTO(issue *core.Issue) IssueFullDTO { Name: comment.User.Name, }, CreatedAt: comment.CreatedAt, - } + }) } } @@ -392,18 +407,19 @@ func IssueToFullDTO(issue *core.Issue) IssueFullDTO { func IssueToDetailedDTO(issue *core.Issue) IssueDetailedDTO { dto := IssueDetailedDTO{issueBaseFields: populateIssueBase(issue)} - if issue.Comments != nil && len(issue.Comments.Nodes) > 0 { - dto.Comments = make([]CommentSummaryDTO, len(issue.Comments.Nodes)) - for i, comment := range issue.Comments.Nodes { - dto.Comments[i] = CommentSummaryDTO{ - ID: comment.ID, - Body: truncate(cleanDescription(comment.Body), 200), + // Empty renders as [], matching IssueToFullDTO and populateIssueBase. + dto.Comments = []CommentSummaryDTO{} + if issue.Comments != nil { + for _, comment := range issue.Comments.Nodes { + dto.Comments = append(dto.Comments, CommentSummaryDTO{ + ID: comment.ID, + Body: truncate(cleanDescription(comment.Body), 200), User: &UserDTO{ ID: comment.User.ID, Name: comment.User.Name, }, CreatedAt: comment.CreatedAt, - } + }) } } diff --git a/internal/format/json_dtos_test.go b/internal/format/json_dtos_test.go new file mode 100644 index 0000000..de8d2b2 --- /dev/null +++ b/internal/format/json_dtos_test.go @@ -0,0 +1,182 @@ +package format + +import ( + "encoding/json" + "strings" + "testing" + + "github.com/joa23/linear-cli/pkg/linear/core" +) + +// emptyCollectionKeys are the JSON keys that must render as `[]` rather than +// `null` when the issue carries none of that collection. +// +// comments is included even though create never requests it, so a created issue +// reports "comments": [] for a field the server was never asked about. That is +// the accepted cost of a uniform contract: the DTO describes the shape it +// renders, not the wire response it was built from. +var emptyCollectionKeys = []string{"labels", "children", "attachments", "comments"} + +// minimalIssue is an issue with every collection left unset — the shape a freshly +// created, label-less issue arrives in. +func minimalIssue() *core.Issue { + return &core.Issue{ + ID: "issue-uuid", + Identifier: "TL-1", + Title: "Test issue", + URL: "https://linear.app/team/issue/TL-1", + } +} + +// marshalJSON renders a DTO and returns the raw string, so tests can assert on +// the wire shape rather than on the Go value. +func marshalJSON(t *testing.T, v interface{}) string { + t.Helper() + + out, err := json.Marshal(v) + if err != nil { + t.Fatalf("failed to marshal DTO: %v", err) + } + return string(out) +} + +func TestIssueToFullDTO_EmptyCollectionsRenderAsArrays(t *testing.T) { + got := marshalJSON(t, IssueToFullDTO(minimalIssue())) + + for _, key := range emptyCollectionKeys { + t.Run(key, func(t *testing.T) { + if !strings.Contains(got, `"`+key+`":[]`) { + t.Errorf("expected %q to render as an empty array\ngot: %s", key, got) + } + if strings.Contains(got, `"`+key+`":null`) { + // A null collection is ambiguous between "none" and "not reported", + // which is the defect TL-572 fixed. + t.Errorf("%q rendered as null\ngot: %s", key, got) + } + }) + } +} + +func TestIssueToDetailedDTO_EmptyCollectionsRenderAsArrays(t *testing.T) { + // IssueDetailedDTO shares populateIssueBase, so it must behave identically. + got := marshalJSON(t, IssueToDetailedDTO(minimalIssue())) + + for _, key := range emptyCollectionKeys { + t.Run(key, func(t *testing.T) { + if !strings.Contains(got, `"`+key+`":[]`) { + t.Errorf("expected %q to render as an empty array\ngot: %s", key, got) + } + if strings.Contains(got, `"`+key+`":null`) { + t.Errorf("%q rendered as null\ngot: %s", key, got) + } + }) + } +} + +func TestIssueToFullDTO_NilAndEmptyLabelConnectionsBothRenderAsArray(t *testing.T) { + // populateIssueBase collapses "no connection" and "connection with no nodes" + // into one branch; both must reach the same wire shape. + tests := []struct { + name string + setup func(*core.Issue) + }{ + {"nil connection", func(i *core.Issue) { i.Labels = nil }}, + {"empty nodes", func(i *core.Issue) { i.Labels = &core.LabelConnection{Nodes: []core.Label{}} }}, + {"nil nodes", func(i *core.Issue) { i.Labels = &core.LabelConnection{} }}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + issue := minimalIssue() + tt.setup(issue) + + got := marshalJSON(t, IssueToFullDTO(issue)) + if !strings.Contains(got, `"labels":[]`) { + t.Errorf("expected labels to render as [], got: %s", got) + } + }) + } +} + +func TestIssueToFullDTO_LabelsReportIDAndNameOnly(t *testing.T) { + issue := minimalIssue() + issue.Labels = &core.LabelConnection{ + Nodes: []core.Label{ + {ID: "label-1", Name: "Bugfix", Color: "#eb5757"}, + { + ID: "label-2", + Name: "iOS", + Color: "#0f7488", + Parent: &core.LabelRef{ID: "label-parent", Name: "Platform"}, + }, + }, + } + + dto := IssueToFullDTO(issue) + + // Order is legitimate to assert here: this test supplies the core.Issue, and + // the DTO loop preserves slice order. + if len(dto.Labels) != 2 { + t.Fatalf("expected 2 labels, got %d", len(dto.Labels)) + } + if dto.Labels[0].ID != "label-1" || dto.Labels[0].Name != "Bugfix" { + t.Errorf("labels[0] = {%s %s}, want {label-1 Bugfix}", dto.Labels[0].ID, dto.Labels[0].Name) + } + if dto.Labels[1].ID != "label-2" || dto.Labels[1].Name != "iOS" { + t.Errorf("labels[1] = {%s %s}, want {label-2 iOS}", dto.Labels[1].ID, dto.Labels[1].Name) + } + + // LabelDTO deliberately bounds the output surface: color and parent are + // selected from the API but must not reach the rendered JSON. + // + // Asserted against the labels elements' own key sets rather than by scanning + // the whole document, so the test keeps guarding what it means to guard if + // minimalIssue() ever gains a field whose text contains "color" or "Platform". + got := marshalJSON(t, dto) + for i, keys := range unmarshalKeySets(t, got, "labels") { + for key := range keys { + if key != "id" && key != "name" { + t.Errorf("labels[%d] leaked %q — LabelDTO should expose only id and name\ngot: %s", i, key, got) + } + } + } +} + +func TestIssueToFullDTO_NilDelegateOmitsTheKey(t *testing.T) { + // Guards the deliberate asymmetry documented on populateIssueBase against a + // later "cleanup" that harmonises Delegate with the collections. + got := marshalJSON(t, IssueToFullDTO(minimalIssue())) + + // Checked against the top-level key set, not the raw document, so unrelated + // content that happens to spell "delegate" cannot fail this test. + var top map[string]json.RawMessage + if err := json.Unmarshal([]byte(got), &top); err != nil { + t.Fatalf("DTO output is not a JSON object: %v\n%s", err, got) + } + if _, present := top["delegate"]; present { + t.Errorf("expected the delegate key to be omitted entirely for a nil delegate\ngot: %s", got) + } +} + +// unmarshalKeySets returns the key set of every element of the named array field, +// so tests can assert on a field's actual shape instead of substring-scanning the +// whole marshalled document. +func unmarshalKeySets(t *testing.T, doc, field string) []map[string]json.RawMessage { + t.Helper() + + var top map[string]json.RawMessage + if err := json.Unmarshal([]byte(doc), &top); err != nil { + t.Fatalf("DTO output is not a JSON object: %v\n%s", err, doc) + } + + raw, present := top[field] + if !present { + t.Fatalf("DTO output has no %q key\n%s", field, doc) + } + + var elements []map[string]json.RawMessage + if err := json.Unmarshal(raw, &elements); err != nil { + t.Fatalf("%q is not an array of objects: %v\n%s", field, err, doc) + } + return elements +} diff --git a/internal/service/issue_create_test.go b/internal/service/issue_create_test.go index c538e8c..5e7dd06 100644 --- a/internal/service/issue_create_test.go +++ b/internal/service/issue_create_test.go @@ -3,6 +3,9 @@ package service import ( "encoding/json" "fmt" + "net/http" + "net/http/httptest" + "regexp" "strings" "testing" @@ -28,11 +31,23 @@ type mockIssueClientForCreate struct { // Configured return values createResult *core.Issue createErr error + + // resolveLabelErr, when set, makes label resolution fail. Its zero value + // preserves the default resolver behaviour so existing tests are unaffected. + resolveLabelErr error + + // realCreateClient, when set, makes CreateIssue delegate to a real issues + // client instead of returning createResult. That lets one test exercise the + // genuine GraphQL mutation and deserializer against a fake server. + realCreateClient *issues.Client } func (m *mockIssueClientForCreate) CreateIssue(input *core.IssueCreateInput) (*core.Issue, error) { m.createCalled = true m.lastCreateInput = input + if m.realCreateClient != nil { + return m.realCreateClient.CreateIssue(input) + } return m.createResult, m.createErr } @@ -52,6 +67,9 @@ func (m *mockIssueClientForCreate) ResolveCycleIdentifier(num, team string) (str return "cycle-uuid", nil } func (m *mockIssueClientForCreate) ResolveLabelIdentifier(label, team string) (string, error) { + if m.resolveLabelErr != nil { + return "", m.resolveLabelErr + } return "label-uuid-" + label, nil } @@ -265,3 +283,359 @@ func TestIssueService_Create_ReportsIdentifierNotDescription(t *testing.T) { } }) } + +// Create's JSON output must report the labels that were applied. It previously +// reported `"labels": null` whatever was passed, because the issueCreate mutation +// never asked for them back — so a scripted caller confirming label application +// from the create response got a false negative and had to pay an extra +// `issues get` round-trip. +// +// These tests exercise the DTO/formatter path only: the mock hands back a +// core.Issue the test built itself, so they can never prove the GraphQL selection +// set is correct. That guard lives in pkg/linear/issues/client_create_test.go. +// Both layers are required; neither covers this on its own. +func TestIssueService_Create_ReportsAppliedLabels(t *testing.T) { + const identifier = "ABC-123" + const issueURL = "https://linear.app/acme/issue/ABC-123" + + // createdIssueWithLabels mirrors what the fixed mutation now returns. The + // mock's default createResult carries no labels at all, so a test copied from + // the surrounding pattern would pass vacuously without this. + createdIssueWithLabels := func(labels ...core.Label) *core.Issue { + issue := &core.Issue{ + ID: "issue-uuid", + Identifier: identifier, + Title: "Fix the thing", + URL: issueURL, + } + if labels != nil { + issue.Labels = &core.LabelConnection{Nodes: labels} + } + return issue + } + + // labelsFrom unmarshals just the labels array out of create's JSON output. + labelsFrom := func(t *testing.T, out string) []struct { + ID string `json:"id"` + Name string `json:"name"` + } { + t.Helper() + var got struct { + Labels []struct { + ID string `json:"id"` + Name string `json:"name"` + } `json:"labels"` + } + if err := json.Unmarshal([]byte(out), &got); err != nil { + t.Fatalf("create --output json is not valid JSON: %v\n%s", err, out) + } + return got.Labels + } + + t.Run("single label is reported, not null", func(t *testing.T) { + mock := &mockIssueClientForCreate{ + createResult: createdIssueWithLabels(core.Label{ID: "label-1", Name: "Bugfix"}), + } + svc := makeIssueServiceForCreate(mock) + + out, err := svc.Create(&CreateIssueInput{ + Title: "Fix the thing", + TeamID: "ABC", + LabelIDs: []string{"Bugfix"}, + }, format.OutputJSON) + if err != nil { + t.Fatalf("Create: %v", err) + } + + labels := labelsFrom(t, out) + if len(labels) != 1 { + t.Fatalf("expected 1 label, got %d\n%s", len(labels), out) + } + if labels[0].ID != "label-1" || labels[0].Name != "Bugfix" { + t.Errorf("labels[0] = {%s %s}, want {label-1 Bugfix}", labels[0].ID, labels[0].Name) + } + }) + + t.Run("multiple labels are all reported", func(t *testing.T) { + mock := &mockIssueClientForCreate{ + createResult: createdIssueWithLabels( + core.Label{ID: "label-1", Name: "Bugfix"}, + core.Label{ID: "label-2", Name: "Feature"}, + ), + } + svc := makeIssueServiceForCreate(mock) + + out, err := svc.Create(&CreateIssueInput{ + Title: "Fix the thing", + TeamID: "ABC", + LabelIDs: []string{"Bugfix", "Feature"}, + }, format.OutputJSON) + if err != nil { + t.Fatalf("Create: %v", err) + } + + // Order is legitimate to assert here because this test supplies the + // core.Issue itself and the DTO loop preserves slice order. Live responses + // are not documented to preserve labelIds order, so the manual + // create-vs-get parity check compares them as a set. + labels := labelsFrom(t, out) + if len(labels) != 2 { + t.Fatalf("expected 2 labels, got %d\n%s", len(labels), out) + } + if labels[0].Name != "Bugfix" || labels[1].Name != "Feature" { + t.Errorf("labels = [%s %s], want [Bugfix Feature]", labels[0].Name, labels[1].Name) + } + }) + + t.Run("no labels renders an empty array, never null", func(t *testing.T) { + mock := &mockIssueClientForCreate{createResult: createdIssueWithLabels()} + svc := makeIssueServiceForCreate(mock) + + out, err := svc.Create(&CreateIssueInput{Title: "Fix the thing", TeamID: "ABC"}, format.OutputJSON) + if err != nil { + t.Fatalf("Create: %v", err) + } + + // The service path pretty-prints, so match the key tolerantly rather than + // pinning the exact spacing. + if !regexp.MustCompile(`"labels":\s*\[\s*\]`).MatchString(out) { + t.Errorf("expected labels to render as [], got:\n%s", out) + } + if regexp.MustCompile(`"labels":\s*null`).MatchString(out) { + t.Errorf("labels rendered as null — indistinguishable from 'not reported':\n%s", out) + } + }) + + t.Run("text output is unchanged by the labels fix", func(t *testing.T) { + // Text mode renders via formatter.IssueCreated, a path the DTO change does + // not touch at all. + mock := &mockIssueClientForCreate{ + createResult: createdIssueWithLabels(core.Label{ID: "label-1", Name: "Bugfix"}), + } + svc := makeIssueServiceForCreate(mock) + + out, err := svc.Create(&CreateIssueInput{ + Title: "Fix the thing", + TeamID: "ABC", + LabelIDs: []string{"Bugfix"}, + }, format.OutputText) + if err != nil { + t.Fatalf("Create: %v", err) + } + + firstLine, _, _ := strings.Cut(out, "\n") + if firstLine != identifier+": Fix the thing" { + t.Errorf("first line = %q, want %q", firstLine, identifier+": Fix the thing") + } + if !strings.Contains(out, issueURL) { + t.Errorf("text output did not report the issue URL:\n%s", out) + } + if strings.Contains(out, "Bugfix") || strings.Contains(out, "label") { + t.Errorf("text output should not mention labels:\n%s", out) + } + }) + + t.Run("fields create already reported correctly still survive", func(t *testing.T) { + issue := createdIssueWithLabels(core.Label{ID: "label-1", Name: "Bugfix"}) + issue.State.ID = "state-uuid" + issue.State.Name = "Todo" + issue.Assignee = &core.User{ID: "user-1", Name: "Ada", Email: "ada@example.com"} + issue.Creator = &core.User{ID: "user-2", Name: "Grace", Email: "grace@example.com"} + issue.Project = &core.Project{ID: "project-uuid", Name: "Platform"} + issue.Parent = &core.ParentIssue{ID: "parent-uuid", Identifier: "ABC-100", Title: "Parent issue"} + issue.CreatedAt = "2026-03-01T10:00:00.000Z" + issue.UpdatedAt = "2026-03-01T11:00:00.000Z" + + mock := &mockIssueClientForCreate{createResult: issue} + svc := makeIssueServiceForCreate(mock) + + out, err := svc.Create(&CreateIssueInput{ + Title: "Fix the thing", + TeamID: "ABC", + LabelIDs: []string{"Bugfix"}, + }, format.OutputJSON) + if err != nil { + t.Fatalf("Create: %v", err) + } + + var got struct { + Identifier string `json:"identifier"` + Title string `json:"title"` + URL string `json:"url"` + CreatedAt string `json:"createdAt"` + UpdatedAt string `json:"updatedAt"` + State *struct{ Name string } `json:"state"` + Assignee *struct{ Name string } `json:"assignee"` + Creator *struct{ Name string } `json:"creator"` + Project *struct{ Name string } `json:"project"` + Parent *struct { + Identifier string `json:"identifier"` + } `json:"parent"` + } + if err := json.Unmarshal([]byte(out), &got); err != nil { + t.Fatalf("create --output json is not valid JSON: %v\n%s", err, out) + } + + if got.Identifier != identifier || got.Title != "Fix the thing" || got.URL != issueURL { + t.Errorf("identifier/title/url = %q/%q/%q", got.Identifier, got.Title, got.URL) + } + if got.CreatedAt == "" || got.UpdatedAt == "" { + t.Errorf("timestamps missing: createdAt=%q updatedAt=%q", got.CreatedAt, got.UpdatedAt) + } + if got.State == nil || got.State.Name != "Todo" { + t.Errorf("state = %+v, want Todo", got.State) + } + if got.Assignee == nil || got.Assignee.Name != "Ada" { + t.Errorf("assignee = %+v, want Ada", got.Assignee) + } + if got.Creator == nil || got.Creator.Name != "Grace" { + t.Errorf("creator = %+v, want Grace", got.Creator) + } + if got.Project == nil || got.Project.Name != "Platform" { + t.Errorf("project = %+v, want Platform", got.Project) + } + if got.Parent == nil || got.Parent.Identifier != "ABC-100" { + t.Errorf("parent = %+v, want ABC-100", got.Parent) + } + }) +} + +// A label name that cannot be resolved must fail before the mutation runs, so no +// issue is created with the wrong labels and no JSON is emitted for it. +func TestIssueService_Create_UnresolvableLabelFailsBeforeMutation(t *testing.T) { + mock := &mockIssueClientForCreate{ + resolveLabelErr: fmt.Errorf("label not found"), + createResult: &core.Issue{ID: "issue-uuid", Identifier: "ABC-123", Title: "Fix the thing"}, + } + svc := makeIssueServiceForCreate(mock) + + out, err := svc.Create(&CreateIssueInput{ + Title: "Fix the thing", + TeamID: "ABC", + LabelIDs: []string{"definitely-not-a-label"}, + }, format.OutputJSON) + + if err == nil { + t.Fatal("Create() should have failed on an unresolvable label") + } + if !strings.Contains(err.Error(), "definitely-not-a-label") { + t.Errorf("error should name the offending label, got: %v", err) + } + if mock.createCalled { + t.Error("CreateIssue was called despite label resolution failing — orphaned issue risk") + } + if out != "" { + t.Errorf("no JSON should be emitted on failure, got: %s", out) + } +} + +// A failed mutation must surface as an error, never as a partially-populated +// JSON issue a caller could mistake for a successful create. +func TestIssueService_Create_MutationFailureEmitsNoJSON(t *testing.T) { + mock := &mockIssueClientForCreate{ + createErr: fmt.Errorf("issue creation was not successful"), + } + svc := makeIssueServiceForCreate(mock) + + out, err := svc.Create(&CreateIssueInput{ + Title: "Fix the thing", + TeamID: "ABC", + LabelIDs: []string{"Bugfix"}, + }, format.OutputJSON) + + if err == nil { + t.Fatal("Create() should have returned an error") + } + if out != "" { + t.Errorf("no JSON should be emitted on failure, got: %s", out) + } +} + +// TestIssueService_Create_EndToEndReportsLabels drives Create through a real +// issues.Client pointed at a fake Linear server, so the assertion covers the +// whole chain: the GraphQL selection set, the response deserializer, the DTO +// layer, and the formatter. +// +// The mock-based tests above cannot do this — they hand back a core.Issue the +// test built itself, so they would stay green if the mutation stopped asking for +// labels. This one fails if either layer regresses, but only because its handler +// asserts on the captured mutation: the canned response below carries labels +// unconditionally, so without that assertion the selection set would go +// uncovered here. The per-field guard lives in +// TestCreateIssue_MutationRequestsCallerSettableFields +// (pkg/linear/issues/client_create_test.go). +// +// This test does not replace the mocks either: their injectable failures are +// what cover the error paths. +func TestIssueService_Create_EndToEndReportsLabels(t *testing.T) { + const response = `{ + "data": { + "issueCreate": { + "success": true, + "issue": { + "id": "issue-uuid", + "identifier": "ABC-123", + "title": "Fix the thing", + "url": "https://linear.app/acme/issue/ABC-123", + "labels": { + "nodes": [ + {"id": "label-1", "name": "Bugfix", "color": "#eb5757"} + ] + } + } + } + } + }` + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // The canned response carries labels whatever the mutation asked for, so + // the selection set is only covered if the request is actually inspected. + // Brace-anchored: a bare "labels" would also match labelIds in the input. + var payload struct { + Query string `json:"query"` + } + if err := json.NewDecoder(r.Body).Decode(&payload); err != nil { + t.Errorf("outgoing GraphQL request is not valid JSON: %v", err) + } else if !regexp.MustCompile(`(?m)^\s*labels\s*\{\s*$`).MatchString(payload.Query) { + t.Errorf("the issueCreate mutation never asks for labels back:\n%s", payload.Query) + } + + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(response)) + })) + defer server.Close() + + realClient := issues.NewClient(core.NewTestBaseClient("test-token", server.URL, server.Client())) + mock := &mockIssueClientForCreate{realCreateClient: realClient} + svc := makeIssueServiceForCreate(mock) + + out, err := svc.Create(&CreateIssueInput{ + Title: "Fix the thing", + TeamID: "ABC", + LabelIDs: []string{"Bugfix"}, + }, format.OutputJSON) + if err != nil { + t.Fatalf("Create: %v", err) + } + + var got struct { + Identifier string `json:"identifier"` + Labels []struct { + ID string `json:"id"` + Name string `json:"name"` + } `json:"labels"` + } + if err := json.Unmarshal([]byte(out), &got); err != nil { + t.Fatalf("create --output json is not valid JSON: %v\n%s", err, out) + } + + if got.Identifier != "ABC-123" { + t.Errorf("identifier = %q, want ABC-123", got.Identifier) + } + if len(got.Labels) != 1 { + t.Fatalf("expected 1 label to survive the full chain, got %d\n%s", len(got.Labels), out) + } + if got.Labels[0].ID != "label-1" || got.Labels[0].Name != "Bugfix" { + t.Errorf("labels[0] = {%s %s}, want {label-1 Bugfix}", got.Labels[0].ID, got.Labels[0].Name) + } +} diff --git a/pkg/linear/issues/client.go b/pkg/linear/issues/client.go index 1631f88..df833d7 100644 --- a/pkg/linear/issues/client.go +++ b/pkg/linear/issues/client.go @@ -73,6 +73,25 @@ linear_create_issue("Task title", "Description", teams[0].id)`) name email } + priority + estimate + dueDate + labels { + nodes { + id + name + color + parent { + id + name + } + } + } + cycle { + id + number + name + } createdAt updatedAt url diff --git a/pkg/linear/issues/client_create_test.go b/pkg/linear/issues/client_create_test.go new file mode 100644 index 0000000..5b1700e --- /dev/null +++ b/pkg/linear/issues/client_create_test.go @@ -0,0 +1,301 @@ +package issues + +import ( + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "regexp" + "testing" + + "github.com/joa23/linear-cli/pkg/linear/core" +) + +// newCreateTestClient wires a Client to an httptest server so tests can inspect +// the outgoing GraphQL mutation and control the response. +// +// Why this exists: the service-layer create tests mock the whole issue client and +// hand back a pre-built core.Issue, so they never see the mutation string at all. +// Deleting a field from the issueCreate selection set would leave every one of +// those tests green while the CLI silently reported `null` again — which is the +// exact defect TL-572 fixed. Only a test that captures the real request body can +// fail on that regression. +func newCreateTestClient(t *testing.T, handler func(w http.ResponseWriter, body []byte)) *Client { + t.Helper() + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, err := io.ReadAll(r.Body) + if err != nil { + t.Errorf("failed to read request body: %v", err) + return + } + w.Header().Set("Content-Type", "application/json") + handler(w, body) + })) + t.Cleanup(server.Close) + + return NewClient(core.NewTestBaseClient("test-token", server.URL, server.Client())) +} + +// graphQLQuery pulls the "query" field out of a captured request body. +func graphQLQuery(t *testing.T, body []byte) string { + t.Helper() + + var payload struct { + Query string `json:"query"` + } + if err := json.Unmarshal(body, &payload); err != nil { + t.Fatalf("request body is not valid JSON: %v", err) + } + return payload.Query +} + +// minimalCreateResponse is a valid issueCreate payload with no optional fields set. +const minimalCreateResponse = `{ + "data": { + "issueCreate": { + "success": true, + "issue": { + "id": "issue-uuid", + "identifier": "TL-1", + "title": "Test issue" + } + } + } +}` + +func TestCreateIssue_MutationRequestsCallerSettableFields(t *testing.T) { + // Each of these fields is something the caller passes *into* create, so each + // carries the "I set it, the response says null, did it work?" failure mode. + // + // The patterns are deliberately anchored to a whole line: a bare "labels" + // substring would also match "labelIds" in the mutation input, so a naive + // check would pass against the unfixed selection set and prove nothing. + tests := []struct { + field string + pattern *regexp.Regexp + }{ + {"priority", regexp.MustCompile(`(?m)^\s*priority\s*$`)}, + {"estimate", regexp.MustCompile(`(?m)^\s*estimate\s*$`)}, + {"dueDate", regexp.MustCompile(`(?m)^\s*dueDate\s*$`)}, + {"labels", regexp.MustCompile(`(?m)^\s*labels\s*\{\s*$`)}, + {"cycle", regexp.MustCompile(`(?m)^\s*cycle\s*\{\s*$`)}, + } + + var capturedQuery string + client := newCreateTestClient(t, func(w http.ResponseWriter, body []byte) { + capturedQuery = graphQLQuery(t, body) + _, _ = w.Write([]byte(minimalCreateResponse)) + }) + + _, err := client.CreateIssue(&core.IssueCreateInput{ + Title: "Test issue", + TeamID: "team-uuid", + }) + if err != nil { + t.Fatalf("CreateIssue() returned unexpected error: %v", err) + } + + for _, tt := range tests { + t.Run(tt.field, func(t *testing.T) { + if !tt.pattern.MatchString(capturedQuery) { + t.Errorf("issueCreate selection set does not request %q\nmutation was:\n%s", tt.field, capturedQuery) + } + }) + } + + // The label sub-selection must match the read paths verbatim, so create and + // get deserialize the same shape. + for _, sub := range []string{"color", "parent"} { + if !regexp.MustCompile(`(?m)^\s*` + sub + `[\s{]`).MatchString(capturedQuery) { + t.Errorf("label sub-selection does not request %q\nmutation was:\n%s", sub, capturedQuery) + } + } +} + +func TestCreateIssue_DeserializesLabels(t *testing.T) { + // Two labels, one carrying a parent — asserting id, name, color AND parent + // proves the deserializer handles the full selected shape, even though + // LabelDTO later drops color/parent from the rendered JSON. + const response = `{ + "data": { + "issueCreate": { + "success": true, + "issue": { + "id": "issue-uuid", + "identifier": "TL-1", + "title": "Test issue", + "labels": { + "nodes": [ + {"id": "label-1", "name": "Bugfix", "color": "#eb5757"}, + {"id": "label-2", "name": "iOS", "color": "#0f7488", + "parent": {"id": "label-parent", "name": "Platform"}} + ] + } + } + } + } + }` + + client := newCreateTestClient(t, func(w http.ResponseWriter, body []byte) { + _, _ = w.Write([]byte(response)) + }) + + issue, err := client.CreateIssue(&core.IssueCreateInput{ + Title: "Test issue", + TeamID: "team-uuid", + LabelIDs: []string{"label-1", "label-2"}, + }) + if err != nil { + t.Fatalf("CreateIssue() returned unexpected error: %v", err) + } + + if issue.Labels == nil { + t.Fatal("issue.Labels is nil — the create response's labels were not deserialized") + } + if got := len(issue.Labels.Nodes); got != 2 { + t.Fatalf("expected 2 labels, got %d", got) + } + + // Order is asserted only against this test's own canned response. Linear does + // not document that live responses preserve labelIds order, so the live + // verification compares labels as a set instead. + first := issue.Labels.Nodes[0] + if first.ID != "label-1" || first.Name != "Bugfix" || first.Color != "#eb5757" { + t.Errorf("first label = {%s %s %s}, want {label-1 Bugfix #eb5757}", first.ID, first.Name, first.Color) + } + if first.Parent != nil { + t.Errorf("first label should have no parent, got %+v", first.Parent) + } + + second := issue.Labels.Nodes[1] + if second.ID != "label-2" || second.Name != "iOS" || second.Color != "#0f7488" { + t.Errorf("second label = {%s %s %s}, want {label-2 iOS #0f7488}", second.ID, second.Name, second.Color) + } + if second.Parent == nil { + t.Fatal("second label's parent was not deserialized") + } + if second.Parent.ID != "label-parent" || second.Parent.Name != "Platform" { + t.Errorf("second label parent = {%s %s}, want {label-parent Platform}", second.Parent.ID, second.Parent.Name) + } +} + +func TestCreateIssue_DeserializesSiblingFields(t *testing.T) { + const response = `{ + "data": { + "issueCreate": { + "success": true, + "issue": { + "id": "issue-uuid", + "identifier": "TL-1", + "title": "Test issue", + "priority": 2, + "estimate": 3, + "dueDate": "2026-03-01", + "cycle": {"id": "cycle-uuid", "number": 65, "name": "Sprint 65"} + } + } + } + }` + + client := newCreateTestClient(t, func(w http.ResponseWriter, body []byte) { + _, _ = w.Write([]byte(response)) + }) + + issue, err := client.CreateIssue(&core.IssueCreateInput{ + Title: "Test issue", + TeamID: "team-uuid", + }) + if err != nil { + t.Fatalf("CreateIssue() returned unexpected error: %v", err) + } + + if issue.Priority == nil || *issue.Priority != 2 { + t.Errorf("Priority = %v, want 2", issue.Priority) + } + if issue.Estimate == nil || *issue.Estimate != 3 { + t.Errorf("Estimate = %v, want 3", issue.Estimate) + } + if issue.DueDate == nil || *issue.DueDate != "2026-03-01" { + t.Errorf("DueDate = %v, want 2026-03-01", issue.DueDate) + } + if issue.Cycle == nil { + t.Fatal("Cycle was not deserialized") + } + if issue.Cycle.ID != "cycle-uuid" || issue.Cycle.Number != 65 || issue.Cycle.Name != "Sprint 65" { + t.Errorf("Cycle = {%s %d %s}, want {cycle-uuid 65 Sprint 65}", issue.Cycle.ID, issue.Cycle.Number, issue.Cycle.Name) + } +} + +func TestCreateIssue_UnsuccessfulMutationReturnsError(t *testing.T) { + const response = `{ + "data": { + "issueCreate": { + "success": false, + "issue": {"id": "issue-uuid", "identifier": "TL-1", "title": "Test issue"} + } + } + }` + + client := newCreateTestClient(t, func(w http.ResponseWriter, body []byte) { + _, _ = w.Write([]byte(response)) + }) + + issue, err := client.CreateIssue(&core.IssueCreateInput{ + Title: "Test issue", + TeamID: "team-uuid", + }) + if err == nil { + t.Fatal("CreateIssue() returned no error for an unsuccessful mutation") + } + if issue != nil { + // A partially-populated issue would let a caller act on an issue that was + // never created. + t.Errorf("CreateIssue() returned a non-nil issue alongside the error: %+v", issue) + } +} + +func TestCreateIssue_ExtractsMetadataFromDescription(t *testing.T) { + // Guards the post-mutation metadata handling against the selection-set edit: + // the description must still be cleaned and its metadata lifted out. + const response = `{ + "data": { + "issueCreate": { + "success": true, + "issue": { + "id": "issue-uuid", + "identifier": "TL-1", + "title": "Test issue", + "description": "Real body.\n\n
🤖 Metadata\n\n` + "```" + `json\n{\"depends_on\":[\"TL-9\"]}\n` + "```" + `\n\n
" + } + } + } + }` + + client := newCreateTestClient(t, func(w http.ResponseWriter, body []byte) { + _, _ = w.Write([]byte(response)) + }) + + issue, err := client.CreateIssue(&core.IssueCreateInput{ + Title: "Test issue", + TeamID: "team-uuid", + }) + if err != nil { + t.Fatalf("CreateIssue() returned unexpected error: %v", err) + } + + if issue.Description != "Real body." { + t.Errorf("Description = %q, want %q (metadata section not stripped)", issue.Description, "Real body.") + } + if issue.Metadata == nil { + t.Fatal("Metadata was not extracted from the description") + } + deps, ok := issue.Metadata["depends_on"] + if !ok { + t.Fatalf("Metadata is missing depends_on: %+v", issue.Metadata) + } + list, ok := deps.([]interface{}) + if !ok || len(list) != 1 || list[0] != "TL-9" { + t.Errorf("depends_on = %v, want [TL-9]", deps) + } +}