From ab172c133ed2a1ae127175ed4ef8033529d997e7 Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Fri, 14 Aug 2026 17:47:02 +0100 Subject: [PATCH 1/9] feat!: migrate to the new update-flag contract, and `flag update --weight` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The experimental update-flag endpoints are being consolidated into one (Flagsmith/flagsmith#8102), and the CLI's flag mutations move to it: PATCH|PUT /api/__future__/environments/{key}/features/{feature_id}/ The feature moves from the body to the path, so mutations resolve it to an id; segments are named by `{"segment": {"id": n}}`; a refused write is a 400 with a `detail` rather than a 403; and the response carries the flag's whole state in the environment, so the post-update render reads what the server actually wrote. PATCH writes only the properties it carries. That removes the read-then- echo the old endpoint forced on every call: `--enable` no longer restates the value, a segment edit no longer restates the environment default, and `flag reorder` sends priorities alone — so a move can no longer disturb a state, and it needs one request fewer. Deleting an override loses its dedicated endpoint: a PUT replaces the set and whatever it leaves out is gone. So `flag delete --segment` reads the survivors and restates them in full, since a partial echo would reset the state it omits. That makes it a read-modify-write, and a concurrent change to another override is lost — the tradeoff the contract imposes. In exchange, a segment with no override is now caught before the write. `--weight =` re-weights a multivariate flag, per environment or per segment, closing #46. Weights are per scope, so they are read from the scope's feature state rather than from the variants' project-level defaults, and merged: a partial `--weight` re-weights what it names and keeps the rest, which the endpoint requires anyway since it rejects a variant list that isn't complete. Everything the endpoint would take as a new variant, or reject, is caught first — an unknown key or id, a total over 100, and `control`, which is not a variant but the share the weights leave unallocated. The flag detail views grow the Variants block the RFC describes, showing the weights in force for that scope; it costs one extra read, and only for a feature that has variants. The endpoint is not implemented server-side yet — #8102 is tests, types and docs — so this is verified against the contract those tests describe, not against a live API. --- README.md | 2 +- internal/api/client.go | 244 +++++--- internal/api/client_test.go | 122 +++- internal/cmd/cmd_test.go | 1099 +++++++++++++++++++++++++++------ internal/cmd/feature.go | 49 +- internal/cmd/flag_identity.go | 2 +- internal/cmd/flag_reorder.go | 75 +-- internal/cmd/flag_update.go | 388 +++++++++--- internal/cmd/flags.go | 183 +++++- 9 files changed, 1671 insertions(+), 493 deletions(-) diff --git a/README.md b/README.md index 3755395..6771d7e 100644 --- a/README.md +++ b/README.md @@ -59,7 +59,7 @@ flagsmith flag list # list the flags in the current environment - `flagsmith init` — bind the current directory to a project (writes `flagsmith.json`). - `flagsmith flag list` — list feature flags in the current environment. - `flagsmith flag get ` — show a single flag's state (`--segment ` or `--identifier ` for an override). -- `flagsmith flag update ` — toggle (`--enable`/`--disable`) or set the value (`--value`, `--type`); `--segment ` or `--identifier ` targets an override. +- `flagsmith flag update ` — toggle (`--enable`/`--disable`), set the value (`--value`, `--type`), or re-weight a multivariate flag's variants (`--weight =`); `--segment ` or `--identifier ` targets an override. - `flagsmith flag enable|disable ` — shorthand for `flag update --enable`/`--disable` (same `--segment`/`--identifier` targeting). - `flagsmith flag delete --segment |--identifier ` — delete a segment or identity override. - `flagsmith segment list` — list segments (`--include-feature-specific` to include feature-scoped ones). diff --git a/internal/api/client.go b/internal/api/client.go index 7558a60..f561b3d 100644 --- a/internal/api/client.go +++ b/internal/api/client.go @@ -339,27 +339,47 @@ func apiMessage(body []byte) string { if json.Unmarshal(body, &detail) == nil && detail.Detail != "" { return detail.Detail } - // Field-keyed validation errors: {"field": ["msg"]} or {"field": "msg"}. + // Field-keyed validation errors: {"field": ["msg"]} or {"field": "msg"}, + // nested to whatever depth the field's own shape has — update-flag reports + // {"segment_overrides": [{"segment": {"id": ["Segment not found."]}}]}. var fields map[string]json.RawMessage if json.Unmarshal(body, &fields) != nil { return "" } var msgs []string for _, raw := range fields { - var s string - if json.Unmarshal(raw, &s) == nil { - msgs = append(msgs, s) - continue - } - var arr []string - if json.Unmarshal(raw, &arr) == nil { - msgs = append(msgs, arr...) - } + msgs = append(msgs, errorStrings(raw)...) } sort.Strings(msgs) // map order is random; keep the message stable return strings.Join(msgs, "; ") } +// errorStrings collects every string leaf of a DRF error value. +func errorStrings(raw json.RawMessage) []string { + var s string + if json.Unmarshal(raw, &s) == nil { + return []string{s} + } + var list []json.RawMessage + if json.Unmarshal(raw, &list) == nil { + var msgs []string + for _, item := range list { + msgs = append(msgs, errorStrings(item)...) + } + return msgs + } + var fields map[string]json.RawMessage + if json.Unmarshal(raw, &fields) == nil { + var msgs []string + for _, value := range fields { + msgs = append(msgs, errorStrings(value)...) + } + sort.Strings(msgs) // map order is random; keep the message stable + return msgs + } + return nil +} + // Verbatim backend exception detail strings, matched case-insensitively against // the API's detail message — the only signal on the wire. var ( @@ -707,15 +727,24 @@ func (v TypedValue) Scalar() any { return nil } +// MultivariateStateValue is one variant's weight in a single feature state — +// the per-scope allocation, which starts from the option's project-level +// default and drifts as environments and segments are re-weighted. +type MultivariateStateValue struct { + OptionID int `json:"multivariate_feature_option"` + Allocation float64 `json:"percentage_allocation"` +} + // EnvironmentFeatureState is one row of the admin featurestates list: a // feature's state for the environment default (feature_segment null), one // segment override, or (in v2-versioned environments) an identity override. type EnvironmentFeatureState struct { - ID int `json:"id"` - Enabled bool `json:"enabled"` - FeatureSegment *int `json:"feature_segment"` - Identity *int `json:"identity"` - Value TypedValue `json:"feature_state_value"` + ID int `json:"id"` + Enabled bool `json:"enabled"` + FeatureSegment *int `json:"feature_segment"` + Identity *int `json:"identity"` + Value TypedValue `json:"feature_state_value"` + Multivariate []MultivariateStateValue `json:"multivariate_feature_state_values"` } // FeatureStates lists a feature's live states in one environment. @@ -776,90 +805,138 @@ func (c *Client) EdgeIdentityOverrides(ctx context.Context, envKey string, featu return rows, nil } -// FeatureRef targets a feature by name or id (exactly one) in update-flag-v2. -type FeatureRef struct { - Name string `json:"name,omitempty"` - ID int `json:"id,omitempty"` -} - -// FeatureValue is a typed flag value in the update-flag-v2 wire form: the type -// as a word and the value always as a string. +// FeatureValue is a typed flag value in the update-flag wire form: the type as +// a word and the value always as a string. type FeatureValue struct { Type string `json:"type"` // "integer" | "string" | "boolean" Value string `json:"value"` // always a string; parsed server-side per type } -// EnvironmentDefault is the environment-wide state update-flag-v2 requires in -// full on every call. -type EnvironmentDefault struct { - Enabled bool `json:"enabled"` - Value FeatureValue `json:"value"` +// Variant is one multivariate variant's share of a scope's traffic, as a +// percentage between 0 and 100. Weights that don't add up to 100 leave the +// remainder serving the flag's own value. +type Variant struct { + ID int `json:"id"` + Weight float64 `json:"weight"` } -// SegmentOverride is one segment's state in the update-flag-v2 body. Priority, -// when set, moves the override to that position — the server renumbers the -// others around it, preserving their relative order. -type SegmentOverride struct { - SegmentID int `json:"segment_id"` - Enabled bool `json:"enabled"` - Value FeatureValue `json:"value"` - Priority *int `json:"priority,omitempty"` +// SegmentTarget names the segment an override belongs to. +type SegmentTarget struct { + ID int `json:"id"` } -// UpdateFlagRequest is the update-flag-v2 body. environment_default is always -// required; segment_overrides only creates/updates the segments listed and -// never removes others. This endpoint does not manage identity overrides. +// FlagStateUpdate is a change to a flag's state in one scope. Every field is +// optional: PATCH leaves an omitted one unchanged, and PUT resets it to the +// server's default — so a partial PUT is a destructive write. +// +// Variants is all-or-nothing: the endpoint rejects a list that doesn't name +// every variant of the feature, whatever the verb. +type FlagStateUpdate struct { + Enabled *bool `json:"enabled,omitempty"` + Value *FeatureValue `json:"value,omitempty"` + Variants []Variant `json:"variants,omitempty"` +} + +// SegmentOverrideUpdate is a change to one segment's override. Priority, when +// set, moves the override to that position — the server renumbers the others +// around it, preserving their relative order. +type SegmentOverrideUpdate struct { + Segment SegmentTarget `json:"segment"` + Enabled *bool `json:"enabled,omitempty"` + Priority *int `json:"priority,omitempty"` + Value *FeatureValue `json:"value,omitempty"` + Variants []Variant `json:"variants,omitempty"` +} + +// UpdateFlagRequest is the update-flag body. Under PATCH the overrides listed +// are created or updated and the rest are left alone; under PUT the list +// replaces the whole set, so an override missing from it is deleted — which is +// why SegmentOverrides distinguishes nil (absent) from empty (delete them all). +// The endpoint does not manage identity overrides. type UpdateFlagRequest struct { - Feature FeatureRef `json:"feature"` - EnvironmentDefault EnvironmentDefault `json:"environment_default"` - SegmentOverrides []SegmentOverride `json:"segment_overrides,omitempty"` + EnvironmentDefault *FlagStateUpdate `json:"environment_default,omitempty"` + SegmentOverrides []SegmentOverrideUpdate `json:"segment_overrides,omitzero"` } -// postUpdateFlags posts a body to one of the experimental flags-update -// endpoints (update-flag-v2, delete-segment-override), which share their status -// protocol: 403 means the environment is workflow-gated, 404 maps to notFound -// when it is non-nil, and 204/200 are success. -func (c *Client) postUpdateFlags(ctx context.Context, path string, payload any, notFound error) error { - body, err := json.Marshal(payload) - if err != nil { - return err - } - req, err := c.newRequest(ctx, http.MethodPost, path, bytes.NewReader(body)) - if err != nil { - return err +// FlagState is a flag's resulting state in one scope, as the update-flag +// endpoint reports it. Value is null for a feature with no value at all. +type FlagState struct { + Enabled bool `json:"enabled"` + Value *FeatureValue `json:"value"` + Variants []Variant `json:"variants"` +} + +// SegmentOverrideState is one segment override's resulting state. +type SegmentOverrideState struct { + Segment SegmentTarget `json:"segment"` + Priority int `json:"priority"` + Enabled bool `json:"enabled"` + Value *FeatureValue `json:"value"` + Variants []Variant `json:"variants"` +} + +// UpdateFlagResponse is the flag's complete state in the environment after the +// write, whichever properties the request carried. +type UpdateFlagResponse struct { + EnvironmentDefault FlagState `json:"environment_default"` + SegmentOverrides []SegmentOverrideState `json:"segment_overrides"` +} + +// Override returns the resulting state of one segment's override, or nil when +// the response carries none for it. +func (r *UpdateFlagResponse) Override(segmentID int) *SegmentOverrideState { + for i := range r.SegmentOverrides { + if r.SegmentOverrides[i].Segment.ID == segmentID { + return &r.SegmentOverrides[i] + } } - req.Header.Set("Content-Type", "application/json") - resp, err := c.httpClient.Do(req) + return nil +} + +func updateFlagPath(environmentKey string, featureID int) string { + return fmt.Sprintf("/api/__future__/environments/%s/features/%d/", environmentKey, featureID) +} + +// UpdateFlag applies a partial change to a flag's state in one environment, +// keyed by the environment's client-side key and the feature's id. Properties +// the request omits are left as they are. +func (c *Client) UpdateFlag(ctx context.Context, environmentKey string, featureID int, in UpdateFlagRequest) (*UpdateFlagResponse, error) { + return c.writeFlag(ctx, http.MethodPatch, environmentKey, featureID, in) +} + +// ReplaceFlag replaces each property the request carries in full — the only way +// to delete a segment override, by PUTting the overrides that survive it. +// Anything a replaced property leaves out is reset, so callers must echo the +// state they mean to keep. +func (c *Client) ReplaceFlag(ctx context.Context, environmentKey string, featureID int, in UpdateFlagRequest) (*UpdateFlagResponse, error) { + return c.writeFlag(ctx, http.MethodPut, environmentKey, featureID, in) +} + +func (c *Client) writeFlag(ctx context.Context, method, environmentKey string, featureID int, in UpdateFlagRequest) (*UpdateFlagResponse, error) { + out, err := send[UpdateFlagResponse](ctx, c, method, updateFlagPath(environmentKey, featureID), in) if err != nil { - return err - } - defer resp.Body.Close() - switch { - case resp.StatusCode == http.StatusForbidden: - return ErrWorkflowGated - case resp.StatusCode == http.StatusNotFound && notFound != nil: - return notFound - case resp.StatusCode != http.StatusNoContent && resp.StatusCode != http.StatusOK: - return responseError(http.MethodPost, req.URL.String(), resp) + return nil, classifyFlagWrite(err) } - return nil + return out, nil } -// DeleteSegmentOverride removes a feature's override for one segment, via the -// experimental delete-segment-override endpoint keyed by the environment key. -func (c *Client) DeleteSegmentOverride(ctx context.Context, environmentKey string, feature FeatureRef, segmentID int) error { - payload := map[string]any{ - "feature": feature, - "segment": map[string]int{"id": segmentID}, +// changeRequestPhrase is the verbatim backend detail for a refused write — +// matched case-insensitively, since the status (400) is shared with every +// validation error. +const changeRequestPhrase = "change requests enabled" + +// classifyFlagWrite recognises the one update-flag failure the user can act on. +func classifyFlagWrite(err error) error { + var e *statusError + if errors.As(err, &e) && e.code == http.StatusBadRequest && + strings.Contains(strings.ToLower(e.message), changeRequestPhrase) { + return ErrWorkflowGated } - return c.postUpdateFlags(ctx, - "/api/experiments/environments/"+environmentKey+"/delete-segment-override/", - payload, - fmt.Errorf("no override exists for segment %d", segmentID)) + return err } -// ErrWorkflowGated is returned when update-flag-v2 refuses because the -// environment has change-request workflows enabled. +// ErrWorkflowGated is returned when update-flag refuses because the environment +// has change-request workflows enabled. var ErrWorkflowGated = fmt.Errorf("this environment uses change-request workflows; direct updates are disabled") // ErrPlanGated marks a self-serve plan limit a user lifts by upgrading — seats, @@ -873,15 +950,6 @@ var ErrPlanGated = errors.New("not available on your organisation's current plan // classifyLimit); Flagsmith ships no machine-readable error code. var ErrQuotaExceeded = errors.New("resource limit reached on your organisation's current plan") -// UpdateFlag applies an environment-default change via the experimental -// update-flag-v2 endpoint, keyed by the environment's client-side key. The -// endpoint returns 204 No Content on success. -func (c *Client) UpdateFlag(ctx context.Context, environmentKey string, in UpdateFlagRequest) error { - return c.postUpdateFlags(ctx, - "/api/experiments/environments/"+environmentKey+"/update-flag-v2/", - in, nil) -} - // Environment carries the curated fields plus the raw API item, so JSON output // mirrors the server's full field set. Identified by APIKey, not id. type Environment struct { diff --git a/internal/api/client_test.go b/internal/api/client_test.go index 673c092..05cfb59 100644 --- a/internal/api/client_test.go +++ b/internal/api/client_test.go @@ -499,46 +499,120 @@ func TestFeatures(t *testing.T) { }) } -func TestDeleteSegmentOverride(t *testing.T) { - t.Run("posts the feature and segment, accepts 204", func(t *testing.T) { +// updateFlagServer answers one update-flag request with status and body, +// recording the request it saw. +func updateFlagServer(t *testing.T, status int, response string) (*httptest.Server, *http.Request, *map[string]any) { + t.Helper() + var seen http.Request + body := map[string]any{} + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + seen = *r + json.NewDecoder(r.Body).Decode(&body) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + fmt.Fprint(w, response) + })) + t.Cleanup(srv.Close) + return srv, &seen, &body +} + +func TestUpdateFlag(t *testing.T) { + enabled := true + + t.Run("patches the environment-keyed feature path, and decodes the resulting state", func(t *testing.T) { // Given - var body map[string]any - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.Method != http.MethodPost || - r.URL.Path != "/api/experiments/environments/envkey/delete-segment-override/" { - t.Errorf("request = %s %s", r.Method, r.URL.Path) - } - json.NewDecoder(r.Body).Decode(&body) - w.WriteHeader(http.StatusNoContent) - })) - defer srv.Close() + srv, seen, body := updateFlagServer(t, http.StatusOK, `{ + "environment_default": {"enabled": true, "value": {"type": "integer", "value": "10"}, "variants": [{"id": 7, "weight": 25.5}]}, + "segment_overrides": [{"segment": {"id": 12}, "priority": 1, "enabled": false, "value": null, "variants": []}] + }`) // When - err := testClient(srv.URL, APIKey("k.s"), srv).DeleteSegmentOverride(context.Background(), "envkey", FeatureRef{Name: "max_items"}, 12) + resp, err := testClient(srv.URL, APIKey("k.s"), srv).UpdateFlag(context.Background(), "envkey", 42, + UpdateFlagRequest{EnvironmentDefault: &FlagStateUpdate{Enabled: &enabled}}) // Then if err != nil { t.Fatal(err) } - if body["feature"].(map[string]any)["name"] != "max_items" || - body["segment"].(map[string]any)["id"] != float64(12) { - t.Errorf("body = %+v", body) + if seen.Method != http.MethodPatch || seen.URL.Path != "/api/__future__/environments/envkey/features/42/" { + t.Errorf("request = %s %s", seen.Method, seen.URL.Path) + } + if _, ok := (*body)["segment_overrides"]; ok { + t.Errorf("body = %+v, want no segment_overrides key", *body) + } + if !resp.EnvironmentDefault.Enabled || resp.EnvironmentDefault.Value.Value != "10" { + t.Errorf("environment default = %+v", resp.EnvironmentDefault) + } + if len(resp.EnvironmentDefault.Variants) != 1 || resp.EnvironmentDefault.Variants[0].Weight != 25.5 { + t.Errorf("variants = %+v", resp.EnvironmentDefault.Variants) + } + override := resp.Override(12) + if override == nil || override.Priority != 1 || override.Value != nil { + t.Errorf("override = %+v", override) + } + if resp.Override(13) != nil { + t.Errorf("override for an unwritten segment = %+v, want none", resp.Override(13)) } }) - t.Run("404 becomes a no-override error", func(t *testing.T) { + t.Run("puts an empty override list verbatim, so the last override can be deleted", func(t *testing.T) { // Given - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusNotFound) - })) - defer srv.Close() + srv, seen, body := updateFlagServer(t, http.StatusOK, + `{"environment_default": {"enabled": false, "value": null, "variants": []}, "segment_overrides": []}`) + + // When + _, err := testClient(srv.URL, APIKey("k.s"), srv).ReplaceFlag(context.Background(), "envkey", 42, + UpdateFlagRequest{SegmentOverrides: []SegmentOverrideUpdate{}}) + + // Then + if err != nil { + t.Fatal(err) + } + if seen.Method != http.MethodPut { + t.Errorf("method = %s, want PUT", seen.Method) + } + overrides, ok := (*body)["segment_overrides"] + if !ok { + t.Fatalf("body = %+v, want an empty segment_overrides list", *body) + } + if got := overrides.([]any); len(got) != 0 { + t.Errorf("segment_overrides = %+v, want empty", got) + } + if _, ok := (*body)["environment_default"]; ok { + t.Errorf("body = %+v, want no environment_default — a PUT would reset it", *body) + } + }) + + t.Run("a change-request refusal is the workflow sentinel, not a bug", func(t *testing.T) { + // Given + srv, _, _ := updateFlagServer(t, http.StatusBadRequest, + `{"detail": "Cannot update flags in an environment with change requests enabled."}`) + + // When + _, err := testClient(srv.URL, APIKey("k.s"), srv).UpdateFlag(context.Background(), "envkey", 42, + UpdateFlagRequest{EnvironmentDefault: &FlagStateUpdate{Enabled: &enabled}}) + + // Then + if !errors.Is(err, ErrWorkflowGated) { + t.Errorf("err = %v, want ErrWorkflowGated", err) + } + }) + + t.Run("a validation error surfaces the field message, however deeply nested", func(t *testing.T) { + // Given + srv, _, _ := updateFlagServer(t, http.StatusBadRequest, + `{"segment_overrides": [{"segment": {"id": ["Segment not found."]}}]}`) // When - err := testClient(srv.URL, APIKey("k.s"), srv).DeleteSegmentOverride(context.Background(), "envkey", FeatureRef{Name: "max_items"}, 12) + _, err := testClient(srv.URL, APIKey("k.s"), srv).UpdateFlag(context.Background(), "envkey", 42, + UpdateFlagRequest{SegmentOverrides: []SegmentOverrideUpdate{{Segment: SegmentTarget{ID: 12}}}}) // Then - if err == nil || !strings.Contains(err.Error(), "segment 12") { - t.Errorf("err = %v, want a no-override error", err) + if err == nil || !strings.Contains(err.Error(), "Segment not found.") { + t.Errorf("err = %v, want the API's own message", err) + } + if errors.Is(err, ErrWorkflowGated) { + t.Errorf("err = %v, want a plain failure", err) } }) } diff --git a/internal/cmd/cmd_test.go b/internal/cmd/cmd_test.go index 0b98cd2..f6b29db 100644 --- a/internal/cmd/cmd_test.go +++ b/internal/cmd/cmd_test.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "io" "net/http" "net/http/httptest" @@ -86,6 +87,7 @@ func resetFlags() { apiFieldFlags = nil apiRawFields = nil evalTraitFlags = nil + flagWeightFlags = nil } // setEnvCred exports a credential variable host-scoped to url, as a @@ -167,11 +169,11 @@ type fakeInstance struct { projGetCalls int // count of GET /projects/{id}/ (retrieve) orgListCalls int // count of GET /organisations/ list calls tokenPosts int // count of POST /o/token/ (refresh) calls - updateCalls int // count of update-flag-v2 calls - lastUpdate map[string]any // last update-flag-v2 request body - lastDelete map[string]any // last delete-segment-override request body - workflowGated bool // when true, update endpoints return 403 - segmentMissing bool // when true, delete-segment-override returns 404 + updateCalls int // count of update-flag calls + lastUpdate map[string]any // last update-flag request body + lastUpdateVerb string // method of the last update-flag call + lastUpdateFeat string // feature id in the last update-flag path + workflowGated bool // when true, update-flag reports change requests useEdge bool // GET /projects/{id}/ use_edge_identities coreIdentities map[string]int // identifier -> identity id @@ -627,46 +629,36 @@ func newFakeInstance(t *testing.T) *fakeInstance { "count": len(items), "next": nil, "previous": nil, "results": items, }) }) - mux.HandleFunc("POST /api/experiments/environments/{env}/update-flag-v2/", func(w http.ResponseWriter, r *http.Request) { - if !authorized(r) { - w.WriteHeader(http.StatusUnauthorized) - return - } - f.mu.Lock() - gated := f.workflowGated - f.mu.Unlock() - if gated { - w.WriteHeader(http.StatusForbidden) - return - } - var body map[string]any - json.NewDecoder(r.Body).Decode(&body) - f.mu.Lock() - f.updateCalls++ - f.lastUpdate = body - f.applyFlagUpdate(body) - f.mu.Unlock() - w.WriteHeader(http.StatusNoContent) - }) - mux.HandleFunc("POST /api/experiments/environments/{env}/delete-segment-override/", func(w http.ResponseWriter, r *http.Request) { - if !authorized(r) { - w.WriteHeader(http.StatusUnauthorized) - return - } - f.mu.Lock() - missing := f.segmentMissing - f.mu.Unlock() - if missing { - w.WriteHeader(http.StatusNotFound) - return - } - var body map[string]any - json.NewDecoder(r.Body).Decode(&body) - f.mu.Lock() - f.lastDelete = body - f.mu.Unlock() - w.WriteHeader(http.StatusNoContent) - }) + // update-flag: PATCH writes only the properties it carries, PUT replaces + // each property it carries in full (so an override it leaves out is gone). + for _, method := range []string{http.MethodPatch, http.MethodPut} { + mux.HandleFunc(method+" /api/__future__/environments/{env}/features/{feature}/", func(w http.ResponseWriter, r *http.Request) { + if !authorized(r) { + w.WriteHeader(http.StatusUnauthorized) + return + } + f.mu.Lock() + gated := f.workflowGated + f.mu.Unlock() + if gated { + w.WriteHeader(http.StatusBadRequest) + json.NewEncoder(w).Encode(map[string]any{ + "detail": "Cannot update flags in an environment with change requests enabled.", + }) + return + } + var body map[string]any + json.NewDecoder(r.Body).Decode(&body) + f.mu.Lock() + f.updateCalls++ + f.lastUpdate = body + f.lastUpdateVerb = r.Method + f.lastUpdateFeat = r.PathValue("feature") + resp := f.applyFlagUpdate(r.PathValue("feature"), r.Method == http.MethodPut, body) + f.mu.Unlock() + json.NewEncoder(w).Encode(resp) + }) + } // Project retrieve — carries use_edge_identities. mux.HandleFunc("GET /api/v1/projects/{project}/", func(w http.ResponseWriter, r *http.Request) { if !authorized(r) { @@ -1372,58 +1364,232 @@ func newFakeInstance(t *testing.T) *fakeInstance { return f } -// applyFlagUpdate mutates the stored features to reflect an update-flag-v2 -// body, so a re-fetch after the mutation sees the new state. Called under lock. -func (f *fakeInstance) applyFlagUpdate(body map[string]any) { - feature, _ := body["feature"].(map[string]any) - name, _ := feature["name"].(string) - def, _ := body["environment_default"].(map[string]any) - enabled, _ := def["enabled"].(bool) - val, _ := def["value"].(map[string]any) - overrides, _ := body["segment_overrides"].([]any) +// applyFlagUpdate mutates the stored feature to reflect an update-flag body, so +// a re-fetch after the mutation sees the new state, and returns the flag's +// resulting state in the environment as the endpoint reports it. replace models +// PUT: a property the body carries is written in full, so anything it leaves +// out is reset, and an override missing from the list is deleted. Called under +// lock. +func (f *fakeInstance) applyFlagUpdate(featureKey string, replace bool, body map[string]any) map[string]any { + item := f.featureItem(featureKey) + if item == nil { + return map[string]any{} + } + if def, ok := body["environment_default"].(map[string]any); ok { + f.writeScope(ensureMap(item, "environment_feature_state"), f.stateRow(featureKey, 0), def, replace) + } + if overrides, ok := body["segment_overrides"].([]any); ok { + written := map[int]bool{} + for _, o := range overrides { + ov, _ := o.(map[string]any) + segment := wireSegmentID(ov) + written[segment] = true + row := f.ensureFeatureSegment(featureKey, segment) + if priority, ok := ov["priority"].(float64); ok { + row["priority"] = int(priority) + } + stateID, _ := row["id"].(int) + // The fake keeps one segment_feature_state per feature: it is only + // ever read back through the features list's ?segment= filter. + f.writeScope(ensureMap(item, "segment_feature_state"), f.stateRow(featureKey, stateID), ov, replace) + } + if replace { + f.deleteOverridesExcept(featureKey, item, written) + } + f.sortOverrides(featureKey) + } + return f.flagStateResponse(featureKey, item) +} + +// writeScope applies one scope's requested state to the features-list view of +// it and, when the test registered one, to its featurestates row — the only +// place a scope's variant weights live. +func (f *fakeInstance) writeScope(view, row, in map[string]any, replace bool) { + if enabled, ok := in["enabled"].(bool); ok { + view["enabled"] = enabled + if row != nil { + row["enabled"] = enabled + } + } else if replace { + view["enabled"] = false + if row != nil { + row["enabled"] = false + } + } + if value, ok := in["value"].(map[string]any); ok { + view["feature_state_value"] = scalarFromWire(value) + if row != nil { + row["feature_state_value"] = typedFromWire(value) + } + } else if replace { + view["feature_state_value"] = nil + if row != nil { + row["feature_state_value"] = typedFromWire(nil) + } + } + if row == nil { + return + } + if variants, ok := in["variants"].([]any); ok { + row["multivariate_feature_state_values"] = allocationsFromWire(variants) + } else if replace { + row["multivariate_feature_state_values"] = []map[string]any{} + } +} + +// flagStateResponse is the flag's whole state in the environment: the default, +// plus one entry per surviving segment override in priority order. +func (f *fakeInstance) flagStateResponse(featureKey string, item map[string]any) map[string]any { + view, _ := item["environment_feature_state"].(map[string]any) + enabled, _ := view["enabled"].(bool) + overrides := []map[string]any{} + for _, fs := range f.featureSegments[featureKey] { + stateID, _ := fs["id"].(int) + segment, _ := fs["segment"].(int) + priority, _ := fs["priority"].(int) + // Without a registered featurestates row the fake has only the single + // segment_feature_state to report the override from. + ovEnabled, ovValue := false, any(nil) + if seg, ok := item["segment_feature_state"].(map[string]any); ok { + ovEnabled, _ = seg["enabled"].(bool) + ovValue = seg["feature_state_value"] + } + row := f.stateRow(featureKey, stateID) + if row != nil { + ovEnabled, _ = row["enabled"].(bool) + typed, _ := row["feature_state_value"].(map[string]any) + ovValue = scalarFromTyped(typed) + } + overrides = append(overrides, map[string]any{ + "segment": map[string]any{"id": segment}, + "priority": priority, + "enabled": ovEnabled, + "value": wireValue(ovValue), + "variants": wireVariants(row), + }) + } + return map[string]any{ + "environment_default": map[string]any{ + "enabled": enabled, + "value": wireValue(view["feature_state_value"]), + "variants": wireVariants(f.stateRow(featureKey, 0)), + }, + "segment_overrides": overrides, + } +} + +// featureItem finds a stored feature item by its id, as a path parameter +// spells it, across every project (caller holds the lock). +func (f *fakeInstance) featureItem(featureKey string) map[string]any { + id, err := strconv.Atoi(featureKey) + if err != nil { + return nil + } for _, items := range f.features { for _, item := range items { - if item["name"] != name { - continue - } - state, _ := item["environment_feature_state"].(map[string]any) - if state == nil { - state = map[string]any{} - item["environment_feature_state"] = state - } - state["enabled"] = enabled - state["feature_state_value"] = scalarFromWire(val) - featureKey := "" - if id, ok := item["id"].(int); ok { - featureKey = strconv.Itoa(id) - } - for _, o := range overrides { - ov, _ := o.(map[string]any) - segEnabled, _ := ov["enabled"].(bool) - segVal, _ := ov["value"].(map[string]any) - item["segment_feature_state"] = map[string]any{ - "enabled": segEnabled, "feature_state_value": scalarFromWire(segVal), - } - // A priority write moves the feature-segment row, so a re-fetch - // sees the new order. - if prio, ok := ov["priority"].(float64); ok { - for _, row := range f.featureSegments[featureKey] { - if seg, _ := row["segment"].(int); float64(seg) == ov["segment_id"] { - row["priority"] = int(prio) - } - } - } + if got, _ := item["id"].(int); got == id { + return item } - sort.SliceStable(f.featureSegments[featureKey], func(a, b int) bool { - pa, _ := f.featureSegments[featureKey][a]["priority"].(int) - pb, _ := f.featureSegments[featureKey][b]["priority"].(int) - return pa < pb - }) } } + return nil +} + +// stateRow returns the registered featurestates row for one scope — the +// environment default for stateID 0, otherwise the override linked by that +// feature-segment id — or nil when the test registered none. +func (f *fakeInstance) stateRow(featureKey string, stateID int) map[string]any { + for _, row := range f.featureStates[featureKey] { + link, ok := row["feature_segment"].(int) + if !ok && stateID == 0 { + return row + } + if ok && link == stateID { + return row + } + } + return nil +} + +// ensureFeatureSegment returns the feature-segment row for a segment, adding +// one for an override that doesn't exist yet. +func (f *fakeInstance) ensureFeatureSegment(featureKey string, segment int) map[string]any { + for _, row := range f.featureSegments[featureKey] { + if got, _ := row["segment"].(int); got == segment { + return row + } + } + if f.featureSegments == nil { + f.featureSegments = map[string][]map[string]any{} + } + row := map[string]any{ + "id": 9000 + segment, "segment": segment, "segment_name": "", + "priority": len(f.featureSegments[featureKey]), + } + f.featureSegments[featureKey] = append(f.featureSegments[featureKey], row) + return row +} + +// deleteOverridesExcept drops the overrides a replacing write left out. +func (f *fakeInstance) deleteOverridesExcept(featureKey string, item map[string]any, keep map[int]bool) { + kept := []map[string]any{} + for _, row := range f.featureSegments[featureKey] { + segment, _ := row["segment"].(int) + if keep[segment] { + kept = append(kept, row) + continue + } + stateID, _ := row["id"].(int) + f.deleteStateRow(featureKey, stateID) + } + f.featureSegments[featureKey] = kept + if len(kept) == 0 { + delete(item, "segment_feature_state") + } +} + +func (f *fakeInstance) deleteStateRow(featureKey string, stateID int) { + if f.featureStates[featureKey] == nil { + return + } + kept := []map[string]any{} + for _, row := range f.featureStates[featureKey] { + if link, ok := row["feature_segment"].(int); ok && link == stateID { + continue + } + kept = append(kept, row) + } + f.featureStates[featureKey] = kept } -// scalarFromWire turns an update-flag-v2 {type,value} into the bare scalar the +// sortOverrides keeps the feature-segment rows in priority order, as the +// endpoint returns them. +func (f *fakeInstance) sortOverrides(featureKey string) { + rows := f.featureSegments[featureKey] + sort.SliceStable(rows, func(a, b int) bool { + pa, _ := rows[a]["priority"].(int) + pb, _ := rows[b]["priority"].(int) + return pa < pb + }) +} + +// ensureMap returns a nested map on item, creating it when absent. +func ensureMap(item map[string]any, key string) map[string]any { + nested, _ := item[key].(map[string]any) + if nested == nil { + nested = map[string]any{} + item[key] = nested + } + return nested +} + +func wireSegmentID(override map[string]any) int { + segment, _ := override["segment"].(map[string]any) + id, _ := segment["id"].(float64) + return int(id) +} + +// scalarFromWire turns an update-flag {type,value} into the bare scalar the // features list would report. func scalarFromWire(val map[string]any) any { t, _ := val["type"].(string) @@ -1439,6 +1605,108 @@ func scalarFromWire(val map[string]any) any { } } +// typedFromWire turns an update-flag {type,value} into the nested typed form +// the featurestates rows carry. +func typedFromWire(val map[string]any) map[string]any { + switch scalar := scalarFromWire(val).(type) { + case int: + return map[string]any{"type": "int", "integer_value": scalar} + case bool: + return map[string]any{"type": "bool", "boolean_value": scalar} + default: + return map[string]any{"type": "unicode", "string_value": scalar} + } +} + +// scalarFromTyped reads a featurestates row's typed value back as a scalar. +func scalarFromTyped(typed map[string]any) any { + switch typed["type"] { + case "int": + return typed["integer_value"] + case "bool": + return typed["boolean_value"] + default: + return typed["string_value"] + } +} + +// wireValue types a scalar for an update-flag response. A feature with no value +// at all reports null. +func wireValue(scalar any) any { + switch v := scalar.(type) { + case nil: + return nil + case int: + return map[string]any{"type": "integer", "value": strconv.Itoa(v)} + case float64: + return map[string]any{"type": "integer", "value": strconv.Itoa(int(v))} + case bool: + return map[string]any{"type": "boolean", "value": strconv.FormatBool(v)} + default: + return map[string]any{"type": "string", "value": fmt.Sprint(v)} + } +} + +// allocationsFromWire turns requested variants into the featurestates row's +// per-scope allocations. +func allocationsFromWire(variants []any) []map[string]any { + allocations := make([]map[string]any, 0, len(variants)) + for _, v := range variants { + variant, _ := v.(map[string]any) + id, _ := variant["id"].(float64) + weight, _ := variant["weight"].(float64) + allocations = append(allocations, map[string]any{ + "multivariate_feature_option": int(id), "percentage_allocation": weight, + }) + } + return allocations +} + +// wireVariants reports a scope's allocations the way the update-flag response +// does. A scope with none — every standard feature — reports an empty list. +func wireVariants(row map[string]any) []map[string]any { + variants := []map[string]any{} + if row == nil { + return variants + } + for _, a := range allocationRows(row) { + variants = append(variants, map[string]any{ + "id": int(number(a["multivariate_feature_option"])), + "weight": number(a["percentage_allocation"]), + }) + } + return variants +} + +// allocationRows reads a featurestates row's allocations, whether they were +// registered by a test or written by an update. +func allocationRows(row map[string]any) []map[string]any { + switch allocations := row["multivariate_feature_state_values"].(type) { + case []map[string]any: + return allocations + case []any: + rows := make([]map[string]any, 0, len(allocations)) + for _, a := range allocations { + if row, ok := a.(map[string]any); ok { + rows = append(rows, row) + } + } + return rows + } + return nil +} + +// number reads a JSON-ish number written as any of Go's numeric types. +func number(v any) float64 { + switch n := v.(type) { + case int: + return float64(n) + case float64: + return n + } + return 0 +} + func (f *fakeInstance) revokeCount() int { f.mu.Lock() defer f.mu.Unlock() @@ -3546,7 +3814,9 @@ func flagUpdateEnv(t *testing.T) *fakeInstance { } func TestFlagUpdate(t *testing.T) { - t.Run("--enable preserves the current value and reprints", func(t *testing.T) { + // A partial write is the point of the endpoint: --enable sends the state it + // changes and nothing else, so the value is left alone rather than echoed. + t.Run("--enable sends only the state, and reprints the value it left alone", func(t *testing.T) { // Given f := flagUpdateEnv(t) @@ -3557,14 +3827,22 @@ func TestFlagUpdate(t *testing.T) { if err != nil { t.Fatalf("flag update: %v\noutput: %s", err, out) } + if f.lastUpdateVerb != http.MethodPatch || f.lastUpdateFeat != "2" { + t.Errorf("request = %s feature %s, want PATCH feature 2", f.lastUpdateVerb, f.lastUpdateFeat) + } def := f.lastUpdate["environment_default"].(map[string]any) - val := def["value"].(map[string]any) - if def["enabled"] != true || val["type"] != "integer" || val["value"] != "25" { - t.Errorf("environment_default = %+v", def) + if def["enabled"] != true || len(def) != 1 { + t.Errorf("environment_default = %+v, want enabled alone", def) + } + if _, ok := f.lastUpdate["segment_overrides"]; ok { + t.Errorf("body = %+v, want no segment_overrides key", f.lastUpdate) } if !strings.Contains(out, "Enabled max_items") { t.Errorf("output = %q, want an Enabled confirmation", out) } + if !strings.Contains(out, "25") { + t.Errorf("output = %q, want the untouched value reprinted", out) + } }) t.Run("--value infers integer", func(t *testing.T) { @@ -3685,20 +3963,14 @@ func withFeatureStates(f *fakeInstance, featureID int, rows ...map[string]any) { // The fake serves requests concurrently (see fsPeak), so every field a handler // reads is set through a locked setter rather than assigned directly. -// withWorkflowGating makes the update endpoints answer 403. +// withWorkflowGating makes update-flag refuse the write, as it does for an +// environment with change requests enabled. func withWorkflowGating(f *fakeInstance) { f.mu.Lock() defer f.mu.Unlock() f.workflowGated = true } -// withMissingSegmentOverride makes delete-segment-override answer 404. -func withMissingSegmentOverride(f *fakeInstance) { - f.mu.Lock() - defer f.mu.Unlock() - f.segmentMissing = true -} - // withEdgeIdentities makes the project report use_edge_identities. func withEdgeIdentities(f *fakeInstance) { f.mu.Lock() @@ -3840,19 +4112,18 @@ func TestSegmentOverridePriorityView(t *testing.T) { } func TestFlagEnableDisable(t *testing.T) { - t.Run("enable turns the environment default on, preserving value", func(t *testing.T) { + t.Run("enable turns the environment default on, leaving the value alone", func(t *testing.T) { f := flagUpdateEnv(t) // max_items is off, integer 25 out, err := run("", "flag", "enable", "max_items", "--yes") if err != nil { t.Fatalf("flag enable: %v\noutput: %s", err, out) } def := f.lastUpdate["environment_default"].(map[string]any) - val := def["value"].(map[string]any) - if def["enabled"] != true || val["type"] != "integer" || val["value"] != "25" { - t.Errorf("environment_default = %+v, want enabled with the value carried", def) + if def["enabled"] != true || len(def) != 1 { + t.Errorf("environment_default = %+v, want enabled alone", def) } - if !strings.Contains(out, "Enabled max_items") { - t.Errorf("output = %q, want an Enabled confirmation", out) + if !strings.Contains(out, "Enabled max_items") || !strings.Contains(out, "25") { + t.Errorf("output = %q, want an Enabled confirmation and the kept value", out) } }) @@ -3877,7 +4148,7 @@ func TestFlagEnableDisable(t *testing.T) { t.Fatalf("flag enable --segment: %v", err) } ov := f.lastUpdate["segment_overrides"].([]any)[0].(map[string]any) - if ov["segment_id"] != float64(7) || ov["enabled"] != true { + if wireSegmentID(ov) != 7 || ov["enabled"] != true { t.Errorf("segment override = %+v, want enabled for segment 7", ov) } }) @@ -3893,7 +4164,7 @@ func TestFlagEnableDisable(t *testing.T) { } func TestFlagUpdateSegment(t *testing.T) { - t.Run("updates the override and carries the env default unchanged", func(t *testing.T) { + t.Run("writes the override alone, touching neither the default nor the rest", func(t *testing.T) { // Given f := flagUpdateEnv(t) withSegmentOverride(f, true) @@ -3905,26 +4176,28 @@ func TestFlagUpdateSegment(t *testing.T) { if err != nil { t.Fatalf("flag update --segment: %v\noutput: %s", err, out) } - def := f.lastUpdate["environment_default"].(map[string]any) - defVal := def["value"].(map[string]any) - if def["enabled"] != false || defVal["type"] != "integer" || defVal["value"] != "25" { - t.Errorf("environment_default = %+v, want the current default carried unchanged", def) + if _, ok := f.lastUpdate["environment_default"]; ok { + t.Errorf("body = %+v, want no environment_default — nothing about it changed", f.lastUpdate) } ovs := f.lastUpdate["segment_overrides"].([]any) ov := ovs[0].(map[string]any) ovVal := ov["value"].(map[string]any) - if ov["segment_id"] != float64(12) || ov["enabled"] != true || - ovVal["type"] != "string" || ovVal["value"] != "new" { - t.Errorf("segment override = %+v, want enabled preserved and value \"new\"", ov) + if wireSegmentID(ov) != 12 || ovVal["type"] != "string" || ovVal["value"] != "new" { + t.Errorf("segment override = %+v, want value \"new\" for segment 12", ov) + } + if _, ok := ov["enabled"]; ok { + t.Errorf("segment override = %+v, want no enabled key — a value edit must not touch the state", ov) } if !strings.Contains(out, `Set max_items to "new" for segment 12 in environment`) { t.Errorf("output = %q", out) } }) - t.Run("a new override inherits the env default state", func(t *testing.T) { + // A new override is the one case the CLI states the inherited parts of + // outright, rather than leaving them to the server. + t.Run("a new override states the env default's state and joins at the end", func(t *testing.T) { // Given - f := flagUpdateEnv(t) + f := flagUpdateEnv(t) // onboarding_banner is on, with no overrides // When out, err := run("", "flag", "update", "onboarding_banner", "--segment", "7", "--value", "yo", "--yes") @@ -3937,11 +4210,14 @@ func TestFlagUpdateSegment(t *testing.T) { if ov["enabled"] != true { t.Errorf("segment override = %+v, want enabled inherited from the env default (on)", ov) } + if ov["priority"] != float64(0) { + t.Errorf("segment override = %+v, want priority 0 — the first override", ov) + } }) - t.Run("a new override inherits the env default value", func(t *testing.T) { + t.Run("a new override leaves the value to the server, which inherits it", func(t *testing.T) { // Given - f := flagUpdateEnv(t) + f := flagUpdateEnv(t) // max_items has 1 override, integer 25, off withSegmentOverride(f, false) // When @@ -3952,10 +4228,11 @@ func TestFlagUpdateSegment(t *testing.T) { t.Fatalf("flag update --segment: %v", err) } ov := f.lastUpdate["segment_overrides"].([]any)[0].(map[string]any) - ovVal := ov["value"].(map[string]any) - if ov["segment_id"] != float64(7) || ov["enabled"] != true || - ovVal["type"] != "integer" || ovVal["value"] != "25" { - t.Errorf("segment override = %+v, want inherited integer 25", ov) + if wireSegmentID(ov) != 7 || ov["enabled"] != true || ov["priority"] != float64(1) { + t.Errorf("segment override = %+v, want segment 7 enabled at priority 1", ov) + } + if _, ok := ov["value"]; ok { + t.Errorf("segment override = %+v, want no value key — the server inherits it", ov) } }) } @@ -4088,8 +4365,8 @@ func TestFlagSegmentByName(t *testing.T) { t.Fatalf("flag update --segment beta-optin: %v", err) } ov := f.lastUpdate["segment_overrides"].([]any)[0].(map[string]any) - if ov["segment_id"] != float64(57) { - t.Errorf("segment_id = %v, want 57 resolved from beta-optin", ov["segment_id"]) + if wireSegmentID(ov) != 57 { + t.Errorf("segment = %v, want 57 resolved from beta-optin", ov["segment"]) } }) @@ -4102,7 +4379,7 @@ func TestFlagSegmentByName(t *testing.T) { t.Fatalf("flag disable --segment us-adults: %v", err) } ov := f.lastUpdate["segment_overrides"].([]any)[0].(map[string]any) - if ov["segment_id"] != float64(42) || ov["enabled"] != false { + if wireSegmentID(ov) != 42 || ov["enabled"] != false { t.Errorf("segment override = %+v, want segment 42 disabled", ov) } }) @@ -4110,13 +4387,16 @@ func TestFlagSegmentByName(t *testing.T) { t.Run("flag delete --segment resolves a name", func(t *testing.T) { f := flagUpdateEnv(t) withSegmentOverride(f, true) + withFeatureSegments(f, 2, map[string]any{ + "id": 4200, "segment": 42, "segment_name": "us-adults", "priority": 0, + }) _, err := run("", "flag", "delete", "max_items", "--segment", "us-adults", "--yes") if err != nil { t.Fatalf("flag delete --segment us-adults: %v", err) } - if f.lastDelete["segment"].(map[string]any)["id"] != float64(42) { - t.Errorf("delete body = %+v, want segment id 42", f.lastDelete) + if got := f.lastUpdate["segment_overrides"].([]any); len(got) != 0 { + t.Errorf("segment_overrides = %+v, want segment 42's override gone", got) } }) @@ -4133,18 +4413,19 @@ func TestFlagSegmentByName(t *testing.T) { } }) - t.Run("delete names the segment from the cache", func(t *testing.T) { + t.Run("delete names the segment from the override it removes", func(t *testing.T) { f := flagUpdateEnv(t) withSegmentOverride(f, true) + withFeatureSegments(f, 2, map[string]any{ + "id": 4200, "segment": 42, "segment_name": "us-adults", "priority": 0, + }) - // Resolving the name lists segments, which warms the cache the - // delete message reads. out, err := run("", "flag", "delete", "max_items", "--segment", "us-adults", "--yes") if err != nil { t.Fatalf("flag delete --segment us-adults: %v\noutput: %s", err, out) } if !strings.Contains(out, "Deleted max_items override for segment us-adults (42) in environment") { - t.Errorf("output = %q, want the segment named from the cache", out) + t.Errorf("output = %q, want the segment named from its override row", out) } }) @@ -4360,6 +4641,380 @@ func TestFlagListIdentityOverrides(t *testing.T) { }) } +// withMultivariateFlag adds banner_copy (feature 3) to project 101: two +// variants whose project-level defaults (30/50) have already drifted from the +// weights in force — 25/75 in the environment, 100/0 for segment 12's override +// — so a test can tell the scopes apart. +func withMultivariateFlag(f *fakeInstance) { + f.features["101"] = append(f.features["101"], map[string]any{ + "id": 3, "name": "banner_copy", "type": "MULTIVARIATE", + "description": "A/B banner text", "lifecycle_stage": "live", + "num_segment_overrides": 1, "num_identity_overrides": 0, + "code_references_counts": []any{}, + "environment_feature_state": map[string]any{"enabled": true, "feature_state_value": "hello"}, + "segment_feature_state": map[string]any{"enabled": true, "feature_state_value": "hello"}, + "multivariate_options": []any{ + map[string]any{"id": 30011, "type": "unicode", "string_value": "headline", "default_percentage_allocation": 30, "key": "hero"}, + map[string]any{"id": 30010, "type": "unicode", "string_value": "subhead", "default_percentage_allocation": 50, "key": "sub"}, + }, + }) + withFeatureSegments(f, 3, map[string]any{ + "id": 3100, "segment": 12, "segment_name": "early-adopters", "priority": 0, + }) + allocations := func(hero, sub float64) []map[string]any { + return []map[string]any{ + {"multivariate_feature_option": 30011, "percentage_allocation": hero}, + {"multivariate_feature_option": 30010, "percentage_allocation": sub}, + } + } + hello := map[string]any{"type": "unicode", "string_value": "hello"} + withFeatureStates(f, 3, + map[string]any{"id": 30, "enabled": true, "feature_segment": nil, + "feature_state_value": hello, "multivariate_feature_state_values": allocations(25, 75)}, + map[string]any{"id": 31, "enabled": true, "feature_segment": 3100, + "feature_state_value": hello, "multivariate_feature_state_values": allocations(100, 0)}, + ) +} + +// wireWeights reads the variants of one scope in an update-flag request body. +func wireWeights(scope map[string]any) map[int]float64 { + weights := map[int]float64{} + variants, _ := scope["variants"].([]any) + for _, v := range variants { + variant, _ := v.(map[string]any) + weights[int(number(variant["id"]))] = number(variant["weight"]) + } + return weights +} + +func TestFlagUpdateWeight(t *testing.T) { + t.Run("sends every variant, merged onto the environment's own weights", func(t *testing.T) { + // Given + f := flagUpdateEnv(t) + withMultivariateFlag(f) + + // When + out, err := run("", "flag", "update", "banner_copy", "--weight", "hero=20", "--yes") + + // Then + if err != nil { + t.Fatalf("flag update --weight: %v\noutput: %s", err, out) + } + def := f.lastUpdate["environment_default"].(map[string]any) + // The unnamed variant keeps the environment's 75, not its project + // default of 50 — and is still sent, since a partial list is rejected. + if got := wireWeights(def); got[30011] != 20 || got[30010] != 75 || len(got) != 2 { + t.Errorf("variants = %+v, want hero re-weighted to 20 and sub kept at 75", got) + } + if !strings.Contains(out, "Set banner_copy weights to hero=20 in environment") { + t.Errorf("output = %q, want a weights confirmation", out) + } + // The detail reprint grows a Variants block, showing the new weights. + for _, want := range []string{"Variants", "VALUE", "WEIGHT", "KEY", "ID", "headline", "20%", "subhead", "75%", "30011"} { + if !strings.Contains(out, want) { + t.Errorf("output = %q, want %q", out, want) + } + } + }) + + t.Run("one --weight can carry comma-separated pairs, by key or id", func(t *testing.T) { + // Given + f := flagUpdateEnv(t) + withMultivariateFlag(f) + + // When + _, err := run("", "flag", "update", "banner_copy", "--weight", "hero=10,30010=20", "--yes") + + // Then + if err != nil { + t.Fatalf("flag update --weight: %v", err) + } + def := f.lastUpdate["environment_default"].(map[string]any) + if got := wireWeights(def); got[30011] != 10 || got[30010] != 20 { + t.Errorf("variants = %+v, want hero=10 and the id-referenced sub=20", got) + } + }) + + t.Run("re-weights a segment override from its own distribution", func(t *testing.T) { + // Given + f := flagUpdateEnv(t) + withMultivariateFlag(f) + + // When + out, err := run("", "flag", "update", "banner_copy", "--segment", "12", "--weight", "hero=70", "--yes") + + // Then + if err != nil { + t.Fatalf("flag update --segment --weight: %v\noutput: %s", err, out) + } + ov := f.lastUpdate["segment_overrides"].([]any)[0].(map[string]any) + // sub keeps the override's own 0, not the environment's 75. + if got := wireWeights(ov); got[30011] != 70 || got[30010] != 0 { + t.Errorf("variants = %+v, want the override's own sub=0 kept", got) + } + if !strings.Contains(out, "Set banner_copy weights to hero=70 for segment early-adopters (12)") { + t.Errorf("output = %q", out) + } + }) + + t.Run("composes with a state and value change in one request", func(t *testing.T) { + // Given + f := flagUpdateEnv(t) + withMultivariateFlag(f) + + // When + out, err := run("", "flag", "update", "banner_copy", + "--disable", "--value", "bye", "--weight", "hero=50,sub=50", "--yes") + + // Then + if err != nil { + t.Fatalf("flag update: %v\noutput: %s", err, out) + } + f.mu.Lock() + calls := f.updateCalls + f.mu.Unlock() + if calls != 1 { + t.Errorf("update calls = %d, want one request", calls) + } + def := f.lastUpdate["environment_default"].(map[string]any) + if def["enabled"] != false || def["value"].(map[string]any)["value"] != "bye" { + t.Errorf("environment_default = %+v, want disabled with the new value", def) + } + if got := wireWeights(def); got[30011] != 50 || got[30010] != 50 { + t.Errorf("variants = %+v, want an even split", got) + } + }) + + // Weights that overflow, name nothing, or aren't weights at all are caught + // before the write: the endpoint would reject some of them, and take an + // unknown key as a new variant. + for _, tc := range []struct { + name, arg, want string + }{ + {"over 100 in total", "hero=40", ""}, // filled in below + {"not a pair", "hero", "is not ="}, + {"not a percentage", "hero=lots", "is not a percentage"}, + {"over 100 on its own", "hero=140", "is not a percentage"}, + {"negative", "hero=-1", "is not a percentage"}, + {"the same variant twice", "hero=10,hero=20", "given twice"}, + {"an unknown key", "heroic=10", "is not a variant of banner_copy"}, + {"an unknown id", "99999=10", "is not a variant of banner_copy"}, + {"the control", "control=10", `"control" is not a variant`}, + } { + t.Run(tc.name+" exits 2 without writing", func(t *testing.T) { + // Given + f := flagUpdateEnv(t) + withMultivariateFlag(f) + arg, want := tc.arg, tc.want + if want == "" { // the environment's sub=75 makes hero=40 overflow + want = "add up to 115%" + arg = "hero=40,sub=75" + } + + // When + _, err := run("", "flag", "update", "banner_copy", "--weight", arg, "--yes") + + // Then + var ue *usageError + if !errors.As(err, &ue) || !strings.Contains(err.Error(), want) { + t.Errorf("err = %v, want a usage error containing %q", err, want) + } + if f.lastUpdate != nil { + t.Errorf("lastUpdate = %+v, want no write", f.lastUpdate) + } + }) + } + + t.Run("a feature with no variants exits 2, pointing at feature variant", func(t *testing.T) { + // Given + f := flagUpdateEnv(t) + + // When + _, err := run("", "flag", "update", "max_items", "--weight", "hero=10", "--yes") + + // Then + var ue *usageError + if !errors.As(err, &ue) || !strings.Contains(err.Error(), "no variants to weight") { + t.Errorf("err = %v, want a usage error", err) + } + if hint := hintFor(err); !strings.Contains(hint, "feature variant add") { + t.Errorf("hint = %q, want a pointer to `feature variant add`", hint) + } + if f.lastUpdate != nil { + t.Errorf("lastUpdate = %+v, want no write", f.lastUpdate) + } + }) + + t.Run("an identity takes a value, not a distribution", func(t *testing.T) { + // Given + f := flagUpdateEnv(t) + withMultivariateFlag(f) + + // When + _, err := run("", "flag", "update", "banner_copy", "--identifier", "user-1", "--weight", "hero=10", "--yes") + + // Then + var ue *usageError + if !errors.As(err, &ue) || !strings.Contains(err.Error(), "mutually exclusive") { + t.Errorf("err = %v, want a usage error", err) + } + if f.lastUpdate != nil { + t.Errorf("lastUpdate = %+v, want no write", f.lastUpdate) + } + }) + + t.Run("bad syntax costs no request at all", func(t *testing.T) { + // Given + f := flagUpdateEnv(t) + withMultivariateFlag(f) + + // When + _, err := run("", "flag", "update", "banner_copy", "--weight", "=10", "--yes") + + // Then + var ue *usageError + if !errors.As(err, &ue) { + t.Errorf("err = %v, want a usage error", err) + } + if got := f.featuresCalls(); got != 0 { + t.Errorf("features calls = %d, want the flag parsed before any request", got) + } + }) +} + +func TestFlagGetVariants(t *testing.T) { + t.Run("shows the environment's weights, not the variants' defaults", func(t *testing.T) { + // Given + f := flagUpdateEnv(t) + withMultivariateFlag(f) + + // When + out, err := run("", "flag", "get", "banner_copy") + + // Then + if err != nil { + t.Fatalf("flag get: %v\noutput: %s", err, out) + } + for _, want := range []string{"multivariate", "Variants", "headline", "25%", "subhead", "75%", "hero", "30011"} { + if !strings.Contains(out, want) { + t.Errorf("output = %q, want %q", out, want) + } + } + if strings.Contains(out, "30%") || strings.Contains(out, "50%") { + t.Errorf("output = %q, want the environment's weights rather than the project defaults", out) + } + _ = f + }) + + t.Run("--json carries the variants", func(t *testing.T) { + // Given + f := flagUpdateEnv(t) + withMultivariateFlag(f) + + // When + out, err := run("", "flag", "get", "banner_copy", "--json") + + // Then + if err != nil { + t.Fatalf("flag get --json: %v", err) + } + var view struct { + Type string `json:"type"` + Variants []struct { + ID int `json:"id"` + Key string `json:"key"` + Value any `json:"value"` + Weight float64 `json:"weight"` + } `json:"variants"` + } + if err := json.Unmarshal([]byte(out), &view); err != nil { + t.Fatalf("json: %v\noutput: %s", err, out) + } + if view.Type != "multivariate" || len(view.Variants) != 2 { + t.Fatalf("view = %+v", view) + } + if v := view.Variants[0]; v.ID != 30011 || v.Key != "hero" || v.Value != "headline" || v.Weight != 25 { + t.Errorf("first variant = %+v", v) + } + _ = f + }) + + t.Run("--segment shows the override's weights", func(t *testing.T) { + // Given + f := flagUpdateEnv(t) + withMultivariateFlag(f) + + // When + out, err := run("", "flag", "get", "banner_copy", "--segment", "12") + + // Then + if err != nil { + t.Fatalf("flag get --segment: %v\noutput: %s", err, out) + } + for _, want := range []string{"early-adopters (12)", "Variants", "headline", "100%", "subhead", "0%"} { + if !strings.Contains(out, want) { + t.Errorf("output = %q, want %q", out, want) + } + } + _ = f + }) + + t.Run("a flag without variants shows no Variants block, and costs no state read", func(t *testing.T) { + // Given + f := flagUpdateEnv(t) + + // When + out, err := run("", "flag", "get", "max_items") + + // Then + if err != nil { + t.Fatalf("flag get: %v\noutput: %s", err, out) + } + if strings.Contains(out, "Variants") { + t.Errorf("output = %q, want no Variants block", out) + } + if got := f.featureStatesCalls(); got != 0 { + t.Errorf("featurestates calls = %d, want 0 for a standard feature", got) + } + }) +} + +func TestFlagDeleteKeepsVariants(t *testing.T) { + // Given a multivariate flag with two overrides, one of them being deleted. + f := flagUpdateEnv(t) + withMultivariateFlag(f) + withFeatureSegments(f, 3, + map[string]any{"id": 3100, "segment": 12, "segment_name": "early-adopters", "priority": 0}, + map[string]any{"id": 3200, "segment": 42, "segment_name": "us-adults", "priority": 1}, + ) + withFeatureStates(f, 3, + map[string]any{"id": 30, "enabled": true, "feature_segment": nil, + "feature_state_value": map[string]any{"type": "unicode", "string_value": "hello"}}, + map[string]any{"id": 32, "enabled": true, "feature_segment": 3200, + "feature_state_value": map[string]any{"type": "unicode", "string_value": "hi"}, + "multivariate_feature_state_values": []map[string]any{ + {"multivariate_feature_option": 30011, "percentage_allocation": 60}, + {"multivariate_feature_option": 30010, "percentage_allocation": 40}, + }}, + ) + + // When + out, err := run("", "flag", "delete", "banner_copy", "--segment", "12", "--yes") + + // Then the survivor's weights are restated, so replacing the set can't + // blank them. + if err != nil { + t.Fatalf("flag delete: %v\noutput: %s", err, out) + } + survivor := f.lastUpdate["segment_overrides"].([]any)[0].(map[string]any) + if wireSegmentID(survivor) != 42 { + t.Fatalf("survivor = %+v, want segment 42", survivor) + } + if got := wireWeights(survivor); got[30011] != 60 || got[30010] != 40 { + t.Errorf("survivor variants = %+v, want its own 60/40 restated", got) + } +} + func TestFlagUpdatePriority(t *testing.T) { // max_items (feature 2) has one override, for segment 12 at priority 1. overrideMeta := func(f *fakeInstance) { @@ -4378,13 +5033,8 @@ func TestFlagUpdatePriority(t *testing.T) { t.Fatalf("flag update --priority: %v\noutput: %s", err, out) } ov := f.lastUpdate["segment_overrides"].([]any)[0].(map[string]any) - if ov["priority"] != float64(0) { - t.Errorf("override = %+v, want priority 0", ov) - } - // The override's current state rides along unchanged. - ovVal := ov["value"].(map[string]any) - if ov["enabled"] != true || ovVal["value"] != "special" { - t.Errorf("override = %+v, want current state echoed", ov) + if ov["priority"] != float64(0) || len(ov) != 2 { + t.Errorf("override = %+v, want the segment and priority 0 alone", ov) } if !strings.Contains(out, "Set max_items priority to 0 for segment powerusers (12) in environment") { t.Errorf("output = %q, want a priority confirmation naming the segment", out) @@ -4458,24 +5108,26 @@ func TestFlagFeatureByID(t *testing.T) { if err != nil { t.Fatalf("flag update 2: %v\noutput: %s", err, out) } - feature := f.lastUpdate["feature"].(map[string]any) - if feature["name"] != "max_items" { - t.Errorf("feature ref = %+v, want the canonical name on the wire", feature) + if f.lastUpdateFeat != "2" { + t.Errorf("feature on the wire = %q, want id 2", f.lastUpdateFeat) } if !strings.Contains(out, "Enabled max_items") { t.Errorf("output = %q, want the canonical name in the message", out) } }) - t.Run("delete --segment by id targets the id on the wire", func(t *testing.T) { + t.Run("delete --segment by id targets the feature id in the path", func(t *testing.T) { f := flagUpdateEnv(t) + withFeatureSegments(f, 2, map[string]any{ + "id": 1200, "segment": 12, "segment_name": "powerusers", "priority": 0, + }) _, err := run("", "flag", "delete", "2", "--segment", "12", "--yes") if err != nil { t.Fatalf("flag delete 2: %v", err) } - if f.lastDelete["feature"].(map[string]any)["id"] != float64(2) { - t.Errorf("delete body = %+v, want the feature targeted by id", f.lastDelete) + if f.lastUpdateFeat != "2" { + t.Errorf("feature on the wire = %q, want id 2", f.lastUpdateFeat) } }) @@ -4487,8 +5139,8 @@ func TestFlagFeatureByID(t *testing.T) { if err != nil { t.Fatalf("flag reorder 2: %v", err) } - if f.lastUpdate["feature"].(map[string]any)["name"] != "max_items" { - t.Errorf("feature ref = %+v, want the canonical name on the wire", f.lastUpdate["feature"]) + if f.lastUpdateFeat != "2" { + t.Errorf("feature on the wire = %q, want id 2", f.lastUpdateFeat) } }) } @@ -4497,33 +5149,7 @@ func TestFlagReorder(t *testing.T) { // Fixture: max_items has overrides beta-optin (57, priority 0, "blue", // on) and us-adults (42, priority 1, 25, off); env default off/25. - t.Run("refuses to reorder when an override has no state row", func(t *testing.T) { - // Given us-adults' state row missing — as a concurrent delete would - // leave it. Echoing a zero state would disable and blank the override. - f := flagUpdateEnv(t) - withFeatureOverridesFixture(f) - str := func(s string) map[string]any { return map[string]any{"type": "unicode", "string_value": s} } - withFeatureStates(f, 2, - map[string]any{"id": 9000, "feature_segment": nil, "enabled": false, "feature_state_value": str("default")}, - map[string]any{"id": 9001, "feature_segment": 1200, "enabled": true, "feature_state_value": str("blue")}, - ) - - // When - _, err := run("", "flag", "reorder", "max_items", "us-adults", "beta-optin", "--yes") - - // Then - if err == nil || !strings.Contains(err.Error(), "42") { - t.Errorf("err = %v, want a refusal naming the segment", err) - } - f.mu.Lock() - calls := f.updateCalls - f.mu.Unlock() - if calls != 0 { - t.Errorf("update calls = %d, want no write at all", calls) - } - }) - - t.Run("re-permutes every override in one request", func(t *testing.T) { + t.Run("re-permutes every override in one request, writing priorities alone", func(t *testing.T) { f := flagUpdateEnv(t) withFeatureOverridesFixture(f) @@ -4543,33 +5169,35 @@ func TestFlagReorder(t *testing.T) { } first := ovs[0].(map[string]any) second := ovs[1].(map[string]any) - if first["segment_id"] != float64(42) || first["priority"] != float64(0) { + if wireSegmentID(first) != 42 || first["priority"] != float64(0) { t.Errorf("first override = %+v, want us-adults at priority 0", first) } - if second["segment_id"] != float64(57) || second["priority"] != float64(1) { + if wireSegmentID(second) != 57 || second["priority"] != float64(1) { t.Errorf("second override = %+v, want beta-optin at priority 1", second) } - // Each override echoes its current state so nothing else changes. - firstVal := first["value"].(map[string]any) - secondVal := second["value"].(map[string]any) - if first["enabled"] != false || firstVal["type"] != "integer" || firstVal["value"] != "25" { - t.Errorf("first override = %+v, want current state echoed", first) - } - if second["enabled"] != true || secondVal["value"] != "blue" { - t.Errorf("second override = %+v, want current state echoed", second) + // Nothing but the priorities is sent, so no state can be disturbed by + // a move — not the overrides', and not the environment default's. + for _, ov := range []map[string]any{first, second} { + if len(ov) != 2 { + t.Errorf("override = %+v, want the segment and its priority alone", ov) + } } - // The environment default rides along unchanged. - def := f.lastUpdate["environment_default"].(map[string]any) - if def["enabled"] != false { - t.Errorf("environment_default = %+v, want carried unchanged", def) + if _, ok := f.lastUpdate["environment_default"]; ok { + t.Errorf("body = %+v, want no environment_default", f.lastUpdate) } if !strings.Contains(out, "Reordered 2 segment overrides for max_items") { t.Errorf("output = %q, want a reorder confirmation", out) } - // The resulting order is printed, us-adults now first. + // The resulting order is printed, us-adults now first, with each + // override's untouched state read back from the response. if us, beta := strings.Index(out, "us-adults"), strings.Index(out, "beta-optin"); us == -1 || us > beta { t.Errorf("output = %q, want the resulting table with us-adults first", out) } + for _, want := range []string{"25", "blue"} { + if !strings.Contains(out, want) { + t.Errorf("output = %q, want each override's kept value (%s)", out, want) + } + } }) t.Run("resolves refs and renders from data already in hand", func(t *testing.T) { @@ -4590,8 +5218,8 @@ func TestFlagReorder(t *testing.T) { if got := f.featureSegmentsCalls(); got != 1 { t.Errorf("feature-segments calls = %d, want 1", got) } - if got := f.featureStatesCalls(); got != 1 { - t.Errorf("featurestates calls = %d, want 1", got) + if got := f.featureStatesCalls(); got != 0 { + t.Errorf("featurestates calls = %d, want 0 (a priority move needs no state)", got) } if us, beta := strings.Index(out, "us-adults"), strings.Index(out, "beta-optin"); us == -1 || us > beta { t.Errorf("output = %q, want the resulting table with us-adults first", out) @@ -4672,10 +5300,31 @@ func TestFlagReorder(t *testing.T) { }) } +// withTwoSegmentOverrides gives max_items overrides for segments 12 and 42, +// each with its own state, so a write that must preserve one can be seen doing +// it. +func withTwoSegmentOverrides(f *fakeInstance) { + withFeatureSegments(f, 2, + map[string]any{"id": 1200, "segment": 12, "segment_name": "powerusers", "priority": 0}, + map[string]any{"id": 4200, "segment": 42, "segment_name": "us-adults", "priority": 1}, + ) + withFeatureStates(f, 2, + map[string]any{"id": 10, "enabled": false, "feature_segment": nil, + "feature_state_value": map[string]any{"type": "int", "integer_value": 25}}, + map[string]any{"id": 11, "enabled": true, "feature_segment": 1200, + "feature_state_value": map[string]any{"type": "unicode", "string_value": "special"}}, + map[string]any{"id": 12, "enabled": false, "feature_segment": 4200, + "feature_state_value": map[string]any{"type": "int", "integer_value": 99}}, + ) +} + func TestFlagDelete(t *testing.T) { - t.Run("deletes a segment override", func(t *testing.T) { + // The endpoint has no delete verb, so an override is removed by replacing + // the whole set with the overrides that survive it. + t.Run("replaces the override set with the survivors, echoed in full", func(t *testing.T) { // Given f := flagUpdateEnv(t) + withTwoSegmentOverrides(f) // When out, err := run("", "flag", "delete", "max_items", "--segment", "12", "--yes") @@ -4684,15 +5333,49 @@ func TestFlagDelete(t *testing.T) { if err != nil { t.Fatalf("flag delete: %v\noutput: %s", err, out) } - if f.lastDelete["feature"].(map[string]any)["name"] != "max_items" || - f.lastDelete["segment"].(map[string]any)["id"] != float64(12) { - t.Errorf("delete body = %+v", f.lastDelete) + if f.lastUpdateVerb != http.MethodPut || f.lastUpdateFeat != "2" { + t.Errorf("request = %s feature %s, want PUT feature 2", f.lastUpdateVerb, f.lastUpdateFeat) } - if !strings.Contains(out, "Deleted max_items override for segment 12") { + if _, ok := f.lastUpdate["environment_default"]; ok { + t.Errorf("body = %+v, want no environment_default — a PUT would reset it", f.lastUpdate) + } + overrides := f.lastUpdate["segment_overrides"].([]any) + if len(overrides) != 1 { + t.Fatalf("segment_overrides = %+v, want only the survivor", overrides) + } + survivor := overrides[0].(map[string]any) + if wireSegmentID(survivor) != 42 || survivor["priority"] != float64(1) || + survivor["enabled"] != false || survivor["value"].(map[string]any)["value"] != "99" { + t.Errorf("survivor = %+v, want segment 42's state restated in full", survivor) + } + if !strings.Contains(out, "Deleted max_items override for segment powerusers (12)") { t.Errorf("output = %q", out) } }) + t.Run("deleting the last override sends an empty list", func(t *testing.T) { + // Given + f := flagUpdateEnv(t) + withFeatureSegments(f, 2, map[string]any{ + "id": 1200, "segment": 12, "segment_name": "powerusers", "priority": 0, + }) + + // When + out, err := run("", "flag", "delete", "max_items", "--segment", "12", "--yes") + + // Then + if err != nil { + t.Fatalf("flag delete: %v\noutput: %s", err, out) + } + overrides, ok := f.lastUpdate["segment_overrides"] + if !ok { + t.Fatalf("body = %+v, want an empty segment_overrides list", f.lastUpdate) + } + if got := overrides.([]any); len(got) != 0 { + t.Errorf("segment_overrides = %+v, want empty", got) + } + }) + t.Run("without --segment exits 2", func(t *testing.T) { f := flagUpdateEnv(t) _, err := run("", "flag", "delete", "max_items", "--yes") @@ -4700,18 +5383,26 @@ func TestFlagDelete(t *testing.T) { if !errors.As(err, &ue) || !strings.Contains(err.Error(), "--segment") { t.Errorf("err = %v, want a usage error naming --segment", err) } - if f.lastDelete != nil { - t.Errorf("lastDelete = %+v, want no call", f.lastDelete) + if f.lastUpdate != nil { + t.Errorf("lastUpdate = %+v, want no call", f.lastUpdate) } }) - t.Run("missing override reports not found", func(t *testing.T) { + t.Run("a segment with no override errors before any write", func(t *testing.T) { + // Given f := flagUpdateEnv(t) - withMissingSegmentOverride(f) - _, err := run("", "flag", "delete", "max_items", "--segment", "99", "--yes") - if err == nil || !strings.Contains(err.Error(), "segment 99") { + withTwoSegmentOverrides(f) + + // When + _, err := run("", "flag", "delete", "max_items", "--segment", "57", "--yes") + + // Then + if err == nil || !strings.Contains(err.Error(), "segment 57") { t.Errorf("err = %v, want a not-found error naming the segment", err) } + if f.lastUpdate != nil { + t.Errorf("lastUpdate = %+v, want no write", f.lastUpdate) + } }) } diff --git a/internal/cmd/feature.go b/internal/cmd/feature.go index bfcf52a..242341d 100644 --- a/internal/cmd/feature.go +++ b/internal/cmd/feature.go @@ -86,6 +86,40 @@ func formatWeight(w float64) string { return strconv.FormatFloat(w, 'f', -1, 64) } +// weightPercent marks a weight as the percentage it is. Flag views use it: a +// flag's distribution is one scope's share of traffic, unlike the variant's own +// default weight the feature views show. +func weightPercent(w float64) string { + return formatWeight(w) + "%" +} + +// writeVariants prints the indented Variants block shared by the feature and +// flag detail views, formatting each weight with weight. Nothing is printed for +// a feature without variants. +func writeVariants(w io.Writer, views []variantView, weight func(float64) string) error { + if len(views) == 0 { + return nil + } + if _, err := fmt.Fprintln(w, "\nVariants"); err != nil { + return err + } + var buf bytes.Buffer + tw := tabwriter.NewWriter(&buf, 0, 0, 2, ' ', 0) + fmt.Fprintln(tw, "VALUE\tWEIGHT\tKEY\tID") + for _, v := range views { + fmt.Fprintf(tw, "%s\t%s\t%s\t%d\n", fmt.Sprint(v.Value), weight(v.Weight), v.Key, v.ID) + } + if err := tw.Flush(); err != nil { + return err + } + for _, line := range strings.Split(strings.TrimRight(buf.String(), "\n"), "\n") { + if _, err := fmt.Fprintf(w, " %s\n", line); err != nil { + return err + } + } + return nil +} + var featureListCmd = &cobra.Command{ Use: "list", Short: "List features in the current project", @@ -150,20 +184,7 @@ func renderFeature(cmd *cobra.Command, f *api.Feature) error { }); err != nil { return err } - if len(view.Variants) > 0 { - fmt.Fprintln(w, "\nVariants") - var buf bytes.Buffer - tw := tabwriter.NewWriter(&buf, 0, 0, 2, ' ', 0) - fmt.Fprintln(tw, "VALUE\tWEIGHT\tKEY\tID") - for _, v := range view.Variants { - fmt.Fprintf(tw, "%s\t%s\t%s\t%d\n", fmt.Sprint(v.Value), formatWeight(v.Weight), v.Key, v.ID) - } - tw.Flush() - for _, line := range strings.Split(strings.TrimRight(buf.String(), "\n"), "\n") { - fmt.Fprintf(w, " %s\n", line) - } - } - return nil + return writeVariants(w, view.Variants, formatWeight) }) } diff --git a/internal/cmd/flag_identity.go b/internal/cmd/flag_identity.go index 086ae64..ee1078d 100644 --- a/internal/cmd/flag_identity.go +++ b/internal/cmd/flag_identity.go @@ -67,7 +67,7 @@ func readIdentityOverride(cmd *cobra.Command, cred *activeCredential, envKey str } // nativeScalar converts a typed value into the native scalar the identity -// endpoints expect (they infer the type from the value, unlike update-flag-v2). +// endpoints expect (they infer the type from the value, unlike update-flag). func nativeScalar(v api.FeatureValue) (any, error) { switch v.Type { case "boolean": diff --git a/internal/cmd/flag_reorder.go b/internal/cmd/flag_reorder.go index bffe19f..5a005db 100644 --- a/internal/cmd/flag_reorder.go +++ b/internal/cmd/flag_reorder.go @@ -2,6 +2,7 @@ package cmd import ( "fmt" + "sort" "strconv" "strings" @@ -21,9 +22,9 @@ var flagReorderCmd = &cobra.Command{ } // runFlagReorder assigns priorities 0..n-1 to a feature's segment overrides in -// the input order, in one update-flag-v2 request (one published version under -// v2 versioning). The input must name every overridden segment exactly once — -// a partial list would make the result depend on the current order. +// the input order, in one update-flag request (one published version under v2 +// versioning). The input must name every overridden segment exactly once — a +// partial list would make the result depend on the current order. func runFlagReorder(cmd *cobra.Command, args []string) error { name := args[0] _, cred, projectID, env, err := flagContext(cmd) @@ -95,37 +96,14 @@ func runFlagReorder(cmd *cobra.Command, args []string) error { name, strings.Join(missing, ", ")) } - // Each override echoes its current state and value; only priorities move. - states, err := cred.client().FeatureStates(cmd.Context(), env.ID, feature.ID) - if err != nil { - return err - } - stateByFS := make(map[int]api.EnvironmentFeatureState, len(states)) - for _, s := range states { - if s.FeatureSegment != nil { - stateByFS[*s.FeatureSegment] = s - } - } - req := api.UpdateFlagRequest{ - Feature: api.FeatureRef{Name: feature.Name}, - EnvironmentDefault: api.EnvironmentDefault{ - Enabled: flagEnabled(feature.EnvironmentState), - Value: featureValueFromScalar(currentScalar(feature.EnvironmentState)), - }, - } + // Only priorities move: a partial write leaves every other property of + // each override — its state, value and weights — exactly as it is. + var req api.UpdateFlagRequest for i, segmentID := range ordered { priority := i - // A zero state would echo the override as off with an empty value, so a - // priority-only move would quietly rewrite it. - state, ok := stateByFS[current[segmentID].ID] - if !ok { - return fmt.Errorf("no feature state found for the override on segment %d; refusing to reorder", segmentID) - } - req.SegmentOverrides = append(req.SegmentOverrides, api.SegmentOverride{ - SegmentID: segmentID, - Enabled: state.Enabled, - Value: featureValueFromScalar(state.Value.Scalar()), - Priority: &priority, + req.SegmentOverrides = append(req.SegmentOverrides, api.SegmentOverrideUpdate{ + Segment: api.SegmentTarget{ID: segmentID}, + Priority: &priority, }) } @@ -134,26 +112,31 @@ func runFlagReorder(cmd *cobra.Command, args []string) error { if ok, err := confirmed(cmd, prompt, "changed"); !ok || err != nil { return err } - if err := cred.client().UpdateFlag(cmd.Context(), env.APIKey, req); err != nil { + resp, err := cred.client().UpdateFlag(cmd.Context(), env.APIKey, feature.ID, req) + if err != nil { return err } output.Success(errOut, "Reordered %d segment overrides for %s in environment %s", len(ordered), name, environmentLabel(env)) - // Result model: print the resulting override list. It is fully known — - // the write assigned priorities 0..n-1 in input order, and each - // override's state was read (and echoed unchanged) before the write. - views := make([]segmentFlagView, len(ordered)) - for i, segmentID := range ordered { - fs := current[segmentID] - state := stateByFS[fs.ID] - views[i] = segmentFlagView{ + // Result model: print the resulting override list, in the new priority + // order the response reports. Only the segment names come from the rows + // read before the write — the response identifies segments by id alone. + views := make([]segmentFlagView, 0, len(resp.SegmentOverrides)) + for _, override := range resp.SegmentOverrides { + scalar, err := scalarOfValue(override.Value) + if err != nil { + return err + } + views = append(views, segmentFlagView{ Feature: feature.Name, Type: featureTypeLabel(feature.Type), - Segment: segmentRef{ID: segmentID, Name: fs.SegmentName}, - Priority: i, - Enabled: state.Enabled, - Value: state.Value.Scalar(), - } + Segment: segmentRef{ID: override.Segment.ID, Name: current[override.Segment.ID].SegmentName}, + Priority: override.Priority, + Enabled: override.Enabled, + Value: scalar, + Variants: variantViews(feature, weightsOf(override.Variants)), + }) } + sort.SliceStable(views, func(a, b int) bool { return views[a].Priority < views[b].Priority }) return renderSegmentOverrideList(cmd, views) } diff --git a/internal/cmd/flag_update.go b/internal/cmd/flag_update.go index d0e5558..5e644ba 100644 --- a/internal/cmd/flag_update.go +++ b/internal/cmd/flag_update.go @@ -3,10 +3,12 @@ package cmd import ( "fmt" "strconv" + "strings" "github.com/spf13/cobra" "github.com/Flagsmith/flagsmith-cli/v2/internal/api" + "github.com/Flagsmith/flagsmith-cli/v2/internal/bug" "github.com/Flagsmith/flagsmith-cli/v2/internal/cache" "github.com/Flagsmith/flagsmith-cli/v2/internal/output" ) @@ -16,6 +18,7 @@ var ( flagDisableFlag bool flagValueFlag string flagTypeFlag string + flagWeightFlags []string flagUpdateSegment string flagUpdateIdentifier string flagUpdatePriority int @@ -36,7 +39,11 @@ var flagUpdateCmd = &cobra.Command{ # target a segment or identity override instead of the default flagsmith flag update onboarding --enable --segment 12 - flagsmith flag update onboarding --value beta --identifier user-123`, + flagsmith flag update onboarding --value beta --identifier user-123 + + # re-weight a multivariate flag's variants (by key or id) + flagsmith flag update banner-copy --weight hero=25 --weight sub=75 + flagsmith flag update banner-copy --segment 12 --weight hero=100,sub=0`, Args: cobra.ExactArgs(1), RunE: runFlagUpdate, } @@ -58,14 +65,109 @@ func runFlagUpdate(cmd *cobra.Command, args []string) error { return usageErrorf("--enable and --disable are mutually exclusive") case m.setPriority && m.segmentRef == "": return usageErrorf("--priority only applies together with --segment") - case !m.enable && !m.disable && !m.setValue && !m.setPriority: - return usageErrorf("nothing to update — pass --enable, --disable, --value, or --priority") + case len(flagWeightFlags) > 0 && m.identifier != "": + return hintf(usageErrorf("--weight and --identifier are mutually exclusive"), + "An identity is served one concrete value, not a distribution — use --value.") + case !m.enable && !m.disable && !m.setValue && !m.setPriority && len(flagWeightFlags) == 0: + return usageErrorf("nothing to update — pass --enable, --disable, --value, --weight, or --priority") case cmd.Flags().Changed("type") && !m.setValue: return usageErrorf("--type only applies together with --value") } + // Parsed up front: bad syntax should cost no request. + weights, err := parseWeights(flagWeightFlags) + if err != nil { + return err + } + m.weights = weights return applyFlagMutation(cmd, args[0], m) } +// weightRef is one --weight pair: a variant reference (key or id) and the +// percentage of traffic it should serve. +type weightRef struct { + ref string + weight float64 +} + +// controlVariantKey is the backend's reserved name for the share of traffic no +// variant claims. It is not a variant, so it cannot be weighted directly. +const controlVariantKey = "control" + +// parseWeights parses repeated --weight values, each one or more +// comma-separated = pairs. +func parseWeights(args []string) ([]weightRef, error) { + var refs []weightRef + seen := map[string]bool{} + for _, arg := range args { + for _, pair := range strings.Split(arg, ",") { + ref, raw, ok := strings.Cut(strings.TrimSpace(pair), "=") + if !ok || ref == "" || raw == "" { + return nil, usageErrorf("--weight %q is not =", pair) + } + weight, err := strconv.ParseFloat(raw, 64) + if err != nil || weight < 0 || weight > 100 { + return nil, usageErrorf("--weight %s: %q is not a percentage between 0 and 100", ref, raw) + } + if ref == controlVariantKey { + return nil, hintf(usageErrorf("%q is not a variant", controlVariantKey), + "Whatever the variants leave unallocated serves the flag's own value — set it with --value.") + } + if seen[ref] { + return nil, usageErrorf("--weight %s is given twice", ref) + } + seen[ref] = true + refs = append(refs, weightRef{ref: ref, weight: weight}) + } + } + return refs, nil +} + +// weightSummary renders the requested weights for a confirmation line, in the +// order and by the references the user gave. +func weightSummary(refs []weightRef) string { + pairs := make([]string, len(refs)) + for i, r := range refs { + pairs[i] = r.ref + "=" + formatWeight(r.weight) + } + return strings.Join(pairs, ", ") +} + +// weightTolerance absorbs the rounding of percentages that are exact in decimal +// but not in binary, so weights a user reads as summing to 100 are accepted. +const weightTolerance = 1e-9 + +// mergeWeights overlays the requested weights onto the scope's current +// distribution, and returns the whole variant list: the endpoint rejects a +// partial one, and sending everything is also what keeps an unnamed variant at +// the weight it already had. +func mergeWeights(feature *api.Feature, current variantWeights, refs []weightRef) ([]api.Variant, error) { + weights := make(variantWeights, len(feature.MultivariateOptions)) + for _, o := range feature.MultivariateOptions { + weights[o.ID] = current[o.ID] + } + for _, r := range refs { + // Resolved against the feature, never created: the endpoint would take + // an unknown key as a new variant, so a typo would silently add one. + option := findVariant(feature, r.ref) + if option == nil { + return nil, hintf(usageErrorf("%q is not a variant of %s", r.ref, feature.Name), + "Run `flagsmith feature variant list %s` to see its variants, or `flagsmith feature variant add %s` to add one.", + feature.Name, feature.Name) + } + weights[option.ID] = r.weight + } + total := 0.0 + variants := make([]api.Variant, 0, len(feature.MultivariateOptions)) + for _, o := range feature.MultivariateOptions { + variants = append(variants, api.Variant{ID: o.ID, Weight: weights[o.ID]}) + total += weights[o.ID] + } + if total > 100+weightTolerance { + return nil, usageErrorf("the merged weights add up to %s%%, over the 100%% available", formatWeight(total)) + } + return variants, nil +} + // flagMutation is the state change a flag command applies to one feature — some // combination of enable/disable/set-value, optionally scoped to a segment or // identity override. `flag update` builds it from its flags; `flag enable` and @@ -75,13 +177,14 @@ type flagMutation struct { enable, disable, setValue bool setPriority bool priority int + weights []weightRef segmentRef string identifier string } -// applyFlagMutation resolves the feature and applies m via update-flag-v2 (which -// requires the whole environment default, so the rest is carried forward -// unchanged), then reprints the resulting flag. +// applyFlagMutation resolves the feature and applies m as a partial update-flag +// write — only the properties m changes are sent, and the rest are left as they +// are — then reprints the resulting flag from the endpoint's response. func applyFlagMutation(cmd *cobra.Command, name string, m flagMutation) error { _, cred, projectID, env, err := flagContext(cmd) if err != nil { @@ -95,37 +198,26 @@ func applyFlagMutation(cmd *cobra.Command, name string, m flagMutation) error { if err != nil { return err } - name = feature.Name // canonical for the wire ref and messages + name = feature.Name // canonical for messages if m.identifier != "" { return runIdentityUpdate(cmd, cred, env, projectID, feature, m.identifier, m.enable, m.disable, m.setValue) } - req := api.UpdateFlagRequest{ - Feature: api.FeatureRef{Name: name}, - EnvironmentDefault: api.EnvironmentDefault{ - Enabled: flagEnabled(feature.EnvironmentState), - Value: featureValueFromScalar(currentScalar(feature.EnvironmentState)), - }, - } - // The state being changed: the environment default, or a segment override. // The scope carries its own preposition: "in environment …" for the // default, "for segment name (id) in environment …" for an override. - // The override's metadata (name, current priority) is fetched once and - // reused for the post-update render. + // The override's metadata (name, priority, feature-state link) is fetched + // once and reused for the weights and the post-update render. target := feature.EnvironmentState scope := "in environment " + environmentLabel(env) - var segment segmentRef - var priority int + var meta overrideMeta if segmentID != 0 { target = feature.SegmentState // nil when the override does not exist yet - var err error - segment, priority, err = segmentOverrideMeta(cmd, cred, env.ID, feature.ID, segmentID) - if err != nil { + if meta, err = segmentOverrideMeta(cmd, cred, env.ID, feature.ID, segmentID); err != nil { return err } - scope = fmt.Sprintf("for segment %s in environment %s", segment.display(), environmentLabel(env)) + scope = fmt.Sprintf("for segment %s in environment %s", meta.segment.display(), environmentLabel(env)) } // Priorities are a dense 0-based order; a new override joins it, growing the @@ -141,38 +233,58 @@ func applyFlagMutation(cmd *cobra.Command, name string, m flagMutation) error { } } - // A new segment override inherits the environment default — enabled state - // and value alike; an existing one keeps its current state. A value-only - // edit must never silently switch the segment off. - enabled := flagEnabled(target) - if target == nil { - enabled = flagEnabled(feature.EnvironmentState) - } - if m.enable { - enabled = true - } - if m.disable { - enabled = false - } - value := req.EnvironmentDefault.Value - if target != nil { - value = featureValueFromScalar(currentScalar(target)) + state := api.FlagStateUpdate{} + if m.enable || m.disable { + state.Enabled = &m.enable } if m.setValue { - if value, err = inferFeatureValue(flagValueFlag, flagTypeFlag); err != nil { + value, err := inferFeatureValue(flagValueFlag, flagTypeFlag) + if err != nil { + return err + } + state.Value = &value + } + if len(m.weights) > 0 { + if len(feature.MultivariateOptions) == 0 { + return hintf(usageErrorf("%s has no variants to weight", name), + "Add one with `flagsmith feature variant add %s --value `.", name) + } + current, err := scopeWeights(cmd, cred, env.ID, feature, meta.stateID) + if err != nil { + return err + } + if state.Variants, err = mergeWeights(feature, current, m.weights); err != nil { return err } } + var req api.UpdateFlagRequest if segmentID == 0 { - req.EnvironmentDefault.Enabled = enabled - req.EnvironmentDefault.Value = value + req.EnvironmentDefault = &state } else { - override := api.SegmentOverride{SegmentID: segmentID, Enabled: enabled, Value: value} + override := api.SegmentOverrideUpdate{ + Segment: api.SegmentTarget{ID: segmentID}, + Enabled: state.Enabled, + Value: state.Value, + Variants: state.Variants, + } if m.setPriority { override.Priority = &m.priority } - req.SegmentOverrides = []api.SegmentOverride{override} + // A new override starts from the environment default — enabled state and + // position — stated outright rather than left to the server, so that a + // value-only edit can't switch the segment off or jump the queue. + if target == nil { + if override.Enabled == nil { + enabled := flagEnabled(feature.EnvironmentState) + override.Enabled = &enabled + } + if override.Priority == nil { + priority := feature.NumSegmentOverrides // joins at the end + override.Priority = &priority + } + } + req.SegmentOverrides = []api.SegmentOverrideUpdate{override} } errOut := cmd.ErrOrStderr() @@ -180,12 +292,16 @@ func applyFlagMutation(cmd *cobra.Command, name string, m flagMutation) error { return err } - if err := cred.client().UpdateFlag(cmd.Context(), env.APIKey, req); err != nil { + resp, err := cred.client().UpdateFlag(cmd.Context(), env.APIKey, feature.ID, req) + if err != nil { return err } if m.setValue { - output.Success(errOut, "Set %s to %s %s", name, displayValue(value), scope) + output.Success(errOut, "Set %s to %s %s", name, displayValue(*state.Value), scope) + } + if len(m.weights) > 0 { + output.Success(errOut, "Set %s weights to %s %s", name, weightSummary(m.weights), scope) } if m.enable { output.Success(errOut, "Enabled %s %s", name, scope) @@ -198,25 +314,39 @@ func applyFlagMutation(cmd *cobra.Command, name string, m flagMutation) error { } // Result model: an update also prints the resulting resource to stdout. The - // request carried the written state in full, so the detail renders from it - // rather than re-fetching the features list. - scalar, err := nativeScalar(value) - if err != nil { - return err - } + // response carries the flag's whole state in the environment, so the detail + // renders from it rather than re-fetching the features list. updated := *feature if segmentID != 0 { - updated.SegmentState = &api.FeatureState{Enabled: enabled, Value: scalar} - if target == nil { - priority = feature.NumSegmentOverrides // a new override joins at the end + override := resp.Override(segmentID) + if override == nil { + return bug.Mark(fmt.Errorf("the update of %s left no override for segment %s", name, meta.segment.display())) } - if m.setPriority { - priority = m.priority + scalar, err := scalarOfValue(override.Value) + if err != nil { + return err } - return renderSegmentDetail(cmd, newSegmentFlagView(&updated, segment, priority)) + updated.SegmentState = &api.FeatureState{Enabled: override.Enabled, Value: scalar} + view := newSegmentFlagView(&updated, meta.segment, override.Priority) + view.Variants = variantViews(feature, weightsOf(override.Variants)) + return renderSegmentDetail(cmd, view) } - updated.EnvironmentState = &api.FeatureState{Enabled: enabled, Value: scalar} - return renderFlagDetail(cmd, &updated) + scalar, err := scalarOfValue(resp.EnvironmentDefault.Value) + if err != nil { + return err + } + updated.EnvironmentState = &api.FeatureState{Enabled: resp.EnvironmentDefault.Enabled, Value: scalar} + return renderFlagDetail(cmd, &updated, weightsOf(resp.EnvironmentDefault.Variants)) +} + +// scalarOfValue converts a typed value from an update-flag response into the +// bare scalar the flag views render. A feature with no value at all reads as +// unset rather than as an empty string. +func scalarOfValue(v *api.FeatureValue) (any, error) { + if v == nil { + return nil, nil + } + return nativeScalar(*v) } var flagDeleteCmd = &cobra.Command{ @@ -243,25 +373,126 @@ var flagDeleteCmd = &cobra.Command{ if hasIdentifier { return runIdentityDelete(cmd, cred, env, projectID, name, flagDeleteIdentifier) } - segmentID, err := resolveSegmentID(cmd, cred, projectID, flagDeleteSegment) + feature, err := requireFeature(cmd, cred, projectID, env, 0, name) if err != nil { return err } - // Nothing on this path carries the segment's name, so the display comes - // from the name cache (seeded by resolving a name ref) and degrades to - // the bare id. - segmentLabel := label(cache.Load(apiURL).Segments[strconv.Itoa(segmentID)], segmentID) - errOut := cmd.ErrOrStderr() - prompt := fmt.Sprintf("delete %s override for segment %s in %s", name, segmentLabel, environmentLabel(env)) - if ok, err := confirmed(cmd, prompt+"?", "changed"); !ok || err != nil { + segmentID, err := resolveSegmentID(cmd, cred, projectID, flagDeleteSegment) + if err != nil { return err } - if err := cred.client().DeleteSegmentOverride(cmd.Context(), env.APIKey, featureRefFor(name), segmentID); err != nil { - return err + return deleteSegmentOverride(cmd, cred, env, feature, segmentID) + }, +} + +// deleteSegmentOverride removes one segment's override. The endpoint has no verb +// for deleting a single override: a PUT replaces the whole set, and whatever it +// leaves out is deleted. So the surviving overrides are read and echoed back in +// full — a partial echo would reset the state it omits. Being a +// read-modify-write, a change made to another override in between is lost. +func deleteSegmentOverride(cmd *cobra.Command, cred *activeCredential, env api.Environment, feature *api.Feature, segmentID int) error { + fss, err := cred.client().FeatureSegments(cmd.Context(), env.ID, feature.ID) + if err != nil { + return err + } + survivors := make([]api.FeatureSegment, 0, len(fss)) + var target *api.FeatureSegment + for i, fs := range fss { + if fs.Segment == segmentID { + target = &fss[i] + continue } - output.Success(errOut, "Deleted %s override for segment %s in environment %s", name, segmentLabel, environmentLabel(env)) + survivors = append(survivors, fs) + } + if target == nil { + // The name cache (seeded by resolving a name ref) is all there is to go + // on for a segment with no override; the display degrades to the id. + missing := label(cache.Load(apiURL).Segments[strconv.Itoa(segmentID)], segmentID) + return withHint( + fmt.Errorf("%s has no override for segment %s in %s", feature.Name, missing, environmentLabel(env)), + fmt.Sprintf("Run `flagsmith flag list --feature %s` to see its segment overrides.", feature.Name)) + } + segmentLabel := segmentRef{ID: segmentID, Name: target.SegmentName}.display() + + errOut := cmd.ErrOrStderr() + prompt := fmt.Sprintf("delete %s override for segment %s in %s", feature.Name, segmentLabel, environmentLabel(env)) + if ok, err := confirmed(cmd, prompt+"?", "changed"); !ok || err != nil { + return err + } + + overrides, err := echoOverrides(cmd, cred, env, feature, survivors) + if err != nil { + return err + } + if _, err := cred.client().ReplaceFlag(cmd.Context(), env.APIKey, feature.ID, + api.UpdateFlagRequest{SegmentOverrides: overrides}); err != nil { + return err + } + output.Success(errOut, "Deleted %s override for segment %s in environment %s", feature.Name, segmentLabel, environmentLabel(env)) + return nil +} + +// echoOverrides reads the current state of the given overrides and restates it +// in full, for a PUT that must preserve them. The result is never nil: an empty +// list is what deletes the last override, and must reach the wire as one. +func echoOverrides(cmd *cobra.Command, cred *activeCredential, env api.Environment, feature *api.Feature, keep []api.FeatureSegment) ([]api.SegmentOverrideUpdate, error) { + overrides := make([]api.SegmentOverrideUpdate, 0, len(keep)) + if len(keep) == 0 { + return overrides, nil + } + states, err := cred.client().FeatureStates(cmd.Context(), env.ID, feature.ID) + if err != nil { + return nil, err + } + byOverride := make(map[int]api.EnvironmentFeatureState, len(states)) + for _, s := range states { + if s.FeatureSegment != nil { + byOverride[*s.FeatureSegment] = s + } + } + for _, fs := range keep { + // Echoing a zero state would rewrite the override as off with no value, + // so a missing one stops the write rather than guessing. + state, ok := byOverride[fs.ID] + if !ok { + return nil, bug.Mark(fmt.Errorf("no feature state found for the override on segment %d; refusing to write", fs.Segment)) + } + enabled, priority := state.Enabled, fs.Priority + override := api.SegmentOverrideUpdate{ + Segment: api.SegmentTarget{ID: fs.Segment}, + Enabled: &enabled, + Priority: &priority, + Variants: echoVariants(feature, state.Multivariate), + } + // An override with no value of its own is restated by omission: what a + // replacing write leaves out is reset, which is the null it already has. + if scalar := state.Value.Scalar(); scalar != nil { + value := featureValueFromScalar(scalar) + override.Value = &value + } + overrides = append(overrides, override) + } + return overrides, nil +} + +// echoVariants restates a feature state's distribution as the full variant list +// the endpoint requires, so a replacing write leaves the weights alone. A +// feature without variants has none to send, and neither has a state with no +// allocations of its own — sending zeros would be a change, where omission +// restates the nothing that is already there. +func echoVariants(feature *api.Feature, allocations []api.MultivariateStateValue) []api.Variant { + if len(feature.MultivariateOptions) == 0 || len(allocations) == 0 { return nil - }, + } + weights := make(variantWeights, len(allocations)) + for _, a := range allocations { + weights[a.OptionID] = a.Allocation + } + variants := make([]api.Variant, 0, len(feature.MultivariateOptions)) + for _, o := range feature.MultivariateOptions { + variants = append(variants, api.Variant{ID: o.ID, Weight: weights[o.ID]}) + } + return variants } var ( @@ -323,15 +554,6 @@ var flagCreateCmd = &cobra.Command{ }, } -// featureRefFor parses a feature reference into the update-flag wire form: -// all-digit → id, anything else → name, resolved server-side. -func featureRefFor(ref string) api.FeatureRef { - if id, err := strconv.Atoi(ref); err == nil { - return api.FeatureRef{ID: id} - } - return api.FeatureRef{Name: ref} -} - // currentScalar returns a feature state's current value, or nil. func currentScalar(fs *api.FeatureState) any { if fs == nil { @@ -342,7 +564,7 @@ func currentScalar(fs *api.FeatureState) any { // featureValueFromScalar converts a bare scalar — read from the features list // (JSON numbers arrive as float64) or from api.TypedValue.Scalar() (ints stay -// int) — into the {type, value} wire form update-flag-v2 expects. +// int) — into the {type, value} wire form update-flag expects. func featureValueFromScalar(v any) api.FeatureValue { switch t := v.(type) { case bool: @@ -409,6 +631,8 @@ func init() { flagUpdateCmd.Flags().IntVar(&flagUpdatePriority, "priority", 0, "move the segment override to this priority (0 is evaluated first)") flagDeleteCmd.Flags().StringVar(&flagDeleteSegment, "segment", "", "the segment (id or name) whose override to delete") flagDeleteCmd.Flags().StringVarP(&flagDeleteIdentifier, "identifier", "i", "", "the identity whose override to delete") + flagUpdateCmd.Flags().StringArrayVar(&flagWeightFlags, "weight", nil, + "set a variant's weight, as = (repeatable, or comma-separated)") for _, c := range []*cobra.Command{flagEnableCmd, flagDisableCmd} { c.Flags().StringVar(&flagToggleSegment, "segment", "", "target this segment's override (id or name) instead of the environment default") c.Flags().StringVarP(&flagToggleIdentifier, "identifier", "i", "", "target this identity's override instead of the environment default") diff --git a/internal/cmd/flags.go b/internal/cmd/flags.go index c184fab..50835df 100644 --- a/internal/cmd/flags.go +++ b/internal/cmd/flags.go @@ -26,15 +26,16 @@ var flagCmd = &cobra.Command{ // state hoisted to the top, with the metadata the human view shows, and none // of the raw features-endpoint noise. Human output and JSON stay in lockstep. type flagView struct { - Feature string `json:"feature"` - Type string `json:"type"` - Description string `json:"description"` - Enabled bool `json:"enabled"` - Value any `json:"value"` - SegmentOverrides int `json:"segment_overrides"` - IdentityOverrides int `json:"identity_overrides"` - CodeReferences int `json:"code_references"` - LifecycleStage string `json:"lifecycle_stage"` + Feature string `json:"feature"` + Type string `json:"type"` + Description string `json:"description"` + Enabled bool `json:"enabled"` + Value any `json:"value"` + SegmentOverrides int `json:"segment_overrides"` + IdentityOverrides int `json:"identity_overrides"` + CodeReferences int `json:"code_references"` + LifecycleStage string `json:"lifecycle_stage"` + Variants []variantView `json:"variants,omitempty"` } func newFlagView(f *api.Feature) flagView { @@ -65,12 +66,13 @@ func (s segmentRef) display() string { // segmentFlagView is the curated shape for a flag's state in one segment. type segmentFlagView struct { - Feature string `json:"feature"` - Type string `json:"type"` - Segment segmentRef `json:"segment"` - Priority int `json:"priority"` - Enabled bool `json:"enabled"` - Value any `json:"value"` + Feature string `json:"feature"` + Type string `json:"type"` + Segment segmentRef `json:"segment"` + Priority int `json:"priority"` + Enabled bool `json:"enabled"` + Value any `json:"value"` + Variants []variantView `json:"variants,omitempty"` } func newSegmentFlagView(f *api.Feature, segment segmentRef, priority int) segmentFlagView { @@ -84,14 +86,23 @@ func newSegmentFlagView(f *api.Feature, segment segmentRef, priority int) segmen } } -// segmentOverrideMeta reads the segment's name and the override's priority -// from the feature-segments endpoint, seeding the name cache on the way. A -// missing row (e.g. an override created a moment ago) degrades to the bare id -// and priority 0 rather than failing. -func segmentOverrideMeta(cmd *cobra.Command, cred *activeCredential, environmentID, featureID, segmentID int) (segmentRef, int, error) { +// overrideMeta is what the feature-segments endpoint knows about one segment +// override: the segment's name, the override's priority, and the id linking it +// to its feature state. stateID is 0 when the override does not exist yet. +type overrideMeta struct { + segment segmentRef + priority int + stateID int +} + +// segmentOverrideMeta reads one segment override's metadata from the +// feature-segments endpoint, seeding the name cache on the way. A missing row +// (an override that doesn't exist, or one created a moment ago) degrades to the +// bare id and priority 0 rather than failing. +func segmentOverrideMeta(cmd *cobra.Command, cred *activeCredential, environmentID, featureID, segmentID int) (overrideMeta, error) { fss, err := cred.client().FeatureSegments(cmd.Context(), environmentID, featureID) if err != nil { - return segmentRef{}, 0, err + return overrideMeta{}, err } names := make(map[string]string, len(fss)) for _, fs := range fss { @@ -100,10 +111,96 @@ func segmentOverrideMeta(cmd *cobra.Command, cred *activeCredential, environment _ = cache.Merge(apiURL, &cache.Names{Segments: names}) // opportunistic for _, fs := range fss { if fs.Segment == segmentID { - return segmentRef{ID: segmentID, Name: fs.SegmentName}, fs.Priority, nil + return overrideMeta{ + segment: segmentRef{ID: segmentID, Name: fs.SegmentName}, + priority: fs.Priority, + stateID: fs.ID, + }, nil + } + } + return overrideMeta{segment: segmentRef{ID: segmentID}}, nil +} + +// variantWeights maps a variant id to its weight in one scope. +type variantWeights map[int]float64 + +// scopeWeights reads the variant weights in force for one scope: the +// environment default, or a segment override identified by its feature-state +// link (stateID from overrideMeta). Weights are per scope, so they live on the +// feature state rather than on the variant, and the variants' own +// project-level defaults are only a starting point — they stand in for a scope +// with no allocations of its own, as does the environment default for an +// override that doesn't exist yet. +// +// A feature with no variants has no weights, and costs no request. +func scopeWeights(cmd *cobra.Command, cred *activeCredential, environmentID int, feature *api.Feature, stateID int) (variantWeights, error) { + if len(feature.MultivariateOptions) == 0 { + return nil, nil + } + states, err := cred.client().FeatureStates(cmd.Context(), environmentID, feature.ID) + if err != nil { + return nil, err + } + byScope := make(map[int][]api.MultivariateStateValue, len(states)) + for _, s := range states { + if s.Identity != nil { + continue // an identity gets a concrete value, never a distribution + } + scope := 0 // the environment default + if s.FeatureSegment != nil { + scope = *s.FeatureSegment } + byScope[scope] = s.Multivariate + } + allocations, ok := byScope[stateID] + if !ok { + allocations = byScope[0] + } + if len(allocations) == 0 { + return defaultWeights(feature), nil + } + weights := make(variantWeights, len(allocations)) + for _, a := range allocations { + weights[a.OptionID] = a.Allocation + } + return weights, nil +} + +// defaultWeights is the distribution described by the variants themselves. +func defaultWeights(feature *api.Feature) variantWeights { + weights := make(variantWeights, len(feature.MultivariateOptions)) + for _, o := range feature.MultivariateOptions { + weights[o.ID] = weightOf(o) + } + return weights +} + +// weightsOf reads a scope's weights back from an update-flag response. +func weightsOf(variants []api.Variant) variantWeights { + if len(variants) == 0 { + return nil } - return segmentRef{ID: segmentID}, 0, nil + weights := make(variantWeights, len(variants)) + for _, v := range variants { + weights[v.ID] = v.Weight + } + return weights +} + +// variantViews joins a feature's variants — their ids, keys and values, which +// are project-level — with one scope's weights. Variants keep the feature's own +// order, so the same flag renders the same way in every scope. +func variantViews(feature *api.Feature, weights variantWeights) []variantView { + if weights == nil { + return nil + } + views := make([]variantView, 0, len(feature.MultivariateOptions)) + for _, o := range feature.MultivariateOptions { + views = append(views, variantView{ + ID: o.ID, Value: mvOptionValue(o), Weight: weights[o.ID], Key: o.Key, + }) + } + return views } func flagEnabled(fs *api.FeatureState) bool { @@ -367,7 +464,11 @@ var flagGetCmd = &cobra.Command{ } return renderSegmentDetail(cmd, v) } - return renderFlagDetail(cmd, feature) + weights, err := scopeWeights(cmd, cred, env.ID, feature, 0) + if err != nil { + return err + } + return renderFlagDetail(cmd, feature, weights) }, } @@ -489,10 +590,13 @@ func listFeatureIdentityOverrides(cmd *cobra.Command, cred *activeCredential, en } // renderFlagDetail prints one flag's curated detail view (or its JSON). -func renderFlagDetail(cmd *cobra.Command, feature *api.Feature) error { +// weights are the environment's variant weights, nil for a feature without +// variants; the renderer does no fetching of its own. +func renderFlagDetail(cmd *cobra.Command, feature *api.Feature, weights variantWeights) error { v := newFlagView(feature) + v.Variants = variantViews(feature, weights) return output.Render(cmd.OutOrStdout(), v, outputOpts(), func(w io.Writer) error { - return output.Detail(w, []output.Field{ + if err := output.Detail(w, []output.Field{ {Label: "Feature", Value: v.Feature}, {Label: "Description", Value: v.Description}, {Label: "Type", Value: v.Type}, @@ -502,31 +606,44 @@ func renderFlagDetail(cmd *cobra.Command, feature *api.Feature) error { {Label: "Identity overrides", Value: strconv.Itoa(v.IdentityOverrides)}, {Label: "Code references", Value: strconv.Itoa(v.CodeReferences)}, {Label: "Lifecycle stage", Value: lifecycleOrDash(v.LifecycleStage)}, - }) + }); err != nil { + return err + } + return writeVariants(w, v.Variants, weightPercent) }) } -// buildSegmentFlagView resolves the override's segment name and priority and -// assembles the curated view; the renderers stay free of fetching. +// buildSegmentFlagView resolves the override's segment name, priority and +// variant weights and assembles the curated view; the renderers stay free of +// fetching. func buildSegmentFlagView(cmd *cobra.Command, cred *activeCredential, env api.Environment, feature *api.Feature, segmentID int) (segmentFlagView, error) { - segment, priority, err := segmentOverrideMeta(cmd, cred, env.ID, feature.ID, segmentID) + meta, err := segmentOverrideMeta(cmd, cred, env.ID, feature.ID, segmentID) if err != nil { return segmentFlagView{}, err } - return newSegmentFlagView(feature, segment, priority), nil + weights, err := scopeWeights(cmd, cred, env.ID, feature, meta.stateID) + if err != nil { + return segmentFlagView{}, err + } + v := newSegmentFlagView(feature, meta.segment, meta.priority) + v.Variants = variantViews(feature, weights) + return v, nil } // renderSegmentDetail prints a flag's curated state for one segment override. func renderSegmentDetail(cmd *cobra.Command, v segmentFlagView) error { return output.Render(cmd.OutOrStdout(), v, outputOpts(), func(w io.Writer) error { - return output.Detail(w, []output.Field{ + if err := output.Detail(w, []output.Field{ {Label: "Feature", Value: v.Feature}, {Label: "Type", Value: v.Type}, {Label: "Segment", Value: v.Segment.display()}, {Label: "Priority", Value: strconv.Itoa(v.Priority)}, {Label: "State", Value: boolState(v.Enabled)}, {Label: "Value", Value: valueDisplay(v.Value)}, - }) + }); err != nil { + return err + } + return writeVariants(w, v.Variants, weightPercent) }) } From 402de8f1d711a21a18a562e7fa3f2bd53536b289 Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Fri, 14 Aug 2026 17:55:50 +0100 Subject: [PATCH 2/9] refactor: show every weight as a percentage in human output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The RFC marked weights with a % in the flag views but not the feature ones, so the same table read two ways. Human output now marks them all; JSON keeps numeric weights, and a weight echoed back as the user typed it (`--weight hero=40`) or read inside a sentence that carries its own % is still bare. RFC amended to match: the three `feature` examples in §9 now show 30%/50%. --- internal/cmd/cmd_test.go | 5 +++-- internal/cmd/feature.go | 10 +++++----- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/internal/cmd/cmd_test.go b/internal/cmd/cmd_test.go index f6b29db..24e24a4 100644 --- a/internal/cmd/cmd_test.go +++ b/internal/cmd/cmd_test.go @@ -5961,7 +5961,8 @@ func TestFeatureGet(t *testing.T) { if err != nil { t.Fatalf("feature get: %v\noutput: %s", err, out) } - for _, want := range []string{"banner-copy (91)", "multivariate", "hello", "Variants", "headline", "30", "hero", "subhead"} { + // Human views mark every weight as the percentage it is. + for _, want := range []string{"banner-copy (91)", "multivariate", "hello", "Variants", "headline", "30%", "hero", "subhead", "70%"} { if !strings.Contains(out, want) { t.Errorf("output = %q, want %q", out, want) } @@ -6284,7 +6285,7 @@ func TestFeatureVariant(t *testing.T) { if err != nil { t.Fatalf("variant list: %v\noutput: %s", err, out) } - for _, want := range []string{"VALUE", "WEIGHT", "KEY", "ID", "headline", "30", "hero", "201", "subhead"} { + for _, want := range []string{"VALUE", "WEIGHT", "KEY", "ID", "headline", "30%", "hero", "201", "subhead", "70%"} { if !strings.Contains(out, want) { t.Errorf("output = %q, want %q", out, want) } diff --git a/internal/cmd/feature.go b/internal/cmd/feature.go index 242341d..a7e0182 100644 --- a/internal/cmd/feature.go +++ b/internal/cmd/feature.go @@ -86,9 +86,9 @@ func formatWeight(w float64) string { return strconv.FormatFloat(w, 'f', -1, 64) } -// weightPercent marks a weight as the percentage it is. Flag views use it: a -// flag's distribution is one scope's share of traffic, unlike the variant's own -// default weight the feature views show. +// weightPercent marks a weight as the percentage it is. Every human view shows +// weights this way; formatWeight stays bare for the places a weight is echoed +// back as the user typed it, or read inside a sentence that carries its own %. func weightPercent(w float64) string { return formatWeight(w) + "%" } @@ -184,7 +184,7 @@ func renderFeature(cmd *cobra.Command, f *api.Feature) error { }); err != nil { return err } - return writeVariants(w, view.Variants, formatWeight) + return writeVariants(w, view.Variants, weightPercent) }) } @@ -432,7 +432,7 @@ var featureVariantListCmd = &cobra.Command{ return renderList(cmd, variants, "No variants.", []string{"VALUE", "WEIGHT", "KEY", "ID"}, func(_ int, v variantView) []string { - return []string{fmt.Sprint(v.Value), formatWeight(v.Weight), v.Key, strconv.Itoa(v.ID)} + return []string{fmt.Sprint(v.Value), weightPercent(v.Weight), v.Key, strconv.Itoa(v.ID)} }, "", "") }, } From 8d82b56d006aa0595077721c37b2cf95b8cb4b88 Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Fri, 14 Aug 2026 18:40:50 +0100 Subject: [PATCH 3/9] fix: name the segment when an update creates its override MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A new override has no feature-segment row yet, so there was no name to read for the confirmation or the detail view, and both showed a bare id — right after the user had named the segment themselves: ✓ Set banner_copy weights to hero=100, sub=0 for segment 1 in ... Segment 1 Resolving a name warms the segment name cache, so read the name back from there, and keep the bare id for the case that is genuinely nameless: an override referenced by id that does not exist yet. Found testing against the update-flag implementation (Flagsmith/flagsmith#8102). --- internal/cmd/cmd_test.go | 21 +++++++++++++++++++++ internal/cmd/flag_update.go | 17 ++++++++--------- internal/cmd/flags.go | 14 ++++++++++---- 3 files changed, 39 insertions(+), 13 deletions(-) diff --git a/internal/cmd/cmd_test.go b/internal/cmd/cmd_test.go index 24e24a4..165bd1b 100644 --- a/internal/cmd/cmd_test.go +++ b/internal/cmd/cmd_test.go @@ -4215,6 +4215,27 @@ func TestFlagUpdateSegment(t *testing.T) { } }) + // A new override has no feature-segment row to read a name from, so the + // name the user typed (cached when it was resolved) is what names it. + t.Run("a new override is still named, not shown as a bare id", func(t *testing.T) { + // Given + flagUpdateEnv(t) + + // When + out, err := run("", "flag", "update", "onboarding_banner", "--segment", "us-adults", "--value", "yo", "--yes") + + // Then + if err != nil { + t.Fatalf("flag update --segment: %v\noutput: %s", err, out) + } + if !strings.Contains(out, "for segment us-adults (42)") { + t.Errorf("output = %q, want the segment named in the confirmation", out) + } + if strings.Contains(out, "Segment 42\n") { + t.Errorf("output = %q, want the detail to name the segment too", out) + } + }) + t.Run("a new override leaves the value to the server, which inherits it", func(t *testing.T) { // Given f := flagUpdateEnv(t) // max_items has 1 override, integer 25, off diff --git a/internal/cmd/flag_update.go b/internal/cmd/flag_update.go index 5e644ba..ae0092e 100644 --- a/internal/cmd/flag_update.go +++ b/internal/cmd/flag_update.go @@ -9,7 +9,6 @@ import ( "github.com/Flagsmith/flagsmith-cli/v2/internal/api" "github.com/Flagsmith/flagsmith-cli/v2/internal/bug" - "github.com/Flagsmith/flagsmith-cli/v2/internal/cache" "github.com/Flagsmith/flagsmith-cli/v2/internal/output" ) @@ -340,8 +339,8 @@ func applyFlagMutation(cmd *cobra.Command, name string, m flagMutation) error { } // scalarOfValue converts a typed value from an update-flag response into the -// bare scalar the flag views render. A feature with no value at all reads as -// unset rather than as an empty string. +// bare scalar the flag views render. A flag with no value at all reads as unset +// rather than as an empty string. func scalarOfValue(v *api.FeatureValue) (any, error) { if v == nil { return nil, nil @@ -405,11 +404,9 @@ func deleteSegmentOverride(cmd *cobra.Command, cred *activeCredential, env api.E survivors = append(survivors, fs) } if target == nil { - // The name cache (seeded by resolving a name ref) is all there is to go - // on for a segment with no override; the display degrades to the id. - missing := label(cache.Load(apiURL).Segments[strconv.Itoa(segmentID)], segmentID) return withHint( - fmt.Errorf("%s has no override for segment %s in %s", feature.Name, missing, environmentLabel(env)), + fmt.Errorf("%s has no override for segment %s in %s", + feature.Name, label(cachedSegmentName(segmentID), segmentID), environmentLabel(env)), fmt.Sprintf("Run `flagsmith flag list --feature %s` to see its segment overrides.", feature.Name)) } segmentLabel := segmentRef{ID: segmentID, Name: target.SegmentName}.display() @@ -464,8 +461,10 @@ func echoOverrides(cmd *cobra.Command, cred *activeCredential, env api.Environme Priority: &priority, Variants: echoVariants(feature, state.Multivariate), } - // An override with no value of its own is restated by omission: what a - // replacing write leaves out is reset, which is the null it already has. + // An override with no value of its own is restated by omission, since the + // wire form has no way to say "no value": a replacing write inherits the + // environment default for what it omits, which for a valueless flag is + // the same nothing. if scalar := state.Value.Scalar(); scalar != nil { value := featureValueFromScalar(scalar) override.Value = &value diff --git a/internal/cmd/flags.go b/internal/cmd/flags.go index 50835df..b2dd342 100644 --- a/internal/cmd/flags.go +++ b/internal/cmd/flags.go @@ -96,9 +96,10 @@ type overrideMeta struct { } // segmentOverrideMeta reads one segment override's metadata from the -// feature-segments endpoint, seeding the name cache on the way. A missing row -// (an override that doesn't exist, or one created a moment ago) degrades to the -// bare id and priority 0 rather than failing. +// feature-segments endpoint, seeding the name cache on the way. An override +// with no row yet — one about to be created, or created a moment ago — has no +// name to read there, so it falls back to the cache (warm whenever the segment +// was named rather than given by id) and then to the bare id. func segmentOverrideMeta(cmd *cobra.Command, cred *activeCredential, environmentID, featureID, segmentID int) (overrideMeta, error) { fss, err := cred.client().FeatureSegments(cmd.Context(), environmentID, featureID) if err != nil { @@ -118,7 +119,12 @@ func segmentOverrideMeta(cmd *cobra.Command, cred *activeCredential, environment }, nil } } - return overrideMeta{segment: segmentRef{ID: segmentID}}, nil + return overrideMeta{segment: segmentRef{ID: segmentID, Name: cachedSegmentName(segmentID)}}, nil +} + +// cachedSegmentName is a segment's name if the cache happens to hold it, or "". +func cachedSegmentName(segmentID int) string { + return cache.Load(apiURL).Segments[strconv.Itoa(segmentID)] } // variantWeights maps a variant id to its weight in one scope. From 3842a65395fdcad58dcbd3aab84e6dd1532bae89 Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Sat, 15 Aug 2026 11:07:59 +0100 Subject: [PATCH 4/9] fix: recognise the change-request refusal by its code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The merged endpoint answers a workflow-gated write with 409 and `"code": "change_requests_enabled"`, where the branch this was built against used 400 and prose. Matching prose would have broken on the next rewording anyway, so read the code, and don't lean on the status alone — 409 will cover any conflict added later. Without this, an environment that simply uses change requests reports as something to report as a bug. --- internal/api/client.go | 41 ++++++++++++++++++++++++++----------- internal/api/client_test.go | 24 +++++++++++++++++++--- internal/cmd/cmd_test.go | 3 ++- 3 files changed, 52 insertions(+), 16 deletions(-) diff --git a/internal/api/client.go b/internal/api/client.go index f561b3d..4642f8c 100644 --- a/internal/api/client.go +++ b/internal/api/client.go @@ -297,18 +297,35 @@ func responseError(method, u string, resp *http.Response) error { if e := classifyLimit(msg); e != nil { return e } - return bug.Mark(&statusError{code: resp.StatusCode, status: resp.Status, message: msg, method: method, url: u}) + return bug.Mark(&statusError{ + code: resp.StatusCode, status: resp.Status, message: msg, + errorCode: apiErrorCode(body), method: method, url: u, + }) +} + +// apiErrorCode extracts the machine-readable code from an error body, or "". +// Only the newer endpoints send one; it is the only part of an error a client +// should branch on. +func apiErrorCode(body []byte) string { + var coded struct { + Code string `json:"code"` + } + if json.Unmarshal(bytes.TrimSpace(body), &coded) != nil { + return "" + } + return coded.Code } // statusError is a non-2xx response that wasn't classified as a plan limit. It // carries the HTTP status so it can be special-cased. bug.Mark wraps it, so it // still reads as unexpected. type statusError struct { - code int - status string // e.g. "403 Forbidden" - message string // the API's detail, if any - method string - url string + code int + status string // e.g. "403 Forbidden" + message string // the API's detail, if any + errorCode string // the API's machine-readable code, if any + method string + url string } func (e *statusError) Error() string { @@ -920,16 +937,16 @@ func (c *Client) writeFlag(ctx context.Context, method, environmentKey string, f return out, nil } -// changeRequestPhrase is the verbatim backend detail for a refused write — -// matched case-insensitively, since the status (400) is shared with every -// validation error. -const changeRequestPhrase = "change requests enabled" +// changeRequestCode is the error code update-flag returns when it refuses a +// write outright, alongside a 409. +const changeRequestCode = "change_requests_enabled" // classifyFlagWrite recognises the one update-flag failure the user can act on. +// The code is what identifies it — the status alone would catch any future +// conflict, and the detail is prose that is free to change. func classifyFlagWrite(err error) error { var e *statusError - if errors.As(err, &e) && e.code == http.StatusBadRequest && - strings.Contains(strings.ToLower(e.message), changeRequestPhrase) { + if errors.As(err, &e) && e.code == http.StatusConflict && e.errorCode == changeRequestCode { return ErrWorkflowGated } return err diff --git a/internal/api/client_test.go b/internal/api/client_test.go index 05cfb59..61d0c8d 100644 --- a/internal/api/client_test.go +++ b/internal/api/client_test.go @@ -584,9 +584,10 @@ func TestUpdateFlag(t *testing.T) { }) t.Run("a change-request refusal is the workflow sentinel, not a bug", func(t *testing.T) { - // Given - srv, _, _ := updateFlagServer(t, http.StatusBadRequest, - `{"detail": "Cannot update flags in an environment with change requests enabled."}`) + // Given: the code identifies it — the status is shared with any other + // conflict, and the detail is prose. + srv, _, _ := updateFlagServer(t, http.StatusConflict, + `{"detail": "Cannot update flags in an environment with change requests enabled.", "code": "change_requests_enabled"}`) // When _, err := testClient(srv.URL, APIKey("k.s"), srv).UpdateFlag(context.Background(), "envkey", 42, @@ -598,6 +599,23 @@ func TestUpdateFlag(t *testing.T) { } }) + t.Run("a conflict without the code stays a plain failure", func(t *testing.T) { + // Given + srv, _, _ := updateFlagServer(t, http.StatusConflict, `{"detail": "Something else conflicted."}`) + + // When + _, err := testClient(srv.URL, APIKey("k.s"), srv).UpdateFlag(context.Background(), "envkey", 42, + UpdateFlagRequest{EnvironmentDefault: &FlagStateUpdate{Enabled: &enabled}}) + + // Then + if errors.Is(err, ErrWorkflowGated) { + t.Errorf("err = %v, want a plain failure", err) + } + if err == nil || !strings.Contains(err.Error(), "Something else conflicted.") { + t.Errorf("err = %v, want the API's own message", err) + } + }) + t.Run("a validation error surfaces the field message, however deeply nested", func(t *testing.T) { // Given srv, _, _ := updateFlagServer(t, http.StatusBadRequest, diff --git a/internal/cmd/cmd_test.go b/internal/cmd/cmd_test.go index 165bd1b..be82467 100644 --- a/internal/cmd/cmd_test.go +++ b/internal/cmd/cmd_test.go @@ -641,9 +641,10 @@ func newFakeInstance(t *testing.T) *fakeInstance { gated := f.workflowGated f.mu.Unlock() if gated { - w.WriteHeader(http.StatusBadRequest) + w.WriteHeader(http.StatusConflict) json.NewEncoder(w).Encode(map[string]any{ "detail": "Cannot update flags in an environment with change requests enabled.", + "code": "change_requests_enabled", }) return } From 9e54f7166aeeeaadb045413aabf6270e8cbb0c92 Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Sat, 15 Aug 2026 11:08:16 +0100 Subject: [PATCH 5/9] fix: reject two weights for one variant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--weight hero=10,30011=20` names one variant twice when 30011 is hero's id. The duplicate check keyed on the reference as typed, so this passed and the last weight quietly won. Check once the reference has resolved, and name both spellings in the error — the point is that the user probably doesn't realise they are the same variant. --- internal/cmd/cmd_test.go | 2 ++ internal/cmd/flag_update.go | 9 +++++++++ 2 files changed, 11 insertions(+) diff --git a/internal/cmd/cmd_test.go b/internal/cmd/cmd_test.go index be82467..6211edb 100644 --- a/internal/cmd/cmd_test.go +++ b/internal/cmd/cmd_test.go @@ -4819,6 +4819,8 @@ func TestFlagUpdateWeight(t *testing.T) { {"over 100 on its own", "hero=140", "is not a percentage"}, {"negative", "hero=-1", "is not a percentage"}, {"the same variant twice", "hero=10,hero=20", "given twice"}, + {"the same variant by key and by id", "hero=10,30011=20", "hero and 30011 are the same variant"}, + {"the same variant by id and by key", "30011=10,hero=20", "30011 and hero are the same variant"}, {"an unknown key", "heroic=10", "is not a variant of banner_copy"}, {"an unknown id", "99999=10", "is not a variant of banner_copy"}, {"the control", "control=10", `"control" is not a variant`}, diff --git a/internal/cmd/flag_update.go b/internal/cmd/flag_update.go index ae0092e..3aba673 100644 --- a/internal/cmd/flag_update.go +++ b/internal/cmd/flag_update.go @@ -111,6 +111,8 @@ func parseWeights(args []string) ([]weightRef, error) { return nil, hintf(usageErrorf("%q is not a variant", controlVariantKey), "Whatever the variants leave unallocated serves the flag's own value — set it with --value.") } + // Two references to one variant are caught after they resolve, in + // mergeWeights; this only catches the same one written twice. if seen[ref] { return nil, usageErrorf("--weight %s is given twice", ref) } @@ -144,6 +146,7 @@ func mergeWeights(feature *api.Feature, current variantWeights, refs []weightRef for _, o := range feature.MultivariateOptions { weights[o.ID] = current[o.ID] } + namedBy := make(map[int]string, len(refs)) for _, r := range refs { // Resolved against the feature, never created: the endpoint would take // an unknown key as a new variant, so a typo would silently add one. @@ -153,6 +156,12 @@ func mergeWeights(feature *api.Feature, current variantWeights, refs []weightRef "Run `flagsmith feature variant list %s` to see its variants, or `flagsmith feature variant add %s` to add one.", feature.Name, feature.Name) } + // A variant named twice — by key and by id, say — is two weights for one + // variant, and picking one of them silently is the wrong answer. + if first, ok := namedBy[option.ID]; ok { + return nil, usageErrorf("--weight %s and %s are the same variant", first, r.ref) + } + namedBy[option.ID] = r.ref weights[option.ID] = r.weight } total := 0.0 From 23690b93d1dbe15ce2da0e997c0693e07f90e836 Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Sat, 15 Aug 2026 11:08:22 +0100 Subject: [PATCH 6/9] fix: restate an override's zero weights when deleting a sibling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting an override replaces the whole set, and a replacing write that omits `variants` inherits the environment default's weights rather than leaving the override alone. So an override with no allocations of its own could not be restated by omission, as it was — it came back weighted like the environment. Send the full list for any feature with variants, zeros included. --- internal/cmd/cmd_test.go | 33 +++++++++++++++++++++++++++++++++ internal/cmd/flag_update.go | 10 +++++----- 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/internal/cmd/cmd_test.go b/internal/cmd/cmd_test.go index 6211edb..75d88e9 100644 --- a/internal/cmd/cmd_test.go +++ b/internal/cmd/cmd_test.go @@ -5003,6 +5003,39 @@ func TestFlagGetVariants(t *testing.T) { }) } +// An override with no allocations of its own can't be restated by omission: a +// replacing write reads that as "inherit the environment default's weights". +func TestFlagDeleteKeepsUnallocatedVariants(t *testing.T) { + // Given a multivariate flag whose surviving override has no weights of its + // own, while the environment default is on 20/30. + f := flagUpdateEnv(t) + withMultivariateFlag(f) + withFeatureSegments(f, 3, + map[string]any{"id": 3100, "segment": 12, "segment_name": "early-adopters", "priority": 0}, + map[string]any{"id": 3200, "segment": 42, "segment_name": "us-adults", "priority": 1}, + ) + withFeatureStates(f, 3, + map[string]any{"id": 30, "enabled": true, "feature_segment": nil, + "feature_state_value": map[string]any{"type": "unicode", "string_value": "hello"}, + "multivariate_feature_state_values": []map[string]any{{"multivariate_feature_option": 30011, "percentage_allocation": 20}, {"multivariate_feature_option": 30010, "percentage_allocation": 30}}}, + map[string]any{"id": 32, "enabled": true, "feature_segment": 3200, + "feature_state_value": map[string]any{"type": "unicode", "string_value": "hi"}}, + ) + + // When + if _, err := run("", "flag", "delete", "banner_copy", "--segment", "12", "--yes"); err != nil { + t.Fatalf("flag delete: %v", err) + } + + // Then the survivor's zeros are stated outright, rather than left to be + // filled in from the environment default. + survivor := f.lastUpdate["segment_overrides"].([]any)[0].(map[string]any) + got := wireWeights(survivor) + if len(got) != 2 || got[30011] != 0 || got[30010] != 0 { + t.Errorf("survivor variants = %+v, want both variants at 0", got) + } +} + func TestFlagDeleteKeepsVariants(t *testing.T) { // Given a multivariate flag with two overrides, one of them being deleted. f := flagUpdateEnv(t) diff --git a/internal/cmd/flag_update.go b/internal/cmd/flag_update.go index 3aba673..48af9f0 100644 --- a/internal/cmd/flag_update.go +++ b/internal/cmd/flag_update.go @@ -484,12 +484,12 @@ func echoOverrides(cmd *cobra.Command, cred *activeCredential, env api.Environme } // echoVariants restates a feature state's distribution as the full variant list -// the endpoint requires, so a replacing write leaves the weights alone. A -// feature without variants has none to send, and neither has a state with no -// allocations of its own — sending zeros would be a change, where omission -// restates the nothing that is already there. +// the endpoint requires, so a replacing write leaves the weights alone. Only a +// feature without variants has none to send: an override with no allocations of +// its own must still say so with zeros, because a replacing write that omits +// them inherits the environment default's weights instead. func echoVariants(feature *api.Feature, allocations []api.MultivariateStateValue) []api.Variant { - if len(feature.MultivariateOptions) == 0 || len(allocations) == 0 { + if len(feature.MultivariateOptions) == 0 { return nil } weights := make(variantWeights, len(allocations)) From d64324c561350cc779c07b04fd0d2c4963bf98ee Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Sat, 15 Aug 2026 11:08:30 +0100 Subject: [PATCH 7/9] fix: don't assume segment override priorities are dense MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Priorities order the overrides but needn't be contiguous — the API's own examples use 10/20/30 — and the merged endpoint now rejects two overrides sharing one. Two things followed from the old assumption: - a new override joined at `num_segment_overrides`, which collides with an existing override whenever the priorities are sparse. It now joins past the highest, read from the rows already fetched for the segment's name. - `--priority` was range-checked against that count, rejecting a move the server would accept. Only a negative is rejected now; the server owns the rest. --- internal/cmd/cmd_test.go | 53 ++++++++++++++++++++++++++++++++++--- internal/cmd/flag_update.go | 18 +++++-------- internal/cmd/flags.go | 25 ++++++++++++----- 3 files changed, 73 insertions(+), 23 deletions(-) diff --git a/internal/cmd/cmd_test.go b/internal/cmd/cmd_test.go index 75d88e9..52d9d08 100644 --- a/internal/cmd/cmd_test.go +++ b/internal/cmd/cmd_test.go @@ -4237,10 +4237,36 @@ func TestFlagUpdateSegment(t *testing.T) { } }) + // The server rejects two overrides sharing a priority, so a new one has to + // clear every existing priority rather than count them. + t.Run("a new override joins past sparse priorities, not at their count", func(t *testing.T) { + // Given + f := flagUpdateEnv(t) + withSegmentOverride(f, false) + withFeatureSegments(f, 2, + map[string]any{"id": 1200, "segment": 12, "segment_name": "powerusers", "priority": 10}, + map[string]any{"id": 4200, "segment": 42, "segment_name": "us-adults", "priority": 20}, + ) + + // When + if _, err := run("", "flag", "update", "max_items", "--segment", "7", "--enable", "--yes"); err != nil { + t.Fatalf("flag update --segment: %v", err) + } + + // Then + ov := f.lastUpdate["segment_overrides"].([]any)[0].(map[string]any) + if ov["priority"] != float64(21) { + t.Errorf("segment override = %+v, want priority 21 — past the highest, not the count", ov) + } + }) + t.Run("a new override leaves the value to the server, which inherits it", func(t *testing.T) { // Given f := flagUpdateEnv(t) // max_items has 1 override, integer 25, off withSegmentOverride(f, false) + withFeatureSegments(f, 2, map[string]any{ + "id": 1200, "segment": 12, "segment_name": "powerusers", "priority": 0, + }) // When _, err := run("", "flag", "update", "max_items", "--segment", "7", "--enable", "--yes") @@ -4335,7 +4361,10 @@ func TestFlagUpdateRendersWithoutRefetch(t *testing.T) { t.Run("a new override reports the appended priority", func(t *testing.T) { // Given f := flagUpdateEnv(t) - withSegmentOverride(f, false) // num_segment_overrides: 1 + withSegmentOverride(f, false) + withFeatureSegments(f, 2, map[string]any{ + "id": 1200, "segment": 12, "segment_name": "powerusers", "priority": 0, + }) // When out, err := run("", "flag", "update", "max_items", "--segment", "7", "--enable", "--yes") @@ -5128,12 +5157,28 @@ func TestFlagUpdatePriority(t *testing.T) { } }) - t.Run("out of range exits 2 before any write", func(t *testing.T) { + // Priorities order the overrides but needn't be dense, so a number past + // their count is a legitimate move, not a mistake to reject. + t.Run("a sparse priority is sent as given", func(t *testing.T) { + f := flagUpdateEnv(t) + withSegmentOverride(f, true) + overrideMeta(f) + + if _, err := run("", "flag", "update", "max_items", "--segment", "12", "--priority", "50", "--yes"); err != nil { + t.Fatalf("flag update --priority 50: %v", err) + } + ov := f.lastUpdate["segment_overrides"].([]any)[0].(map[string]any) + if ov["priority"] != float64(50) { + t.Errorf("override = %+v, want priority 50", ov) + } + }) + + t.Run("a negative priority exits 2 before any write", func(t *testing.T) { f := flagUpdateEnv(t) - withSegmentOverride(f, true) // num_segment_overrides: 1 → valid range 0..0 + withSegmentOverride(f, true) overrideMeta(f) - _, err := run("", "flag", "update", "max_items", "--segment", "12", "--priority", "5", "--yes") + _, err := run("", "flag", "update", "max_items", "--segment", "12", "--priority", "-1", "--yes") var ue *usageError if !errors.As(err, &ue) || !strings.Contains(err.Error(), "--priority") { t.Errorf("err = %v, want a usage error naming --priority", err) diff --git a/internal/cmd/flag_update.go b/internal/cmd/flag_update.go index 48af9f0..abe3d8f 100644 --- a/internal/cmd/flag_update.go +++ b/internal/cmd/flag_update.go @@ -228,17 +228,11 @@ func applyFlagMutation(cmd *cobra.Command, name string, m flagMutation) error { scope = fmt.Sprintf("for segment %s in environment %s", meta.segment.display(), environmentLabel(env)) } - // Priorities are a dense 0-based order; a new override joins it, growing the - // valid range by one. The server treats the write as a move, so only the - // bounds need checking. - if m.setPriority { - limit := feature.NumSegmentOverrides - if target == nil { - limit++ - } - if m.priority < 0 || m.priority >= limit { - return usageErrorf("--priority %d is out of range (0..%d)", m.priority, limit-1) - } + // Priorities order the overrides but needn't be dense — 10/20/30 is as valid + // as 0/1/2 — so there is no upper bound to check against. The server rejects + // a priority that would collide with another override. + if m.setPriority && m.priority < 0 { + return usageErrorf("--priority %d is negative", m.priority) } state := api.FlagStateUpdate{} @@ -288,7 +282,7 @@ func applyFlagMutation(cmd *cobra.Command, name string, m flagMutation) error { override.Enabled = &enabled } if override.Priority == nil { - priority := feature.NumSegmentOverrides // joins at the end + priority := meta.nextPriority // joins at the end override.Priority = &priority } } diff --git a/internal/cmd/flags.go b/internal/cmd/flags.go index b2dd342..ba9d7cc 100644 --- a/internal/cmd/flags.go +++ b/internal/cmd/flags.go @@ -89,10 +89,13 @@ func newSegmentFlagView(f *api.Feature, segment segmentRef, priority int) segmen // overrideMeta is what the feature-segments endpoint knows about one segment // override: the segment's name, the override's priority, and the id linking it // to its feature state. stateID is 0 when the override does not exist yet. +// nextPriority is a free priority past every existing override, for one that is +// about to be created — priorities need not be dense, so it is not the count. type overrideMeta struct { - segment segmentRef - priority int - stateID int + segment segmentRef + priority int + stateID int + nextPriority int } // segmentOverrideMeta reads one segment override's metadata from the @@ -106,20 +109,28 @@ func segmentOverrideMeta(cmd *cobra.Command, cred *activeCredential, environment return overrideMeta{}, err } names := make(map[string]string, len(fss)) + next := 0 for _, fs := range fss { names[strconv.Itoa(fs.Segment)] = fs.SegmentName + if fs.Priority >= next { + next = fs.Priority + 1 + } } _ = cache.Merge(apiURL, &cache.Names{Segments: names}) // opportunistic for _, fs := range fss { if fs.Segment == segmentID { return overrideMeta{ - segment: segmentRef{ID: segmentID, Name: fs.SegmentName}, - priority: fs.Priority, - stateID: fs.ID, + segment: segmentRef{ID: segmentID, Name: fs.SegmentName}, + priority: fs.Priority, + stateID: fs.ID, + nextPriority: next, }, nil } } - return overrideMeta{segment: segmentRef{ID: segmentID, Name: cachedSegmentName(segmentID)}}, nil + return overrideMeta{ + segment: segmentRef{ID: segmentID, Name: cachedSegmentName(segmentID)}, + nextPriority: next, + }, nil } // cachedSegmentName is a segment's name if the cache happens to hold it, or "". From b4172362be3714fee736e5a75ac92fbc82170551 Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Sat, 15 Aug 2026 13:24:49 +0100 Subject: [PATCH 8/9] test: anchor the segment-naming assertion to the detail row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The check that a new override's detail names its segment was a negative assertion on `"Segment 42\n"` — an exact run of four spaces, which tabwriter sets from the widest label in the view. Any change to the padding would have made it pass while testing nothing. Match the row instead, with `\s+` for the padding, so the assertion says where the name has to appear rather than only that the bare id is absent from one particular rendering. --- internal/cmd/cmd_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/cmd/cmd_test.go b/internal/cmd/cmd_test.go index 52d9d08..952b675 100644 --- a/internal/cmd/cmd_test.go +++ b/internal/cmd/cmd_test.go @@ -4232,7 +4232,9 @@ func TestFlagUpdateSegment(t *testing.T) { if !strings.Contains(out, "for segment us-adults (42)") { t.Errorf("output = %q, want the segment named in the confirmation", out) } - if strings.Contains(out, "Segment 42\n") { + // Anchored to the detail's own row: the confirmation names the segment + // too, so counting occurrences would not say where they are. + if !regexp.MustCompile(`(?m)^Segment\s+us-adults \(42\)$`).MatchString(out) { t.Errorf("output = %q, want the detail to name the segment too", out) } }) From bab2f7a841809156730a0dc37cb520544d17028a Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Sat, 15 Aug 2026 15:01:23 +0100 Subject: [PATCH 9/9] feat: delete a segment override with its own verb MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Flagsmith/flagsmith#8307 adds `DELETE …/features/{id}/segment-overrides/{segment_id}/`, so removing one override no longer means replacing the whole set. That takes the read-modify-write out of `flag delete --segment`: the surviving overrides are neither read nor restated, so a concurrent change to one can't be lost, and the endpoint's answer is the whole flag, as it is for the writes. `echoOverrides` and `echoVariants` go with it — restating an override's weights only mattered because a replacing write inherits the environment default's when `variants` is omitted. The override rows are still read, for two reasons a 404 doesn't cover: the prompt names the segment, and a segment with no override should be caught before asking rather than after. --- internal/api/client.go | 41 +++++--- internal/api/client_test.go | 38 ++++--- internal/cmd/cmd_test.go | 191 +++++++++++++--------------------- internal/cmd/flag_update.go | 123 ++-------------------- internal/cmd/sentinel_test.go | 4 + 5 files changed, 132 insertions(+), 265 deletions(-) diff --git a/internal/api/client.go b/internal/api/client.go index 4642f8c..6e0a262 100644 --- a/internal/api/client.go +++ b/internal/api/client.go @@ -843,11 +843,10 @@ type SegmentTarget struct { } // FlagStateUpdate is a change to a flag's state in one scope. Every field is -// optional: PATCH leaves an omitted one unchanged, and PUT resets it to the -// server's default — so a partial PUT is a destructive write. +// optional, and an omitted one is left unchanged. // // Variants is all-or-nothing: the endpoint rejects a list that doesn't name -// every variant of the feature, whatever the verb. +// every variant of the feature. type FlagStateUpdate struct { Enabled *bool `json:"enabled,omitempty"` Value *FeatureValue `json:"value,omitempty"` @@ -865,14 +864,12 @@ type SegmentOverrideUpdate struct { Variants []Variant `json:"variants,omitempty"` } -// UpdateFlagRequest is the update-flag body. Under PATCH the overrides listed -// are created or updated and the rest are left alone; under PUT the list -// replaces the whole set, so an override missing from it is deleted — which is -// why SegmentOverrides distinguishes nil (absent) from empty (delete them all). -// The endpoint does not manage identity overrides. +// UpdateFlagRequest is the update-flag body: the overrides it lists are created +// or updated, and every property it leaves out is left as it is. The endpoint +// does not manage identity overrides. type UpdateFlagRequest struct { EnvironmentDefault *FlagStateUpdate `json:"environment_default,omitempty"` - SegmentOverrides []SegmentOverrideUpdate `json:"segment_overrides,omitzero"` + SegmentOverrides []SegmentOverrideUpdate `json:"segment_overrides,omitempty"` } // FlagState is a flag's resulting state in one scope, as the update-flag @@ -921,14 +918,6 @@ func (c *Client) UpdateFlag(ctx context.Context, environmentKey string, featureI return c.writeFlag(ctx, http.MethodPatch, environmentKey, featureID, in) } -// ReplaceFlag replaces each property the request carries in full — the only way -// to delete a segment override, by PUTting the overrides that survive it. -// Anything a replaced property leaves out is reset, so callers must echo the -// state they mean to keep. -func (c *Client) ReplaceFlag(ctx context.Context, environmentKey string, featureID int, in UpdateFlagRequest) (*UpdateFlagResponse, error) { - return c.writeFlag(ctx, http.MethodPut, environmentKey, featureID, in) -} - func (c *Client) writeFlag(ctx context.Context, method, environmentKey string, featureID int, in UpdateFlagRequest) (*UpdateFlagResponse, error) { out, err := send[UpdateFlagResponse](ctx, c, method, updateFlagPath(environmentKey, featureID), in) if err != nil { @@ -937,6 +926,24 @@ func (c *Client) writeFlag(ctx context.Context, method, environmentKey string, f return out, nil } +// DeleteSegmentOverride removes a flag's override for one segment, leaving the +// rest of the flag alone. Like the writes, it answers with the flag's whole +// resulting state. A segment with no override of its own is ErrNoSuchOverride. +func (c *Client) DeleteSegmentOverride(ctx context.Context, environmentKey string, featureID, segmentID int) (*UpdateFlagResponse, error) { + path := fmt.Sprintf("%ssegment-overrides/%d/", updateFlagPath(environmentKey, featureID), segmentID) + out, err := send[UpdateFlagResponse](ctx, c, http.MethodDelete, path, nil) + if err != nil { + if statusOf(err) == http.StatusNotFound { + return nil, ErrNoSuchOverride + } + return nil, classifyFlagWrite(err) + } + return out, nil +} + +// ErrNoSuchOverride is returned when a flag serves a segment nothing of its own. +var ErrNoSuchOverride = errors.New("no override exists for that segment") + // changeRequestCode is the error code update-flag returns when it refuses a // write outright, alongside a 409. const changeRequestCode = "change_requests_enabled" diff --git a/internal/api/client_test.go b/internal/api/client_test.go index 61d0c8d..cb381b1 100644 --- a/internal/api/client_test.go +++ b/internal/api/client_test.go @@ -555,31 +555,39 @@ func TestUpdateFlag(t *testing.T) { } }) - t.Run("puts an empty override list verbatim, so the last override can be deleted", func(t *testing.T) { + t.Run("deletes one override by its own verb, and decodes what is left", func(t *testing.T) { // Given - srv, seen, body := updateFlagServer(t, http.StatusOK, - `{"environment_default": {"enabled": false, "value": null, "variants": []}, "segment_overrides": []}`) + srv, seen, _ := updateFlagServer(t, http.StatusOK, + `{"environment_default": {"enabled": false, "value": null, "variants": []}, + "segment_overrides": [{"segment": {"id": 12}, "priority": 0, "enabled": true, "value": null, "variants": []}]}`) // When - _, err := testClient(srv.URL, APIKey("k.s"), srv).ReplaceFlag(context.Background(), "envkey", 42, - UpdateFlagRequest{SegmentOverrides: []SegmentOverrideUpdate{}}) + resp, err := testClient(srv.URL, APIKey("k.s"), srv).DeleteSegmentOverride(context.Background(), "envkey", 42, 99) // Then if err != nil { t.Fatal(err) } - if seen.Method != http.MethodPut { - t.Errorf("method = %s, want PUT", seen.Method) - } - overrides, ok := (*body)["segment_overrides"] - if !ok { - t.Fatalf("body = %+v, want an empty segment_overrides list", *body) + if seen.Method != http.MethodDelete || + seen.URL.Path != "/api/__future__/environments/envkey/features/42/segment-overrides/99/" { + t.Errorf("request = %s %s", seen.Method, seen.URL.Path) } - if got := overrides.([]any); len(got) != 0 { - t.Errorf("segment_overrides = %+v, want empty", got) + // The response is the whole flag, so the survivor is readable from it. + if resp.Override(12) == nil || resp.Override(99) != nil { + t.Errorf("segment_overrides = %+v, want only segment 12 left", resp.SegmentOverrides) } - if _, ok := (*body)["environment_default"]; ok { - t.Errorf("body = %+v, want no environment_default — a PUT would reset it", *body) + }) + + t.Run("deleting an override that is not there is a no-override error", func(t *testing.T) { + // Given + srv, _, _ := updateFlagServer(t, http.StatusNotFound, `{"detail": "Segment override not found."}`) + + // When + _, err := testClient(srv.URL, APIKey("k.s"), srv).DeleteSegmentOverride(context.Background(), "envkey", 42, 99) + + // Then + if !errors.Is(err, ErrNoSuchOverride) { + t.Errorf("err = %v, want ErrNoSuchOverride", err) } }) diff --git a/internal/cmd/cmd_test.go b/internal/cmd/cmd_test.go index 952b675..5c318a1 100644 --- a/internal/cmd/cmd_test.go +++ b/internal/cmd/cmd_test.go @@ -173,6 +173,7 @@ type fakeInstance struct { lastUpdate map[string]any // last update-flag request body lastUpdateVerb string // method of the last update-flag call lastUpdateFeat string // feature id in the last update-flag path + deletedSegment int // segment id of the last override delete, 0 for none workflowGated bool // when true, update-flag reports change requests useEdge bool // GET /projects/{id}/ use_edge_identities @@ -629,9 +630,46 @@ func newFakeInstance(t *testing.T) *fakeInstance { "count": len(items), "next": nil, "previous": nil, "results": items, }) }) - // update-flag: PATCH writes only the properties it carries, PUT replaces - // each property it carries in full (so an override it leaves out is gone). - for _, method := range []string{http.MethodPatch, http.MethodPut} { + // Deleting one segment's override, which answers with the whole flag as the + // writes do. + mux.HandleFunc("DELETE /api/__future__/environments/{env}/features/{feature}/segment-overrides/{segment}/", func(w http.ResponseWriter, r *http.Request) { + if !authorized(r) { + w.WriteHeader(http.StatusUnauthorized) + return + } + f.mu.Lock() + gated := f.workflowGated + f.mu.Unlock() + if gated { + w.WriteHeader(http.StatusConflict) + json.NewEncoder(w).Encode(map[string]any{ + "detail": "Cannot update flags in an environment with change requests enabled.", + "code": "change_requests_enabled", + }) + return + } + featureKey, segment := r.PathValue("feature"), r.PathValue("segment") + segmentID, _ := strconv.Atoi(segment) + f.mu.Lock() + defer f.mu.Unlock() + item := f.featureItem(featureKey) + if item == nil || f.overrideRow(featureKey, segmentID) == nil { + w.WriteHeader(http.StatusNotFound) + json.NewEncoder(w).Encode(map[string]any{"detail": "Segment override not found."}) + return + } + f.deletedSegment = segmentID + keep := map[int]bool{} + for _, row := range f.featureSegments[featureKey] { + if id, _ := row["segment"].(int); id != segmentID { + keep[id] = true + } + } + f.deleteOverridesExcept(featureKey, item, keep) + json.NewEncoder(w).Encode(f.flagStateResponse(featureKey, item)) + }) + // update-flag: PATCH writes only the properties it carries. + for _, method := range []string{http.MethodPatch} { mux.HandleFunc(method+" /api/__future__/environments/{env}/features/{feature}/", func(w http.ResponseWriter, r *http.Request) { if !authorized(r) { w.WriteHeader(http.StatusUnauthorized) @@ -1496,6 +1534,16 @@ func (f *fakeInstance) featureItem(featureKey string) map[string]any { return nil } +// overrideRow returns the feature-segment row for one segment, or nil. +func (f *fakeInstance) overrideRow(featureKey string, segmentID int) map[string]any { + for _, row := range f.featureSegments[featureKey] { + if id, _ := row["segment"].(int); id == segmentID { + return row + } + } + return nil +} + // stateRow returns the registered featurestates row for one scope — the // environment default for stateID 0, otherwise the override linked by that // feature-segment id — or nil when the test registered none. @@ -4448,8 +4496,8 @@ func TestFlagSegmentByName(t *testing.T) { if err != nil { t.Fatalf("flag delete --segment us-adults: %v", err) } - if got := f.lastUpdate["segment_overrides"].([]any); len(got) != 0 { - t.Errorf("segment_overrides = %+v, want segment 42's override gone", got) + if f.deletedSegment != 42 { + t.Errorf("deleted segment = %d, want 42 resolved from the name", f.deletedSegment) } }) @@ -5034,75 +5082,6 @@ func TestFlagGetVariants(t *testing.T) { }) } -// An override with no allocations of its own can't be restated by omission: a -// replacing write reads that as "inherit the environment default's weights". -func TestFlagDeleteKeepsUnallocatedVariants(t *testing.T) { - // Given a multivariate flag whose surviving override has no weights of its - // own, while the environment default is on 20/30. - f := flagUpdateEnv(t) - withMultivariateFlag(f) - withFeatureSegments(f, 3, - map[string]any{"id": 3100, "segment": 12, "segment_name": "early-adopters", "priority": 0}, - map[string]any{"id": 3200, "segment": 42, "segment_name": "us-adults", "priority": 1}, - ) - withFeatureStates(f, 3, - map[string]any{"id": 30, "enabled": true, "feature_segment": nil, - "feature_state_value": map[string]any{"type": "unicode", "string_value": "hello"}, - "multivariate_feature_state_values": []map[string]any{{"multivariate_feature_option": 30011, "percentage_allocation": 20}, {"multivariate_feature_option": 30010, "percentage_allocation": 30}}}, - map[string]any{"id": 32, "enabled": true, "feature_segment": 3200, - "feature_state_value": map[string]any{"type": "unicode", "string_value": "hi"}}, - ) - - // When - if _, err := run("", "flag", "delete", "banner_copy", "--segment", "12", "--yes"); err != nil { - t.Fatalf("flag delete: %v", err) - } - - // Then the survivor's zeros are stated outright, rather than left to be - // filled in from the environment default. - survivor := f.lastUpdate["segment_overrides"].([]any)[0].(map[string]any) - got := wireWeights(survivor) - if len(got) != 2 || got[30011] != 0 || got[30010] != 0 { - t.Errorf("survivor variants = %+v, want both variants at 0", got) - } -} - -func TestFlagDeleteKeepsVariants(t *testing.T) { - // Given a multivariate flag with two overrides, one of them being deleted. - f := flagUpdateEnv(t) - withMultivariateFlag(f) - withFeatureSegments(f, 3, - map[string]any{"id": 3100, "segment": 12, "segment_name": "early-adopters", "priority": 0}, - map[string]any{"id": 3200, "segment": 42, "segment_name": "us-adults", "priority": 1}, - ) - withFeatureStates(f, 3, - map[string]any{"id": 30, "enabled": true, "feature_segment": nil, - "feature_state_value": map[string]any{"type": "unicode", "string_value": "hello"}}, - map[string]any{"id": 32, "enabled": true, "feature_segment": 3200, - "feature_state_value": map[string]any{"type": "unicode", "string_value": "hi"}, - "multivariate_feature_state_values": []map[string]any{ - {"multivariate_feature_option": 30011, "percentage_allocation": 60}, - {"multivariate_feature_option": 30010, "percentage_allocation": 40}, - }}, - ) - - // When - out, err := run("", "flag", "delete", "banner_copy", "--segment", "12", "--yes") - - // Then the survivor's weights are restated, so replacing the set can't - // blank them. - if err != nil { - t.Fatalf("flag delete: %v\noutput: %s", err, out) - } - survivor := f.lastUpdate["segment_overrides"].([]any)[0].(map[string]any) - if wireSegmentID(survivor) != 42 { - t.Fatalf("survivor = %+v, want segment 42", survivor) - } - if got := wireWeights(survivor); got[30011] != 60 || got[30010] != 40 { - t.Errorf("survivor variants = %+v, want its own 60/40 restated", got) - } -} - func TestFlagUpdatePriority(t *testing.T) { // max_items (feature 2) has one override, for segment 12 at priority 1. overrideMeta := func(f *fakeInstance) { @@ -5220,18 +5199,21 @@ func TestFlagFeatureByID(t *testing.T) { } }) - t.Run("delete --segment by id targets the feature id in the path", func(t *testing.T) { + t.Run("delete --segment by id resolves the feature", func(t *testing.T) { f := flagUpdateEnv(t) withFeatureSegments(f, 2, map[string]any{ "id": 1200, "segment": 12, "segment_name": "powerusers", "priority": 0, }) - _, err := run("", "flag", "delete", "2", "--segment", "12", "--yes") + out, err := run("", "flag", "delete", "2", "--segment", "12", "--yes") if err != nil { t.Fatalf("flag delete 2: %v", err) } - if f.lastUpdateFeat != "2" { - t.Errorf("feature on the wire = %q, want id 2", f.lastUpdateFeat) + if f.deletedSegment != 12 { + t.Errorf("deleted segment = %d, want 12", f.deletedSegment) + } + if !strings.Contains(out, "Deleted max_items override") { + t.Errorf("output = %q, want the canonical name in the message", out) } }) @@ -5423,9 +5405,7 @@ func withTwoSegmentOverrides(f *fakeInstance) { } func TestFlagDelete(t *testing.T) { - // The endpoint has no delete verb, so an override is removed by replacing - // the whole set with the overrides that survive it. - t.Run("replaces the override set with the survivors, echoed in full", func(t *testing.T) { + t.Run("deletes the one override, leaving the others alone", func(t *testing.T) { // Given f := flagUpdateEnv(t) withTwoSegmentOverrides(f) @@ -5437,49 +5417,18 @@ func TestFlagDelete(t *testing.T) { if err != nil { t.Fatalf("flag delete: %v\noutput: %s", err, out) } - if f.lastUpdateVerb != http.MethodPut || f.lastUpdateFeat != "2" { - t.Errorf("request = %s feature %s, want PUT feature 2", f.lastUpdateVerb, f.lastUpdateFeat) - } - if _, ok := f.lastUpdate["environment_default"]; ok { - t.Errorf("body = %+v, want no environment_default — a PUT would reset it", f.lastUpdate) - } - overrides := f.lastUpdate["segment_overrides"].([]any) - if len(overrides) != 1 { - t.Fatalf("segment_overrides = %+v, want only the survivor", overrides) + if f.deletedSegment != 12 { + t.Errorf("deleted segment = %d, want 12", f.deletedSegment) } - survivor := overrides[0].(map[string]any) - if wireSegmentID(survivor) != 42 || survivor["priority"] != float64(1) || - survivor["enabled"] != false || survivor["value"].(map[string]any)["value"] != "99" { - t.Errorf("survivor = %+v, want segment 42's state restated in full", survivor) + // Nothing is restated, so no write can touch the survivor. + if f.lastUpdate != nil { + t.Errorf("lastUpdate = %+v, want no update-flag write at all", f.lastUpdate) } if !strings.Contains(out, "Deleted max_items override for segment powerusers (12)") { t.Errorf("output = %q", out) } }) - t.Run("deleting the last override sends an empty list", func(t *testing.T) { - // Given - f := flagUpdateEnv(t) - withFeatureSegments(f, 2, map[string]any{ - "id": 1200, "segment": 12, "segment_name": "powerusers", "priority": 0, - }) - - // When - out, err := run("", "flag", "delete", "max_items", "--segment", "12", "--yes") - - // Then - if err != nil { - t.Fatalf("flag delete: %v\noutput: %s", err, out) - } - overrides, ok := f.lastUpdate["segment_overrides"] - if !ok { - t.Fatalf("body = %+v, want an empty segment_overrides list", f.lastUpdate) - } - if got := overrides.([]any); len(got) != 0 { - t.Errorf("segment_overrides = %+v, want empty", got) - } - }) - t.Run("without --segment exits 2", func(t *testing.T) { f := flagUpdateEnv(t) _, err := run("", "flag", "delete", "max_items", "--yes") @@ -5487,8 +5436,8 @@ func TestFlagDelete(t *testing.T) { if !errors.As(err, &ue) || !strings.Contains(err.Error(), "--segment") { t.Errorf("err = %v, want a usage error naming --segment", err) } - if f.lastUpdate != nil { - t.Errorf("lastUpdate = %+v, want no call", f.lastUpdate) + if f.deletedSegment != 0 { + t.Errorf("deleted segment = %d, want no call", f.deletedSegment) } }) @@ -5504,8 +5453,8 @@ func TestFlagDelete(t *testing.T) { if err == nil || !strings.Contains(err.Error(), "segment 57") { t.Errorf("err = %v, want a not-found error naming the segment", err) } - if f.lastUpdate != nil { - t.Errorf("lastUpdate = %+v, want no write", f.lastUpdate) + if f.deletedSegment != 0 { + t.Errorf("deleted segment = %d, want no call", f.deletedSegment) } }) } diff --git a/internal/cmd/flag_update.go b/internal/cmd/flag_update.go index abe3d8f..4d20258 100644 --- a/internal/cmd/flag_update.go +++ b/internal/cmd/flag_update.go @@ -387,116 +387,36 @@ var flagDeleteCmd = &cobra.Command{ }, } -// deleteSegmentOverride removes one segment's override. The endpoint has no verb -// for deleting a single override: a PUT replaces the whole set, and whatever it -// leaves out is deleted. So the surviving overrides are read and echoed back in -// full — a partial echo would reset the state it omits. Being a -// read-modify-write, a change made to another override in between is lost. +// deleteSegmentOverride removes one segment's override, in one call that leaves +// the rest of the flag alone. The override rows are read first all the same: +// they name the segment for the prompt, and a segment with no override is +// better caught before asking than as a 404 afterwards. func deleteSegmentOverride(cmd *cobra.Command, cred *activeCredential, env api.Environment, feature *api.Feature, segmentID int) error { - fss, err := cred.client().FeatureSegments(cmd.Context(), env.ID, feature.ID) + meta, err := segmentOverrideMeta(cmd, cred, env.ID, feature.ID, segmentID) if err != nil { return err } - survivors := make([]api.FeatureSegment, 0, len(fss)) - var target *api.FeatureSegment - for i, fs := range fss { - if fs.Segment == segmentID { - target = &fss[i] - continue - } - survivors = append(survivors, fs) - } - if target == nil { + if meta.stateID == 0 { return withHint( fmt.Errorf("%s has no override for segment %s in %s", - feature.Name, label(cachedSegmentName(segmentID), segmentID), environmentLabel(env)), + feature.Name, meta.segment.display(), environmentLabel(env)), fmt.Sprintf("Run `flagsmith flag list --feature %s` to see its segment overrides.", feature.Name)) } - segmentLabel := segmentRef{ID: segmentID, Name: target.SegmentName}.display() errOut := cmd.ErrOrStderr() - prompt := fmt.Sprintf("delete %s override for segment %s in %s", feature.Name, segmentLabel, environmentLabel(env)) + prompt := fmt.Sprintf("delete %s override for segment %s in %s", feature.Name, meta.segment.display(), environmentLabel(env)) if ok, err := confirmed(cmd, prompt+"?", "changed"); !ok || err != nil { return err } - overrides, err := echoOverrides(cmd, cred, env, feature, survivors) - if err != nil { - return err - } - if _, err := cred.client().ReplaceFlag(cmd.Context(), env.APIKey, feature.ID, - api.UpdateFlagRequest{SegmentOverrides: overrides}); err != nil { + if _, err := cred.client().DeleteSegmentOverride(cmd.Context(), env.APIKey, feature.ID, segmentID); err != nil { return err } - output.Success(errOut, "Deleted %s override for segment %s in environment %s", feature.Name, segmentLabel, environmentLabel(env)) + output.Success(errOut, "Deleted %s override for segment %s in environment %s", + feature.Name, meta.segment.display(), environmentLabel(env)) return nil } -// echoOverrides reads the current state of the given overrides and restates it -// in full, for a PUT that must preserve them. The result is never nil: an empty -// list is what deletes the last override, and must reach the wire as one. -func echoOverrides(cmd *cobra.Command, cred *activeCredential, env api.Environment, feature *api.Feature, keep []api.FeatureSegment) ([]api.SegmentOverrideUpdate, error) { - overrides := make([]api.SegmentOverrideUpdate, 0, len(keep)) - if len(keep) == 0 { - return overrides, nil - } - states, err := cred.client().FeatureStates(cmd.Context(), env.ID, feature.ID) - if err != nil { - return nil, err - } - byOverride := make(map[int]api.EnvironmentFeatureState, len(states)) - for _, s := range states { - if s.FeatureSegment != nil { - byOverride[*s.FeatureSegment] = s - } - } - for _, fs := range keep { - // Echoing a zero state would rewrite the override as off with no value, - // so a missing one stops the write rather than guessing. - state, ok := byOverride[fs.ID] - if !ok { - return nil, bug.Mark(fmt.Errorf("no feature state found for the override on segment %d; refusing to write", fs.Segment)) - } - enabled, priority := state.Enabled, fs.Priority - override := api.SegmentOverrideUpdate{ - Segment: api.SegmentTarget{ID: fs.Segment}, - Enabled: &enabled, - Priority: &priority, - Variants: echoVariants(feature, state.Multivariate), - } - // An override with no value of its own is restated by omission, since the - // wire form has no way to say "no value": a replacing write inherits the - // environment default for what it omits, which for a valueless flag is - // the same nothing. - if scalar := state.Value.Scalar(); scalar != nil { - value := featureValueFromScalar(scalar) - override.Value = &value - } - overrides = append(overrides, override) - } - return overrides, nil -} - -// echoVariants restates a feature state's distribution as the full variant list -// the endpoint requires, so a replacing write leaves the weights alone. Only a -// feature without variants has none to send: an override with no allocations of -// its own must still say so with zeros, because a replacing write that omits -// them inherits the environment default's weights instead. -func echoVariants(feature *api.Feature, allocations []api.MultivariateStateValue) []api.Variant { - if len(feature.MultivariateOptions) == 0 { - return nil - } - weights := make(variantWeights, len(allocations)) - for _, a := range allocations { - weights[a.OptionID] = a.Allocation - } - variants := make([]api.Variant, 0, len(feature.MultivariateOptions)) - for _, o := range feature.MultivariateOptions { - variants = append(variants, api.Variant{ID: o.ID, Weight: weights[o.ID]}) - } - return variants -} - var ( flagToggleSegment string flagToggleIdentifier string @@ -564,27 +484,6 @@ func currentScalar(fs *api.FeatureState) any { return fs.Value } -// featureValueFromScalar converts a bare scalar — read from the features list -// (JSON numbers arrive as float64) or from api.TypedValue.Scalar() (ints stay -// int) — into the {type, value} wire form update-flag expects. -func featureValueFromScalar(v any) api.FeatureValue { - switch t := v.(type) { - case bool: - return api.FeatureValue{Type: "boolean", Value: strconv.FormatBool(t)} - case int: - return api.FeatureValue{Type: "integer", Value: strconv.Itoa(t)} - case float64: - if t == float64(int64(t)) { - return api.FeatureValue{Type: "integer", Value: strconv.FormatInt(int64(t), 10)} - } - return api.FeatureValue{Type: "string", Value: strconv.FormatFloat(t, 'f', -1, 64)} - case string: - return api.FeatureValue{Type: "string", Value: t} - default: // nil or an unexpected shape → empty string, Flagsmith's default - return api.FeatureValue{Type: "string", Value: ""} - } -} - // inferFeatureValue types a --value literal, honouring an explicit --type. // Inference: true/false → boolean, all-digit → integer, otherwise string. func inferFeatureValue(raw, typeFlag string) (api.FeatureValue, error) { diff --git a/internal/cmd/sentinel_test.go b/internal/cmd/sentinel_test.go index 2a5db67..f76ab01 100644 --- a/internal/cmd/sentinel_test.go +++ b/internal/cmd/sentinel_test.go @@ -24,6 +24,7 @@ func TestEverySentinelHasAHintDecision(t *testing.T) { sentinels := map[string]error{ "api.ErrPlanGated": api.ErrPlanGated, "api.ErrQuotaExceeded": api.ErrQuotaExceeded, + "api.ErrNoSuchOverride": api.ErrNoSuchOverride, "api.ErrWorkflowGated": api.ErrWorkflowGated, "auth.ErrNotLoggedIn": auth.ErrNotLoggedIn, "auth.ErrKeychainUnavailable": auth.ErrKeychainUnavailable, @@ -40,6 +41,9 @@ func TestEverySentinelHasAHintDecision(t *testing.T) { consciouslyUnhinted := map[string]bool{ // The user chose to abort; there is nothing to recover from. "prompt.ErrCancelled": true, + // Only reachable when the override goes between the check and the + // delete; `flag delete` hints for the case the user can act on. + "api.ErrNoSuchOverride": true, } found := scanSentinelNames(t)