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

include options, to be default in 0.8, for literal_binds=True for --sql mode #255

Closed
sqlalchemy-bot opened this issue Dec 19, 2014 · 15 comments

Comments

@sqlalchemy-bot
Copy link

Migrated issue, originally created by Wichert Akkerman (@wichert)

I have a very simple migration:

def upgrade():
    stmt = sa.text('ALTER TYPE article_type ADD VALUE IF NOT EXISTS :value')
    for article_type in ['foo']:
        op.execute(stmt.bindparams(value=article_type))

When running that with alembic upgrade head --sql the bind parameter is not substituted in the generated SQL:

-- Running upgrade  -> 4bd730372bac

ALTER TYPE article_type ADD VALUE IF NOT EXISTS %(value)s;
@sqlalchemy-bot
Copy link
Author

Michael Bayer (@zzzeek) wrote:

this is probably on the SQLAlchemy side for text().

@sqlalchemy-bot
Copy link
Author

Wichert Akkerman (@wichert) wrote:

For what it's worth the reason I tried this approach is that

  1. the op.execute documentation mentioned a text() construct would work, and
  2. the alembic.ddl.impl.text documentation mentioned using text() should be preferred over plain strings

Rambling on a bit: reading documentation I could not see the relation between alembic.ddl.impl.text() and sqlalchemy.text(), especially because the example for alembic.ddl.impl.text uses sqlalchemy.text. I was also wondering why the example there uses a connection connection object, when that isn't something that is generally available/used in an alembic migration context.migrations. Perhaps that documentation was copied over from SQLAlchemy? That would also explain why it says the bindparams argument has been deprecated since 0.9.0 when alembic is only up to version 0.7.2.

@sqlalchemy-bot
Copy link
Author

Michael Bayer (@zzzeek) wrote:

yeah it's quite simple that is an .. automodule:: pulling it in from the import space. A bug, but a pretty ordinary one :)

@sqlalchemy-bot
Copy link
Author

Michael Bayer (@zzzeek) wrote:

so the code is

#!

.. automodule:: alembic.ddl.impl
    :members:
    :undoc-members:

and sphinx says:

In an automodule directive with the members option set, only module members whose module attribute is equal to the module name as given to automodule will be documented. This is to prevent documentation of imported classes or functions. Set the imported-members option if you want to prevent this behavior and document all available members. Note that attributes from imported modules will not be documented, because attribute documentation is discovered by parsing the source file of the current module.

and:

>>> from alembic.ddl import impl
impl.>>> impl.text
<function text at 0x102e43ed8>
>>> impl.text.__module__
'sqlalchemy.expression'
>>> 

so, I don't know wtf this is about.

@sqlalchemy-bot
Copy link
Author

Michael Bayer (@zzzeek) wrote:

and it is not generating on a local build. which means....read the docs is on a stale version of sphinx? That seems very hard to believe, I just used some very recent features the other day on it. perhaps it installs sphinx into the build environment....that might be it

@sqlalchemy-bot
Copy link
Author

Michael Bayer (@zzzeek) wrote:

ok doing a wipe + rebuild there, for this one, text() works on SQLA, and I'm still trying to remember if the issue of alembic setting literal_binds for sql mode exists somewhere, because at the moment it's not doing it at all.

@sqlalchemy-bot
Copy link
Author

Michael Bayer (@zzzeek) wrote:

OK we have it for autogenerate, so that's different, #219.

@sqlalchemy-bot
Copy link
Author

Michael Bayer (@zzzeek) wrote:

I'm pretty sure that the alembic docs so far have recommended that you don't use actual bound parameters...e.g. if you were using insert() constructs, we tell you to use bulk_insert(), as well as op.inline_literal().

