Preserve comment and info in Drop/CreateTableOp #881

cansjt · 2021-07-31T13:24:57Z

This is a fix for issue #879

Description

The DropTableOp.from_table() method loses the table's SchemaItem.info and comment attributes while the CreateTableOp.from_table() only loses the info attribute.

This means the following assertion does not hold, when a table goes through a from_table() to_table() round trip:

assert table == Op.from_table(table).to_table()  # Symbolic (see note below)

This commit fixes both issues.

Notes:

While fixing this I found the implementations of the Create/DropTableOp to be strangely assymetrical, in particular wrt. the way each deals with extra keywords one can pass to a table. Happy to refactor this a little if you'd like me to.
There is table equality notion implemented. But it seems reliant on the identity of the tables MetaData though, which kind of makes sense if you want to be 100% it is the same object, in the exact same database (given the DB URI+schema name+table name, etc.).

>>> from sqlalchemy import Table, MetaData
>>> t1 = Table('name', MetaData(schema='plop'))
>>> t2 = Table('name', MetaData(schema='plop'))
>>> t1 == t2
False
>>> t3 = Table('name', t1.metadata)
>>> t1 == t3
True

Given that the MetaData object is going "bind-free" in SQLAlchemy 2.0, I wonder if this definition of table equality is still relevant, though.

Checklist

This pull request is:

A documentation / typographical error fix
- Good to go, no issue or tests are needed
A short code fix
- please include the issue number, and create an issue if none exists, which
  must include a complete example of the issue. one line code fixes without an
  issue and demonstration will not be accepted.
- Please include: Fixes: #<issue number> in the commit message
- please include tests. one line code fixes without tests will not be accepted.
A new feature implementation
- please include the issue number, and create an issue if none exists, which must
  include a complete example of how the feature would look.
- Please include: Fixes: #<issue number> in the commit message
- please include tests.

Have a nice day!

Thanks, you too !

zzzeek · 2021-07-31T22:34:55Z

hello there -

thanks for this! has a test and everything. I am on vacation this week so have limited time to check into things. will run this through CI.

sqla-tester

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 6ed4ed0 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2021-07-31T22:35:13Z

New Gerrit review created for change 6ed4ed0: https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/2989

sqla-tester

mike bayer (zzzeek) wrote:

hi and thanks for doing this!

changes we need include:

a changelog file docs/build/unreleased/879.rst needs to be added
we do need some factoring as you mention to make DropTableOp look like CreateTableOp but I think it should be for now that CreateTableOp->**kw and DropTableOp->table_kw are treated equivalently for now, no new arguments added
for pep8/flake8, black formatting needs to be run on the files. Best way to do this is to use pre-commit:

$ cd /path/to/alembic
$ pre-commit install

the correct formatting steps will take place when you next commit. do a "git commit --amend" and force-push the PR to make sure it gets all the files that were changed.

im on vacation this week so i may not respond quickly.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/2989

/COMMIT_MSG (line 24): that's public API and while I can see DropTable is different, that's because the arguments sent to Table are currently only used during CREATE TABLE; I believe table_kw was added to better support "reversal" of the DropTableOp back into CreateTableOp. would not change any of that within this change.
/COMMIT_MSG (line 36): this definition of "==" is still relevant; if you want a comparison of two Table objects based on their contents, use the .compare() method, 1.4 and above:

from sqlalchemy import Table, MetaData
t1 = Table('name', MetaData(schema='plop'))
t2 = Table('name', MetaData(schema='plop'))
t1.compare(t2)
True
t3 = Table('notname', MetaData(schema='plop'))
t1.compare(t3)
False
/COMMIT_MSG (line 62): please add:

Fixes: Drop/CreateTableOp.from_table() no longer copy the table's SchemaItem.info #879

sqla-tester · 2021-08-04T18:00:12Z

alembic/operations/ops.py

