AssociationProxy needs to act more like a descriptor; "owning_class" is misleading

[sqlalchemy-bot]

Migrated issue, originally created by Brendan Abel (@babel)

This is using sqlalchemy 0.9.8

Assuming a declarative class called "MyClass" with an association proxy field call "my_field"

>>> ap = MyClass.__mapper__.all_orm_descriptors['my_field']
>>> ap.owning_class
None

>>> ap = MyClass.my_field
>>> ap.owning_class
<class 'MyClass'>

>>> ap = MyClass.__mapper__.all_orm_descriptors['my_field']
>>> ap.owning_class
<class 'MyClass'>

Without the owning_class set, most of the other properties will raise exceptions when trying to access them. This can be problematic when using the mapper class to procedurally access fields via the mapper.

---

Attachments: rework_attribute_proxy_owner.patch | aptest.py

[sqlalchemy-bot]

Changes by Brendan Abel (@babel):

• edited description

[sqlalchemy-bot]

Michael Bayer (@zzzeek) wrote:

can you make this a little easier for me and show me a script that raises an exception? thanks.

[sqlalchemy-bot]

Michael Bayer (@zzzeek) wrote:

I say this partially because the assocationproxy is a Python descriptor. It should not be called outside of the context of being bound to a class (e.g. from all_orm_descriptors). the all_orm_descriptors use case is strictly so you can get at the .info dictionary.

[sqlalchemy-bot]

Brendan Abel (@babel) wrote:

Example script reproducing error.

[sqlalchemy-bot]

Changes by Brendan Abel (@babel):

• attached file aptest.py

[sqlalchemy-bot]

Michael Bayer (@zzzeek) wrote:

yeah not sure what can be done here. It's a Python descriptor. It has no idea what the class is until it is called. There is no internal system to support objects that are not mapped attributes such that they get "blessed" as part of a mapping system with a full lifecycle.

[sqlalchemy-bot]

Brendan Abel (@babel) wrote:

Just attached a short script that reproduces the error. It's basically the same example used in the Association Proxy docs.

My current use case is for a REST framework that automatically unserializes JSON data that corresponds to a table field. It's currently doing this by getting the column/relationship/association_proxy from the mapper and using the data type to decide how to process the JSON data. I'm using the all_orm_descriptors because that was the only place in the mapper (AFAIK) that listed the association proxies.

[sqlalchemy-bot]

Michael Bayer (@zzzeek) wrote:

for example, if you initialized it as Python expects, then it's fine:

User.__mapper__.all_orm_descriptors['keywords'].__get__(None, User).target_class

[sqlalchemy-bot]

Michael Bayer (@zzzeek) wrote:

here's one reason it can't work completely - because mapping has no metaclass requirement, and without that we have no way to know when an attribute is added to a class. So even if we did some kind of "sweep" for attributes that are on the class at the time that we map it, that would fail when the attribute were added after the fact and therefore be inconsistent in its behavior.

[sqlalchemy-bot]

Michael Bayer (@zzzeek) wrote:

though that case indicates that all_orm_descriptors is then broken, because it memoizes on first access and wouldn't pick up such attributes anyway...

[sqlalchemy-bot]

Michael Bayer (@zzzeek) wrote:

a related issue also came up recently regarding hybrid properties. The object that we get back from all_orm_descriptors is again the descriptor itself; it doesn't represent any SQL expression by itself, only when its invoked. There is a path for that to work also, if it knew its owning class up front.

[sqlalchemy-bot]

Michael Bayer (@zzzeek) wrote:

the association proxy mis-implements Python descriptors in any case because it attempts to refer to a single class as state, and this is incorrect. this patch begins to split it out into two objects. the per-owner class needs to be put into a weakref dictionary of some kind so that it can be cached.

[sqlalchemy-bot]

Changes by Michael Bayer (@zzzeek):

• attached file rework_attribute_proxy_owner.patch

[sqlalchemy-bot]

Michael Bayer (@zzzeek) wrote:

There still needs to be something done with all_orm_descriptors such that differentiating between a "descriptor" and the "actual thing the descriptor returns" is more clear, for assocaition proxies, hybrids, and other things; the latter always requires a parent object in order to be correct, and it cannot be assumed to be the same class every time. When you call an AP from an AliasedClass like aliased(User) for example, that is different than calling the original. the AP needs to be refactored to work in this way cleanly, more like a hybrid does.

[sqlalchemy-bot]

Changes by Michael Bayer (@zzzeek):

• changed title from "AssociationProxy owning_class not set when accesse" to "AssociationProxy needs to act more like a descript"

[sqlalchemy-bot]

Changes by Michael Bayer (@zzzeek):

• added labels: ext

[sqlalchemy-bot]

Changes by Michael Bayer (@zzzeek):

• set milestone to "1.1"

[sqlalchemy-bot]

Michael Bayer (@zzzeek) wrote:

this should be done but for now trying to trim down 1.1 to essentials

[sqlalchemy-bot]

Changes by Michael Bayer (@zzzeek):

• changed milestone from "1.1" to "1.2"

[sqlalchemy-bot]

Changes by Michael Bayer (@zzzeek):

• changed milestone from "1.2" to "1.3"

[sqlalchemy-bot]

Danilo Tuler (@tuler) wrote:

Why this was pushed to 1.2 and then to 1.3?
Isn't this a major (priority) bug (type)?

I hit a bug that may be related to this, which is a AssociationProxy applied to a AliasedClass generated by with_polymorphic.

