diff --git a/CHANGELOG.md b/CHANGELOG.md index f54342b..8b3e75a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed +- `--output json` now writes status/progress messages to stderr instead of stdout, so stdout is valid, parseable JSON (#52) +- `associations list` no longer fails to parse responses with results: `toObjectId` is now decoded as a number (#51) + ### Changed - Improved init/config UX with huh forms, config pre-population, and --force flag on clear (#44) - Removed `config set` command - use `init` for configuration changes (#44) diff --git a/api/associations.go b/api/associations.go index eab4419..6613433 100644 --- a/api/associations.go +++ b/api/associations.go @@ -5,9 +5,15 @@ import ( "fmt" ) -// Association represents a HubSpot association between objects +// Association represents a HubSpot association between objects. +// +// ToObjectID uses json.Number because the HubSpot CRM v4 associations API +// returns toObjectId as a JSON number (e.g. 98765), not a string. A plain +// string field would fail to unmarshal with "cannot unmarshal number into Go +// struct field". The number is kept as-is and ToObjectID.String() yields it +// for display without forcing a string-vs-int type choice up front. type Association struct { - ToObjectID string `json:"toObjectId"` + ToObjectID json.Number `json:"toObjectId"` AssociationTypes []AssociationType `json:"associationTypes"` } diff --git a/api/associations_test.go b/api/associations_test.go new file mode 100644 index 0000000..8cb5907 --- /dev/null +++ b/api/associations_test.go @@ -0,0 +1,111 @@ +package api + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestClient_ListAssociations(t *testing.T) { + t.Run("results with numeric toObjectId parse successfully", func(t *testing.T) { + // The HubSpot CRM v4 associations API returns toObjectId as a JSON + // number, not a string. This used to fail with: + // json: cannot unmarshal number into Go struct field + // Association.results.toObjectId of type string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "/crm/v4/objects/contacts/12345/associations/notes", r.URL.Path) + assert.Equal(t, http.MethodGet, r.Method) + + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{ + "results": [ + { + "toObjectId": 98765, + "associationTypes": [ + { + "category": "HUBSPOT_DEFINED", + "typeId": 202, + "label": "Contact to Note" + } + ] + } + ] + }`)) + })) + defer server.Close() + + client := &Client{ + BaseURL: server.URL, + AccessToken: "test-token", + HTTPClient: server.Client(), + } + + result, err := client.ListAssociations(ObjectTypeContacts, "12345", ObjectTypeNotes, ListOptions{}) + require.NoError(t, err) + require.Len(t, result.Results, 1) + assert.Equal(t, "98765", result.Results[0].ToObjectID.String()) + require.Len(t, result.Results[0].AssociationTypes, 1) + assert.Equal(t, "HUBSPOT_DEFINED", result.Results[0].AssociationTypes[0].Category) + assert.Equal(t, 202, result.Results[0].AssociationTypes[0].TypeID) + }) + + t.Run("empty results parse cleanly", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"results": []}`)) + })) + defer server.Close() + + client := &Client{ + BaseURL: server.URL, + AccessToken: "test-token", + HTTPClient: server.Client(), + } + + result, err := client.ListAssociations(ObjectTypeContacts, "12345", ObjectTypeNotes, ListOptions{}) + require.NoError(t, err) + assert.Empty(t, result.Results) + }) + + t.Run("result re-serializes to valid JSON with numeric id", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{ + "results": [ + {"toObjectId": 98765, "associationTypes": []} + ] + }`)) + })) + defer server.Close() + + client := &Client{ + BaseURL: server.URL, + AccessToken: "test-token", + HTTPClient: server.Client(), + } + + result, err := client.ListAssociations(ObjectTypeContacts, "12345", ObjectTypeNotes, ListOptions{}) + require.NoError(t, err) + + // Re-marshalling produces valid JSON; json.Number preserves the + // number as a number (not a quoted string). + out, err := json.Marshal(result) + require.NoError(t, err) + + var roundtrip map[string]interface{} + require.NoError(t, json.Unmarshal(out, &roundtrip), "marshalled output must be valid JSON") + assert.Contains(t, string(out), `"toObjectId":98765`) + }) + + t.Run("empty from ID returns error", func(t *testing.T) { + client := &Client{BaseURL: "https://api.hubapi.com"} + result, err := client.ListAssociations(ObjectTypeContacts, "", ObjectTypeNotes, ListOptions{}) + assert.Error(t, err) + assert.Contains(t, err.Error(), "from object ID is required") + assert.Nil(t, result) + }) +} diff --git a/internal/cmd/associations/associations.go b/internal/cmd/associations/associations.go index 8e2174e..6032a09 100644 --- a/internal/cmd/associations/associations.go +++ b/internal/cmd/associations/associations.go @@ -78,7 +78,7 @@ func newListCmd(opts *root.Options) *cobra.Command { label = fmt.Sprintf("Type %d", at.TypeID) } rows = append(rows, []string{ - assoc.ToObjectID, + assoc.ToObjectID.String(), label, at.Category, }) diff --git a/internal/cmd/configcmd/configcmd.go b/internal/cmd/configcmd/configcmd.go index d69e203..3297a69 100644 --- a/internal/cmd/configcmd/configcmd.go +++ b/internal/cmd/configcmd/configcmd.go @@ -140,14 +140,14 @@ pass/fail status and troubleshooting suggestions on failure.`, token := config.GetAccessToken() if token == "" { v.Error("No HubSpot access token configured") - v.Println("") + v.PrintlnStatus("") v.Info("Configure with: hspt init") v.Info("Or set environment variable: HUBSPOT_ACCESS_TOKEN") return nil } - v.Println("Testing connection to HubSpot...") - v.Println("") + v.PrintlnStatus("Testing connection to HubSpot...") + v.PrintlnStatus("") client, err := opts.APIClient() if err != nil { @@ -159,13 +159,13 @@ pass/fail status and troubleshooting suggestions on failure.`, if err != nil { if api.IsUnauthorized(err) { v.Error("Authentication failed: invalid access token") - v.Println("") + v.PrintlnStatus("") v.Info("Check your token at: HubSpot Settings > Integrations > Private Apps") return nil } if api.IsForbidden(err) { v.Error("Authentication failed: missing required scopes") - v.Println("") + v.PrintlnStatus("") v.Info("Ensure your private app has the required scopes enabled") return nil } @@ -174,7 +174,7 @@ pass/fail status and troubleshooting suggestions on failure.`, } v.Success("Connection successful!") - v.Println("") + v.PrintlnStatus("") v.Info("HubSpot account has %d owners", len(owners)) if len(owners) > 0 { v.Info("First owner: %s (%s)", owners[0].FullName(), owners[0].Email) diff --git a/internal/cmd/forms/forms.go b/internal/cmd/forms/forms.go index ca490f9..332b4a2 100644 --- a/internal/cmd/forms/forms.go +++ b/internal/cmd/forms/forms.go @@ -141,7 +141,7 @@ func newGetCmd(opts *root.Options) *cobra.Command { // Show fields in verbose mode if opts.Verbose && len(form.FieldGroups) > 0 { - v.Println("") + v.PrintlnStatus("") v.Info("Form Fields:") fieldHeaders := []string{"NAME", "LABEL", "TYPE", "REQUIRED"} fieldRows := make([][]string, 0) diff --git a/internal/view/view.go b/internal/view/view.go index c4c1735..45971bb 100644 --- a/internal/view/view.go +++ b/internal/view/view.go @@ -20,7 +20,13 @@ const ( FormatPlain Format = "plain" ) -// View handles output formatting +// View handles output formatting. +// +// Status, progress, and banner text are written to Err (stderr) so that Out +// (stdout) carries only the primary rendered result. This keeps `--output json` +// output valid and parseable by tools like jq: in JSON mode stdout +// is only the JSON payload, and in human/plain mode only the rendered result +// lands on stdout while status chatter goes to stderr. type View struct { Format Format NoColor bool @@ -95,36 +101,36 @@ func (v *View) Render(headers []string, rows [][]string, jsonData interface{}) e } } -// Success prints a success message +// Success prints a success status message to stderr. func (v *View) Success(format string, args ...interface{}) { msg := fmt.Sprintf(format, args...) - fmt.Fprintln(v.Out, color.GreenString("✓ %s", msg)) + fmt.Fprintln(v.Err, color.GreenString("✓ %s", msg)) } -// Error prints an error message +// Error prints an error message to stderr. func (v *View) Error(format string, args ...interface{}) { msg := fmt.Sprintf(format, args...) fmt.Fprintln(v.Err, color.RedString("✗ %s", msg)) } -// Warning prints a warning message +// Warning prints a warning message to stderr. func (v *View) Warning(format string, args ...interface{}) { msg := fmt.Sprintf(format, args...) fmt.Fprintln(v.Err, color.YellowString("⚠ %s", msg)) } -// Info prints an info message +// Info prints an informational status message to stderr. func (v *View) Info(format string, args ...interface{}) { msg := fmt.Sprintf(format, args...) - fmt.Fprintln(v.Out, msg) + fmt.Fprintln(v.Err, msg) } -// Print prints a message without formatting -func (v *View) Print(format string, args ...interface{}) { - fmt.Fprintf(v.Out, format, args...) +// PrintStatus prints an unformatted status message to stderr (no trailing newline). +func (v *View) PrintStatus(format string, args ...interface{}) { + fmt.Fprintf(v.Err, format, args...) } -// Println prints a message with newline -func (v *View) Println(format string, args ...interface{}) { - fmt.Fprintln(v.Out, fmt.Sprintf(format, args...)) +// PrintlnStatus prints a status message to stderr with a trailing newline. +func (v *View) PrintlnStatus(format string, args ...interface{}) { + fmt.Fprintln(v.Err, fmt.Sprintf(format, args...)) } diff --git a/internal/view/view_test.go b/internal/view/view_test.go new file mode 100644 index 0000000..a25a052 --- /dev/null +++ b/internal/view/view_test.go @@ -0,0 +1,126 @@ +package view + +import ( + "bytes" + "encoding/json" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// newTestView returns a View that writes to the provided buffers, with color +// disabled so assertions can match raw text without ANSI escape codes. +func newTestView(format string) (*View, *bytes.Buffer, *bytes.Buffer) { + var out, errBuf bytes.Buffer + v := New(format, true) // noColor=true + v.Out = &out + v.Err = &errBuf + return v, &out, &errBuf +} + +// TestStatusMessagesGoToStderr ensures human-facing status/progress messages +// are written to stderr, not stdout. This is what keeps `--output json` +// output valid (issue #52): only the JSON payload may land on stdout. +func TestStatusMessagesGoToStderr(t *testing.T) { + tests := []struct { + name string + call func(v *View) + want string + }{ + {"Success", func(v *View) { v.Success("created with ID %s", "98765") }, "created with ID 98765"}, + {"Info", func(v *View) { v.Info("Found %d contact(s)", 3) }, "Found 3 contact(s)"}, + {"PrintStatus", func(v *View) { v.PrintStatus("progress %d%%", 50) }, "progress 50%"}, + {"PrintlnStatus", func(v *View) { v.PrintlnStatus("More results available") }, "More results available"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + v, out, errBuf := newTestView("json") + tt.call(v) + + assert.Empty(t, out.String(), "status message must NOT be written to stdout") + assert.Contains(t, errBuf.String(), tt.want, "status message must be written to stderr") + }) + } +} + +// TestErrorAndWarningGoToStderr documents that Error and Warning remain on +// stderr (they always have); this is now consistent with the status methods +// Success/Info/PrintStatus/PrintlnStatus. +func TestErrorAndWarningGoToStderr(t *testing.T) { + v, out, errBuf := newTestView("json") + + v.Error("boom %s", "x") + v.Warning("careful %s", "y") + + assert.Empty(t, out.String()) + assert.Contains(t, errBuf.String(), "boom x") + assert.Contains(t, errBuf.String(), "careful y") +} + +// TestStdoutIsValidJSON simulates a command that emits a status message +// followed by a JSON payload, and verifies stdout alone is parseable JSON. +func TestStdoutIsValidJSON(t *testing.T) { + v, out, _ := newTestView("json") + + // Order mirrors real commands: status first, then structured payload. + v.Success("Note created with ID: %s", "98765") + v.Info("Found 1 result") + require.NoError(t, v.JSON(map[string]interface{}{ + "id": "98765", + "name": "Demo", + })) + v.Info("More results available. Use --after abc123 to get the next page.") + + stdout := out.String() + assert.NotContains(t, stdout, "Note created", "no status banner may leak to stdout") + assert.NotContains(t, stdout, "More results", "no pagination text may leak to stdout") + + var parsed map[string]interface{} + require.NoError(t, json.Unmarshal([]byte(stdout), &parsed), + "stdout must be valid JSON, got: %q", stdout) + assert.Equal(t, "98765", parsed["id"]) +} + +// TestPrimaryOutputGoesToStdout guarantees the other half of the invariant: +// the primary rendered result (Table/Plain/Render/JSON) always lands on stdout, +// never stderr, even while status chatter is routed to stderr. If a "status" +// method ever leaked primary output to stderr, this would catch it. +func TestPrimaryOutputGoesToStdout(t *testing.T) { + t.Run("table renders to stdout in human mode", func(t *testing.T) { + v, out, errBuf := newTestView("table") + // Interleave status messages with the primary rendered result. + v.Info("Found 1 result") + require.NoError(t, v.Render([]string{"ID", "NAME"}, [][]string{{"98765", "Demo"}}, nil)) + v.PrintlnStatus("More results available") + + stdout := out.String() + assert.Contains(t, stdout, "98765", "primary rendered data must be on stdout") + assert.Contains(t, stdout, "Demo", "primary rendered data must be on stdout") + // Status chatter must not leak into the primary stdout stream. + assert.NotContains(t, stdout, "Found 1 result") + assert.NotContains(t, stdout, "More results available") + assert.Contains(t, errBuf.String(), "Found 1 result") + assert.Contains(t, errBuf.String(), "More results available") + }) + + t.Run("plain renders to stdout in plain mode", func(t *testing.T) { + v, out, errBuf := newTestView("plain") + v.Info("Found 1 result") + require.NoError(t, v.Render(nil, [][]string{{"98765", "Demo"}}, nil)) + + assert.Contains(t, out.String(), "98765", "primary rendered data must be on stdout") + assert.NotContains(t, out.String(), "Found 1 result") + assert.Contains(t, errBuf.String(), "Found 1 result") + }) +} + +// TestJSONOutputUnaffectedByColorFlag is a small sanity check that JSON output +// is plain (no ANSI codes regardless of color setting). +func TestJSONOutputUnaffectedByColorFlag(t *testing.T) { + v, out, _ := newTestView("json") + require.NoError(t, v.JSON([]string{"a", "b"})) + assert.False(t, strings.Contains(out.String(), "\x1b["), "JSON output must not contain ANSI escapes") +}