Mapper setup does not run __declare_first__ or __declare_last__ in mixins anymore
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
Comments (16)
-
repo owner -
repo owner - changed status to resolved
- 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>>
-
repo owner thanks for reporting!
-
repo owner - changed status to open
crap! that broke automap
-
repo owner - changed status to resolved
- add the "strict" version of this lookup for abstract as well,
fixes
#3383
→ <<cset 01700759346c>>
-
reporter - changed status to closed
Works, now. Thank you!
-
reporter - changed status to open
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 (usingBase.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...
-
repo owner 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?
-
reporter 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...
-
reporter 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...
-
reporter 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...
-
repo owner 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 ?
-
reporter 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. -
reporter Heh, I just noticed that you do call
configure_mappers()
manually in atmcraft'smigrations/env.py
for Alembic... I guess I will just start doing that as required in my own, as well as before any potential calls toBase.metadata.create_all()
. -
repo owner so.... if there's no longer things broken in 1.0 that aren't in 0.9...we close yes?
-
reporter - changed status to closed
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. - Log in to comment
test case is my own code. drat!