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..6e0a262 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 { @@ -339,27 +356,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 +744,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 +822,145 @@ 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, 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. +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: 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 { - 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,omitempty"` } -// 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 +// 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, err := c.newRequest(ctx, http.MethodPost, path, bytes.NewReader(body)) + 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) +} + +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 + return nil, classifyFlagWrite(err) } - req.Header.Set("Content-Type", "application/json") - resp, err := c.httpClient.Do(req) + 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 { - 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) + if statusOf(err) == http.StatusNotFound { + return nil, ErrNoSuchOverride + } + 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}, +// 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" + +// 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.StatusConflict && e.errorCode == changeRequestCode { + 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 +974,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..cb381b1 100644 --- a/internal/api/client_test.go +++ b/internal/api/client_test.go @@ -499,46 +499,146 @@ 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("deletes one override by its own verb, and decodes what is left", 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, _ := updateFlagServer(t, http.StatusOK, + `{"environment_default": {"enabled": false, "value": null, "variants": []}, + "segment_overrides": [{"segment": {"id": 12}, "priority": 0, "enabled": true, "value": null, "variants": []}]}`) + + // When + 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.MethodDelete || + seen.URL.Path != "/api/__future__/environments/envkey/features/42/segment-overrides/99/" { + t.Errorf("request = %s %s", seen.Method, seen.URL.Path) + } + // 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) + } + }) + + 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) + } + }) + + t.Run("a change-request refusal is the workflow sentinel, not a bug", func(t *testing.T) { + // 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, + UpdateFlagRequest{EnvironmentDefault: &FlagStateUpdate{Enabled: &enabled}}) + + // Then + if !errors.Is(err, ErrWorkflowGated) { + t.Errorf("err = %v, want ErrWorkflowGated", err) + } + }) + + 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, + `{"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..5c318a1 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,12 @@ 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 + 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 coreIdentities map[string]int // identifier -> identity id @@ -627,7 +630,9 @@ 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) { + // 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 @@ -636,37 +641,63 @@ func newFakeInstance(t *testing.T) *fakeInstance { 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) + 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() - missing := f.segmentMissing - f.mu.Unlock() - if missing { + 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 } - var body map[string]any - json.NewDecoder(r.Body).Decode(&body) - f.mu.Lock() - f.lastDelete = body - f.mu.Unlock() - w.WriteHeader(http.StatusNoContent) + 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) + 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 + } + 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 +1403,242 @@ 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) + if got, _ := item["id"].(int); got == id { + return item } - 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) - } - } - } - } - 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 +} + +// 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 } -// scalarFromWire turns an update-flag-v2 {type,value} into the bare scalar the +// 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 +} + +// 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 +1654,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 +3863,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 +3876,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 +4012,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 +4161,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 +4197,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 +4213,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 +4225,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,12 +4259,64 @@ 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) + } + }) + + // 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) + } + // 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) + } }) - t.Run("a new override inherits the env default value", func(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") @@ -3952,10 +4326,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) } }) } @@ -4036,7 +4411,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") @@ -4088,8 +4466,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 +4480,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 +4488,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 f.deletedSegment != 42 { + t.Errorf("deleted segment = %d, want 42 resolved from the name", f.deletedSegment) } }) @@ -4133,18 +4514,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 +4742,346 @@ 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"}, + {"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`}, + } { + 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 TestFlagUpdatePriority(t *testing.T) { // max_items (feature 2) has one override, for segment 12 at priority 1. overrideMeta := func(f *fakeInstance) { @@ -4378,13 +5100,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) @@ -4421,12 +5138,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) // num_segment_overrides: 1 → valid range 0..0 + withSegmentOverride(f, true) overrideMeta(f) - _, err := run("", "flag", "update", "max_items", "--segment", "12", "--priority", "5", "--yes") + 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) + overrideMeta(f) + + _, 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) @@ -4458,24 +5191,29 @@ 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 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.lastDelete["feature"].(map[string]any)["id"] != float64(2) { - t.Errorf("delete body = %+v, want the feature targeted by id", f.lastDelete) + 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) } }) @@ -4487,8 +5225,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 +5235,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 +5255,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 +5304,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 +5386,29 @@ 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) { + t.Run("deletes the one override, leaving the others alone", func(t *testing.T) { // Given f := flagUpdateEnv(t) + withTwoSegmentOverrides(f) // When out, err := run("", "flag", "delete", "max_items", "--segment", "12", "--yes") @@ -4684,11 +5417,14 @@ 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.deletedSegment != 12 { + t.Errorf("deleted segment = %d, want 12", f.deletedSegment) } - if !strings.Contains(out, "Deleted max_items override for segment 12") { + // 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) } }) @@ -4700,18 +5436,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.deletedSegment != 0 { + t.Errorf("deleted segment = %d, want no call", f.deletedSegment) } }) - 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.deletedSegment != 0 { + t.Errorf("deleted segment = %d, want no call", f.deletedSegment) + } }) } @@ -5270,7 +6014,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) } @@ -5593,7 +6338,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 bfcf52a..a7e0182 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. 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) + "%" +} + +// 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, weightPercent) }) } @@ -411,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)} }, "", "") }, } 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..4d20258 100644 --- a/internal/cmd/flag_update.go +++ b/internal/cmd/flag_update.go @@ -3,11 +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/cache" + "github.com/Flagsmith/flagsmith-cli/v2/internal/bug" "github.com/Flagsmith/flagsmith-cli/v2/internal/output" ) @@ -16,6 +17,7 @@ var ( flagDisableFlag bool flagValueFlag string flagTypeFlag string + flagWeightFlags []string flagUpdateSegment string flagUpdateIdentifier string flagUpdatePriority int @@ -36,7 +38,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 +64,118 @@ 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.") + } + // 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) + } + 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] + } + 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. + 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) + } + // 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 + 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 +185,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,84 +206,87 @@ 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 - // 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) } - // 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 := meta.nextPriority // joins at the end + override.Priority = &priority + } + } + req.SegmentOverrides = []api.SegmentOverrideUpdate{override} } errOut := cmd.ErrOrStderr() @@ -180,12 +294,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 +316,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 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 + } + return nativeScalar(*v) } var flagDeleteCmd = &cobra.Command{ @@ -243,27 +375,48 @@ 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 { - return err - } - if err := cred.client().DeleteSegmentOverride(cmd.Context(), env.APIKey, featureRefFor(name), segmentID); err != nil { + segmentID, err := resolveSegmentID(cmd, cred, projectID, flagDeleteSegment) + if err != nil { return err } - output.Success(errOut, "Deleted %s override for segment %s in environment %s", name, segmentLabel, environmentLabel(env)) - return nil + return deleteSegmentOverride(cmd, cred, env, feature, segmentID) }, } +// 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 { + meta, err := segmentOverrideMeta(cmd, cred, env.ID, feature.ID, segmentID) + if err != nil { + return err + } + if meta.stateID == 0 { + return withHint( + fmt.Errorf("%s has no override for segment %s in %s", + feature.Name, meta.segment.display(), environmentLabel(env)), + fmt.Sprintf("Run `flagsmith flag list --feature %s` to see its segment overrides.", feature.Name)) + } + + errOut := cmd.ErrOrStderr() + 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 + } + + 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, meta.segment.display(), environmentLabel(env)) + return nil +} + var ( flagToggleSegment string flagToggleIdentifier string @@ -323,15 +476,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 { @@ -340,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-v2 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) { @@ -409,6 +532,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..ba9d7cc 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,26 +86,138 @@ 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. +// 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 + nextPriority int +} + +// segmentOverrideMeta reads one segment override's metadata from the +// 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 { - return segmentRef{}, 0, err + 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 segmentRef{ID: segmentID, Name: fs.SegmentName}, fs.Priority, nil + return overrideMeta{ + 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)}, + nextPriority: next, + }, 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. +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 segmentRef{ID: segmentID}, 0, nil + 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 + } + 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 +481,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 +607,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 +623,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) }) } 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)