django-allauth
django-allauth copied to clipboard
account.EmailAddress: (models.W036) MySQL does not support unique constraints with conditions.
When upgrading django-allauth from 0.54.0 to 0.55.0 i get this warning.
account.EmailAddress: (models.W036) MySQL does not support unique constraints with conditions. HINT: A constraint won't be created. Silence this warning if you don't care about it.
I care about it, and would like to leave previous constraints (unique_together = ["user", "email"]
) when using mysql database.
What do you think? I can create PR with changes.
What changes are you proposing?
That unique together (email+user) is still there, see: https://github.com/pennersr/django-allauth/blob/main/allauth/account/models.py#L35
What MySQL does not support is this constraint:
UniqueConstraint(
fields=["email"],
name="unique_verified_email",
condition=Q(verified=True),
In 0.54.0, there was a unique on email
regardless of the verification state:
email = models.EmailField(unique=app_settings.UNIQUE_EMAIL
This was changed in order to fully prevent user account enumeration. It's quite unfortunate that MySQL does not support this as that means that the database model needs to diverge depending on the database that is targeted. Or, we just need to live with the fact that this constraint is not there for MySQL.
See https://bugs.mysql.com/bug.php?id=76631
[26 Aug 2019 11:05] Michal Vorisek
Feature request that needs to implemented with the high priority.
It can save a lot of resources (data/cache/CPU) and improve dev time/experience for common use cases.
Implemented all other major databases:
- Microsoft SQL - fully supported, https://docs.microsoft.com/en-us/sql/relational-databases/indexes/create-filtered-indexes
- Postgres - fully supported, https://www.postgresql.org/docs/current/indexes-partial.html
- Oracle - supported by using function based index, as NULLs are not indexed in Oracle DB, https://dba-presents.com/index.php/databases/oracle/41-filtered-index-equivalent-in-oracle
A workaround, albeit ugly, would be to add another column:
verified_email = models.EmailField(
unique=True,
blank=True,
null=True,
max_length=app_settings.EMAIL_MAX_LENGTH,
)
This column would be equal to email
if verified
, and None
otherwise.
Or, slightly better:
verified = models.BooleanField(
null=True,
verbose_name=_("verified"),
default=None,
)
unique_together = [("email", "verified")]
And then never use False
to indicate verified
-ness, always None
.
TBD:
- Do we want/need to solve this issue?
- If we do, do we want to use a lowest common denominator approach for the model, and drop the use of the constraint with conditions even for more capable databases such as PostgreSQL?
- Or, are we going to make the model definition vary per database?
What changes are you proposing?
I was thinking unique_together
was gone, but now i see that it's still there.
Looking at this commit: https://github.com/pennersr/django-allauth/commit/7491c8c03660839dec00f883fb58905f3e46b3ed
Our app is using UNIQUE_EMAIL = True
, and now behaviour will change from having no unique_together
and unique=True on email field, to having unique_together
and no unique constraint.
Minimal change would be adding unique constraint to email for mysql database. This leads to having account enumeration problem, but only for mysql database.
I think, varying model definition per database is harder, so it's better to use lowest common denominator. Like you suggested here: https://github.com/pennersr/django-allauth/issues/3385#issuecomment-1691997258
Let me know what you think. After we decided what needs to be done, i can start working on a PR.
Moving back to the way it was before breaks enumeration -- which is a security concern. That's bad.
Implementing what's suggested here https://github.com/pennersr/django-allauth/issues/3385#issuecomment-1692030883 breaks backwards compatibility. Any code doing .filter(verified=False)
would no longer find any unverified email addresses. That's bad.
Not solving this issue could, theoretically, lead to having 2 separate user accounts in the database, with an identical verified email. Though, I don't think it could really happen in practice, as it would require weird parallel verification attempts by separate users at the exact same time, all for the same email. Not nice, but not really bad either.
So far, I am leaning towards the latter. If this issue truly concerns you, you could always upgrade to PostgreSQL. Let's wait a bit to see if more feedback arrives on this matter...
Thought: the issue of backwards compatibility could be addressed by creating a custom Django field that uses get_prep_value()
magic to automatically translate False
to None
when doing queries.
@pennersr can we do it with triggers somehow, i explored https://github.com/Opus10/django-pgtrigger but it seems it does not support mysql, although mysql support triggers with conditions.
I have prototyped a proof of concept, with a custom UniqueTrueField
that, depending on the database would behave differently as far as None
/ False
is concerned, with the idea being that we could then fallback to transparently used null
/ true
instead of false
/ true
for verified
. That attempt stranded over at the refactor-unique-true-field
branch -- things quickly became very hairy, rowing upstream against Django internals. So this approach...
Thought: the issue of backwards compatibility could be addressed by creating a custom Django field that uses get_prep_value() magic to automatically translate False to None when doing queries.
... is not going to fly. All things considered now, this option does seem to be the best way forward:
Not solving this issue could, theoretically, lead to having 2 separate user accounts in the database, with an identical verified email. Though, I don't think it could really happen in practice, as it would require weird parallel verification attempts by separate users at the exact same time, all for the same email. Not nice, but not really bad either.
Over all this time, this never really caused any serious issues, and if you truly value strict correctness here, better use a database that supports unique constraints with conditions. It's not up to allauth to try and cover up missing features in the database...
Closing.
This issue is breaking my website deployment procedure. As the warning goes to the stderr
. Is there any fix on the user side?
@Arash-codedev This is a regular Django warning that can be silenced using https://docs.djangoproject.com/en/5.0/ref/settings/#std-setting-SILENCED_SYSTEM_CHECKS -- if that is somehow not working check your LOGGING
.
@pennersr , what if a real problem happens while the warning is silenced. Generally, warning = error. Additionally, I found that all-auth can create duplicate email on user table which is scary. I wonder why user emails are not unique in django.
@Arash-codedev
what if ...
That is explained above -- https://github.com/pennersr/django-allauth/issues/3385#issuecomment-2102519590
If you are really concerned by this issue, then why not upgrade your database to e.g. Postgres?