sqlalchemy: Inheriting from DeclarativeMeta overriding __getattr__ doesn't work with aliased

Thanks a lot @zzzeek for assistance and quick fix!

once that passes I will merge both

yes, sorry this was not a simple change but you did write the tests which I am grateful for

basically we need to go through every test and document every kind of object that comes through this method and what the usage contract should be. then we can try changing it to call getattr() and unwrap what it gets, rather than trying to bypass the attribute mechanics. the change above is one example of getting the full object and unwrapping it, for a very specific case of “unwrap”.

that is, the code should look like:

    def __getattr__(self, key):
        try:
            _aliased_insp = self.__dict__["_aliased_insp"]
        except KeyError:
            raise AttributeError()
        else:
            # maintain all getattr mechanics, let user do anything
            attr = getattr(target, key)

        # now post-unwrap the thing we need so we can adapt it to our
        # SQL expression
        if attr.is_wrapped_like_this:
            attr = attr.get_this_thing
        elif attr.is_wrapped_like_that:
            attr = attr.get_that_thing

        if isinstance(attr, PropComparator):
            ret = attr.adapt_to_entity(_aliased_insp)
            setattr(self, key, ret)
            return ret
        elif hasattr(attr, "func_code"):
            # why?
            return None
        elif hasattr(attr, "__get__"):
            ret = attr.__get__(None, self)
            if isinstance(ret, PropComparator):
                return ret.adapt_to_entity(_aliased_insp)
            else:
                return ret
        else:
            return attr

I’d really have to fully reopen all the use cases for this and fully re-understand the whole thing to write this such that it can expect sub-callables the way it’s doing. The quick hack to make test two work is:

diff --git a/lib/sqlalchemy/orm/util.py b/lib/sqlalchemy/orm/util.py
index 08ffb784d4..fa9af7a7cb 100644
--- a/lib/sqlalchemy/orm/util.py
+++ b/lib/sqlalchemy/orm/util.py
@@ -504,10 +504,9 @@ class AliasedClass(object):
                 else:
                     break
             else:
-                try:
-                    attr = target.__class__.__getattr__(target, key)
-                except AttributeError:
-                    raise AttributeError(key)
+                attr = getattr(target, key)
+                if attr._is_internal_proxy:
+                    attr = attr.descriptor
 
         if isinstance(attr, PropComparator):
             ret = attr.adapt_to_entity(_aliased_insp)

but this is too hacky, it’s very specific to that one test. There also seems to be some uncovered code in here as well, the “func_code” path, there’s no “util.types.MethodType()” so that code is not even used, coverage at https://jenkins.sqlalchemy.org/job/sqlalchemy_coverage/758/cobertura/lib_sqlalchemy_orm/util_py/ confirms it. needs cleanup

I can get test_meta_getattr_two to work. test_meta_getattr_three looks crazy and that doesn’t work at all. You can’t need it to do that.

(sorry for questions bloat)

1. I found `test/orm/test_utils.py::AliasedClassTest`, so probably will add tests there.

perfect! that’s exactly right

2. Also I wanted to clarify if it's possible to target this fix onto 1.2?

I’m done releasing 1.2 releases we are well into 1.3 at this point, the upgrade path should be mostly effortless. This is already a bit of a destabilizing change depending on how it goes.

   Should I branch from 1.2 and then back merge 1.2 to master?

always work in master, we always backport

3. Should I fork this repo to be able to do PR to it?

yes so you can just work with a normal github PR against master here, and I have an elaborate integration with a code review system called Gerrit that you don’t have to deal with at all, you will see code review comments and whatnot on your pull request.

4. Also I am going to investigate the difference between `object.__getattribute___` and `getattr` here, as it looks like sometimes they produce different results (at least for `hybrid_method`).

That stuff is extremely sensitive because there are tons of use cases packed into the attribute mechanics at this point, all of which should be tested and all of which have to continue working. None of these attribute systems started out this complicated, it’s new behavioral requests like this one over the years that has made everything so packed such that if you change anything, tests break.

5. BTW, I am considering using `_aliased_insp._target.__class__.__getattr__` instead of `getattr` - in this case previous item about difference will gone.

sure I thought the same thing.

an overview of how to run the test suite is at https://github.com/sqlalchemy/sqlalchemy/blob/master/README.unittests.rst

here’s how to run the tests en masse, assuming you have tox:

git clone https://github.com/sqlalchemy/sqlalchemy.git
cd sqlalchemy
tox -e py37 -- test/orm

if you want to run the tests more surgically, here’s more of that, assuming you have nothing else set up but virtualenv:

git clone https://github.com/sqlalchemy/sqlalchemy.git
cd sqlalchemy
virtualenv .venv
.venv/bin/pip install pytest
.venv/bin/pip install -e .
.venv/bin/py.test test/orm/test_query.py

basically for us to make this change means we are officially supporting aliased() of an ORM class that defines its own __getattr__() so there needs to be tests that ensure this, otherwise it will just change back someday