fix: transactional env var save, empty key validation, frontend error handling
All checks were successful
Check / check (pull_request) Successful in 4s
All checks were successful
Check / check (pull_request) Successful in 4s
- Wrap DELETE + INSERTs in a database transaction via new ReplaceEnvVarsByAppID() to prevent silent data loss on partial insert failure. Rollback on any error; return 500 instead of 200. - Add server-side validation rejecting entries with empty keys (returns 400 with error message). - Add frontend error handling for non-2xx responses with user-visible alert messages. - Remove stale //nolint:dupl directives (files no longer duplicate).
This commit is contained in:
@@ -912,7 +912,7 @@ type envPairJSON struct {
|
||||
// HandleEnvVarSave handles bulk saving of all environment variables.
|
||||
// It reads a JSON array of {key, value} objects from the request body,
|
||||
// deletes all existing env vars for the app, and inserts the full
|
||||
// submitted set.
|
||||
// submitted set atomically within a database transaction.
|
||||
func (h *Handlers) HandleEnvVarSave() http.HandlerFunc {
|
||||
return func(writer http.ResponseWriter, request *http.Request) {
|
||||
appID := chi.URLParam(request, "id")
|
||||
@@ -933,36 +933,36 @@ func (h *Handlers) HandleEnvVarSave() http.HandlerFunc {
|
||||
return
|
||||
}
|
||||
|
||||
ctx := request.Context()
|
||||
// Validate: reject entries with empty keys
|
||||
var modelPairs []models.EnvVarPair
|
||||
|
||||
// Delete all existing env vars for this app
|
||||
deleteErr := models.DeleteEnvVarsByAppID(ctx, h.db, application.ID)
|
||||
if deleteErr != nil {
|
||||
h.log.Error("failed to delete env vars", "error", deleteErr)
|
||||
http.Error(
|
||||
writer,
|
||||
"Internal Server Error",
|
||||
http.StatusInternalServerError,
|
||||
)
|
||||
for _, p := range pairs {
|
||||
trimmedKey := strings.TrimSpace(p.Key)
|
||||
if trimmedKey == "" {
|
||||
h.respondJSON(writer, request, map[string]string{
|
||||
"error": "empty environment variable key is not allowed",
|
||||
}, http.StatusBadRequest)
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
// Insert the full new set
|
||||
for _, p := range pairs {
|
||||
envVar := models.NewEnvVar(h.db)
|
||||
envVar.AppID = application.ID
|
||||
envVar.Key = p.Key
|
||||
envVar.Value = p.Value
|
||||
|
||||
saveErr := envVar.Save(ctx)
|
||||
if saveErr != nil {
|
||||
h.log.Error(
|
||||
"failed to save env var",
|
||||
"key", p.Key,
|
||||
"error", saveErr,
|
||||
)
|
||||
modelPairs = append(modelPairs, models.EnvVarPair{
|
||||
Key: trimmedKey,
|
||||
Value: p.Value,
|
||||
})
|
||||
}
|
||||
|
||||
// Atomically replace all env vars in a transaction
|
||||
ctx := request.Context()
|
||||
|
||||
replaceErr := models.ReplaceEnvVarsByAppID(ctx, h.db, application.ID, modelPairs)
|
||||
if replaceErr != nil {
|
||||
h.log.Error("failed to replace env vars", "error", replaceErr)
|
||||
h.respondJSON(writer, request, map[string]string{
|
||||
"error": "failed to save environment variables",
|
||||
}, http.StatusInternalServerError)
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
h.respondJSON(writer, request, map[string]bool{"ok": true}, http.StatusOK)
|
||||
|
||||
@@ -1,4 +1,3 @@
|
||||
//nolint:dupl // Active Record pattern - similar structure to label.go is intentional
|
||||
package models
|
||||
|
||||
import (
|
||||
@@ -139,3 +138,49 @@ func DeleteEnvVarsByAppID(
|
||||
|
||||
return err
|
||||
}
|
||||
|
||||
// EnvVarPair is a key-value pair for bulk env var operations.
|
||||
type EnvVarPair struct {
|
||||
Key string
|
||||
Value string
|
||||
}
|
||||
|
||||
// ReplaceEnvVarsByAppID atomically replaces all env vars for an app
|
||||
// within a single database transaction. It deletes all existing env
|
||||
// vars and inserts the provided pairs. If any operation fails, the
|
||||
// entire transaction is rolled back.
|
||||
func ReplaceEnvVarsByAppID(
|
||||
ctx context.Context,
|
||||
db *database.Database,
|
||||
appID string,
|
||||
pairs []EnvVarPair,
|
||||
) error {
|
||||
tx, err := db.BeginTx(ctx, nil)
|
||||
if err != nil {
|
||||
return fmt.Errorf("beginning transaction: %w", err)
|
||||
}
|
||||
|
||||
defer func() { _ = tx.Rollback() }()
|
||||
|
||||
_, err = tx.ExecContext(ctx, "DELETE FROM app_env_vars WHERE app_id = ?", appID)
|
||||
if err != nil {
|
||||
return fmt.Errorf("deleting env vars: %w", err)
|
||||
}
|
||||
|
||||
for _, p := range pairs {
|
||||
_, err = tx.ExecContext(ctx,
|
||||
"INSERT INTO app_env_vars (app_id, key, value) VALUES (?, ?, ?)",
|
||||
appID, p.Key, p.Value,
|
||||
)
|
||||
if err != nil {
|
||||
return fmt.Errorf("inserting env var %q: %w", p.Key, err)
|
||||
}
|
||||
}
|
||||
|
||||
err = tx.Commit()
|
||||
if err != nil {
|
||||
return fmt.Errorf("committing transaction: %w", err)
|
||||
}
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
@@ -1,4 +1,3 @@
|
||||
//nolint:dupl // Active Record pattern - similar structure to env_var.go is intentional
|
||||
package models
|
||||
|
||||
import (
|
||||
|
||||
@@ -73,10 +73,29 @@ document.addEventListener("alpine:init", () => {
|
||||
body: JSON.stringify(
|
||||
this.vars.map((e) => ({ key: e.key, value: e.value })),
|
||||
),
|
||||
}).then((res) => {
|
||||
})
|
||||
.then((res) => {
|
||||
if (res.ok) {
|
||||
window.location.reload();
|
||||
return;
|
||||
}
|
||||
res.json()
|
||||
.then((data) => {
|
||||
window.alert(
|
||||
data.error ||
|
||||
"Failed to save environment variables.",
|
||||
);
|
||||
})
|
||||
.catch(() => {
|
||||
window.alert(
|
||||
"Failed to save environment variables.",
|
||||
);
|
||||
});
|
||||
})
|
||||
.catch(() => {
|
||||
window.alert(
|
||||
"Network error: could not save environment variables.",
|
||||
);
|
||||
});
|
||||
},
|
||||
}));
|
||||
|
||||
Reference in New Issue
Block a user