From 5f52195cdc3866771bc5d032c767997b9aef9404 Mon Sep 17 00:00:00 2001 From: nfebe Date: Fri, 30 Jan 2026 15:46:43 +0100 Subject: [PATCH] fix(api): Merge metadata on partial updates instead of replacing The updateDeploymentMetadata endpoint now properly merges incoming fields with existing metadata instead of replacing the entire object. This prevents accidental data loss when updating only specific fields like credential_id or networking settings. - Parse raw JSON to detect which fields were actually sent - Only update fields that are present in the request - Preserve existing values for fields not included in the update - Add comprehensive tests for merge behavior Signed-off-by: nfebe --- internal/api/metadata_update_test.go | 202 +++++++++++++++++++++++++++ internal/api/server.go | 67 ++++++++- 2 files changed, 265 insertions(+), 4 deletions(-) diff --git a/internal/api/metadata_update_test.go b/internal/api/metadata_update_test.go index e1ef31a..43b652d 100644 --- a/internal/api/metadata_update_test.go +++ b/internal/api/metadata_update_test.go @@ -13,6 +13,208 @@ import ( "github.com/flatrun/agent/pkg/models" ) +func parseTestJSON(t *testing.T, jsonStr string) (map[string]json.RawMessage, models.ServiceMetadata) { + t.Helper() + var sentFields map[string]json.RawMessage + if err := json.Unmarshal([]byte(jsonStr), &sentFields); err != nil { + t.Fatalf("failed to parse sentFields: %v", err) + } + var incoming models.ServiceMetadata + if err := json.Unmarshal([]byte(jsonStr), &incoming); err != nil { + t.Fatalf("failed to parse incoming: %v", err) + } + return sentFields, incoming +} + +func TestMergeMetadata_PartialUpdatePreservesOtherFields(t *testing.T) { + existing := &models.ServiceMetadata{ + Name: "wordpress-app", + Type: "wordpress", + Networking: models.NetworkingConfig{ + Expose: true, + Domain: "blog.example.com", + ContainerPort: 80, + }, + SSL: models.SSLConfig{ + Enabled: true, + AutoCert: true, + }, + QuickActions: []models.QuickAction{ + {ID: "clear-cache", Name: "Clear Cache", Command: "wp cache flush"}, + }, + Security: &models.DeploymentSecurityConfig{ + Enabled: true, + ProtectedPaths: []models.ProtectedPath{ + {Pattern: "/.env", Enabled: true}, + }, + }, + } + + sentFields, incoming := parseTestJSON(t, `{"credential_id": "cred-123"}`) + + merged := mergeMetadata(existing, &incoming, sentFields) + + if merged.CredentialID != "cred-123" { + t.Errorf("sent field credential_id should be updated: expected 'cred-123', got '%s'", merged.CredentialID) + } + + if merged.Name != "wordpress-app" { + t.Errorf("unsent field Name should be preserved: expected 'wordpress-app', got '%s'", merged.Name) + } + if merged.Type != "wordpress" { + t.Errorf("unsent field Type should be preserved: expected 'wordpress', got '%s'", merged.Type) + } + if !merged.Networking.Expose || merged.Networking.Domain != "blog.example.com" { + t.Error("unsent field Networking should be preserved") + } + if !merged.SSL.Enabled || !merged.SSL.AutoCert { + t.Error("unsent field SSL should be preserved") + } + if len(merged.QuickActions) != 1 || merged.QuickActions[0].ID != "clear-cache" { + t.Error("unsent field QuickActions should be preserved") + } + if merged.Security == nil || !merged.Security.Enabled { + t.Error("unsent field Security should be preserved") + } +} + +func TestMergeMetadata_SentFieldOverwritesExisting(t *testing.T) { + existing := &models.ServiceMetadata{ + Name: "old-name", + Type: "custom", + CredentialID: "old-cred", + Networking: models.NetworkingConfig{ + Expose: true, + Domain: "old.example.com", + ContainerPort: 80, + }, + } + + sentFields, incoming := parseTestJSON(t, `{ + "networking": { + "expose": true, + "domain": "new.example.com", + "container_port": 8080, + "protocol": "http", + "proxy_type": "http" + } + }`) + + merged := mergeMetadata(existing, &incoming, sentFields) + + if merged.Networking.Domain != "new.example.com" { + t.Errorf("sent field Networking.Domain should be updated: expected 'new.example.com', got '%s'", merged.Networking.Domain) + } + if merged.Networking.ContainerPort != 8080 { + t.Errorf("sent field Networking.ContainerPort should be updated: expected 8080, got %d", merged.Networking.ContainerPort) + } + + if merged.CredentialID != "old-cred" { + t.Errorf("unsent field CredentialID should be preserved: expected 'old-cred', got '%s'", merged.CredentialID) + } + if merged.Name != "old-name" { + t.Errorf("unsent field Name should be preserved: expected 'old-name', got '%s'", merged.Name) + } +} + +func TestMergeMetadata_CanSetFieldToFalseOrEmpty(t *testing.T) { + existing := &models.ServiceMetadata{ + Name: "test-app", + CredentialID: "has-credential", + Networking: models.NetworkingConfig{ + Expose: true, + Domain: "test.example.com", + }, + SSL: models.SSLConfig{ + Enabled: true, + AutoCert: true, + }, + } + + sentFields, incoming := parseTestJSON(t, `{ + "credential_id": "", + "networking": {"expose": false, "domain": "", "container_port": 0, "protocol": "", "proxy_type": ""}, + "ssl": {"enabled": false, "auto_cert": false} + }`) + + merged := mergeMetadata(existing, &incoming, sentFields) + + if merged.CredentialID != "" { + t.Errorf("credential_id should be cleared when explicitly sent as empty, got '%s'", merged.CredentialID) + } + if merged.Networking.Expose { + t.Error("Networking.Expose should be set to false when explicitly sent") + } + if merged.SSL.Enabled { + t.Error("SSL.Enabled should be set to false when explicitly sent") + } + + if merged.Name != "test-app" { + t.Errorf("unsent field Name should be preserved, got '%s'", merged.Name) + } +} + +func TestMergeMetadata_MultipleFieldsUpdated(t *testing.T) { + existing := &models.ServiceMetadata{ + Name: "app", + Type: "custom", + Networking: models.NetworkingConfig{ + Expose: false, + }, + QuickActions: []models.QuickAction{ + {ID: "action1", Name: "Action 1"}, + }, + } + + sentFields, incoming := parseTestJSON(t, `{ + "name": "updated-app", + "type": "wordpress", + "credential_id": "new-cred" + }`) + + merged := mergeMetadata(existing, &incoming, sentFields) + + if merged.Name != "updated-app" { + t.Errorf("Name should be updated, got '%s'", merged.Name) + } + if merged.Type != "wordpress" { + t.Errorf("Type should be updated, got '%s'", merged.Type) + } + if merged.CredentialID != "new-cred" { + t.Errorf("CredentialID should be updated, got '%s'", merged.CredentialID) + } + + if merged.Networking.Expose { + t.Error("Networking should be preserved (not sent)") + } + if len(merged.QuickActions) != 1 { + t.Error("QuickActions should be preserved (not sent)") + } +} + +func TestMergeMetadata_NilExisting(t *testing.T) { + sentFields, incoming := parseTestJSON(t, `{"name": "new-app", "credential_id": "cred-123"}`) + + merged := mergeMetadata(nil, &incoming, sentFields) + + if merged.Name != "new-app" || merged.CredentialID != "cred-123" { + t.Error("when existing is nil, incoming should be returned as-is") + } +} + +func TestMergeMetadata_NilIncoming(t *testing.T) { + existing := &models.ServiceMetadata{ + Name: "existing-app", + Type: "custom", + } + + merged := mergeMetadata(existing, nil, nil) + + if merged != existing { + t.Error("when incoming is nil, existing should be returned as-is") + } +} + type mockManager struct { deployment *models.Deployment savedMeta *models.ServiceMetadata diff --git a/internal/api/server.go b/internal/api/server.go index ed93bae..67b7916 100644 --- a/internal/api/server.go +++ b/internal/api/server.go @@ -1002,8 +1002,24 @@ func (s *Server) updateDeployment(c *gin.Context) { func (s *Server) updateDeploymentMetadata(c *gin.Context) { name := c.Param("name") - var metadata models.ServiceMetadata - if err := c.ShouldBindJSON(&metadata); err != nil { + bodyBytes, err := c.GetRawData() + if err != nil { + c.JSON(http.StatusBadRequest, gin.H{ + "error": "Failed to read request body", + }) + return + } + + var sentFields map[string]json.RawMessage + if err := json.Unmarshal(bodyBytes, &sentFields); err != nil { + c.JSON(http.StatusBadRequest, gin.H{ + "error": err.Error(), + }) + return + } + + var incoming models.ServiceMetadata + if err := json.Unmarshal(bodyBytes, &incoming); err != nil { c.JSON(http.StatusBadRequest, gin.H{ "error": err.Error(), }) @@ -1018,14 +1034,16 @@ func (s *Server) updateDeploymentMetadata(c *gin.Context) { return } - if err := s.manager.SaveMetadata(name, &metadata); err != nil { + metadata := mergeMetadata(deployment.Metadata, &incoming, sentFields) + + if err := s.manager.SaveMetadata(name, metadata); err != nil { c.JSON(http.StatusInternalServerError, gin.H{ "error": err.Error(), }) return } - deployment.Metadata = &metadata + deployment.Metadata = metadata var proxyResult *proxy.SetupResult if metadata.Networking.Expose { @@ -1041,6 +1059,47 @@ func (s *Server) updateDeploymentMetadata(c *gin.Context) { }) } +func mergeMetadata(existing, incoming *models.ServiceMetadata, sentFields map[string]json.RawMessage) *models.ServiceMetadata { + if existing == nil { + return incoming + } + if incoming == nil { + return existing + } + + merged := *existing + + if _, ok := sentFields["name"]; ok { + merged.Name = incoming.Name + } + if _, ok := sentFields["type"]; ok { + merged.Type = incoming.Type + } + if _, ok := sentFields["credential_id"]; ok { + merged.CredentialID = incoming.CredentialID + } + if _, ok := sentFields["networking"]; ok { + merged.Networking = incoming.Networking + } + if _, ok := sentFields["ssl"]; ok { + merged.SSL = incoming.SSL + } + if _, ok := sentFields["healthcheck"]; ok { + merged.HealthCheck = incoming.HealthCheck + } + if _, ok := sentFields["quick_actions"]; ok { + merged.QuickActions = incoming.QuickActions + } + if _, ok := sentFields["security"]; ok { + merged.Security = incoming.Security + } + if _, ok := sentFields["backup"]; ok { + merged.Backup = incoming.Backup + } + + return &merged +} + func (s *Server) deleteDeployment(c *gin.Context) { name := c.Param("name")