I think we probably should have literal_bindparam turned on for --sql, and I think when alembic was first developed we didn't really have literal_bindparam going on at that time. So at the moment I'm not comfortable flipping it on unconditionally in the 0.7 series, it'll have to be an option that we'd probably default to True in 0.8. the flag needs to have its effect right here: https://bitbucket.org/zzzeek/alembic/src/082a6c63668a601c0833c99b4ca23fe9459a3a40/alembic/ddl/impl.py?at=master#cl-99

as for the readthedocs build, their stuff is not working and I am very annoyed so I'm going to bug them now.

@sqlalchemy-bot
Copy link
Author

Changes by Michael Bayer (@zzzeek):

  • removed labels: bug
  • added labels: feature

@sqlalchemy-bot
Copy link
Author

Changes by Michael Bayer (@zzzeek):

  • changed title from "TextClause parameters not substituted in SQL outpu" to "include options to be default in 0.8 for literal_b"

@sqlalchemy-bot
Copy link
Author

Changes by Michael Bayer (@zzzeek):

  • set milestone to "tier 1"

@sqlalchemy-bot
Copy link
Author

Michael Bayer (@zzzeek) wrote:

OK docs are fixed, was a SQLAlchemy issue in 0.9

@sqlalchemy-bot
Copy link
Author

Michael Bayer (@zzzeek) wrote:

  • Added a new option
    :paramref:.EnvironmentContext.configure.literal_binds, which
    will pass the literal_binds flag into the compilation of SQL
    constructs when using "offline" mode. This has the effect that
    SQL objects like inserts, updates, deletes as well as textual
    statements sent using text() will be compiled such that the dialect
    will attempt to render literal values "inline" automatically.
    Only a subset of types is typically supported; the
    :meth:.Operations.inline_literal construct remains as the construct
    used to force a specific literal representation of a value.
    The :paramref:.EnvironmentContext.configure.literal_binds flag
    is added to the "offline" section of the env.py files generated
    in new environments.
    fixes include options, to be default in 0.8, for literal_binds=True for --sql mode #255
  • enhance the op_fixture as well as MigrationContext._stdout_connection()
    so that it uses the real DefaultImpl
    and MigrationContext fully in tests.

0e1c098

@sqlalchemy-bot
Copy link
Author

Changes by Michael Bayer (@zzzeek):

  • changed status to closed

@sqlalchemy-bot
Copy link
Author

Michael Bayer (@zzzeek) wrote:

OK so, this is just a runtime env.py flag, so it's not that big a deal to add it into the template for new migration environments. You'll need to add it to your own env.py.

here's a migration:

def upgrade():
    op.execute(sa.text("update table set foo=:bar").bindparams(bar='bat'))

when i run without the changeset:

INFO  [alembic.migration] Context impl SQLiteImpl.
INFO  [alembic.migration] Generating static SQL
INFO  [alembic.migration] Will assume non-transactional DDL.
CREATE TABLE alembic_version (
    version_num VARCHAR(32) NOT NULL
);

INFO  [alembic.migration] Running upgrade  -> 2037f8370292, rev1
-- Running upgrade  -> 2037f8370292

update table set foo=?;

INSERT INTO alembic_version (version_num) VALUES ('2037f8370292');

when I run with it:

INFO  [alembic.migration] Context impl SQLiteImpl.
INFO  [alembic.migration] Generating static SQL
INFO  [alembic.migration] Will assume non-transactional DDL.
CREATE TABLE alembic_version (
    version_num VARCHAR(32) NOT NULL
);

INFO  [alembic.migration] Running upgrade  -> 2037f8370292, rev1
-- Running upgrade  -> 2037f8370292

update table set foo='bat';

INSERT INTO alembic_version (version_num) VALUES ('2037f8370292');

this requires that you add literal_binds=True to your offline environment:

def run_migrations_offline():
    url = config.get_main_option("sqlalchemy.url")
    context.configure(
        url=url, target_metadata=target_metadata, literal_binds=True)

    with context.begin_transaction():
        context.run_migrations()

this flag is added to the env.py files for new environments as well.

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

No branches or pull requests

1 participant