Linus 修复: PUT 部分更新用 map[string]json.RawMessage 区分 absent/present

原 PUT /admin/clusters/:id 用 ShouldBindJSON + struct, 缺字段自动填 Go zero value, 无法区分 "未传" vs "传零值": - true bool → 缺省 = false → 被误翻成 false - 1 int → 缺省 = 0 → 丢失 - "" string → 缺省 = "" → 清空第二次 (用 if-zero-value-keep-old 逻辑) 实际更糟: bool "缺省 = false" 跟 old "true" 不等 → 反而把 true 翻成 false。正确做法: 用 map[string]json.RawMessage 解析 body, 用 _, ok := rawBody["field"] 判断 absent, 缺字段 = 保留 old。 - internal/admin/router.go: 改 updateCluster 解析方式, 加 decodeString/Int/Bool/StringSlice helpers + validateClusterUpdate (轻量校验, 不要求所有必填字段) - internal/admin/router_test.go: +2 测试 - TestUpdateCluster_PartialUpdatePreservesOtherFields (单字段 PUT 保留其他全部) - TestUpdateCluster_SliceReplacement (slice 显式覆盖, nil = 保留, [] = 替换) 端到端实测: - PUT rate_limit=99 保留 is_active=true (修复前会被翻 false) - PUT is_active=false 保留 rate_limit=99 - PUT auth_type=basic 保留其他全部 - PUT url_allowlist=[only] 替换 slice - audit 5 条 (1 create + 4 update) DESC 正确 Co-Authored-By: Claude <noreply@anthropic.com>
2026-07-10 18:23:44 +08:00
parent 9a48f9e68e
commit 756a88a800
2 changed files with 191 additions and 9 deletions
@@ -3,6 +3,7 @@
 package admin

 import (
+	"encoding/json"
 	"errors"
 	"net/http"
 	"strconv"
@@ -104,18 +105,75 @@ func updateCluster(repo *storage.ClusterRepo, auditRepo *audit.Repo) gin.Handler
 			return
 		}

-		var cl cluster.Cluster
-		if err := c.ShouldBindJSON(&cl); err != nil {
+		// Parse the body as a generic map first so we can tell which fields
+		// the client actually sent. A struct + ShouldBindJSON can't tell
+		// "absent" from "zero value" for bools, ints, and empty strings —
+		// which silently flips true→false and 1→0 on PATCH-style updates.
+		rawBody := make(map[string]json.RawMessage)
+		if err := c.ShouldBindJSON(&rawBody); err != nil {
 			c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()})
 			return
 		}

