sqlalchemy-bot / test_alembic_1

0 stars 0 forks source link

produce "raise on execute" symbols that we can put in places we know that "None" will fail, like UniqueConstraint(None), etc. #169

Open sqlalchemy-bot opened 10 years ago

sqlalchemy-bot commented 10 years ago

Migrated issue, originally created by Ulrich Petri (ulope)

When adding unique=True to an existing column the generated migration looks like this:

def upgrade():
    op.create_unique_constraint(None, 'table', ['field'])

def downgrade():
    op.drop_constraint(None, 'table')

I think alembic should either autogenerate a constraint name (I realize this might be difficult across backends) or print a very explicit warning that the migration needs to be 'fixed' manually.

sqlalchemy-bot commented 10 years ago

Michael Bayer (zzzeek) wrote:

the constraint name thing here is something SQLA just decides not to do...we do it for Indexes because there was never a way to have an Index without a name. so its kind of like, does sqlalchemy break its longstanding policy of not autogenerating any names (except for indexes). I think the user should always have an autogenerate policy set up though (see http://www.sqlalchemy.org/trac/wiki/UsageRecipes/NamingConventions), I'm trying to figure out how to make an auto-policy like that very accessible even though not implicit, like some extension that's easy to use.

so then on the alembic side, there's some other issues as well where it seems like we're pointing to some kind of catch-all "FIXME" placeholder that goes in all these spots that Alembic doesn't want to make a decision.

I know its not as nice as django's "its all done for you" approach, but I'm sure you can appreciate the end result is potentially more desirable for shops that need more than just the out of the box defaults :).

sqlalchemy-bot commented 10 years ago

Michael Bayer (zzzeek) wrote:

oh, funny thing though, "None" is a valid argument for those functions, specifically when one is using the auto-generate events as described in that wiki recipe. I'm using it and all of our constraint instructions start with "None" - the name is assigned as the constraint is created within the function.

sqlalchemy-bot commented 10 years ago

Ulrich Petri (ulope) wrote:

Thanks for the pointer to NamingConventions, I will definitely have a look at that.

Regarding the "it's all done for you" notion - I'm aware that SQLAlchemy (and by extension Alembic) is always taking the flexibility first approach and most of the time that's fine. However I would wish that in cases like this there were reasonable defaults in place that minimize work for the common case, or in absence of that at least checks that prevent producing or warn about broken/unfinished migrations.

As for the original problem: None is only valid for the upgrade operation. In downgrade it breaks because it tries to .lower() None (but even if that wouldn't be the case it couldn't work since there is no information what constraint to delete).

sqlalchemy-bot commented 10 years ago

Michael Bayer (zzzeek) wrote:

see patch at http://www.sqlalchemy.org/trac/ticket/2923.

on this side, the "None" name will be fixed before autogenerate gets to the constraint objects, assuming a naming convention is in use. When autogenerate produces a constraint object that has "None" for the name, it will emit a warning. When the migration file is run, the "None" name should be emitting a CompileError within the SQLAlchemy side of things - otherwise the "None" should go all the way through. It's even possible someone is setting up the name in a @compiles step so I'd like to keep that supported.

sqlalchemy-bot commented 10 years ago

Michael Bayer (zzzeek) wrote:

NamingConventions is now a feature for SQLAlchemy 0.9.2

sqlalchemy-bot commented 9 years ago

Kevin Cantu (killerswan) wrote:

No, @zzzeek, None seems valid when adding a unique constraint, but not when dropping it.

For example, in a downgrade with this:

op.drop_constraint(None, 'commits', type_='unique')

I end up with this stack:

  File "C:\Python27\lib\site-packages\alembic\environment.py", line 742, in run_migrations
    self.get_context().run_migrations(**kw)
  File "C:\Python27\lib\site-packages\alembic\migration.py", line 305, in run_migrations
    step.migration_fn(**kw)
  File "C:\code\infr\indium\alembic\versions\58c40a66cd98_try_to_use_unique_constraint_in_.py", line 29, in downgrade

    op.drop_constraint(None, 'commits', type_='unique')
  File "<string>", line 7, in drop_constraint
  File "<string>", line 1, in <lambda>
  File "C:\Python27\lib\site-packages\alembic\util.py", line 386, in go
    return fn(*arg, **kw)
  File "C:\Python27\lib\site-packages\alembic\operations.py", line 1084, in drop_constraint
    self.impl.drop_constraint(const)
  File "C:\Python27\lib\site-packages\alembic\ddl\impl.py", line 171, in drop_constraint
    self._exec(schema.DropConstraint(const))
  File "C:\Python27\lib\site-packages\alembic\ddl\impl.py", line 106, in _exec
    return conn.execute(construct, *multiparams, **params)
  File "C:\Python27\lib\site-packages\sqlalchemy\engine\base.py", line 729, in execute
    return meth(self, multiparams, params)
  File "C:\Python27\lib\site-packages\sqlalchemy\sql\ddl.py", line 69, in _execute_on_connection
    return connection._execute_ddl(self, multiparams, params)
  File "C:\Python27\lib\site-packages\sqlalchemy\engine\base.py", line 777, in _execute_ddl
    compiled = ddl.compile(dialect=dialect)
  File "<string>", line 1, in <lambda>
  File "C:\Python27\lib\site-packages\sqlalchemy\sql\elements.py", line 493, in compile
    return self._compiler(dialect, bind=bind, **kw)
  File "C:\Python27\lib\site-packages\sqlalchemy\sql\ddl.py", line 27, in _compiler
    return dialect.ddl_compiler(dialect, self, **kw)
  File "C:\Python27\lib\site-packages\sqlalchemy\sql\compiler.py", line 199, in __init__
    self.string = self.process(self.statement, **compile_kwargs)
  File "C:\Python27\lib\site-packages\sqlalchemy\sql\compiler.py", line 222, in process
    return obj._compiler_dispatch(self, **kwargs)
  File "C:\Python27\lib\site-packages\sqlalchemy\ext\compiler.py", line 413, in <lambda>
    lambda *arg, **kw: existing(*arg, **kw))
  File "C:\Python27\lib\site-packages\sqlalchemy\ext\compiler.py", line 451, in __call__
    return fn(element, compiler, **kw)
  File "C:\Python27\lib\site-packages\sqlalchemy\sql\visitors.py", line 80, in _compiler_dispatch
    return meth(self, **kw)
  File "C:\Python27\lib\site-packages\sqlalchemy\sql\compiler.py", line 2518, in visit_drop_constraint
    self.preparer.format_constraint(drop.element),
  File "<string>", line 1, in <lambda>
  File "C:\Python27\lib\site-packages\sqlalchemy\sql\compiler.py", line 2928, in format_constraint
    return self.quote(constraint.name)
  File "C:\Python27\lib\site-packages\sqlalchemy\sql\compiler.py", line 2893, in quote
    if self._requires_quotes(ident):
  File "C:\Python27\lib\site-packages\sqlalchemy\sql\compiler.py", line 2864, in _requires_quotes
    lc_value = value.lower()
AttributeError: 'NoneType' object has no attribute 'lower'
sqlalchemy-bot commented 9 years ago

Michael Bayer (zzzeek) wrote:

@killerswan - just use naming conventions as I've referred to here. No hacks needed. You will never get a "drop_constraint(None)" if this is properly used.

sqlalchemy-bot commented 9 years ago

Kevin Cantu (killerswan) wrote:

Ooooh, nifty!

sqlalchemy-bot commented 10 years ago

Changes by Michael Bayer (zzzeek):