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

new op.f() construct not rendered correctly within an Index #194

Closed
sqlalchemy-bot opened this Issue Apr 1, 2014 · 18 comments

Comments

Projects
None yet
1 participant
@sqlalchemy-bot

sqlalchemy-bot commented Apr 1, 2014

Migrated issue, originally created by Iuri de Silvio (@iurisilvio)

I know it is difficult to fix these bogus indexes, but they are failing my migration.

My upgrade drop my index and create a new one, maybe because names didn't match:

    op.create_index('op.f('ix_residencial_Categoria TV')', 'residencial', ['Categoria TV'], unique=False)
    op.drop_index('ix_residencial_desembolso_tv', table_name='residencial')

The downgrade just revert the change, with the same error. You probably have to escape the quotes or use double quotes instead.

It happens with 0.6.4. The 0.6.3 generates bogus indexes but at least doesn't break with SyntaxError.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 1, 2014

Michael Bayer (@zzzeek) wrote:

i dont understand the context. what is:

op.create_index('op.f('ix_residencial_Categoria TV')', 'residencial', ['Categoria TV'], unique=False)

with the quotes around "op.f()"? autogenerate created that? or you typed that (and if so, why?) ?

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 1, 2014

Iuri de Silvio (@iurisilvio) wrote:

Yes, the line was autogenerated. I didn't changed the generated script.

residencial is my table and Categoria TV is the column to be indexed
with Column(..., index=True).

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 1, 2014

Michael Bayer (@zzzeek) wrote:

Fixed another bug regarding naming conventions, continuing
from 🎫183, where add_index()
drop_index() directives would not correctly render the f()
construct when the index contained a convention-driven name.
fixes #194 re: #183

a9fa7d9

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 1, 2014

Changes by Michael Bayer (@zzzeek):

  • changed status to closed
@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 1, 2014

Michael Bayer (@zzzeek) wrote:

can you try out git master please and make sure it's resolved on your end, thanks

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 1, 2014

Iuri de Silvio (@iurisilvio) wrote:

I tested it. Works for me. Thanks!

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 1, 2014

Michael Bayer (@zzzeek) wrote:

thanks

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 29, 2014

Chris Reeves (@krak3n) wrote:

Any news on a release that would include this fix, currently alembic is unusable for me considering I can't put indexes on fields.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 29, 2014

Michael Bayer (@zzzeek) wrote:

from what I can tell, nothing i did in the fix here should have an impact on bogus indexes being created, the issue I could see is that the "op.f()" directive, which is only there if you're using SQLA 0.9.4 or so with the latest alembic, as well as using anonymously-named indexes (e.g. index=True) which will make use of naming_convention.

so to that extent I'm not exactly sure why luri reported that it all works now, the "op.f()" should render correctly but rendering happens long after we've determined what's to be added and removed.

If you are seeing index add/removes that are incorrect, we've already had three releases each of which resolved ever more issues with this new feature and I have no test cases left that illustrate this issue occurring. So would need positive confirmation that if you run Alembic on master the issue of bogus indexes doesn't exist for you, I'm not seeing how that's possible at the moment.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 29, 2014

Michael Bayer (@zzzeek) wrote:

oh and additionally that you are on 0.6.4 and are seeing the issue on 0.6.4, thanks.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 29, 2014

Changes by Michael Bayer (@zzzeek):

  • changed title from "Bogus create/remove indexes raising SyntaxError" to "new op.f() construct not rendered correctly within"
@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 29, 2014

Michael Bayer (@zzzeek) wrote:

also @iurisilvio the original error you report seems like either there is a case convention mismatch going on (e.g. mixed case names in your model are sent without quoting and therefore come back as all lower case), or you just changed the name of the column from "desembolso_tv" to "Categoria TV", is that the case?

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 29, 2014

Iuri de Silvio (@iurisilvio) wrote:

Just to confirm, I never had this op.f() issue again, your fix works nice.

I know we still have these bogus add/drop indexes, but I can handle that without it and other issues already address this problem.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 29, 2014

Michael Bayer (@zzzeek) wrote:

I really need to know what case causes more index add/drops. There are dozens that I have fixed, covering all kinds of edge cases. What more are there?

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 29, 2014

Iuri de Silvio (@iurisilvio) wrote:

@zzzeek I don't remember what exactly my OP code did, I know desembolso_tv and Categoria TV are different fields, probably these add/drop statements are not related. I remember it was some lines with the broken op.f().

I still have some bogus indexes in this project, probably because my database has some column names with spaces. Do you have an open issue for that or should I open a new one?

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 29, 2014

Michael Bayer (@zzzeek) wrote:

if there's an issue with quoted column names then that could be a new area where fixes are needed here, we probably do not yet have test cases for that. It's more that the casing convention could cause problems if you're on a case-insensitive database like MySQL on certain OS'es.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 29, 2014

Iuri de Silvio (@iurisilvio) wrote:

I checked my database and discovered some indexes never existed, maybe because it was migrated with an old version of alembic.

I wasn't able to reproduce it in a clean environment with SQLAlchemy 0.9.4 and alembic trunk, so it is already fixed.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Apr 29, 2014

Iuri de Silvio (@iurisilvio) wrote:

Oh, I guess @krak3n was talking about release the fix for this issue. The issue is present in alembic 0.6.4. I'm installing alembic from source code with this patch applied. PyPI version is broken for me.

@sqlalchemy-bot sqlalchemy-bot added the bug label Nov 27, 2018

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