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

History command raises ValueError if migration docstring has percent sign #497

Closed
sqlalchemy-bot opened this Issue May 25, 2018 · 7 comments

Comments

Projects
None yet
1 participant
@sqlalchemy-bot

sqlalchemy-bot commented May 25, 2018

Migrated issue, originally created by Wil Tan

When running alembic history, if the docstring of a particular migration module contains a percent (%) sign, Alembic would throw an error while it tries to print it.

Below, the description for revision "0012" is "Data migration to lower rate to 3%":

$ alembic history
0012 -> 0013 (head), Add Foo table

Traceback (most recent call last):
  File "dev\Scripts\alembic-script.py", line 5, in <module>
    exit(main())
  File "dev\lib\site-packages\alembic\config.py", line 486, in main
    CommandLine(prog=prog).main(argv=argv)
  File "dev\lib\site-packages\alembic\config.py", line 480, in main
    self.run_cmd(cfg, options)
  File "dev\lib\site-packages\alembic\config.py", line 463, in run_cmd
    **dict((k, getattr(options, k, None)) for k in kwarg)
  File "dev\lib\site-packages\alembic\command.py", line 386, in history
    _display_history(config, script, base, head)
  File "dev\lib\site-packages\alembic\command.py", line 364, in _display_history
    include_doc=True, include_parents=True))
  File "dev\lib\site-packages\alembic\config.py", line 160, in print_stdout
    (compat.text_type(text) % arg),
ValueError: incomplete format
@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented May 25, 2018

Wil Tan wrote:

I made the following change as a quick fix:

--- "dev/lib/site-packages/alembic/command.py"    2018-05-25 17:04:18.296138200 +1000
+++ "dev/libib/site-packages/alembic/command-new.py"        2018-05-25 16:56:29.237000000 +1000
@@ -358,10 +358,10 @@
             if indicate_current:
                 sc._db_current_indicator = sc.revision in currents

-            config.print_stdout(
-                sc.cmd_format(
+            desc = sc.cmd_format(
                     verbose=verbose, include_branches=True,
-                    include_doc=True, include_parents=True))
+                include_doc=True, include_parents=True)
+            config.print_stdout(desc.replace('%', '%%'))

     def _display_history_w_current(config, script, base, head):
         def _display_current_history(rev, context):
@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented May 25, 2018

Michael Bayer (@zzzeek) wrote:

any chance of a github PR including a test in tests/test_command.py ?

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Jul 13, 2018

Changes by Michael Bayer (@zzzeek):

  • set milestone to "fasttrack"
@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Aug 30, 2018

Mike Waites (@mikeywaites) wrote:

Hey! I picked this issue up and implemented Wil's proposed fix but had a few considerations.

When a revision description contains charactes like %, writing
output to stdout will fail because the call to config.print_stdout
attempts to format any additional args passed to the function.

This fix replaces any % characters found in the text returned
by cmd_format. Whilst this does resolve the issue I think there is a
need to re-think the convenience approach of automatically formatting
all *args passed to print_stdout. Essentially this fix will only
resolve the issue in this one
place (Perhaps it's the only place it might happen?).

Some options:

1 - Use .format()

There are around 4 instances of print_stdout in use with *args
being passed. We could easily resolve this issue anywhere we might
try to print text to stdout that contains a % by instead using .format()

def print_stdout(self, text, *args):
    util.write_outstream(
            self.stdout,
            compat.text_type(text).format(*args)
            "\n"
        )

The downside with this approach is any str containing {} would result in
the same ValueError being raised. (though it's far less likely).

2 - Don't format text in print_stdout.

This is my perferred solution as i think it's 1) the safest across the
board 2) the most explicit. Instead of allowing *args to be passed
to print_stdout we would instead format the text before passing it
to print_stdout. As there are so few instances of this being used it
would not result in a big diff or anything. We could also implement a
util if we feel doing this in situ is annoying. print_stdout therefore
simply becomes:

def print_stdout(self, text):
    util.write_outstream(
            self.stdout,
            compat.text_type(text),
            "\n"
        )

Code calling this function is then responsible for formatting any text it want's to send to stdout.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Aug 30, 2018

Michael Bayer (@zzzeek) wrote:

So in this specific case we can see why #2 is definitely the right way to go, because the call is essentially:

config.print_stdout(sc.cmd_format(...))

there's the bug, we are already formatting a string here, then sending it to another method that also thinks it should have to format it.

Here's another proposal. If *args is empty when passed to print_stdout, then don't call "%". It depends on how useful this formatting is elsewhere. When I do this sort of pattern, what I'm basically imitating is how python logging allows objects to be passed following the log string, which is taken to be a format string and allows that if you aren't actually logging, the expense of calling __str__() on the objects is saved. but that is not needed here.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Sep 24, 2018

Michael Bayer (@zzzeek) wrote:

Don't format output twice in writer

Fixed an issue where revision descriptions were essentially
being formatted twice. Any revision description that contained
characters like %, writing output to stdout will fail because
the call to config.print_stdout attempted to format any
additional args passed to the function.
This fix now only applies string formatting if any args are provided
along with the output text.

Fixes: #497

Change-Id: I64b2f00e8f67b95652bd7cbbe8510f8c5f645af1
Pull-request: zzzeek/alembic#45

a614443

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Sep 24, 2018

Changes by Michael Bayer (@zzzeek):

  • changed status to closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment