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

Not compatible with MySQL #17

Closed
nijel opened this issue Aug 2, 2024 · 3 comments · Fixed by #18
Closed

Not compatible with MySQL #17

nijel opened this issue Aug 2, 2024 · 3 comments · Fixed by #18

Comments

@nijel
Copy link
Contributor

nijel commented Aug 2, 2024

Migrating on MySQL fails with:

django.db.utils.OperationalError: (1170, "BLOB/TEXT column 'credential_id_sha256' used in key specification without a key length")

@nijel
Copy link
Contributor Author

nijel commented Aug 2, 2024

Possible solutions I can see:

  • Manually create the index for MySQL with length specified.

    Needs to be implemented in the migration code, but should be doable.

  • Use CharField instead and store hexdigest() instead of digest() there.

    A bit slower, but works with any database.

  • Use Index(SHA256("credential_id")) and then get using database instead of calculating SHA256 in Python.

    Something like .annotate(credential_id_sha256=SHA256(F("credential_id")).get(credential_id_sha256=SHA256(credential_id)) can be used inside get_by_credential_id.

    The index would be skipped by Django on old MySQL and MariaDB, but will perform good elsewhere and use standard Django syntax. Moreover, this removes the need to calculate something manually in save().

I can prepare a pull request, but I'd like to hear opinion on the preferred approach first.

Stormheg added a commit that referenced this issue Aug 2, 2024
MySQL has issues creating an index on binary field, requiring special
instructions. It is better to go with a slightly more compatible
implementation that utilizes plain CharField than to write special
edge-case instructions just for MySQL.

Squashed migration necessary to avoid ever executing migration code that
crashes on MySQL.

See #17
Stormheg added a commit that referenced this issue Aug 2, 2024
MySQL has issues creating an index on binary field, requiring special
instructions. It is better to go with a slightly more compatible
implementation that utilizes plain CharField than to write special
edge-case instructions just for MySQL.

Squashed migration necessary to avoid ever executing migration code that
crashes on MySQL.

See #17
@Stormheg
Copy link
Member

Stormheg commented Aug 2, 2024

@nijel thanks for listing those suggestion so clearly, I happened to have some time available today so I tried out the third solution as it was most appealing to me because - as you mentioned - it avoids calculating the hash in the save() method.

I ended up discovering that cryptographic functions like SHA256 are not always available. On Postgres for example, it requires the pgcrypto extension to be enabled which I consider too much of a hassle to warrant going with this solution.

@nijel
Copy link
Contributor Author

nijel commented Aug 2, 2024

Ah, I use just MD5 so far and never hit that it would be unavailable, I thought it's the case for all of them.

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

Successfully merging a pull request may close this issue.

2 participants