Skip to content

Commit 9089183

Browse files
leggetterleggetter
authored
Merge pull request #175 from hookdeck/fix/conn-upsert
fix: connection upsert with only destination config flags now works
2 parents 867bf9d + f06659a commit 9089183

File tree

2 files changed

+256
-3
lines changed

2 files changed

+256
-3
lines changed

‎pkg/cmd/connection_upsert.go‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -255,7 +255,8 @@ func (cu *connectionUpsertCmd) hasAnySourceFlag() bool {
255255

256256
// Helper to check if any destination flags are set
257257
func (cu *connectionUpsertCmd) hasAnyDestinationFlag() bool {
258-
return cu.destinationName != "" || cu.destinationType != "" || cu.destinationID != "" || cu.destinationURL != "" ||
258+
return cu.destinationName != "" || cu.destinationType != "" || cu.destinationID != "" ||
259+
cu.destinationURL != "" || cu.destinationCliPath != "" ||
259260
cu.destinationPathForwardingDisabled != nil || cu.destinationHTTPMethod != "" ||
260261
cu.DestinationRateLimit != 0 || cu.DestinationRateLimitPeriod != "" ||
261262
cu.DestinationAuthMethod != ""
@@ -321,7 +322,8 @@ func (cu *connectionUpsertCmd) runConnectionUpsertCmd(cmd *cobra.Command, args [
321322
cu.SourceCustomResponseBody != "" || cu.SourceConfig != "" || cu.SourceConfigFile != "") &&
322323
cu.sourceName == "" && cu.sourceType == "" && cu.sourceID == ""
323324

324-
hasDestinationConfigOnly := (cu.destinationPathForwardingDisabled != nil || cu.destinationHTTPMethod != "" ||
325+
hasDestinationConfigOnly := (cu.destinationURL != "" || cu.destinationCliPath != "" ||
326+
cu.destinationPathForwardingDisabled != nil || cu.destinationHTTPMethod != "" ||
325327
cu.DestinationRateLimit != 0 || cu.DestinationAuthMethod != "") &&
326328
cu.destinationName == "" && cu.destinationType == "" && cu.destinationID == ""
327329

@@ -470,7 +472,8 @@ func (cu *connectionUpsertCmd) buildUpsertRequest(existing *hookdeck.Connection,
470472
req.Destination = destinationInput
471473
} else if isUpdate && existing != nil && existing.Destination != nil {
472474
// Check if any destination config fields are being updated
473-
hasDestinationConfigUpdate := cu.destinationPathForwardingDisabled != nil ||
475+
hasDestinationConfigUpdate := cu.destinationURL != "" || cu.destinationCliPath != "" ||
476+
cu.destinationPathForwardingDisabled != nil ||
474477
cu.destinationHTTPMethod != "" ||
475478
cu.DestinationRateLimit != 0 || cu.DestinationRateLimitPeriod != "" ||
476479
cu.DestinationAuthMethod != ""
@@ -564,6 +567,10 @@ func (cu *connectionUpsertCmd) buildDestinationInputForUpdate(existingDest *hook
564567
destConfig["url"] = cu.destinationURL
565568
}
566569

570+
if cu.destinationCliPath != "" {
571+
destConfig["path"] = cu.destinationCliPath
572+
}
573+
567574
if cu.destinationPathForwardingDisabled != nil {
568575
destConfig["path_forwarding_disabled"] = *cu.destinationPathForwardingDisabled
569576
}

‎test/acceptance/connection_upsert_test.go‎

Lines changed: 246 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,246 @@
1+
package acceptance
2+
3+
import (
4+
"testing"
5+
6+
"github.com/stretchr/testify/assert"
7+
"github.com/stretchr/testify/require"
8+
)
9+
10+
// TestConnectionUpsertPartialUpdates tests that upsert works with partial config updates
11+
// This addresses the bug where updating only destination config (e.g., --destination-url)
12+
// without providing source/destination name/type fails with 422 error
13+
func TestConnectionUpsertPartialUpdates(t *testing.T) {
14+
if testing.Short() {
15+
t.Skip("Skipping acceptance test in short mode")
16+
}
17+
18+
t.Run("UpsertDestinationURLOnly", func(t *testing.T) {
19+
if testing.Short() {
20+
t.Skip("Skipping acceptance test in short mode")
21+
}
22+
23+
cli := NewCLIRunner(t)
24+
timestamp := generateTimestamp()
25+
26+
connName := "test-upsert-url-" + timestamp
27+
sourceName := "test-upsert-src-" + timestamp
28+
destName := "test-upsert-dst-" + timestamp
29+
initialURL := "https://api.example.com/initial"
30+
updatedURL := "https://api.example.com/updated"
31+
32+
// Create initial connection
33+
var createResp map[string]interface{}
34+
err := cli.RunJSON(&createResp,
35+
"connection", "create",
36+
"--name", connName,
37+
"--source-type", "WEBHOOK",
38+
"--source-name", sourceName,
39+
"--destination-type", "HTTP",
40+
"--destination-name", destName,
41+
"--destination-url", initialURL,
42+
)
43+
require.NoError(t, err, "Should create connection")
44+
45+
connID, ok := createResp["id"].(string)
46+
require.True(t, ok && connID != "", "Expected connection ID in creation response")
47+
48+
// Cleanup
49+
t.Cleanup(func() {
50+
deleteConnection(t, cli, connID)
51+
})
52+
53+
// Verify initial URL
54+
dest, ok := createResp["destination"].(map[string]interface{})
55+
require.True(t, ok, "Expected destination object")
56+
destConfig, ok := dest["config"].(map[string]interface{})
57+
require.True(t, ok, "Expected destination config")
58+
assert.Equal(t, initialURL, destConfig["url"], "Initial URL should match")
59+
60+
t.Logf("Created connection %s with initial URL: %s", connID, initialURL)
61+
62+
// Update ONLY the destination URL (this is the bug scenario)
63+
var upsertResp map[string]interface{}
64+
err = cli.RunJSON(&upsertResp,
65+
"connection", "upsert", connName,
66+
"--destination-url", updatedURL,
67+
)
68+
require.NoError(t, err, "Should upsert connection with only destination-url flag")
69+
70+
// Verify the URL was updated
71+
updatedDest, ok := upsertResp["destination"].(map[string]interface{})
72+
require.True(t, ok, "Expected destination object in upsert response")
73+
updatedDestConfig, ok := updatedDest["config"].(map[string]interface{})
74+
require.True(t, ok, "Expected destination config in upsert response")
75+
assert.Equal(t, updatedURL, updatedDestConfig["url"], "URL should be updated")
76+
77+
// Verify source was preserved
78+
updatedSource, ok := upsertResp["source"].(map[string]interface{})
79+
require.True(t, ok, "Expected source object in upsert response")
80+
assert.Equal(t, sourceName, updatedSource["name"], "Source should be preserved")
81+
82+
t.Logf("Successfully updated connection %s URL to: %s", connID, updatedURL)
83+
})
84+
85+
t.Run("UpsertDestinationHTTPMethod", func(t *testing.T) {
86+
if testing.Short() {
87+
t.Skip("Skipping acceptance test in short mode")
88+
}
89+
90+
cli := NewCLIRunner(t)
91+
timestamp := generateTimestamp()
92+
93+
connName := "test-upsert-method-" + timestamp
94+
sourceName := "test-upsert-src-" + timestamp
95+
destName := "test-upsert-dst-" + timestamp
96+
97+
// Create initial connection (default HTTP method is POST)
98+
var createResp map[string]interface{}
99+
err := cli.RunJSON(&createResp,
100+
"connection", "create",
101+
"--name", connName,
102+
"--source-type", "WEBHOOK",
103+
"--source-name", sourceName,
104+
"--destination-type", "HTTP",
105+
"--destination-name", destName,
106+
"--destination-url", "https://api.example.com/webhook",
107+
)
108+
require.NoError(t, err, "Should create connection")
109+
110+
connID, ok := createResp["id"].(string)
111+
require.True(t, ok && connID != "", "Expected connection ID")
112+
113+
// Cleanup
114+
t.Cleanup(func() {
115+
deleteConnection(t, cli, connID)
116+
})
117+
118+
// Update ONLY the HTTP method
119+
var upsertResp map[string]interface{}
120+
err = cli.RunJSON(&upsertResp,
121+
"connection", "upsert", connName,
122+
"--destination-http-method", "PUT",
123+
)
124+
require.NoError(t, err, "Should upsert connection with only http-method flag")
125+
126+
// Verify the method was updated
127+
updatedDest, ok := upsertResp["destination"].(map[string]interface{})
128+
require.True(t, ok, "Expected destination object")
129+
updatedDestConfig, ok := updatedDest["config"].(map[string]interface{})
130+
require.True(t, ok, "Expected destination config")
131+
assert.Equal(t, "PUT", updatedDestConfig["http_method"], "HTTP method should be updated")
132+
133+
t.Logf("Successfully updated connection %s HTTP method to PUT", connID)
134+
})
135+
136+
t.Run("UpsertDestinationAuthMethod", func(t *testing.T) {
137+
if testing.Short() {
138+
t.Skip("Skipping acceptance test in short mode")
139+
}
140+
141+
cli := NewCLIRunner(t)
142+
timestamp := generateTimestamp()
143+
144+
connName := "test-upsert-auth-" + timestamp
145+
sourceName := "test-upsert-src-" + timestamp
146+
destName := "test-upsert-dst-" + timestamp
147+
148+
// Create initial connection without auth
149+
var createResp map[string]interface{}
150+
err := cli.RunJSON(&createResp,
151+
"connection", "create",
152+
"--name", connName,
153+
"--source-type", "WEBHOOK",
154+
"--source-name", sourceName,
155+
"--destination-type", "HTTP",
156+
"--destination-name", destName,
157+
"--destination-url", "https://api.example.com/webhook",
158+
)
159+
require.NoError(t, err, "Should create connection")
160+
161+
connID, ok := createResp["id"].(string)
162+
require.True(t, ok && connID != "", "Expected connection ID")
163+
164+
// Cleanup
165+
t.Cleanup(func() {
166+
deleteConnection(t, cli, connID)
167+
})
168+
169+
// Update ONLY the auth method
170+
var upsertResp map[string]interface{}
171+
err = cli.RunJSON(&upsertResp,
172+
"connection", "upsert", connName,
173+
"--destination-auth-method", "bearer",
174+
"--destination-bearer-token", "test_token_123",
175+
)
176+
require.NoError(t, err, "Should upsert connection with only auth-method flags")
177+
178+
// Verify auth was updated
179+
updatedDest, ok := upsertResp["destination"].(map[string]interface{})
180+
require.True(t, ok, "Expected destination object")
181+
updatedDestConfig, ok := updatedDest["config"].(map[string]interface{})
182+
require.True(t, ok, "Expected destination config")
183+
184+
if authMethod, ok := updatedDestConfig["auth_method"].(map[string]interface{}); ok {
185+
assert.Equal(t, "BEARER", authMethod["type"], "Auth type should be BEARER")
186+
}
187+
188+
t.Logf("Successfully updated connection %s auth method to bearer", connID)
189+
})
190+
191+
t.Run("UpsertSourceConfigFields", func(t *testing.T) {
192+
if testing.Short() {
193+
t.Skip("Skipping acceptance test in short mode")
194+
}
195+
196+
cli := NewCLIRunner(t)
197+
timestamp := generateTimestamp()
198+
199+
connName := "test-upsert-src-config-" + timestamp
200+
sourceName := "test-upsert-src-" + timestamp
201+
destName := "test-upsert-dst-" + timestamp
202+
203+
// Create initial connection
204+
var createResp map[string]interface{}
205+
err := cli.RunJSON(&createResp,
206+
"connection", "create",
207+
"--name", connName,
208+
"--source-type", "WEBHOOK",
209+
"--source-name", sourceName,
210+
"--destination-type", "CLI",
211+
"--destination-name", destName,
212+
"--destination-cli-path", "/webhooks",
213+
)
214+
require.NoError(t, err, "Should create connection")
215+
216+
connID, ok := createResp["id"].(string)
217+
require.True(t, ok && connID != "", "Expected connection ID")
218+
219+
// Cleanup
220+
t.Cleanup(func() {
221+
deleteConnection(t, cli, connID)
222+
})
223+
224+
// Update ONLY source config fields
225+
var upsertResp map[string]interface{}
226+
err = cli.RunJSON(&upsertResp,
227+
"connection", "upsert", connName,
228+
"--source-allowed-http-methods", "POST,PUT",
229+
"--source-custom-response-content-type", "json",
230+
"--source-custom-response-body", `{"status":"ok"}`,
231+
)
232+
require.NoError(t, err, "Should upsert connection with only source config flags")
233+
234+
// Verify source config was updated
235+
updatedSource, ok := upsertResp["source"].(map[string]interface{})
236+
require.True(t, ok, "Expected source object")
237+
updatedSourceConfig, ok := updatedSource["config"].(map[string]interface{})
238+
require.True(t, ok, "Expected source config")
239+
240+
if allowedMethods, ok := updatedSourceConfig["allowed_http_methods"].([]interface{}); ok {
241+
assert.Len(t, allowedMethods, 2, "Should have 2 allowed HTTP methods")
242+
}
243+
244+
t.Logf("Successfully updated connection %s source config", connID)
245+
})
246+
}

0 commit comments

Comments
 (0)