Skip to content
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

check does not work with ExecuteSQLOp present #1335

Closed
CaselIT opened this issue Oct 23, 2023 Discussed in #1334 · 2 comments
Closed

check does not work with ExecuteSQLOp present #1335

CaselIT opened this issue Oct 23, 2023 Discussed in #1334 · 2 comments
Labels
bug Something isn't working PRs (with tests!) welcome

Comments

@CaselIT
Copy link
Member

CaselIT commented Oct 23, 2023

Discussed in #1334

Originally posted by BayerSe October 23, 2023
Hi,

we're using ExecuteSQLOp to add triggers to some columns.

With these added, alembic check nowadays raises a not implemented error, it seems as if because ExecuteSQLOp does not implement .to_diff_tuple().

op = ops.ExecuteSQLOp(sqltext='CREATE OR REPLACE TRIGGER ...')
op.to_diff_tuple()  # NotImplementedError

This used to work in version 1.6.5 and I think this error was introduced in 1.7.0 when .to_diff_tuple() was added to the base class MigrateOperation, see here.

Here's the stacktrace of alembic check:

INFO  [alembic.runtime.migration] Context impl OracleImpl.
INFO  [alembic.runtime.migration] Will assume non-transactional DDL.
INFO  [alembic.autogenerate.compare] Detected added table 'his_test'
Traceback (most recent call last):
  File "/home/user/PycharmProjects/project/.venv/bin/alembic", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/home/user/PycharmProjects/project/.venv/lib/python3.11/site-packages/alembic/config.py", line 630, in main
    CommandLine(prog=prog).main(argv=argv)
  File "/home/user/PycharmProjects/project/.venv/lib/python3.11/site-packages/alembic/config.py", line 624, in main
    self.run_cmd(cfg, options)
  File "/home/user/PycharmProjects/project/.venv/lib/python3.11/site-packages/alembic/config.py", line 601, in run_cmd
    fn(
  File "/home/user/PycharmProjects/project/.venv/lib/python3.11/site-packages/alembic/command.py", line 294, in check
    diffs = migration_script.upgrade_ops.as_diffs()
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/PycharmProjects/project/.venv/lib/python3.11/site-packages/alembic/operations/ops.py", line 2556, in as_diffs
    return list(OpContainer._ops_as_diffs(self))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/PycharmProjects/project/.venv/lib/python3.11/site-packages/alembic/operations/ops.py", line 2566, in _ops_as_diffs
    yield op.to_diff_tuple()
          ^^^^^^^^^^^^^^^^^^
  File "/home/user/PycharmProjects/project/.venv/lib/python3.11/site-packages/alembic/operations/ops.py", line 88, in to_diff_tuple
    raise NotImplementedError
NotImplementedError

I have no idea how this worked in 1.6.5 as already there, ExecuteSQLOp and some other ops did not have .to_diff_tuple(), but with .to_diff_tuple() in the base class check does not work as of today.

Thanks
Sebastian

@CaselIT CaselIT added bug Something isn't working PRs (with tests!) welcome labels Oct 23, 2023
@CaselIT
Copy link
Member Author

CaselIT commented Oct 23, 2023

Proposed solution from mike #1334 (reply in thread):

yes. see it does not normally make sense that ExecuteSQLOp would be in a diff stream unless as you are doing you have a hook adding it in. the "diff" then wants to show "what changed?" so here it would just be some code like "exec" and then a SQL statement. ill try to look at 1.6.5 to see what this would have done then.

BayerSe pushed a commit to BayerSe/alembic that referenced this issue Oct 24, 2023
This implements ExecuteSQLOp.to_diff_tuple, which is necessary when custom SQL statements are added to the diff stream via some hook.
BayerSe pushed a commit to BayerSe/alembic that referenced this issue Oct 24, 2023
This implements ExecuteSQLOp.to_diff_tuple, which is necessary when custom SQL statements are added to the diff stream via some hook.
@sqla-tester
Copy link
Collaborator

Sebastian Bayer (AE/MFD2-SO) has proposed a fix for this issue in the main branch:

Implement ExecuteSQLOp.to_diff_tuple https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/4914

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working PRs (with tests!) welcome
Projects
None yet
Development

No branches or pull requests

2 participants