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

Fix for bug #40 for Password type (only) #233

Closed
wants to merge 1 commit into from

Conversation

dbrumley
Copy link

Fix makes representation consistent with constructor call by printing
length as the expected max_length parameter. Note this helps downstream
alembic migrations work properly too.

Fix makes representation consistent with constructor call by printing
length as the expected max_length parameter. Note this helps downstream
alembic migrations work properly too.
@kvesteri
Copy link
Owner

Thanks for the PR. Before merging this needs unit test(s).

@frol
Copy link
Contributor

frol commented Nov 8, 2016

This doesn't work for me when I use SQLite as a backend as it somehow uses sa.NUMERIC(precision=128), and I still receive a bloated autogenerated migration:

    op.alter_column('user', 'password',
               existing_type=sa.NUMERIC(precision=128),
               type_=sqlalchemy_utils.types.password.PasswordType(max_length=128),
               existing_nullable=False)

@dbrumley
Copy link
Author

dbrumley commented Nov 8, 2016

What doesn't work? The patch? I didn't put a unit test, so there was never a PR that incorporated the patch.

@frol
Copy link
Contributor

frol commented Nov 8, 2016

I applied the patch locally, and it didn't help me.

@frol
Copy link
Contributor

frol commented Nov 8, 2016

Oh, yeah, this patch at least helps to produce a correct type_ value using max_length= argument instead of the autogenerated length=, but Alembic's autogeneration still detects a change where it shouldn't:

Detected type change from NUMERIC(precision=128) to PasswordType(max_length=128) on 'user.password'

@dbrumley
Copy link
Author

dbrumley commented Nov 8, 2016

ah, that i did not look into.

@frol
Copy link
Contributor

frol commented Nov 8, 2016

It seems that I wrongly assumed that #106 refers to that SQLAlchemy-Utils fields are getting into every new Alembic migration, at least that is what I experience using SQLite for prototyping. It turned out that the only raised concern was that incorrect length= argument passed instead of max_length=. I am sorry for hijacking your PR. While it addresses one problem, it doesn't (and, probably, shouldn't) address the SQLite-specific one, so I dug into this issue myself and as a result, I think the bug is in SQLAlchemy, so I reported it there: https://groups.google.com/forum/#!topic/sqlalchemy/GpkSJ0XUgwo

P.S. @dbrumley Why does your commit point to the issue #40? I think, it should point to #106.

@kurtmckee
Copy link
Collaborator

It appears that this PR is not going to get updated or merged. I recommend closing.

@kvesteri kvesteri closed this Mar 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants