Skip to content

Commit 44e3c60

Browse files
francislavoiefrancislavoie
committed
Alternate implementation, using Gemini 3.1
1 parent 0c5d3b0 commit 44e3c60

File tree

2 files changed

+90
-62
lines changed

2 files changed

+90
-62
lines changed

‎caddy.go‎

Lines changed: 22 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -227,12 +227,18 @@ func changeConfig(method, path string, input []byte, ifMatchHeader string, force
227227
idx := make(map[string]string)
228228
err = indexConfigObjects(rawCfg[rawConfigKey], "/"+rawConfigKey, idx)
229229
if err != nil {
230-
err2 := rollbackRawCfg()
231-
if err2 != nil {
232-
err = errors.Join(err, err2)
230+
if len(rawCfgJSON) > 0 {
231+
var oldCfg any
232+
err2 := json.Unmarshal(rawCfgJSON, &oldCfg)
233+
if err2 != nil {
234+
err = fmt.Errorf("%v; additionally, restoring old config: %v", err, err2)
235+
}
236+
rawCfg[rawConfigKey] = oldCfg
237+
} else {
238+
rawCfg[rawConfigKey] = nil
233239
}
234240
return APIError{
235-
HTTPStatus: http.StatusInternalServerError,
241+
HTTPStatus: http.StatusBadRequest,
236242
Err: fmt.Errorf("indexing config: %v", err),
237243
}
238244
}
@@ -246,10 +252,14 @@ func changeConfig(method, path string, input []byte, ifMatchHeader string, force
246252
// with what caddy is still running; we need to
247253
// unmarshal it again because it's likely that
248254
// pointers deep in our rawCfg map were modified
249-
err2 := rollbackRawCfg()
255+
var oldCfg any
256+
err2 := json.Unmarshal(rawCfgJSON, &oldCfg)
250257
if err2 != nil {
251258
err = fmt.Errorf("%v; additionally, restoring old config: %v", err, err2)
252259
}
260+
rawCfg[rawConfigKey] = oldCfg
261+
} else {
262+
rawCfg[rawConfigKey] = nil
253263
}
254264

255265
return fmt.Errorf("loading new config: %v", err)
@@ -266,35 +276,13 @@ func changeConfig(method, path string, input []byte, ifMatchHeader string, force
266276
return nil
267277
}
268278

269-
func rollbackRawCfg() error {
270-
var oldCfg any
271-
if len(rawCfgJSON) != 0 {
272-
err := json.Unmarshal(rawCfgJSON, &oldCfg)
273-
if err != nil {
274-
return err
275-
}
276-
rawCfg[rawConfigKey] = oldCfg
277-
} else {
278-
rawCfg[rawConfigKey] = nil
279-
}
280-
return nil
281-
}
282-
283279
// readConfig traverses the current config to path
284280
// and writes its JSON encoding to out.
285281
func readConfig(path string, out io.Writer) error {
286282
rawCfgMu.RLock()
287283
defer rawCfgMu.RUnlock()
288284
return unsyncedConfigAccess(http.MethodGet, path, nil, out)
289285
}
290-
func writeToIdIndex(index map[string]string, key, val string) error {
291-
_, found := index[key]
292-
if found {
293-
return errors.New("multiple keys found: " + key)
294-
}
295-
index[key] = val
296-
return nil
297-
}
298286

299287
// indexConfigObjects recursively searches ptr for object fields named
300288
// "@id" and maps that ID value to the full configPath in the index.
@@ -305,20 +293,19 @@ func indexConfigObjects(ptr any, configPath string, index map[string]string) err
305293
case map[string]any:
306294
for k, v := range val {
307295
if k == idKey {
296+
var idStr string
308297
switch idVal := v.(type) {
309298
case string:
310-
err := writeToIdIndex(index, idVal, configPath)
311-
if err != nil {
312-
return err
313-
}
299+
idStr = idVal
314300
case float64: // all JSON numbers decode as float64
315-
err := writeToIdIndex(index, fmt.Sprintf("%v", idVal), configPath)
316-
if err != nil {
317-
return err
318-
}
301+
idStr = fmt.Sprintf("%v", idVal)
319302
default:
320303
return fmt.Errorf("%s: %s field must be a string or number", configPath, idKey)
321304
}
305+
if existingPath, ok := index[idStr]; ok {
306+
return fmt.Errorf("duplicate ID '%s' found at %s and %s", idStr, existingPath, configPath)
307+
}
308+
index[idStr] = configPath
322309
continue
323310
}
324311
// traverse this object property recursively

‎caddytest/caddytest_test.go‎

Lines changed: 68 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -158,46 +158,87 @@ func TestCheckID(t *testing.T) {
158158
}
159159
}
160160
}
161-
`, "json")
161+
`, "json")
162162
headers := []string{"Content-Type:application/json"}
163-
sServer1 := []byte(
164-
`{"@id":"s_server","listen":[":9080"],"routes":[{"@id":"route1","handle":[{"handler":"static_response","body":"Hello 2"}]}]}`)
163+
sServer1 := []byte(`{"@id":"s_server","listen":[":9080"],"routes":[{"@id":"route1","handle":[{"handler":"static_response","body":"Hello 2"}]}]}`)
164+
165+
// PUT to an existing ID should fail with a 409 conflict
165166
tester.AssertPutResponseBody(
166167
"http://localhost:2999/id/s_server",
167-
headers, bytes.NewBuffer(sServer1), 409, `{"error":"[/config/apps/http/servers/s_server] key already exists: s_server"}
168-
`)
169-
tester.AssertPostResponseBody("http://localhost:2999/id/s_server", headers, bytes.NewBuffer(sServer1), 200, "")
168+
headers,
169+
bytes.NewBuffer(sServer1),
170+
409,
171+
`{"error":"[/config/apps/http/servers/s_server] key already exists: s_server"}`+"\n")
170172

