Skip to content

Feature/add table support #63

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

scaryclam
Copy link

This adds some support for creating sqlalchemy Table object inserts.

This is mainly to aid in the support of M2M relations in projects that use Table rather than Model.

The PR should be considered a WIP for now as I'd like some feedback on whether the approach is acceptable.

@heavenshell
Copy link
Owner

@scaryclam Thank you for your PR!
I'll look it into later.

BTW, tests are failed. Could you check?
https://travis-ci.org/heavenshell/py-sqlalchemy_seed/jobs/549389130#L557-L558

@@ -90,6 +92,22 @@ def _create_model_instance(fixture):
return instances


def _create_table_object_data(fixture, session):
Copy link
Owner

@heavenshell heavenshell Jun 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add session to docstring param 😉

@@ -105,13 +127,17 @@ def test_load_fixture(self):
def test_load_fixtures(self):
create_table(Base)
fixtures = load_fixture_files(
self.path, ['accounts.yaml', 'pictures.yaml'],
self.path, ['accounts.yaml', 'pictures.yaml', 'picture_categories.yaml', 'picture_category_picture.yaml'],
Copy link
Owner

@heavenshell heavenshell Jun 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add new test case, such as test_load_table_fixtures.
I want test_load_fixtures assure not BCBreak 😌

@@ -0,0 +1,12 @@
- table: tests.test_sqlalchey_seed.picture_category_picture
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙆‍♂

metadata.reflect(bind=session.get_bind())
table = metadata.tables[class_name]
insert = table.insert()
session.execute(insert.values(**data['fields']))
Copy link
Owner

@heavenshell heavenshell Jun 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familier with Table so, if I say something wrong with this review, please point out 😉
What happen if insert failed. Does DB rollback?

What about do something like followings?
Is this satisfy your requirements? 😄
I want controll session flush, commit, rollback at same place.
IMO it's readable 😉

def _create_table_object_data(fixture, session):
    """Create a Table object entry.

    :param fixture: Fixtures
    :param session: Session
    """
    table_values = []
    for data in fixture:
        if 'table' in data:
            module_name, class_name = data['table'].rsplit('.', 1)
            importlib.import_module(module_name)
            metadata = MetaData()
            metadata.reflect(bind=session.get_bind())
            table = metadata.tables[class_name]
            insert = table.insert()
            insert.values(**data['fields'])
            table_values.append(insert)

    return table_values

def load_fixtures(session, fixtures):
    """Load fixture.

    :param base: `sqlalchemy.ext.declarative`
    :param fixtures: Fixture files
    """
    instances = []
    tables = []
    for fixture in fixtures:
        _instances = _create_model_instance(fixture)
        for instance in _instances:
            instances.append(instance)
        _tables = _create_table_object_data(fixture, session)
        for table in _tables:
            tables.append(table)

    try:
        for instance in instances:
            session.merge(instance)

        for table in tables:
            session.execute(table)
        session.flush()
        session.commit()
    except Exception:
        session.rollback()
        raise

Copy link
Owner

@heavenshell heavenshell Jun 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nn- I think I wrote something strange? 🤔

@heavenshell
Copy link
Owner

heavenshell commented Jun 24, 2019

Thank you for PR.
I added some review comments. Please check it 😄

Yaml defeinition is LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants