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

Computed column migration file generated incorrectly #624

Closed
deuce2367 opened this issue Nov 12, 2019 · 13 comments
Closed

Computed column migration file generated incorrectly #624

deuce2367 opened this issue Nov 12, 2019 · 13 comments

Comments

@deuce2367
Copy link

@deuce2367 deuce2367 commented Nov 12, 2019

SQLAlchemy version 1.3.11 (just released on November 11th, 2019) includes support for Computed columns. From their documentation an example Computed column definition would be something like:

Column("area", Integer, Computed("side * side"))

After adding such a Computed column definition to my table model and trying to use alembic to autogenerate a migration file I encounter the following error:

    resolved_id, branch_label = self._resolve_revision_number(id_)
  File "/home/dev.internal/developer/sandbox/mbs/venv/lib/python3.7/site-packages/alembic/script/revision.py", line 501, in _resolve_revision_number
    self._revision_map
  File "/home/dev.internal/developer/sandbox/mbs/venv/lib/python3.7/site-packages/alembic/util/langhelpers.py", line 230, in __get__
    obj.__dict__[self.__name__] = result = self.fget(obj)
  File "/home/dev.internal/developer/sandbox/mbs/venv/lib/python3.7/site-packages/alembic/script/revision.py", line 123, in _revision_map
    for revision in self._generator():
  File "/home/dev.internal/developer/sandbox/mbs/venv/lib/python3.7/site-packages/alembic/script/base.py", line 112, in _load_revisions
    script = Script._from_filename(self, vers, file_)
  File "/home/dev.internal/developer/sandbox/mbs/venv/lib/python3.7/site-packages/alembic/script/base.py", line 906, in _from_filename
    module = util.load_python_file(dir_, filename)
  File "/home/dev.internal/developer/sandbox/mbs/venv/lib/python3.7/site-packages/alembic/util/pyfiles.py", line 98, in load_python_file
    module = load_module_py(module_id, path)
  File "/home/dev.internal/developer/sandbox/mbs/venv/lib/python3.7/site-packages/alembic/util/compat.py", line 173, in load_module_py
    spec.loader.exec_module(module)
  File "<frozen importlib._bootstrap_external>", line 724, in exec_module
  File "<frozen importlib._bootstrap_external>", line 860, in get_code
  File "<frozen importlib._bootstrap_external>", line 791, in source_to_code
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/home/dev.internal/developer/sandbox/mbs/migrations/versions/9396eb21e075_tip_add_user_index_generated_user_.py", line 21
    op.add_column('usertable', sa.Column('user', sa.String(length=36), server_default=Computed(<sqlalchemy.sql.elements.TextClause object at 0x7febef2d30f0>, persisted=False), nullable=True))

I found that that if I hacked in a str() implementation to the Computed class definition in SQLAlchemy I was able to use alembic successfully however I'm not sure if that's the most elegant solution.

I'm using alembic version 1.3.0, sqlalchemy 1.3.11 (and MySQL 8.0.17 although I don't believe that matters as far as this issue is concerned).

I really appreciate any help or suggestions!

@zzzeek

This comment has been minimized.

Copy link
Member

@zzzeek zzzeek commented Nov 12, 2019

well yes, thanks for pointing that out, alembic needs a renderer for the compute construct.

@zzzeek

This comment has been minimized.

Copy link
Member

@zzzeek zzzeek commented Nov 12, 2019

@CaselIT any interest here?

@CaselIT

This comment has been minimized.

Copy link
Contributor

@CaselIT CaselIT commented Nov 12, 2019

I can take a look, but I won't have much time in the next couple of weeks.

@provinzio

This comment has been minimized.

Copy link

@provinzio provinzio commented Nov 28, 2019

I am in need of a quick dirty fix, so I made one. Might me usefull for somebody :)
The SQL Query might include quotes, so I am using """-quotes in sa.text to avoid problems.

in alembic/autogenerate/render.py edit:

def _render_column(column, autogen_context):
    [...]
    opts = []
    **if column.computed:
        opts.append(('server_default', f'sa.Computed(sa.text("""{column.server_default.sqltext}"""), persisted={column.server_default.persisted})'))
    elif column.server_default:**
        rendered = _render_server_default(
            column.server_default, autogen_context
        )
    [...]
@CaselIT

This comment has been minimized.

Copy link
Contributor

@CaselIT CaselIT commented Nov 30, 2019

I've started trying to solve this in #631

@zzzeek I think that without support for computed in sqlalchemy inspection it will not be possible to implement a version that is 100% working

@zzzeek

This comment has been minimized.

Copy link
Member

@zzzeek zzzeek commented Nov 30, 2019

I've started trying to solve this in #631

@zzzeek I think that without support for computed in sqlalchemy inspection it will not be possible to implement a version that is 100% working

OK...usually we start with the "render only" model, that is, if someone has computed columns in their Table metadata and they just generate a plain create_table(), the computed shows up. that's the basic level of functionality. I'm assuming that you don't see any issue with this part proceeding.

the next level is support of changes in "computed", which has to do with reading the table columns in the DB and reading them in the model and comparing, and I'm assuming this is what you're referring towards. we don't implement this feature for CHECK constraints either right now as well as for expression-based indexes and lots of other things. this is not a critical feature though, it only means that we aren't detecting if someone changed the expression in their computed column. we would need to support ALTER COLUMN that changes that for each target DB also.

@CaselIT

This comment has been minimized.

Copy link
Contributor

@CaselIT CaselIT commented Nov 30, 2019

the next level is support of changes in "computed"

The problem is that the computed column is loaded as a normal default. so the case must be handled in alembic, even to just say "no change here"

@CaselIT

This comment has been minimized.

Copy link
Contributor

@CaselIT CaselIT commented Nov 30, 2019

I'll omit the support for changes for now

@zzzeek

This comment has been minimized.

Copy link
Member

@zzzeek zzzeek commented Nov 30, 2019

is this...in compare_server_default things are going wrong?

@CaselIT

This comment has been minimized.

Copy link
Contributor

@CaselIT CaselIT commented Nov 30, 2019

it seems that currently sqlalchemy when inspecting a computed column sets the server default to the normal text of the generated alwayas,

-- table in db
create table foo (
  bar indeger,
  foo integer GENERATED ALWAYS AS (bar + 1) STORED
)
-- is inspected as 
create table foo (
  bar indeger,
  foo integer DEFAULT (bar + 1)
)

I've just tried mysql and it does not have this behavior

@zzzeek

This comment has been minimized.

Copy link
Member

@zzzeek zzzeek commented Nov 30, 2019

so interesting that it even comes up.

the server default comparison logic needs to look at the metadata-side column; if there's a "computed", ignore the whole thing and report no change.

@CaselIT

This comment has been minimized.

Copy link
Contributor

@CaselIT CaselIT commented Nov 30, 2019

check the comment #631 (comment) in the pr, I've tried to explain it a bit better

@sqla-tester

This comment has been minimized.

Copy link
Collaborator

@sqla-tester sqla-tester commented Dec 29, 2019

CaselIT has proposed a fix for this issue in the master branch:

Add support for computed columns https://gerrit.sqlalchemy.org/1600

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.