Mapper setup does not run __declare_first__ or __declare_last__ in mixins anymore

[Thorsten Lockert]

Handling of __declare_first__ and __declare_last__ (as used in e.g. the PyCon 2014 ATM sample) got broken with commit 7f82c55.

One potential fix is the fragment below, but I am not convinced this is correct:

--- ext/declarative/base.py   2015-04-23 20:27:08.000000000 -0700
+++ ext/declarative/base.py   2015-04-23 22:27:52.000000000 -0700
@@ -66,11 +66,10 @@

     for base in cls.__mro__:
         _is_declarative_inherits = hasattr(base, '_decl_class_registry')
-        if attrname in base.__dict__:
+        if attrname in base.__dict__ and (
+                base is cls or not _is_declarative_inherits):
             value = getattr(base, attrname)
-            if (base is cls or
-                    (base in cls.__bases__ and not _is_declarative_inherits)):
-                return value
+            return value
     else:
         return None

[Mike Bayer]

test case is my own code. drat!

[Mike Bayer]

• Fixed regression regarding the declarative __declare_first__ and __declare_last__ accessors where these would no longer be called on the superclass of the declarative base. fixes #3383

→ <<cset e73f73538237>>

[Mike Bayer]

thanks for reporting!

[Mike Bayer]

crap! that broke automap

[Mike Bayer]

• add the "strict" version of this lookup for abstract as well, fixes #3383

→ <<cset 01700759346c>>

[Thorsten Lockert]

Works, now. Thank you!

[Thorsten Lockert]

It seems there is something more going on here — __declare_first__ does get called now, but the columns does not get added to the tables created (using Base.metadata.create_all), and then an exception is raised when one tries to actually insert rows (because now SQLAlchemy expects the columns to be there).

It would seem that this, too, extends to the atmcraft sample. And also to auto-generated migrations with alembic...

[Mike Bayer]

all the atmcraft tests pass with 1.0.2. Also, __declare_first__ is ultimately linked into a mapper setup event, and the mechanics of that haven't changed in 1.0; as long as the __declare_first__ is picked up, the code is called in the same way.

I also used atmcraft and deleted the alembic files, and ran a complete autogenerate from zero, this is what it generated: https://gist.github.com/zzzeek/259872e4816e9358a738 it includes the ForeignKeyConstraint objects that are defined within References._reference_table that is ultimately called only from __declare_first__.

So I can't confirm any of the things you're saying here. Can you provide specifics on what's going wrong in the form of a succinct, single file test case?

[Thorsten Lockert]

That is weird — I basically did that in a new Python 2.7 virtualenv here, with atmcraft, and did not see that. Let me do that again to make sure...

[Thorsten Lockert]

Sigh. I am seeing it build the correct migration for atmcraft too — I guess it was when I forced a Base.metadata.create_all() via pshell that it did not build everything.

I am seeing wrong migrations (missing foreign keys etc) with my own project, I will try to track down why, and failing that, make a minimal test-case if at all possible...

[Thorsten Lockert]

Here is a gist which demonstrates clearly what I see. It may be something I am doing wrong, but I cannot for the life of me see what that might be...

[Mike Bayer]

thanks for taking the effort to provide this.

It seems nothing more than the mappers haven't configured when create_all() is called. Adding configure_mappers() before doing the create resolves:

configure_mappers()

engine = create_engine('sqlite://', echo=True)
Base.metadata.create_all(bind=engine)

Now the issue of configure_mappers() sometimes needing to be called is an ongoing issue. However, here is the thing - the test case here behaves the same on 0.9.9; without a configure step (or a call like User() that will configure the mappers), I get the same behavior, e.g.:

CREATE TABLE recipients (
    id INTEGER NOT NULL, 
    PRIMARY KEY (id)
)

and not:

CREATE TABLE recipients (
    id INTEGER NOT NULL, 
    users_id INTEGER, 
    messages_id INTEGER, 
    PRIMARY KEY (id), 
    FOREIGN KEY(users_id) REFERENCES users (id), 
    FOREIGN KEY(messages_id) REFERENCES messages (id)
)

so that would mean this particular gist isn't a 1.0-specific regression. do you get the same result ?

[Thorsten Lockert]

Yes, I get the same behavior with pre-1.0 as well, so this one is apparently not new (it is just the first time I happen to try that particular way of doing things).

It is worth noting that this problem does extend into Alembic, if I hook Alembic up and try to autogenerate without having manually called configure_mappers() it will not build complete tables.

[Thorsten Lockert]

Heh, I just noticed that you do call configure_mappers() manually in atmcraft's migrations/env.py for Alembic... I guess I will just start doing that as required in my own, as well as before any potential calls to Base.metadata.create_all().

[Mike Bayer]

so.... if there's no longer things broken in 1.0 that aren't in 0.9...we close yes?

[Thorsten Lockert]

It is not 1.0 specific, at any rate. Whether it is correct that one has to manually call configure_mappers() is a separate issue, and should probably be a separate ticket, I guess. So, yes, we close.