It raises the exception at https://bitbucket.org/zzzeek/sqlalchemy/src/a1de76c42f6b64808448aed6e821fbb3b988f99b/lib/sqlalchemy/orm/base.py?at=master&fileviewer=file-view-default#base.py-424

[sqlalchemy-bot]

Michael Bayer (@zzzeek) wrote:

@tuler because it is both low priority and very complicated to do correctly. expect it to be pushed out further unless someone wants to work on it.

the exception you describe sounds like something entirely different so please open a separate issue with a reproduction case.

[sqlalchemy-bot]

Dmytro Starosud (@dima-starosud) wrote:

please open a separate issue

done

[sqlalchemy-bot]

Michael Bayer (@zzzeek) wrote:

with #4116 we've made the concept of "owner" more strictly determined so more folks are hitting this. Here's a test derived from #4202 that shows the issue very simply:

from sqlalchemy import *
from sqlalchemy.orm import *
from sqlalchemy.ext.declarative import declarative_base
from sqlalchemy.ext.associationproxy import association_proxy

Base = declarative_base()


class A(Base):
    __tablename__ = 'a'
    id = Column(Integer, primary_key=True)
    type = Column(String(5), nullable=False)
    d_values = association_proxy("ds", "value")

    __mapper_args__ = {"polymorphic_on": type}


class B(A):
    __tablename__ = 'b'
    id = Column(ForeignKey('a.id'), primary_key=True)

    ds = relationship("D", primaryjoin="D.b_id == B.id")

    __mapper_args__ = {"polymorphic_identity": "b"}


class C(A):
    __tablename__ = 'c'
    id = Column(ForeignKey('a.id'), primary_key=True)

    ds = relationship("D", primaryjoin="D.c_id == C.id")

    __mapper_args__ = {"polymorphic_identity": "c"}


class D(Base):
    __tablename__ = 'd'
    id = Column(Integer, primary_key=True)
    value = Column(String(50))
    b_id = Column(ForeignKey('b.id'))
    c_id = Column(ForeignKey('c.id'))


e = create_engine("sqlite://", echo=True)
Base.metadata.create_all(e)

s = Session(e)

s.add_all([B(ds=[D(value='b1')]), C(ds=[D(value='c1')])])
s.commit()

# works
b1 = s.query(A).filter(B.d_values.contains('b1')).first()

# fails, wrong owner for d_values
c1 = s.query(A).filter(C.d_values.contains('c1')).first()

assert b1
assert c1

note that the "aptest" example here, as given, can never work; the solution likely needs to remove all concepts like "target_class" completely because these make no sense if you aren't giving it the parent class. In 1.2.3, the aptest example now nicely raises this error:

InvalidRequestError: This association proxy has no mapped owning class; can't locate a mapped property

after this change, it will be called more like this:

User.__mapper__.all_orm_descriptors['keywords'].target_class_for(User)

[sqlalchemy-bot]

Changes by Michael Bayer (@zzzeek):

• edited description

[sqlalchemy-bot]

Changes by Michael Bayer (@zzzeek):

• added labels: high priority

[sqlalchemy-bot]

Michael Bayer (@zzzeek) wrote:

Issue #4202 was marked as a duplicate of this issue.

[sqlalchemy-bot]

Michael Bayer (@zzzeek) wrote:

Ahhhh, scratch that, I got confused. the patch here is about splitting the AP into two objects, that will work. big job.

[sqlalchemy-bot]

Michael Bayer (@zzzeek) wrote:

WIP https://gerrit.sqlalchemy.org/#/c/zzzeek/sqlalchemy/+/681

[sqlalchemy-bot]

Michael Bayer (@zzzeek) wrote:

OK so the above gerrit is now working really well. However, this use case still won't work exactly as written:

User.__mapper__.all_orm_descriptors['keywords'].target_class

This is because by accessing the AssociationProxy through the mapper collection, we aren't actually invoking the object as a Python descriptor, which is its normal job. As a Python descriptor, it means we are saying either User.keywords or some_user.keywords. When we call it like that, Python descriptor mechanics provide for us the class that it's being called against, and this is how the AssociationProxy initializes itself and knows what class it refers towards. The change here is that an extra step is added to this resolution where AssociationProxy, given a particular class, returns a sub-object AssociationProxyInstance that is fixed to that parent class, which will then have attributes like owning_class, target_class, and all the others. So you can see that either via:

User.keywords.target_class

Or:

User.__mapper__.all_orm_descriptors['keywords'].__get__(None, User).target_class

The fix includes that there are any number of AssociationProxyInstance objects, so you can have the same AssociationProxy object successfully work against any number of subclasses where they are proxying entirely different attributes and even different collection types, all of which is impossible with the current "single owner" design. Additionally, the AssociationProxyInstance is cached in the class dictionary, so there's no weakref stuff the way I thought there might be above.

Folks who want to use this can use the associationproxy.py code up on the gerrit in their applications right now since this is a standalone extension module.

[sqlalchemy-bot]

Michael Bayer (@zzzeek) wrote:

Break association proxy into a descriptor + per-class accessor

Reworked :class:.AssociationProxy to store state that's specific to a
parent class in a separate object, so that a single
:class:.AssocationProxy can serve for multiple parent classes, as is
intrinsic to inheritance, without any ambiguity in the state returned by it.
A new method :meth:.AssociationProxy.for_class is added to allow
inspection of class-specific state.

Change-Id: I634f88aae6306ac5c5237a0e1acbe07d0481d6b6
Fixes: #3423

→ 6446e0d

[sqlalchemy-bot]

Changes by Michael Bayer (@zzzeek):

• changed status to closed