Skip to content

feat: add update_attributes() to merge user attributes - #139

Open
vazarkevych wants to merge 1 commit into
growthbook:mainfrom
vazarkevych:feat/update-attributes
Open

vazarkevych wants to merge 1 commit into
growthbook:mainfrom
vazarkevych:feat/update-attributes

Conversation

@vazarkevych

Copy link
Copy Markdown
Collaborator

Summary

Adds GrowthBook.update_attributes(), which shallow-merges the given attributes
into the existing ones instead of replacing them — parity with the TS SDK's
updateAttributes().

Previously the only way to change attributes was set_attributes(), which fully
replaces the map. Adding a single attribute meant rebuilding the whole dict, a
frequent source of bugs where previously set attributes get silently lost.

What Changed

  • Added update_attributes(attributes) to the sync GrowthBook class:
    shallow merge — new keys added, existing overwritten, untouched preserved.
  • Delegates to set_attributes(), so sticky-bucket refresh and remote-eval
    cache invalidation (refetch) are reused rather than duplicated.
  • None is a no-op; a None value is stored, not treated as key removal
    (parity with the Ruby and C# internal SDKs).
  • Added a README example describing replace (set_attributes) vs merge
    (update_attributes).

Scope

Only the sync GrowthBook client is affected. The async GrowthBookClient
passes attributes per-request via UserContext and has no persistent global
attribute state to merge into, so no change applies there.

Test plan

  • test_update_attributes_merges_into_existing — new keys are added
  • test_update_attributes_overwrites_and_preserves — existing overwritten, others kept
  • test_update_attributes_merges_not_replaces — merge, unlike set_attributes
  • test_update_attributes_none_is_noop — None argument changes nothing
  • test_update_attributes_stores_none_value — a None value is stored, not removed

All 790 tests pass.

@greptile-apps

greptile-apps Bot commented Sep 15, 2026

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

The PR appears safe to merge, but empty updates can cause an avoidable remote request.

Reviews (1) · Last reviewed commit: "feat: add update_attributes() to merge u..."

Comment thread growthbook/growthbook.py Outdated
this does not replace the whole map. Passing None (or an empty dict) is
a no-op. Sticky buckets and, in remote-eval mode, the feature refetch
are handled by delegating to set_attributes."""
self.set_attributes({**self._attributes, **(attributes or {})})

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 None and {} still call set_attributes. In remote-eval mode, an expired cache makes this no-change call send a blocking HTTP request. Return before calling set_attributes when attributes is empty.

Suggested change
self.set_attributes({**self._attributes, **(attributes or {})})
if not attributes:
return
self.set_attributes({**self._attributes, **attributes})
Prompt To Fix With AI
This is a comment left during a code review.
Path: growthbook/growthbook.py
Line: 1144

Comment:
`None` and `{}` still call `set_attributes`. In remote-eval mode, an expired cache makes this no-change call send a blocking HTTP request. Return before calling `set_attributes` when `attributes` is empty.

```suggestion
        if not attributes:
            return
        self.set_attributes({**self._attributes, **attributes})
```

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

@vazarkevych

Copy link
Copy Markdown
Collaborator Author

confirmed and fixed. Reproduced it on the PR code: two no-op
calls (None and {}) issued two blocking POSTs in remote-eval mode. The
docstring claimed a no-op while the code still delegated to set_attributes,
which refreshes sticky buckets and refetches.

update_attributes now returns before delegating when the update is empty.
Behavior is unchanged — merging nothing produces an equal map — only the side
effects are gone.

The existing test_update_attributes_none_is_noop couldn't catch this: it
asserted on the resulting attributes, which are identical either way. Added a
test that asserts on the side effects instead (no sticky bucket refresh, no
refetch), which fails without the fix.

Also rebased onto current main — the branch was 127 commits behind.

Shallow-merges into the existing attributes (parity with the JS SDK's
updateAttributes): new keys are added, existing keys overwritten, untouched
keys preserved. Unlike set_attributes it does not replace the whole map. A None
value is stored rather than treated as key removal.

Sticky bucket refresh and, in remote-eval mode, the feature refetch come from
delegating to set_attributes — except for an empty update, which returns first.
Merging nothing produces an equal map, but delegating would still refresh
sticky buckets and issue a blocking refetch, so a no-change call cost an HTTP
request whenever the cache had expired.
@vazarkevych
vazarkevych force-pushed the feat/update-attributes branch from acf3ce0 to c645d9c Compare October 5, 2026 18:10
@andywhite37
andywhite37 self-requested a review October 8, 2026 21:28
@andywhite37 andywhite37 added effort:small Narrow change with limited review and testing work. needs-human-input A specific decision or manual task blocks progress; see the queue entry. priority:normal Regular queue priority; no evidence of urgency or low value. risk:low Limited potential impact; does not imply approval or completed testing. labels Oct 20, 2026
@andywhite37

Copy link
Copy Markdown
Contributor

Maintainer decision needed: confirm the new synchronous update_attributes API and its contract: shallow merge, supplied values overwrite existing keys, None values remain values, and None or an empty dictionary does nothing. Nonempty updates use set_attributes and retain its sticky-bucket and remote-evaluation effects. Please record acceptance so API and regression review can proceed; align the empty-input behavior with growthbook-ruby#41.

@vazarkevych

Copy link
Copy Markdown
Collaborator Author

Recording the contract:

  • Shallow merge. Supplied keys are added or overwrite; untouched keys are
    preserved. Unlike set_attributes, the map is not replaced.
  • None is a value, not a deletion. update_attributes({"plan": None})
    stores plan=None; there is no key-removal form.
  • None or {} does nothing — returns before delegating, no sticky-bucket
    refresh and no refetch.
  • Non-empty updates delegate to set_attributes and keep its effects:
    sticky bucket refresh, and in remote-eval mode the blocking refetch.

On aligning empty-input with growthbook-ruby#41: the observable behavior
already matches — an empty update changes nothing in either SDK. What differs
is the path, and the reason is Python-specific.

Ruby's attributes= only calls refresh_sticky_buckets, so always delegating
costs nothing and the question never came up. Python's set_attributes also
issues a blocking POST in remote-eval mode, so delegating on an empty update
spent an HTTP request whenever the cache had expired — measured on this PR's
original code: two no-op calls, two POSTs. That's what greptile flagged (P2)
and what the early return fixes.

Happy to drop the early return and match Ruby literally if you'd rather have
identical code shapes across SDKs, but then update_attributes(None) is a
network call in remote-eval mode. The alternative alignment — adding the same
guard to Ruby — costs nothing there and would make both exact.

Tests assert on the side effects (no refresh, no refetch) rather than the
resulting attributes, since those are identical either way; the existing
test_update_attributes_none_is_noop couldn't catch the original bug for
exactly that reason.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort:small Narrow change with limited review and testing work. needs-human-input A specific decision or manual task blocks progress; see the queue entry. priority:normal Regular queue priority; no evidence of urgency or low value. risk:low Limited potential impact; does not imply approval or completed testing.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants