diff --git a/caddy.go b/caddy.go index 7309d447129..8246385bd44 100644 --- a/caddy.go +++ b/caddy.go @@ -227,8 +227,18 @@ func changeConfig(method, path string, input []byte, ifMatchHeader string, force idx := make(map[string]string) err = indexConfigObjects(rawCfg[rawConfigKey], "/"+rawConfigKey, idx) if err != nil { + if len(rawCfgJSON) > 0 { + var oldCfg any + err2 := json.Unmarshal(rawCfgJSON, &oldCfg) + if err2 != nil { + err = fmt.Errorf("%v; additionally, restoring old config: %v", err, err2) + } + rawCfg[rawConfigKey] = oldCfg + } else { + rawCfg[rawConfigKey] = nil + } return APIError{ - HTTPStatus: http.StatusInternalServerError, + HTTPStatus: http.StatusBadRequest, Err: fmt.Errorf("indexing config: %v", err), } } @@ -248,6 +258,8 @@ func changeConfig(method, path string, input []byte, ifMatchHeader string, force err = fmt.Errorf("%v; additionally, restoring old config: %v", err, err2) } rawCfg[rawConfigKey] = oldCfg + } else { + rawCfg[rawConfigKey] = nil } return fmt.Errorf("loading new config: %v", err) @@ -281,14 +293,19 @@ func indexConfigObjects(ptr any, configPath string, index map[string]string) err case map[string]any: for k, v := range val { if k == idKey { + var idStr string switch idVal := v.(type) { case string: - index[idVal] = configPath + idStr = idVal case float64: // all JSON numbers decode as float64 - index[fmt.Sprintf("%v", idVal)] = configPath + idStr = fmt.Sprintf("%v", idVal) default: return fmt.Errorf("%s: %s field must be a string or number", configPath, idKey) } + if existingPath, ok := index[idStr]; ok { + return fmt.Errorf("duplicate ID '%s' found at %s and %s", idStr, existingPath, configPath) + } + index[idStr] = configPath continue } // traverse this object property recursively diff --git a/caddytest/caddytest_test.go b/caddytest/caddytest_test.go index a9d5da9365b..31266fa8f1f 100644 --- a/caddytest/caddytest_test.go +++ b/caddytest/caddytest_test.go @@ -1,6 +1,7 @@ package caddytest import ( + "bytes" "net/http" "strings" "testing" @@ -126,3 +127,118 @@ func TestLoadUnorderedJSON(t *testing.T) { } tester.AssertResponseCode(req, 200) } + +func TestCheckID(t *testing.T) { + tester := NewTester(t) + tester.InitServer(`{ + "admin": { + "listen": "localhost:2999" + }, + "apps": { + "http": { + "http_port": 9080, + "servers": { + "s_server": { + "@id": "s_server", + "listen": [ + ":9080" + ], + "routes": [ + { + "handle": [ + { + "handler": "static_response", + "body": "Hello" + } + ] + } + ] + } + } + } + } + } + `, "json") + headers := []string{"Content-Type:application/json"} + sServer1 := []byte(`{"@id":"s_server","listen":[":9080"],"routes":[{"@id":"route1","handle":[{"handler":"static_response","body":"Hello 2"}]}]}`) + + // PUT to an existing ID should fail with a 409 conflict + tester.AssertPutResponseBody( + "http://localhost:2999/id/s_server", + headers, + bytes.NewBuffer(sServer1), + 409, + `{"error":"[/config/apps/http/servers/s_server] key already exists: s_server"}`+"\n") + + // POST replaces the object fully + tester.AssertPostResponseBody( + "http://localhost:2999/id/s_server", + headers, + bytes.NewBuffer(sServer1), + 200, + "") + + // Verify the server is running the new route + tester.AssertGetResponse( + "http://localhost:9080/", + 200, + "Hello 2") + + // Update the existing route to ensure IDs are handled correctly when replaced + tester.AssertPostResponseBody( + "http://localhost:2999/id/s_server", + headers, + bytes.NewBuffer([]byte(`{"@id":"s_server","listen":[":9080"],"routes":[{"@id":"route1","handle":[{"handler":"static_response","body":"Hello2"}],"match":[{"path":["/route_1/*"]}]}]}`)), + 200, + "") + + sServer2 := []byte(`{"@id":"s_server","listen":[":9080"],"routes":[{"@id":"route1","handle":[{"handler":"static_response","body":"Hello2"}],"match":[{"path":["/route_1/*"]}]}]}`) + + // Identical patch should succeed and return 200 (config is unchanged branch) + tester.AssertPatchResponseBody( + "http://localhost:2999/id/s_server", + headers, + bytes.NewBuffer(sServer2), + 200, + "") + + route2 := []byte(`{"@id":"route2","handle": [{"handler": "static_response","body": "route2"}],"match":[{"path":["/route_2/*"]}]}`) + + // Put a new route2 object before the route1 object due to the path of /id/route1 + // Being translated to: /config/apps/http/servers/s_server/routes/0 + tester.AssertPutResponseBody( + "http://localhost:2999/id/route1", + headers, + bytes.NewBuffer(route2), + 200, + "") + + // Verify that the whole config looks correct, now containing both route1 and route2 + tester.AssertGetResponse( + "http://localhost:2999/config/", + 200, + `{"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") + + // Try to add another copy of route2 using POST to test duplicate ID handling + // 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 + tester.AssertPostResponseBody( + "http://localhost:2999/id/route2", + headers, + bytes.NewBuffer(route2), + 400, + `{"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") + + // Use PATCH to modify an existing object successfully + tester.AssertPatchResponseBody( + "http://localhost:2999/id/route1", + headers, + bytes.NewBuffer([]byte(`{"@id":"route1","handle":[{"handler":"static_response","body":"route1"}],"match":[{"path":["/route_1/*"]}]}`)), + 200, + "") + + // Verify the PATCH updated the server state + tester.AssertGetResponse( + "http://localhost:9080/route_1/", + 200, + "route1") +}