171-
tester.AssertGetResponse("http://localhost:9080/", 200, "Hello 2")
173+
// POST replaces the object fully
172174
tester.AssertPostResponseBody(
173175
"http://localhost:2999/id/s_server",
174-
headers, bytes.NewBuffer([]byte(
175-
`{"@id":"s_server","listen":[":9080"],"routes":[{"@id":"route1","handle":[
176-
{"handler":"static_response","body":"Hello2"}],"match":[{"path":["/route_1/*"]}]}]}`)), 200, "")
176+
headers,
177+
bytes.NewBuffer(sServer1),
178+
200,
179+
"")
180+
181+
// Verify the server is running the new route
182+
tester.AssertGetResponse(
183+
"http://localhost:9080/",
184+
200,
185+
"Hello 2")
186+
187+
// Update the existing route to ensure IDs are handled correctly when replaced
188+
tester.AssertPostResponseBody(
189+
"http://localhost:2999/id/s_server",
190+
headers,
191+
bytes.NewBuffer([]byte(`{"@id":"s_server","listen":[":9080"],"routes":[{"@id":"route1","handle":[{"handler":"static_response","body":"Hello2"}],"match":[{"path":["/route_1/*"]}]}]}`)),
192+
200,
193+
"")
194+
195+
sServer2 := []byte(`{"@id":"s_server","listen":[":9080"],"routes":[{"@id":"route1","handle":[{"handler":"static_response","body":"Hello2"}],"match":[{"path":["/route_1/*"]}]}]}`)
177196

