Raise a clear error when a hybrid property reads a backend var on a state#6621
Raise a clear error when a hybrid property reads a backend var on a state#6621masenf wants to merge 2 commits into
Conversation
Merging this PR will not alter performance
Comparing Footnotes
|
Greptile SummaryThis PR extends
Confidence Score: 4/5Safe to merge; the core logic is correct and well-tested across all targeted model types. The implementation is clean and the guard correctly blocks backend-only vars when building frontend vars on State classes while leaving plain dataclass/model attribute access unaffected. The only concerns are minor: a lazy import and inspect.getattr_static now run on every concrete-type ObjectVar.getattr invocation, and the inherited descriptor-sharing behaviour of _var is undocumented. No data-correctness or security issues were found. packages/reflex-base/src/reflex_base/vars/object.py - the new hot-path overhead in getattr is worth a second look if var-construction performance becomes a concern. Important Files Changed
Reviews (1): Last reviewed commit: "docs: add news fragments for hybrid prop..." | Re-trigger Greptile |
| if isinstance(fixed_type, type): | ||
| from .hybrid_property import HybridProperty | ||
|
|
||
| # A HybridProperty on the underlying type resolves to a frontend Var with | ||
| # this object var substituted as `self`, so e.g. `State.info.a_b` uses the | ||
| # same Var-access semantics as accessing the hybrid property directly. | ||
| descriptor = inspect.getattr_static(fixed_type, name, None) | ||
| if isinstance(descriptor, HybridProperty): | ||
| return descriptor._get_var(self) |
There was a problem hiding this comment.
The lazy
from .hybrid_property import HybridProperty import and inspect.getattr_static call are now executed on every ObjectVar.__getattr__ invocation for concrete types — including the common case where the attribute is just a regular field. Python’s module cache makes the repeat import very cheap, but not free. Moving the import to module level (guarded by try/except ImportError if needed to avoid circular imports) would eliminate the per-call overhead.
| if isinstance(fixed_type, type): | |
| from .hybrid_property import HybridProperty | |
| # A HybridProperty on the underlying type resolves to a frontend Var with | |
| # this object var substituted as `self`, so e.g. `State.info.a_b` uses the | |
| # same Var-access semantics as accessing the hybrid property directly. | |
| descriptor = inspect.getattr_static(fixed_type, name, None) | |
| if isinstance(descriptor, HybridProperty): | |
| return descriptor._get_var(self) | |
| if isinstance(fixed_type, type): | |
| from .hybrid_property import HybridProperty # noqa: PLC0415 – lazy to avoid circular import | |
| # A HybridProperty on the underlying type resolves to a frontend Var with | |
| # this object var substituted as `self`, so e.g. `State.info.a_b` uses the | |
| # same Var-access semantics as accessing the hybrid property directly. | |
| descriptor = inspect.getattr_static(fixed_type, name, None) | |
| if isinstance(descriptor, HybridProperty): | |
| return descriptor._get_var(self) |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| class HybridProperty(property): | ||
| """A hybrid property that can also be used in frontend/as var.""" | ||
|
|
||
| # The optional var function for the property. | ||
| _var: Callable[[Any], Var] | None = None |
There was a problem hiding this comment.
_var class attribute is mutable via instance sharing through inheritance
_var is defined as a class attribute (None) and set as an instance attribute by var(). When a HybridProperty is defined on a mixin and multiple classes inherit it without overriding, all subclasses share the exact same descriptor object. Calling var() on that shared object mutates it for all inheritors simultaneously — a subclass cannot independently override just the .var() function of an inherited HybridProperty without redefining the whole property. A short note in the docstring would help users avoid the pitfall.
feead2e to
977c6c2
Compare
A hybrid property's frontend logic (its getter, or a custom @<name>.var function) runs with the state class as `self` when building the frontend var. Reading a backend (underscore-prefixed) var there previously baked the var's class-level default into the frontend as a frozen literal — a silent leak that never updates and is not reactive. HybridProperty._get_var now wraps a state owner in a guard that raises HybridPropertyError when a backend var is accessed, with a message that points at the misuse in the user's getter/var function. Object-var owners (nested dataclass/model access) have no backend vars and are unchanged. https://claude.ai/code/session_01DKFiYGnWRQG8wMNKFW7obm
977c6c2 to
c44fc0a
Compare
Type of change
Note
Stacked on #6619 — base branch is
claude/relaxed-cerf-Z110q. This PR contains only the backend-var guard; review it against that base.Description
A hybrid property's frontend logic (its getter, or a custom
@<name>.varfunction) runs with the state class asselfwhen building the frontend var. Reading a backend (underscore-prefixed) var there previously baked the var's class-level default into the frontend as a frozen literal — a silent leak that never updates and is not reactive.This PR makes that misuse fail loudly with a clear, actionable error.
Changes:
HybridProperty._get_varguards the state owner (packages/reflex-base/src/reflex_base/vars/hybrid_property.py): when a hybrid property is resolved against aBaseState, the owner is wrapped in a_StateBackendVarGuard. Accessing a backend var while building the frontend var raises, with the traceback pointing at the offending line in the user's getter/.varfunction. Object-var owners (nested dataclass / pydantic / SQLAlchemy access) have no backend vars and are passed through unchanged.New
HybridPropertyError(packages/reflex-base/src/reflex_base/utils/exceptions.py): a dedicatedReflexError— deliberately not anAttributeError, so it can't be silently swallowed bygetattr(..., default)/hasattr— whose message names the property, the state, and the offending backend var, and suggests using a regular var or a separate@<name>.varimplementation.Tests
tests/units/vars/test_hybrid_property.py:HybridPropertyErrorfrom both the getter and a custom.varfunctionhttps://claude.ai/code/session_01DKFiYGnWRQG8wMNKFW7obm