Polymorphic relations half-propagated on concrete inheritance

[Former user]

(original reporter: ged) I'd think the scoped session wouldn't affect such features, but it seems like it does.

The attached test case fails (on 0.4.8 and on trunk as of now: r5410) with the following traceback:

Traceback (most recent call last):
  File "test_concrete_scoped_sa.py", line 103, in <module>
    c1 = C(data1='c1', data2='c1', data3='c1', some_e=e1)
  File "<string>", line 4, in __init__
  File "/home/ged/devel/sqlalchemy/trunk/lib/sqlalchemy/orm/attributes.py", line 860, in initialize_instance
    return manager.events.original_init(*mixed[1:](1:), **kwargs)
  File "<string>", line 6, in __init__
  File "<string>", line 6, in __init__
  File "test_concrete_scoped_sa.py", line 48, in __init__
    setattr(self, k, v)
  File "/home/ged/devel/sqlalchemy/trunk/lib/sqlalchemy/orm/attributes.py", line 142, in __set__
    self.impl.set(instance_state(instance), value, None)
  File "/home/ged/devel/sqlalchemy/trunk/lib/sqlalchemy/orm/attributes.py", line 531, in set
    value = self.fire_replace_event(state, value, old, initiator)
  File "/home/ged/devel/sqlalchemy/trunk/lib/sqlalchemy/orm/attributes.py", line 553, in fire_replace_event
    value = ext.set(state, value, previous, initiator or self)
  File "/home/ged/devel/sqlalchemy/trunk/lib/sqlalchemy/orm/unitofwork.py", line 65, in set
    prop = _state_mapper(state).get_property(self.key)
  File "/home/ged/devel/sqlalchemy/trunk/lib/sqlalchemy/orm/mapper.py", line 777, in get_property
    return self._get_property(key, resolve_synonyms=resolve_synonyms, raiseerr=raiseerr)
  File "/home/ged/devel/sqlalchemy/trunk/lib/sqlalchemy/orm/mapper.py", line 785, in _get_property
    raise sa_exc.InvalidRequestError("Mapper '%s' has no property '%s'" % (str(self), key))
sqlalchemy.exc.InvalidRequestError: Mapper 'Mapper|C|c' has no property 'some_e'

[Mike Bayer]

if a ticket has "concrete inheritance" and "relation" in it, its almost certainly a bug. this is an undermaintained feature of SQLA since its really not a common use case to use polymorphic concrete + relations - from a relational standpoint it requires the production of large and inefficient queries. That said ill try to take a look at this.

[Former user]

(original author: ged) In fact, the test case is bugged, and it is unrelated to scoped_session. There is something fishy going on though: it seems like the relationships are only partially propagated. I'd expect them to be either propagated like in other kinds of inheritance, or not propagated at all. See updated/corrected/simplified test case.

[Mike Bayer]

partially propagated is what you'll get. we don't move the "relation()" along to subclasses and instead suggest they are explicitly configured on each inheriting class, since the SQL representation is entirely different. But since classes inherit, subclasses do get the instrumented attribute of the superclass in any case. Which means if you don't explicitly lay out all relations, it'll be broken.

[Former user]

(original author: ged) Ok. Well... This is a very weird behavior IMO. An explicit exception might be a good idea, and an even more explicit message in the documentation than there is currently at: http://www.sqlalchemy.org/docs/05/mappers.html#advdatamapping_mapper_inheritance_relations

Something like:

"If you don't, the relations on the child mappers won't work, but the attributes won't be available for other uses either."

after the existing paragraph:

"The big limitation with concrete table inheritance is that relation()s placed on each concrete mapper do not propagate to child mappers. If you want to have the same relation()s set up on all concrete mappers, they must be configured manually on each."

[Mike Bayer]

I think we'd need to go with a warning for 0.5 since its been this way for years and I'm sure some people are relying on it silently. I also need to look at your test case since I'm not entirely sure about the subtleties of this use case right now (im assuming you don't need this for a production project right now).

[Former user]

(original author: ged) I fail to imagine how could one rely on something being broken in this particular way. And as you assumed, I don't need any of this right now.

[Mike Bayer]

they have a subclass and they aren't touching the relation() on instances of that subclass. I think a warning is best in this case. people definitely take warnings seriously.

[Mike Bayer]

a new branch and its initial revision in c56c1b61ccdddb09217c3d7354a3649b240c0cac allows the warnings to take place and also allows forward references from concrete classes. The "explicit backwards relations" feature from #781 is needed as well for it to work completely:

a_m = mapper(A, a_table,
             with_polymorphic=('*', pjoin),
             polymorphic_on=pjoin.c.type,
             polymorphic_identity='a',
             properties={
                'some_c':relation(C, back_populates='many_a')
             }
)

b_m = mapper(B, b_table,
             inherits=a_m, concrete=True,
             polymorphic_identity='b', properties={
                'some_c':relation(C, back_populates='many_a')
             })

c_m = mapper(C, c_table, properties={
    'many_a':relation(A, back_populates='some_c')
})

The branch should reveal just how much is involved to make this work. Abstract base classes that are mapped directly to the polymorphic_union need to be declared as "abstract=True", else its impossible for the subclass to not partially instrument the base's full set of mapped columns. The "polymorphic_on" discriminator too needs to be instrumented differently to work with this system as well.

For mapping the relation, explicit multiple versions of the relation are required for now (the warning will guide you there) and bidirectional relationships now need to represent themselves between sets of relations on either side. The explicit relation() multiple times is so that different join conditions, many-to-many tables etc. can be configured.

So you can see that the changes to the internals are pretty major too and I'd probably want to get 0.5 final out before these are merged. I may have to put some version of the abstract=True + warning in however since this is an API change, which is better added in before the release candidates are up.

[Mike Bayer]

I'd like to get this on deck after 0.5.0 is released.

[Mike Bayer]

I have behavior which I'm satisfied with in 209e888e1bda69924b364ae3394016acef0e9d41. Your test case will now raise: AttributeError: Concrete Mapper|B|b does not implement attribute 'some_c' at the instance level. Add this property explicitly to Mapper|B|b.. A working version of your mapping is then:

a_m = mapper(A, a_table,
             with_polymorphic=('*', pjoin),
             polymorphic_on=pjoin.c.type,
             polymorphic_identity='a')
a_m.add_property('some_c', relation(C, back_populates='many_a'))
b_m = mapper(B, b_table,
             inherits=a_m, concrete=True,
             polymorphic_identity='b')

b_m.add_property('some_c', relation(C, back_populates='many_a'))
c_m = mapper(C, c_table)
c_m.add_property('many_a', relation(A, back_populates='some_c'))

An example has been added to the documentation section you mentioned.

[Mike Bayer]

Removing milestone: 0.5.1 (automated comment)