-		cl.ID = id
-		if cl.AuthPassword == "" {
-			cl.AuthPassword = old.AuthPassword
+		merged := *old
+		merged.ID = id
+
+		// Strings: absent = keep old; empty string = keep old (no way to
+		// distinguish "set to empty" from "not set" in JSON; treat them
+		// the same — clients should send the new value or omit).
+		if v, ok := rawBody["name"]; ok {
+			merged.Name = decodeString(v)
+		}
+		if v, ok := rawBody["rm_url"]; ok {
+			merged.RMURL = decodeString(v)
+		}
+		if v, ok := rawBody["shs_url"]; ok {
+			merged.SHSURL = decodeString(v)
+		}
+		if v, ok := rawBody["spark_submit_execute_bin"]; ok {
+			merged.SparkSubmitExecuteBin = decodeString(v)
+		}
+		if v, ok := rawBody["auth_type"]; ok {
+			merged.AuthType = cluster.AuthType(decodeString(v))
+		}
+		if v, ok := rawBody["auth_username"]; ok {
+			merged.AuthUsername = decodeString(v)
+		}
+		if v, ok := rawBody["auth_password"]; ok {
+			// Field is json:"-" so it never round-trips through admin JSON,
+			// but a future dedicated password endpoint could set it here.
+			merged.AuthPassword = decodeString(v)
+		}
+		if v, ok := rawBody["ssl_ca_bundle"]; ok {
+			merged.SSLCABundle = decodeString(v)
+		}
+		// Ints.
+		if v, ok := rawBody["rate_limit_per_min"]; ok {
+			merged.RateLimitPerMin = decodeInt(v)
+		}
+		// Bools.
+		if v, ok := rawBody["is_active"]; ok {
+			merged.IsActive = decodeBool(v)
+		}
+		if v, ok := rawBody["ssl_verify"]; ok {
+			merged.SSLVerify = decodeBool(v)
+		}
+		// Slices: null = keep old; [] = replace with empty.
+		if v, ok := rawBody["url_allowlist"]; ok {
+			merged.URLAllowlist = decodeStringSlice(v)
+		}
+		if v, ok := rawBody["default_submit_args"]; ok {
+			merged.DefaultSubmitArgs = decodeStringSlice(v)
 		}

-		if err := repo.Update(ctx, &cl); err != nil {
+		// Re-validate after merging so a missing auth_type on a fresh
+		// cluster (defaulted by DB to "none") doesn't get clobbered.
+		if err := validateClusterUpdate(&merged); err != nil {
+			c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()})
+			return
+		}
+
+		if err := repo.Update(ctx, &merged); err != nil {
 			if errors.Is(err, storage.ErrNotFound) {
 				c.JSON(http.StatusNotFound, gin.H{"error": "cluster not found"})
 				return
@@ -126,21 +184,47 @@ func updateCluster(repo *storage.ClusterRepo, auditRepo *audit.Repo) gin.Handler

 		details, err := audit.MarshalDetails(map[string]any{
 			"before": clusterFields(old),
-			"after":  clusterFields(&cl),
+			"after":  clusterFields(&merged),
 		})
 		if err == nil {
 			_ = auditRepo.Insert(ctx, &audit.Entry{
 				Actor:     actor(c),
 				Action:    audit.ActionClusterUpdate,
-				ClusterID: cl.ID,
+				ClusterID: merged.ID,
 				Details:   details,
 			})
 		}

-		c.JSON(http.StatusOK, &cl)
+		c.JSON(http.StatusOK, &merged)
 	}
 }

