Add ability to handle explicit schema name. #70

psipika · 2019-12-31T19:42:13Z

When invoking render_er with a schema name, like:

    render_er(f"postgresql+psycopg2://DB_USERNAME@localhost:5001/DB_NAME",
              "DB_NAME.png",
              mode='graph',
              schema='DB_NAME')

The resulting image is missing the proper relationships. As mentioned in issue #67, the problem is that the left_col argument to Relation is missing the name of the schema. This PR aims to address the problem using the [subjectively] least intrusive means.

Closes #67

See https://docs.travis-ci.com/user/database-setup/#postgresql for details.

psipika · 2019-12-31T20:17:18Z

This PR also takes a crack at fixing Travis-CI configuration to start postgresql.

Alexis-benoist

Thanks for the PR !

eralchemy/sqla.py

Alexis-benoist · 2020-01-03T10:06:00Z

eralchemy/sqla.py

+    # as another parameter into this function from metadata_to_intermediary()
+    left_column = format_name(fk._column_tokens[1])
+    if fk._column_tokens[0]:
+        left_column = "{}.{}".format(format_name(fk._column_tokens[0]),


I require a test of this fix preferably in in integration with a real postgresql

Amended existing schema test to assert that column names match full table names.
Hope that is enough.

I need automated testing for this feature possibly in integration with a real Postgres.

The current test uses the local instance of a postgres database named test and created inside .travis.yml. Is that not enough?

Alexis-benoist · 2020-01-09T18:24:44Z

Didn't saw it. Where is the assertion showing the issue #67 is fixed? Le jeu. 9 janv. 2020 à 18:20, psipika <notifications@github.com> a écrit :

…

***@***.**** commented on this pull request. ------------------------------ In eralchemy/sqla.py <#70 (comment)> : > @@ -13,9 +13,20 @@ def relation_to_intermediary(fk): """Transform an SQLAlchemy ForeignKey object to it's intermediary representation. """ + + # In the cases where schema name is provided, _column_tokens[1] is + # insufficient to correctly identify the relationship. Therefore, assuming + # that the schema name is _column_tokens[0], when set seems the least + # intrusive approach. The other one would be sending in the schema name + # as another parameter into this function from metadata_to_intermediary() + left_column = format_name(fk._column_tokens[1]) + if fk._column_tokens[0]: + left_column = "{}.{}".format(format_name(fk._column_tokens[0]), The current test uses the local instance of a postgres database (named test and created inside .travis.yml <https://github.com/Alexis-benoist/eralchemy/blob/master/.travis.yml#L14-L15>. Is that not enough? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#70?email_source=notifications&email_token=ABB3JJYB6TZOQIOCBEG7XGLQ45MELA5CNFSM4KBXTM62YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCRHHK2Y#discussion_r364864338>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABB3JJ5RFNT2AZQRD3ZH74TQ45MELANCNFSM4KBXTM6Q> .

psipika · 2020-01-09T18:42:10Z

Didn't saw it. Where is the assertion showing the issue #67 is fixed?

Here:

    tables, relationships = database_to_intermediary(db_uri, schema='test')
    table_names = [t.name for t in tables]
....
    # Assert column names match table names, including schema
    assert all(r.right_col in table_names for r in relationships)
    assert all(r.left_col in table_names for r in relationships)

Basically, the table names returned from the function call are in the form <schema name>.<table name>. The names of Relation columns should use the same format. Issue #67 pertains to the fact that the original implementation returned names of the form <table name>, completely omitting the <schema name> portion. The additional assert() calls make sure that the column names returned inside relationships match those in the actual tables.

Alexis-benoist · 2020-01-12T21:39:51Z

tests/test_sqla_to_intermediary.py

@@ -74,6 +75,9 @@ def test_database_to_intermediary_with_schema():
    # Not in because different schema.
    assert relation not in relationships
    assert exclude_relation not in relationships
+    # Assert column names match table names, including schema
+    assert all(r.right_col in table_names for r in relationships)
+    assert all(r.left_col in table_names for r in relationships)


Can you extract this part to another test and make the assertion show the "{}.{}" formatting?

Done, separated out and checking explicitly for presence/lack of schema name in the relationship column names. Thanks for taking the time to review my updates.

Why don't we do a set expected_names like and assert it's equal to the result?

psipika · 2020-01-22T20:52:09Z

Ping @Alexis-benoist -- is there anything else I can address to move this forward?

DanielWeitzenfeld · 2020-02-11T17:39:01Z

@psipika Thank you for this, I hope it gets merged. Just a heads up that for my case, I needed to change eralchemy/sqla.py:74 to

relation_to_intermediary(fk, metadata.schema or table.schema)

My base is a DeclarativeMeta, and I defined schemas as the table level, e.g.:

class Foo(Base):
    __tablename__ = 'foo'
    __table_args__ = {"schema": "bar"}

Alexis-benoist

Sorry for the long delay.

Alexis-benoist · 2020-02-11T21:26:39Z

tests/test_sqla_to_intermediary.py

@@ -74,6 +75,9 @@ def test_database_to_intermediary_with_schema():
    # Not in because different schema.
    assert relation not in relationships
    assert exclude_relation not in relationships
+    # Assert column names match table names, including schema
+    assert all(r.right_col in table_names for r in relationships)
+    assert all(r.left_col in table_names for r in relationships)


Why don't we do a set expected_names like and assert it's equal to the result?

Alexis-benoist · 2020-02-11T21:27:05Z

tests/test_sqla_to_intermediary.py

+    assert all(r.right_col in table_names for r in relationships)
+    assert all(r.left_col in table_names for r in relationships)
+
+    # Assert column names match table names


Same remark as below + code is duplicated.

yabb85 · 2020-05-14T08:23:05Z

The same problem exist also when the schema is not defined.
I create two schemas in postgresql but when i try to generate a diagram the relations are not generated because the left_col is never in include_tables (included_tables contains the fullname).

Piotr Sipika added 3 commits December 31, 2019 14:39

Add ability to handle explicit schema name.

2cf0c06

Make sure to start postgresql service.

b16feb4

See https://docs.travis-ci.com/user/database-setup/#postgresql for details.

Remove explicit module reference.

6e04e42

Fix flake8 errors.

8a9dc09

Alexis-benoist requested changes Jan 3, 2020

View reviewed changes

Added assertion to existing schema test along with helper function.

a29bf15

Alexis-benoist self-requested a review January 9, 2020 17:15

Alexis-benoist reviewed Jan 12, 2020

View reviewed changes

Piotr Sipika added 2 commits January 13, 2020 08:39

Separate out tests for schema name checks.

7435be4

Fix flake8 errors.

0a59ab5

Alexis-benoist requested changes Feb 11, 2020

View reviewed changes

JosePVB mentioned this pull request May 23, 2020

[tests] - Use testing.postgresql to streamline database tests. #71

Open

This was referenced Sep 21, 2020

UML auto. generate with eralachemy OpenEnergyPlatform/academy#117

Merged

Missing relationships when using Base #67

Open

benjamin-kirkbride added a commit to benjamin-kirkbride/eralchemy that referenced this pull request Jan 8, 2022

fix: Alexis-benoist#70

f5649c6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to handle explicit schema name. #70

Add ability to handle explicit schema name. #70

psipika commented Dec 31, 2019

psipika commented Dec 31, 2019

Alexis-benoist left a comment

Alexis-benoist Jan 3, 2020

psipika Jan 8, 2020

Alexis-benoist Jan 9, 2020

psipika Jan 9, 2020 •

edited

Alexis-benoist commented Jan 9, 2020 via email

psipika commented Jan 9, 2020

Alexis-benoist Jan 12, 2020

psipika Jan 13, 2020

Alexis-benoist Feb 11, 2020 •

edited

psipika commented Jan 22, 2020

DanielWeitzenfeld commented Feb 11, 2020

Alexis-benoist left a comment

Alexis-benoist Feb 11, 2020 •

edited

Alexis-benoist Feb 11, 2020

yabb85 commented May 14, 2020

Add ability to handle explicit schema name. #70

Are you sure you want to change the base?

Add ability to handle explicit schema name. #70

Conversation

psipika commented Dec 31, 2019

psipika commented Dec 31, 2019

Alexis-benoist left a comment

Choose a reason for hiding this comment

Alexis-benoist Jan 3, 2020

Choose a reason for hiding this comment

psipika Jan 8, 2020

Choose a reason for hiding this comment

Alexis-benoist Jan 9, 2020

Choose a reason for hiding this comment

psipika Jan 9, 2020 • edited

Choose a reason for hiding this comment

Alexis-benoist commented Jan 9, 2020 via email

psipika commented Jan 9, 2020

Alexis-benoist Jan 12, 2020

Choose a reason for hiding this comment

psipika Jan 13, 2020

Choose a reason for hiding this comment

Alexis-benoist Feb 11, 2020 • edited

Choose a reason for hiding this comment

psipika commented Jan 22, 2020

DanielWeitzenfeld commented Feb 11, 2020

Alexis-benoist left a comment

Choose a reason for hiding this comment

Alexis-benoist Feb 11, 2020 • edited

Choose a reason for hiding this comment

Alexis-benoist Feb 11, 2020

Choose a reason for hiding this comment

yabb85 commented May 14, 2020

psipika Jan 9, 2020 •

edited

Alexis-benoist Feb 11, 2020 •

edited

Alexis-benoist Feb 11, 2020 •

edited