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

Data model allows for having multiple primary e-mail addresses #225

Closed
pennersr opened this issue Apr 1, 2013 · 9 comments
Closed

Data model allows for having multiple primary e-mail addresses #225

pennersr opened this issue Apr 1, 2013 · 9 comments
Labels

Comments

@pennersr
Copy link
Owner

pennersr commented Apr 1, 2013

As discussed in #210, having at most one primary e-mail address is currently not enforced at the database level. To be determined: should we fix this, e.g. by making user and primary "unique together" and using primary=NULL instead of False.

@saifrahmed
Copy link

Thanks for logging this. It is indeed strange that Django allows two "Primary" email addresses. This state appears to break Django Allauth when a user tries to log in via Social media and the social media primary email . However, it seems more an idiosyncrasy of Django than Allauth. How does everyone else handle this?

jnns added a commit to jnns/django-allauth that referenced this issue Dec 17, 2021
Since Django 2.2 the UniqueConstraint supports conditions.
This allows enforcing only one primary email per user on the database
level.

Closes pennersr#225.

The docs [1] mention that Oracle databases don't support partial 
indexes. I'm not sure if that means that the index will silently be 
ignored (just as MariaDB and MySQL do).

[1] https://docs.djangoproject.com/en/2.2/ref/models/indexes/#django.db.models.Index.condition
jnns added a commit to jnns/django-allauth that referenced this issue Dec 17, 2021
Since Django 2.2 the UniqueConstraint supports conditions.
This allows enforcing only one primary email per user on the database
level.

Closes pennersr#225.

The docs [1] mention that Oracle databases don't support partial
indexes. I'm not sure if that means that the index will silently be
ignored (just as MariaDB and MySQL do).

[1] https://docs.djangoproject.com/en/2.2/ref/models/indexes/#django.db.models.Index.condition
jnns added a commit to jnns/django-allauth that referenced this issue Dec 17, 2021
Since Django 2.2 the UniqueConstraint supports conditions.
This allows enforcing only one primary email per user on the database
level.

The docs [1] mention that Oracle databases don't support partial
indexes. I'm not sure if that means that the index will silently be
ignored (just as MariaDB and MySQL do).

Closes pennersr#225.

[1] https://docs.djangoproject.com/en/2.2/ref/models/indexes/#django.db.models.Index.condition
jnns added a commit to jnns/django-allauth that referenced this issue Dec 17, 2021
Since Django 2.2 the UniqueConstraint supports conditions.
This allows enforcing only one primary email per user on the database
level.

The docs [1] mention that Oracle databases don't support partial
indexes. I'm not sure if that means that the index will silently be
ignored (just as MariaDB and MySQL do).

Closes pennersr#225.

[1] https://docs.djangoproject.com/en/2.2/ref/models/indexes/#django.db.models.Index.condition
jnns added a commit to jnns/django-allauth that referenced this issue Dec 17, 2021
Since Django 2.2 the UniqueConstraint supports conditions.
This allows enforcing only one primary email per user on the database
level.

The docs [1] mention that Oracle databases don't support partial
indexes. I'm not sure if that means that the index will silently be
ignored (just as MariaDB and MySQL do).

Closes pennersr#225.

[1] https://docs.djangoproject.com/en/2.2/ref/models/indexes/#django.db.models.Index.condition
jnns added a commit to jnns/django-allauth that referenced this issue Jan 17, 2022
Since Django 2.2 the UniqueConstraint supports conditions.
This allows enforcing only one primary email per user on the database
level.

The docs [1] mention that Oracle databases don't support partial
indexes. I'm not sure if that means that the index will silently be
ignored (just as MariaDB and MySQL do).

Closes pennersr#225.

[1] https://docs.djangoproject.com/en/2.2/ref/models/indexes/#django.db.models.Index.condition
@derek-adair
Copy link

delted past comment -- Misread the relationship between this and #210.


So if we add a natural key to email addresses does that mean we are at 1.0???? ;)

Meta project feedback: This is the type of issue that I will personally look at and judge a code base harshly. Now, i've been a django-allauth user for over a decade now and I know the quality of code here so it doesn't stop me personally; but if you seek to grow this project this type of issue is very bad to leave hanging around.

How do you feel about making this a priority? I've combed through bug requests that haven't been updated since 2018 and this one seems like one of the most important.

@pennersr
Copy link
Owner Author

@derek-adair It simply couldn't be done back in 2013 without resorting to hacks (using NULL for False). These days, with the right Django version, conditional unique indexes are supported, so it can finally be fixed:

https://docs.djangoproject.com/en/4.2/ref/models/constraints/#django.db.models.UniqueConstraint.condition

@pennersr pennersr added TODO and removed Future labels Aug 19, 2023
@pennersr pennersr removed this from the 1.0.0 milestone Aug 19, 2023
@derek-adair
Copy link

absolutely understand. Just being cheeky.

Seems like this would be a good candidate for an upcoming release?? This may actually legit make a case for the 1.0 version as i'm a stickler for semantic versioning but am too ignorant on this code base to say so w/ certainty.

@pennersr
Copy link
Owner Author

Yes. Current development branch already contains some migrations in this area, and it's best to take this step by step, but this could be done for 0.56.0. As for 1.0, I do expect more changes that will break backwards compatibility, so that won't happen soon. Or, another option is to give up on the idea of an 1.0 completely, and release say version 55.0.0 and simply iterate in semver style from there.

@varunsaral
Copy link
Contributor

@pennersr should i add it?

@pennersr
Copy link
Owner Author

pennersr commented Sep 4, 2023

See #3385 - MySQL still does not support this. I would prefer to see how that ticket evolves before going forward with more of these conditional constraints.

@pennersr
Copy link
Owner Author

pennersr commented May 9, 2024

Using null for primary=False would be a huge backwards incompatible change, that is really not worth it given that this issue has not caused any practical issues over all this time. So, opted for adding a conditional unique constraint for databases that support it. See #3791

@pennersr
Copy link
Owner Author

#3791 is merged

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

Successfully merging a pull request may close this issue.

4 participants