TL-572: linear CLI: 'issues create' (and 'update') never return applied labels, only 'issues get' does

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,18 +382,21 @@ 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{
 					ID:   comment.User.ID,
 					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,
-			}
+			})
 		}
 	}
 

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
+}