@@ -959,6 +959,7 @@ def __init__(
        self.comment = kw.pop("comment", None)
        self.prefixes = kw.pop("prefixes", None)
        self.kw = kw
+        self.table_info = kw.pop('info', {})


mike bayer (zzzeek) wrote:

call this ".info"

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/2989

sqla-tester · 2021-08-04T18:00:12Z

alembic/operations/ops.py

+    def __init__(self,
+                 table_name,
+                 schema=None,
+                 table_comment=None,


mike bayer (zzzeek) wrote:

I think these both need to come from table_kw using the "pop" form. if we were to refactor this further, "table_kw" would become "**kw" just like CreateTableOp.

Also we need to add "prefixes" here.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/2989

sqla-tester · 2021-08-04T18:00:12Z

alembic/operations/ops.py

        self.table_kw = table_kw or {}
+        self.table_info = table_info or {}


mike bayer (zzzeek) wrote:

implement as:

self.info = table_kw.pop("info", None)

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/2989

sqla-tester · 2021-08-04T18:00:12Z

tests/test_op.py

        op = ops.DropTableOp.from_table(table)
        is_not_(op.to_table(), table)
+        eq_(op.to_table().comment, table.comment)
+        eq_(op.to_table().info, table.info)



mike bayer (zzzeek) wrote:

see if we can add "prefixes"

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/2989

sqla-tester · 2021-08-04T18:00:12Z

tests/test_op.py

        op = ops.CreateTableOp.from_table(table)
        is_not_(op.to_table(), table)
+        eq_(op.to_table().comment, table.comment)
+        eq_(op.to_table().info, table.info)



mike bayer (zzzeek) wrote:

see if we can add "prefixes"

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/2989

cansjt · 2021-08-04T19:46:16Z

alembic/operations/ops.py

-            table_kw=table.kwargs,
+            comment=table.comment,
+            info=table.info,
+            prefixes=table._prefixes,


Given that this is a list and is hence mutable. shouldn't we copy it ? Same for info, which is a dict.

I guess this is a good idea, seems to not be critical but then i may not be envisioning how these structures may be used, so sure.

cansjt · 2021-08-04T20:09:20Z

alembic/operations/ops.py

+            table_kw={
+                "comment": table.comment,
+                "info": table.info,
+                "prefixes": table._prefixes,
+                **table.kwargs,
+            },


This is the only way I can see to not break the class' __init__() public API.

I cannot help but wonder how much of public API this is though. Indeed, even when reverting a CreateTableOp, for example, DropTableOp.from_table() is used (see l. 967). But on principle I agree, we should not break it.

we are on the edge of a new "minor" series 0.7 so it's a better time to change the API but I felt for the moment it's best to make this PR work without changing any public API, then make public API changes between these two objects something separate.

alembic/operations/ops.py

sqla-tester

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision 257b987 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2021-08-04T20:51:26Z

Patchset 257b987 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/2989

The `DropTableOp.from_table()` method loses the table's `SchemaItem.info` and `comment` attributes while the `CreateTableOp` only loses the `info` attribute. This commit fixes both issues. Fixes sqlalchemy#879.

sqla-tester

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision e509b9e of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2021-08-10T14:36:55Z

Patchset e509b9e added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/2989

zzzeek · 2021-08-10T14:37:38Z

specifically, i want to look at the **kw situation for all the operations objects, like indexes and all that. every object has dialect-specific kw so i want to apply one consistent approach to the whole thing and do real deprecation etc.

sqla-tester · 2021-08-10T15:08:55Z

mike bayer (zzzeek) wrote:

this looks good to me as is, im going to add a changelog file and also revert that formatting change for now, if we want to work with the kw API to make it make more sense we can approach that separately.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/2989

zzzeek · 2021-08-10T15:13:24Z

also #849 was a similar issue

zzzeek · 2021-08-10T15:16:21Z

oh i guess black has made those formatting changes, OK.

zzzeek · 2021-08-10T15:20:17Z

i will also dict-/list- ify info and prefixes.

CaselIT · 2021-08-10T15:33:52Z

oh i guess black has made those formatting changes, OK.

not sure why there where not there already

cansjt · 2021-08-10T16:24:45Z

Just added the copy of .prefixes and .info. is it what you meant here ?

diff --git a/alembic/operations/ops.py b/alembic/operations/ops.py
index 5d04a8e..31da81d 100644
--- a/alembic/operations/ops.py
+++ b/alembic/operations/ops.py
@@ -1,3 +1,4 @@
+import copy
 import re
 
 from sqlalchemy.types import NULLTYPE
@@ -956,9 +957,9 @@ class CreateTableOp(MigrateOperation):
         self.table_name = table_name
         self.columns = columns
         self.schema = schema
-        self.info = kw.pop("info", {})
         self.comment = kw.pop("comment", None)
-        self.prefixes = kw.pop("prefixes", None)
+        self.info = copy.deepcopy(kw.pop("info", None))
+        self.prefixes = copy.copy(kw.pop("prefixes", None))
         self.kw = kw
         self._namespace_metadata = _namespace_metadata
         self._constraints_included = _constraints_included
@@ -1099,8 +1100,8 @@ class DropTableOp(MigrateOperation):
         self.schema = schema
         self.table_kw = table_kw or {}
         self.comment = self.table_kw.pop("comment", None)
-        self.info = self.table_kw.pop("info", None)
-        self.prefixes = self.table_kw.pop("prefixes", None)
+        self.info = copy.deepcopy(self.table_kw.pop("info", None))
+        self.prefixes = copy.copy(self.table_kw.pop("prefixes", None))
         self._reverse = _reverse
 
     def to_diff_tuple(self):

cansjt · 2021-08-10T16:25:39Z

I guess I won't... couldn't push to github.

zzzeek · 2021-08-10T16:51:47Z

yeah i wouldnt use deepcopy() here. this is pushed. thanks!

zzzeek requested a review from sqla-tester July 31, 2021 22:35

sqla-tester reviewed Jul 31, 2021

View reviewed changes

sqla-tester reviewed Aug 4, 2021

View reviewed changes

cans force-pushed the issue-879-from-table-schemaitem-info-copy branch 2 times, most recently from a022835 to 9f711a0 Compare August 4, 2021 19:43

cansjt commented Aug 4, 2021

View reviewed changes

cans force-pushed the issue-879-from-table-schemaitem-info-copy branch from 9f711a0 to 300e862 Compare August 4, 2021 19:58

cansjt commented Aug 4, 2021

View reviewed changes

cans force-pushed the issue-879-from-table-schemaitem-info-copy branch from 300e862 to 257b987 Compare August 4, 2021 20:21

CaselIT requested a review from sqla-tester August 4, 2021 20:51

sqla-tester reviewed Aug 4, 2021

View reviewed changes

cans force-pushed the issue-879-from-table-schemaitem-info-copy branch from 257b987 to f01d5f4 Compare August 5, 2021 06:21

Preserve comment and info in Drop/CreateTableOp

e509b9e

The `DropTableOp.from_table()` method loses the table's `SchemaItem.info` and `comment` attributes while the `CreateTableOp` only loses the `info` attribute. This commit fixes both issues. Fixes sqlalchemy#879.

cans force-pushed the issue-879-from-table-schemaitem-info-copy branch from f01d5f4 to e509b9e Compare August 5, 2021 06:34

zzzeek requested a review from sqla-tester August 10, 2021 14:36

sqla-tester reviewed Aug 10, 2021

View reviewed changes

sqlalchemy-bot closed this in 1ce8166 Aug 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preserve comment and info in Drop/CreateTableOp #881

Preserve comment and info in Drop/CreateTableOp #881

cansjt commented Jul 31, 2021

zzzeek commented Jul 31, 2021

sqla-tester left a comment

sqla-tester commented Jul 31, 2021

sqla-tester left a comment

sqla-tester Aug 4, 2021

sqla-tester Aug 4, 2021

sqla-tester Aug 4, 2021

sqla-tester Aug 4, 2021

sqla-tester Aug 4, 2021

cansjt Aug 4, 2021

zzzeek Aug 10, 2021

cansjt Aug 4, 2021

zzzeek Aug 10, 2021

sqla-tester left a comment

sqla-tester commented Aug 4, 2021

sqla-tester left a comment

sqla-tester commented Aug 10, 2021

zzzeek commented Aug 10, 2021

sqla-tester commented Aug 10, 2021

zzzeek commented Aug 10, 2021

zzzeek commented Aug 10, 2021

zzzeek commented Aug 10, 2021

CaselIT commented Aug 10, 2021

cansjt commented Aug 10, 2021

cansjt commented Aug 10, 2021

zzzeek commented Aug 10, 2021

		self.table_kw = table_kw or {}
		self.table_info = table_info or {}

Preserve comment and info in Drop/CreateTableOp #881

Preserve comment and info in Drop/CreateTableOp #881

Conversation

cansjt commented Jul 31, 2021

Description

Checklist

zzzeek commented Jul 31, 2021

sqla-tester left a comment

Choose a reason for hiding this comment

sqla-tester commented Jul 31, 2021

sqla-tester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sqla-tester left a comment

Choose a reason for hiding this comment

sqla-tester commented Aug 4, 2021

sqla-tester left a comment

Choose a reason for hiding this comment

sqla-tester commented Aug 10, 2021

zzzeek commented Aug 10, 2021

sqla-tester commented Aug 10, 2021

zzzeek commented Aug 10, 2021

zzzeek commented Aug 10, 2021

zzzeek commented Aug 10, 2021

CaselIT commented Aug 10, 2021

cansjt commented Aug 10, 2021

cansjt commented Aug 10, 2021

zzzeek commented Aug 10, 2021