Errors when creating models are hidden

[Matthias Urlichs]

The problem I saw was that the first access to MyDeclarativeTable.query returned None, but after that everything worked.

I would like to actually see the error. I cannot understand why you're hiding it, in multiple places.

The first is in lib/sqlalchemy/orm/scoping.py:query_property() where inside the query class you return None if there was an UnmappedClassError.

The second is in sqlalchemy/orm/base.py:_inspect_mapped_class() where you return None upon catching exc.NO_STATE, which is an AttributeError, which neatly masks off the AttributeError that my model caused because of a typo.

I submit that this makes no sense whatsoever, and I'd like to ask you to do something about these nonproductive try/except statements. They mask real problems. I can't see any practical use for them.

This is what I did:

Traceback (most recent call last):
  File "./manage.py", line 42, in <module>
    manager.run()
  File "/usr/lib/python2.7/dist-packages/flask_script/__init__.py", line 412, in run
    result = self.handle(sys.argv[0], sys.argv[1:])
  File "/usr/lib/python2.7/dist-packages/flask_script/__init__.py", line 383, in handle
    res = handle(*args, **config)
  File "/daten/src/git/pip/pybble/pybble/manager/main.py", line 119, in __call__
    return create_app(**kw)
  File "/daten/src/git/pip/pybble/pybble/app/__init__.py", line 342, in create_app
    site = Site.q.get_by(domain=text_type(site))
  File "/usr/lib/python2.7/dist-packages/sqlalchemy/orm/scoping.py", line 130, in __get__
    mapper = class_mapper(owner)
  File "/usr/lib/python2.7/dist-packages/sqlalchemy/orm/base.py", line 383, in class_mapper
    raise exc.UnmappedClassError(class_)
UnmappedClassError: Class 'pybble.core.models.site.Site' is not mapped
> /usr/lib/python2.7/dist-packages/sqlalchemy/orm/base.py(383)class_mapper()
-> raise exc.UnmappedClassError(class_)
(Pdb) q
smurf@data:/daten/src/git/pip/pybble$ test/run.sh populate
> /daten/src/git/pip/pybble/pybble/app/__init__.py(342)create_app()
-> site = Site.q.get_by(domain=text_type(site))
(Pdb) b /usr/lib/python2.7/dist-packages/sqlalchemy/orm/base.py:378
Breakpoint 1 at /usr/lib/python2.7/dist-packages/sqlalchemy/orm/base.py:378
(Pdb) c
> /usr/lib/python2.7/dist-packages/sqlalchemy/orm/base.py(378)class_mapper()
-> mapper = _inspect_mapped_class(class_, configure=configure)
(Pdb) cl
Clear all breaks? y
(Pdb) s
--Call--
> /usr/lib/python2.7/dist-packages/sqlalchemy/orm/base.py(347)_inspect_mapped_class()
-> @inspection._inspects(type)
(Pdb) n
> /usr/lib/python2.7/dist-packages/sqlalchemy/orm/base.py(349)_inspect_mapped_class()
-> try:
(Pdb) 
> /usr/lib/python2.7/dist-packages/sqlalchemy/orm/base.py(350)_inspect_mapped_class()
-> class_manager = manager_of_class(class_)
(Pdb) 
> /usr/lib/python2.7/dist-packages/sqlalchemy/orm/base.py(351)_inspect_mapped_class()
-> if not class_manager.is_mapped:
(Pdb) 
> /usr/lib/python2.7/dist-packages/sqlalchemy/orm/base.py(353)_inspect_mapped_class()
-> mapper = class_manager.mapper
(Pdb) 
> /usr/lib/python2.7/dist-packages/sqlalchemy/orm/base.py(354)_inspect_mapped_class()
-> if configure and mapper._new_mappers:
(Pdb) 
> /usr/lib/python2.7/dist-packages/sqlalchemy/orm/base.py(355)_inspect_mapped_class()
-> mapper._configure_all()
(Pdb) 
AttributeError: "type object 'SiteConfigVar' has no attribute 'name'"
> /usr/lib/python2.7/dist-packages/sqlalchemy/orm/base.py(355)_inspect_mapped_class()
-> mapper._configure_all()
(Pdb) 

It's not realistic to expect users to find their bugs by trial and error when Python has error handling which is perfectly capable of reporting exactly what went wrong.

Removing these exception handlers is (mostly) straightforward. See the attached patch, which does not break any of the basic (sqlite) test cases.

[Matthias Urlichs]

Missing paragraph after the first one: This was ultimately caused by an error in one of my models, but that error was hidden by sqlalchemy.

[Mike Bayer]

I'd appreciate if you could tone down the suggestions that I have no idea what I'm doing. There are reasons the code is like this, and certainly if it is masking other attribute errors that aren't the ones it seeks to catch, we can work on improving that.

[Mike Bayer]

the contract of the functions where we are catching NO_STATE is explicitly, "is this class mapped? else return None". if you just raise the exception, there are definitely a ton of failures, as this breaks the contract of those functions:

--- a/lib/sqlalchemy/orm/base.py
+++ b/lib/sqlalchemy/orm/base.py
@@ -356,7 +356,7 @@ def _inspect_mapped_class(class_, configure=False):
         return mapper

     except exc.NO_STATE:
-        return None
+        raise

 def class_mapper(class_, configure=True):
     """Given a class, return the primary :class:`.Mapper` associated

#!

classic$ ./sqla_nose.py -w orm   
.....................E................................E...................................................................E....EEEE...............................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................E.....................................................................................................................................................................................................................................................................................................................................................................................................................E............................................................................................................................................EEEEE...............................................................EE...................................................S.....................E........EEEEE................................E.........................................................................................................................................................................................................E......................................S.................................................................................................................................EEEEEEEEEEEEEEEE...................................................................................F.........................................EEE.................................................................S.......S..S.SSS.S...........................................................................E..................................................................E.........................................................................................................................................................................................................................................................................................................EE.E.......E..............................................S........................E.EEE...S..S..................E.....S........E.........................E.............E...................................SSS.S.........SSSSSSSS......SSSSSSS........SS...SS...............S.............................S........................................................ESS..............................................................................................................S
======================================================================

# many many stack traces

FAILED (SKIP=41, errors=58, failures=1)

[Mike Bayer]

• Fixed ORM bug where the :func:.class_mapper function would mask AttributeErrors or KeyErrors that should raise during mapper configuration due to user errors. The catch for attribute/keyerror has been made more specific to not include the configuration step. fixes #3047

→ <<cset 230c0d5a1997>>

[Mike Bayer]

• Fixed ORM bug where the :func:.class_mapper function would mask AttributeErrors or KeyErrors that should raise during mapper configuration due to user errors. The catch for attribute/keyerror has been made more specific to not include the configuration step. fixes #3047

→ <<cset ac68e85e544d>>

[Mike Bayer]

committed is a one line move of the catch to omit where we are calling configure mappers. This satisfies two tests cases I could come up with where an unexecuted mapper configuration contains Attribute or Key errors that were squashed by the _inspect_mapped_class function. I could see no other places (e.g. the "multiple places" you refer to) where this happens, so without an actual test case that's as far as I can go. Reopen with a real test case if further issues remain.

[Matthias Urlichs]

Thanks. Here's a real test case.

And sorry about the annoyed tone. After a couple hours of trying to track down this problem, the frustration was kindof mounting. :-(

from sqlalchemy.ext.declarative import declarative_base, declared_attr
from sqlalchemy import create_engine, Column, Integer
from sqlalchemy.orm import sessionmaker, scoped_session, query

engine = create_engine('sqlite://', echo=True)

db = scoped_session(sessionmaker(autocommit=False, bind=engine))

class MyBase(object):
    q = db.query_property()

Base = declarative_base(cls=MyBase)

class HasAttrError(Base):
    __tablename__ = 'attr_error'
    id = Column(Integer, primary_key=True)
    @classmethod
    def __declare_last__(cls):
        cls.unknown ## AttrError

try:
    if HasAttrError.q is None:
        print "FAIL"
except AttributeError:
    print "The test worked."

[Matthias Urlichs]

testcase attached

[Matthias Urlichs]

One thing I'm also interested in here, besides getting an AttributeError, is that this is difficult to debug because after this first strange None, everything seems to be working otherwise, except that whatever declare_last was supposed to do non-obviously did not happen.

[Mike Bayer]

try out master. Here's what I get:

#!

$ python test.py
The test worked.

so.... we're done. please confirm.

[Matthias Urlichs]

Confirmed.

[Mike Bayer]

confirmed