Skip to content

Skip MutableProxy rebind self for _sa_instrumented functions#6168

Open
masenf wants to merge 2 commits intomainfrom
masenf/mutable-proxy-skip-sa_instrumented
Open

Skip MutableProxy rebind self for _sa_instrumented functions#6168
masenf wants to merge 2 commits intomainfrom
masenf/mutable-proxy-skip-sa_instrumented

Conversation

@masenf
Copy link
Collaborator

@masenf masenf commented Mar 10, 2026

Not a general solution to the issue of cases where rebinding self might break descriptors, but at least fix the case for sqlalchemy models.

Fix #6167

Not a general solution to the issue of cases where rebinding `self` might break
descriptors, but at least fix the case for sqlalchemy models.

Fix #6167
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 10, 2026

Greptile Summary

This PR adds a targeted one-line guard in MutableProxy.__getattr__ to prevent self-rebinding for SQLAlchemy instrumented methods (those flagged with _sa_instrumented = True). When MutableProxy encounters a bound method on a wrapped object, it normally rebinds __self__ to the proxy itself so that nested mutations can be tracked. For SQLAlchemy model instances this behaviour breaks the ORM's descriptor machinery, causing runtime errors (issue #6167). The fix short-circuits the rebinding logic for these specific methods, so SQLAlchemy's instrumentation layer continues to receive the real model instance as self.

  • Change scope: single condition added to the rebinding guard in MutableProxy.__getattr__ (proxy.py:576).
  • Trade-off acknowledged: The PR author explicitly notes this is not a general solution — other descriptor-based frameworks with similar patterns could still be affected.
  • No regression test added: the fix is not covered by a new unit test, which is a minor gap given the targeted nature of the change.

Confidence Score: 4/5

  • This PR is safe to merge; the change is minimal, well-scoped, and correctly addresses the SQLAlchemy breakage.
  • The diff is a single targeted condition that relies on the stable _sa_instrumented marker attribute from SQLAlchemy. The logic is sound and the guard is applied in the right place. The only concern is the absence of a regression test to lock in the fix.
  • No files require special attention beyond the noted lack of a regression test in tests/units/istate/test_proxy.py.

Important Files Changed

Filename Overview
reflex/istate/proxy.py Adds a targeted guard in MutableProxy.__getattr__ to skip self-rebinding for SQLAlchemy instrumented methods by checking _sa_instrumented; no regression test was added.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[MutableProxy.__getattr__ name] --> B[value = super.__getattr__ name]
    B --> C{callable value}
    C -- No --> G[wrap in MutableProxy if mutable]
    C -- Yes --> D{has __func__ AND self is not a class}
    D -- No --> G
    D -- Yes --> E{value._sa_instrumented is True - NEW CHECK}
    E -- Yes --> F[Return value as-is - SQLAlchemy descriptor preserved]
    E -- No --> H[Return functools.partial func self - self rebound to proxy]
    G --> I[Return value]
    F --> I
    H --> I
Loading

Last reviewed commit: faba1e4

and (func := getattr(value, "__func__", None)) is not None
and not inspect.isclass(getattr(value, "__self__", None))
# skip SQLAlchemy instrumented methods
and not getattr(value, "_sa_instrumented", False)
Copy link
Contributor

Choose a reason for hiding this comment

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

No regression test for the SQLAlchemy fix

The fix addresses a real bug (issue #6167) but no test was added to tests/units/istate/test_proxy.py (or elsewhere) to assert the behavior. Without a test, a future refactor of the rebinding logic could silently reintroduce the breakage.

Consider adding a minimal test that:

  1. Creates a mock object with a method that has _sa_instrumented = True set on it.
  2. Wraps it in MutableProxy.
  3. Asserts that accessing the method via the proxy does not rebind self (i.e. calling the method works as expected on the underlying object, not the proxy).
def test_mutable_proxy_skips_sa_instrumented():
    class FakeModel:
        def instrumented_method(self):
            return type(self).__name__

    instrumented_method = FakeModel.instrumented_method
    instrumented_method._sa_instrumented = True
    FakeModel.instrumented_method = instrumented_method

    proxy = MutableProxy(FakeModel(), state=None, field_name="")
    # Should return "FakeModel", not "MutableProxy"
    assert proxy.instrumented_method() == "FakeModel"

@codspeed-hq
Copy link

codspeed-hq bot commented Mar 10, 2026

Merging this PR will not alter performance

✅ 8 untouched benchmarks


Comparing masenf/mutable-proxy-skip-sa_instrumented (abd9047) with main (3c11451)

Open in CodSpeed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TypeError: descriptor 'append' for 'list' objects doesn't apply to a 'MutableProxy' object

2 participants