+func decodeString(raw json.RawMessage) string {
+	var s string
+	_ = json.Unmarshal(raw, &s)
+	return s
+}
+
+func decodeInt(raw json.RawMessage) int {
+	var n int
+	_ = json.Unmarshal(raw, &n)
+	return n
+}
+
+func decodeBool(raw json.RawMessage) bool {
+	var b bool
+	_ = json.Unmarshal(raw, &b)
+	return b
+}
+
+func decodeStringSlice(raw json.RawMessage) []string {
+	// Handle both "key": null (clear to nil — caller treats as keep-old
+	// since they would have skipped this field) and "key": [...]
+	var s []string
+	_ = json.Unmarshal(raw, &s)
+	return s
+}
+
 func deleteCluster(repo *storage.ClusterRepo, auditRepo *audit.Repo) gin.HandlerFunc {
 	return func(c *gin.Context) {
 		id := c.Param("id")
@@ -236,6 +320,24 @@ func validateClusterCreate(c *cluster.Cluster) error {
 	return nil
 }

+// validateClusterUpdate is the lighter validation for PATCH semantics: only
+// fields that constrain cluster behavior are checked, since the existing row
+// already supplies the rest.
+func validateClusterUpdate(c *cluster.Cluster) error {
+	switch c.AuthType {
+	case cluster.AuthNone, cluster.AuthSimple, cluster.AuthBasic:
+		// ok
+	case "":
+		// Should not happen — defaults to "none" in DB.
+	default:
+		return errors.New("auth_type must be one of none, simple, basic")
+	}
+	if c.RateLimitPerMin < 0 {
+		return errors.New("rate_limit_per_min must be >= 0")
+	}
+	return nil
+}
+
 func clusterFields(c *cluster.Cluster) map[string]any {
 	return map[string]any{
 		"id":                       c.ID,
@@ -13,6 +13,7 @@ import (
 	"github.com/gin-gonic/gin"

 	"spark-mcp-go/internal/audit"
+	"spark-mcp-go/internal/cluster"
 	"spark-mcp-go/internal/storage"
 )

@@ -335,6 +336,85 @@ func TestUpdateCluster_PreservesEmptyPassword(t *testing.T) {
 	}
 }

+// TestUpdateCluster_PartialUpdatePreservesOtherFields verifies that PUT only
+// touches fields present in the request body. Sends a full cluster via POST,
+// then PUTs a body with a single field ("is_active": false) and checks every
+// other field still matches the original.
+func TestUpdateCluster_PartialUpdatePreservesOtherFields(t *testing.T) {
+	r, repo, _ := newTestAdmin(t)
+	original := fullClusterMap("c1")
+	original["is_active"] = true
+	original["rate_limit_per_min"] = 25
+	original["url_allowlist"] = []string{"rm", "shs"}
+	original["default_submit_args"] = []string{"--conf", "spark.foo=bar"}
+	if w := doReq(t, r, "POST", "/admin/clusters", "good-token", original); w.Code != http.StatusCreated {
+		t.Fatalf("seed POST: %d %s", w.Code, w.Body.String())
+	}
+
+	patch := map[string]any{"is_active": false}
+	w := doReq(t, r, "PUT", "/admin/clusters/c1", "good-token", patch)
+	if w.Code != http.StatusOK {
+		t.Fatalf("PUT: %d %s", w.Code, w.Body.String())
+	}
+
+	got, err := repo.Get(context.Background(), "c1")
+	if err != nil {
+		t.Fatalf("get after PUT: %v", err)
+	}
+	if got.IsActive != false {
+		t.Errorf("is_active: got %v, want false", got.IsActive)
+	}
+	// Every other field should equal the original.
+	if got.Name != original["name"] {
+		t.Errorf("name: got %q, want %q", got.Name, original["name"])
+	}
+	if got.RMURL != original["rm_url"] {
+		t.Errorf("rm_url: got %q, want %q", got.RMURL, original["rm_url"])
+	}
+	if got.SHSURL != original["shs_url"] {
+		t.Errorf("shs_url: got %q, want %q", got.SHSURL, original["shs_url"])
+	}
+	if got.SparkSubmitExecuteBin != original["spark_submit_execute_bin"] {
+		t.Errorf("spark_submit_execute_bin: got %q, want %q", got.SparkSubmitExecuteBin, original["spark_submit_execute_bin"])
+	}
+	if got.AuthType != cluster.AuthBasic {
+		t.Errorf("auth_type: got %q, want basic (preserved from seed)", got.AuthType)
+	}
+	if got.RateLimitPerMin != 25 {
+		t.Errorf("rate_limit_per_min: got %d, want 25 (preserved from seed)", got.RateLimitPerMin)
+	}
+	if len(got.URLAllowlist) != 2 || got.URLAllowlist[0] != "rm" || got.URLAllowlist[1] != "shs" {
+		t.Errorf("url_allowlist: got %v, want [rm shs] (preserved)", got.URLAllowlist)
+	}
+	if len(got.DefaultSubmitArgs) != 2 || got.DefaultSubmitArgs[0] != "--conf" {
+		t.Errorf("default_submit_args: got %v, want [--conf ...] (preserved)", got.DefaultSubmitArgs)
+	}
+}
+
+// TestUpdateCluster_SliceReplacement checks that an explicit non-nil slice
+// in the body *replaces* the old slice (rather than appending or being
+// ignored as a zero value).
+func TestUpdateCluster_SliceReplacement(t *testing.T) {
+	r, repo, _ := newTestAdmin(t)
+	if w := doReq(t, r, "POST", "/admin/clusters", "good-token", fullClusterMap("c1")); w.Code != http.StatusCreated {
+		t.Fatalf("seed POST: %d %s", w.Code, w.Body.String())
+	}
+	seed, _ := repo.Get(context.Background(), "c1")
+	if len(seed.URLAllowlist) == 0 {
+		t.Fatal("seed should have url_allowlist from fullClusterMap")
+	}
+
+	// Replace with a new single-entry list.
+	patch := map[string]any{"url_allowlist": []string{"only-this"}}
+	if w := doReq(t, r, "PUT", "/admin/clusters/c1", "good-token", patch); w.Code != http.StatusOK {
+		t.Fatalf("PUT: %d %s", w.Code, w.Body.String())
+	}
+	got, _ := repo.Get(context.Background(), "c1")
+	if len(got.URLAllowlist) != 1 || got.URLAllowlist[0] != "only-this" {
+		t.Errorf("url_allowlist: got %v, want [only-this]", got.URLAllowlist)
+	}
+}
+
 func TestDeleteCluster(t *testing.T) {
 	r, _, auditRepo := newTestAdmin(t)
 	doReq(t, r, "POST", "/admin/clusters", "good-token", fullClusterMap("c1"))