178-
sServer2 := []byte(`
179-
{"@id":"s_server","listen":[":9080"],"routes":[{"@id":"route1","handle":[
180-
{"handler":"static_response","body":"Hello2"}],"match":[{"path":["/route_1/*"]}]}]}`)
197+
// Identical patch should succeed and return 200 (config is unchanged branch)
181198
tester.AssertPatchResponseBody(
182199
"http://localhost:2999/id/s_server",
183-
headers, bytes.NewBuffer(sServer2), 200, "")
184-
route2 := []byte(
185-
`{"@id":"route2","handle": [{"handler": "static_response","body": "route2"}],"match":[{"path":["/route_2/*"]}]}`)
200+
headers,
201+
bytes.NewBuffer(sServer2),
202+
200,
203+
"")
204+
205+
route2 := []byte(`{"@id":"route2","handle": [{"handler": "static_response","body": "route2"}],"match":[{"path":["/route_2/*"]}]}`)
206+
207+
// Put a new route2 object before the route1 object due to the path of /id/route1
208+
// Being translated to: /config/apps/http/servers/s_server/routes/0
186209
tester.AssertPutResponseBody(
187210
"http://localhost:2999/id/route1",
188-
headers, bytes.NewBuffer(route2), 200, "")
189-
tester.AssertGetResponse("http://localhost:2999/config", 200,
190-
`{"admin":{"listen":"localhost:2999"},"apps":{"http":{"http_port":9080,"servers":{"s_server":{"@id":"s_server","listen":[":9080"],"routes":[{"@id":"route2","handle":[{"body":"route2","handler":"static_response"}],"match":[{"path":["/route_2/*"]}]},{"@id":"route1","handle":[{"body":"Hello2","handler":"static_response"}],"match":[{"path":["/route_1/*"]}]}]}}}}}
191-
`)
192-
// use post or put to add one
211+
headers,
212+
bytes.NewBuffer(route2),
213+
200,
214+
"")
215+
216+
// Verify that the whole config looks correct, now containing both route1 and route2
217+
tester.AssertGetResponse(
218+
"http://localhost:2999/config/",
219+
200,
220+
`{"admin":{"listen":"localhost:2999"},"apps":{"http":{"http_port":9080,"servers":{"s_server":{"@id":"s_server","listen":[":9080"],"routes":[{"@id":"route2","handle":[{"body":"route2","handler":"static_response"}],"match":[{"path":["/route_2/*"]}]},{"@id":"route1","handle":[{"body":"Hello2","handler":"static_response"}],"match":[{"path":["/route_1/*"]}]}]}}}}}`+"\n")
221+
222+
// Try to add another copy of route2 using POST to test duplicate ID handling
223+
// Since the first route2 ended up at array index 0, and we are appending to the array, the index for the new element would be 2
193224
tester.AssertPostResponseBody(
194225
"http://localhost:2999/id/route2",
195-
headers, bytes.NewBuffer(route2), 500, `{"error":"indexing config: multiple keys found: route2"}
196-
`)
197-
// use patch to update
226+
headers,
227+
bytes.NewBuffer(route2),
228+
400,
229+
`{"error":"indexing config: duplicate ID 'route2' found at /config/apps/http/servers/s_server/routes/0 and /config/apps/http/servers/s_server/routes/2"}`+"\n")
230+
231+
// Use PATCH to modify an existing object successfully
198232
tester.AssertPatchResponseBody(
199233
"http://localhost:2999/id/route1",
200-
headers, bytes.NewBuffer([]byte(`{"@id":"route1","handle":[{"handler":"static_response","body":"route1"}],"match":[{"path":["/route_1/*"]}]}`)),
201-
200, "")
202-
tester.AssertGetResponse("http://localhost:9080/route_1/", 200, "route1")
234+
headers,
235+
bytes.NewBuffer([]byte(`{"@id":"route1","handle":[{"handler":"static_response","body":"route1"}],"match":[{"path":["/route_1/*"]}]}`)),
236+
200,
237+
"")
238+
239+
// Verify the PATCH updated the server state
240+
tester.AssertGetResponse(
241+
"http://localhost:9080/route_1/",
242+
200,
243+
"route1")
203244
}

0 commit comments

Comments
 (0)