Issues

Issue #3022 new

propagate=False for relationship properties

Mike Bayer
repo owner created an issue

patch attached

this feature modifies two things:

  1. we no longer raise an error on backrefs where the name conflicts when doing a concrete mapper. This behavior was inconsistent vs. forward same-named refs created on concrete mappers. more tests will be needed in orm/test_concrete.py for this.

  2. the propagate=False flag on backref and relationship allows you to specify different relationships per subclass even in joined and single inh. tests will be needed to check that a subclass that doesn't re-implement a relationship gets this error:

    AttributeError: Concrete/non-propagated Mapper|Manager|manager does not implement attribute 'versions' at the instance level.  Add this property explicitly to Mapper|Manager|manager.
    
  3. some mechanics are broken on ConcreteInheritedProp so try to test more of it.

A test script is as follows:

from sqlalchemy import *
from sqlalchemy.orm import *
from sqlalchemy.ext.declarative import declarative_base

Base = declarative_base()

class Employee(Base):
    __tablename__ = 'employee'
    id = Column(Integer, primary_key=True)
    name = Column(String(50))
    type = Column(String(50))

    __mapper_args__ = {
        'polymorphic_identity': 'employee', 'polymorphic_on': type
    }
    def __repr__(self):
        return "Ordinary person %s" % self.name

class Engineer(Employee):
    __tablename__ = 'engineer'
    id = Column(Integer, ForeignKey('employee.id'), primary_key=True)
    status = Column(String(30))
    engineer_name = Column(String(30))
    primary_language = Column(String(30))

    __mapper_args__ = {
        'polymorphic_identity': 'engineer',
    }
    def __repr__(self):
        return "Engineer %s, status %s, engineer_name %s, "\
                "primary_language %s" % \
                    (self.name, self.status,
                        self.engineer_name, self.primary_language)

class Manager(Employee):
    __tablename__ = 'manager'
    id = Column(Integer, ForeignKey('employee.id'), primary_key=True)
    status = Column(String(30))
    manager_name = Column(String(30))

    __mapper_args__ = {
        'polymorphic_identity': 'manager',
    }
    def __repr__(self):
        return "Manager %s, status %s, manager_name %s" % \
                    (self.name, self.status, self.manager_name)


class Version(Base):
    __abstract__ = True

    version_id = Column(Integer, primary_key=True)

class EmployeeH(Version):
    __tablename__ = 'employee_h'

    id = Column(Integer, ForeignKey('employee.id'), primary_key=True)
    employee = relationship(Employee, backref=backref('versions', propagate=False))

class EngineerH(Version):
    __tablename__ = 'engineer_h'

    id = Column(Integer, ForeignKey('engineer.id'), primary_key=True)
    employee = relationship(Engineer, backref=backref('versions', propagate=False))

#class ManagerH(Version):
#    __tablename__ = 'manager_h'

#    id = Column(Integer, ForeignKey('manager.id'), primary_key=True)
#    employee = relationship(Manager, backref=backref('versions', propagate=False))


e = create_engine("sqlite://", echo=True)
Base.metadata.create_all(e)

s = Session(e)

s.add_all([
    Employee(name='e1', versions=[EmployeeH(version_id=1), EmployeeH(version_id=2)]),
    Engineer(name='e1', versions=[EngineerH(version_id=1), EngineerH(version_id=2)]),
])

s.commit()
s.close()

print s.query(Engineer).join(Engineer.versions)

e1 = s.query(Engineer).first()
print e1.versions

# assert that these all raise:
# AttributeError: Concrete/non-propagated Mapper|Manager|manager does not
# implement attribute 'versions' at the instance or class level.  Add this property
# explicitly to Mapper|Manager|manager.

try:
    print s.query(Manager).join(Manager.versions)
except:
    pass

try:
    # this should raise ideally, seems to just be ignored at the moment
    print s.query(Manager).options(eagerload(Manager.versions))
except:
    raise
    pass

try:
    # assert that this raises:
    m1 = Manager()
    m1.versions
except:
    pass

Comments (14)

  1. Konsta Vesterinen

    Hi,

    I did the following things:

    1. Forked SA, created a new branch and applied your patch to it.
    2. Created a test case with 3 test methods.

    https://bitbucket.org/kvesteri/sqlalchemy/commits/442d62172fcedeb90bd24c39b36e6d480fe56730

    I have some problems and questions:

    1. The tests are not working as expected. For example accessing Manager.versions does not throw AttributeError and I don't know where to start fixing this. Hopefully you could give me some hints.
    2. There are a lot of pep8 violations in all files. How do you feel of adding a pep8 before commit checker? That way all the files could be fixed gradually. Most good editors have great pep8 plugins so fixing the files wouldn't take that much time.
    3. I put the test in a new directory tests/orm/relationships. I personally think its a good idea to have rather small test files (< 500 lines) but I don't know how you feel about this.
    4. I see most of SA code uses indentation like:
    some_very_long_function_name(first_param,
                                                 second_param,
                                                 third_param)
    

    How do you feel about using the following indentation:

    some_very_long_function_name(
        first_param,
        second_param,
        third_param
    )
    
    1. I used declarative models in tests but don't know if that is suitable. Should I use mappers / tables instead? I just feel much more comfortable using declarative classes :)
    2. I added raises contextmanager (similar to what pytest has natively).
    3. I see you've used lots of different fixtures with different scopes in SA tests. Would it make sense at some point to start using the wonderful pytest functional fixtures (https://pytest.org/latest/fixture.html) for SA test cases?
  2. Mike Bayer reporter

    hey konsta -

    re: pep8, yes these tests span many years before I was doing pep8, and in fact that is an ongoing effort. I don't like to have pep8 refactorings bundled into the same commit as a new feature or bugfix, so often when I need to deal with a particular set of tests, I'll pep8 the file, then commit, then start working on the bug for a new commit. The main library at this point is pep8 to the extent that I want it to be (I don't necessarily follow the vertical whitespace recommendations, but everything else I have checked in my editor).

    A lot of the tests have code that was kind of hastily pep8'ed using a reformatting tool, which I believe often made the code look terrible. There really should be a tool that can reformat Python code correctly and its kind of bizarre we still all do it by hand.

    as far as test/orm/relationships, of course if there is a subcategory of tests that is big enough then I think it warrants its own package, though I don't think there should be dozens of single-file packages. In this case if we were to add test/orm/relationships I'd want to make sure everything to do with relationships is moved into there at the same time, but I'm not sure that the tests are organized in quite that way. Right now there is test/orm/ and test/orm/inheritance and there are lots of tests involving relationships in the context of inheritance under test/orm/inheritance.

    If we wanted to make more subdirectories, I'd actually want to approach the whole issue at once rather than piecemeal, proposing test/orm/unitofwork/ test/orm/query/ test/orm/inheritance/ test/orm/mapping/ test/orm/relationships/ and others. Otherwise, adding just a new package with a few tests in it is just another task that's started without a clear direction.

    as far as long param indentation the example you suggested works fine but at the same time I'd need to see specifics on a case-by-case basis, there are some conventions that I've used for many years, such as:

    Table('name', metadata,
        Column(...), 
        # ...
    )
    

    Above, i like that the 'name' of the Table is right there, as it resembles a CREATE TABLE statement:

    CREATE TABLE name (
         x VARCHAR(...)
     )
    

    more coming...

  3. Mike Bayer reporter

    as far as declarative in tests, we do have some tests that use declarative, using the fixtures.DeclarativeMappedTest fixture. I will use this fixture if I'm porting a specific test from a user and often it applies to a test suite where the mappings don't change. For a test suite where we want to use many different kinds of mappings against the same tables, the separate mapper/Table type, using fixtures.MappedTest, is more appropriate. The mapper/Table fixtures are definitely much more tedious to write, though they are preferred for those tests that aren't testing some declarative behavior.

    As far as pytest fixtures, I definitely do not want any required pytest dependencies within the test suite, we can emulate what they're doing in sqlalchemy.testing, but we are too long term of a project to throw our eggs into one basket - it was an enormous effort to get off of plain Python unittest and then onto Nose, and the idea that Nose would years later fall out of maintenance and we'd have to change again was horrifying. So linkages to test packages have to be minimal and decoupled (as you can see in sqlalchemy/testing/plugin/plugin_base.py.

  4. Mike Bayer reporter

    continuing, that's the reason you see so many different fixtures...because, some year, we think, hey lets use this great new fixture system! which we do. Which means now we have two fixture systems. Then a third, and a fourth, and you see how you get to what we have. If I had real volunteers on this thing we could get all the tests into a very narrow range of fixtures and styles. While work has been done on that, there's many hundreds of tests still on various older styles.

    now I'll look at your code to see what's up with that.

  5. Mike Bayer reporter

    so "with raises", two things:

    1. with this kind of thing we not only test that an AttributeError is raised, which could be raised for a lot of reasons, but we also test that the message looks like what we expect.

    2. using the context manager, not a bad idea, but having just a few tests with it like that looks really inconsistent vs. all the other tests and adds yet another way of doing it. Obviously we don't have a lot of context manager in the tests because we started in Python 2.3 and contextmanagers weren't possible until 2.5. I'd prefer to make a task that just sweeps through the entire test suite and changes all the assert_raises and assert_raises_message to the new style.

  6. Konsta Vesterinen

    Hi Mike,

    Thanks for a very thorough response! I pretty much agree with everything you said. I can help with this pep8 stuff too. If you don't like the indentation changed I can at least remove / add empty lines here and there (according to the pep8 standard) and fix lots of too long lines.

    How do you feel about refactoring some very long methods / functions to smaller pieces? I would love to refactor some parts of SQLAlchemy this way (for example the Mapper class). This way I would:

    1. Get deeper understanding of how SQLAlchemy works internally
    2. Hopefully make the code clearer and more modular

    Also thanks for the code snippet, I will work on this and add more tests.

  7. Mike Bayer reporter

    hi Konsta -

    definitely, feel free to get into the Mapper class and make it clearer. I have made some passes through various parts of the ORM in order to make things clearer, in particular if you look at persistence.py, that all used to be two giant methods in Mapper. The only function that resists being broken down is the "instances()" function in loading.py, this is because it's very performance critical and everything I've tried to make it clearer ends up adding huge function call overhead (for which we have tests, under tests/aaa_profiling).

  8. Log in to comment