Add non-empty constraint for UserSocialAuth uid

[aleksanb]

Proposed changes

Any empty uids delivered by providers are surely erroneous, and better to have integrity errors than having users start sharing accounts when logging in.

A DB check constraint will catch issues like this one, and ensure that no matter the way a usersocialauth model is created, no empty uids can be inserted.

I'd say this is a bugfix mostly, but a breaking bugfix at that, as existing users of this library might have uid='' in their database by accident. These are real errors that those users will want to fix, and we should mention this upgrade in the changelog as a breaking change.

Types of changes

Please check the type of change your PR introduces:

• [ ] Release (new release request)
• [x] Bugfix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] Code style update (PEP8, lint, formatting, renaming, etc)
• [ ] Refactoring (no functional changes, no api changes)
• [x] Breaking change (fix or feature that would cause existing functionality to not work as expected)
• [ ] Build related changes (build process, tests runner, etc)
• [ ] Other (please describe):

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

• [ ] Lint and unit tests pass locally with my changes
• [ ] I have added tests that prove my fix is effective or that my feature works
• [ ] I have added documentation to https://github.com/python-social-auth/social-doc

[aleksanb]

Any thoughts on whether this could be accepted?

[nijel]

I think it makes sense, but I'm not sure about splitting the constraint to UserSocialAuth instead of AbstractUserSocialAuth. Also this makes the tests fail.

[aleksanb]

I can move the constraint up to AbstractUserSocialAuth, i'm not familiar with the difference between AbstractUserSocialAuth and UserSocialAuth though.

I'll have a look at the test failure.

[nijel]

I'm not familiar with it either, I'm just curious about splitting the constraint from the field definition.

[stianjensen]

It would probably work as long as the Meta-class of UserSocialAuth is also updated to explicitly subclass AbstractUserSocialAuth.Meta, like this: class Meta(AbstractUserSocialAuth.Meta): (and as long as UserSocialAuth does not get its own definition of constraints in the future that overrides this one – although if that happened, it would be detected by seeing a new migration operation appearing).

[nijel]

@stianjensen Can you please rebase this? As there was no other feedback, I think it's okay to merge as is, I just want to see the tests pass.

[aleksanb]

Pushed up a rebased and updated version @nijel , but i can't figure out how to run the tests locally, can you approve CI for tests?

[nijel]

There is also pre-commit fixing some formatting. Can you allow edits to the PR so that it can push the fixes?

[nijel]

...and the constraint fails some checks:

sqlite3.IntegrityError: CHECK constraint failed: user_social_auth_uid_required

[aleksanb]

Seems like i can't enable maintainer edits because my fork lives in an organization.

Locally, ruff seems to pass but the local commands might not be the same that CI runs. (.venv) ➜ social-app-django git:(check-for-empty-string) pre-commit run ruff --all-files ruff.....................................................................Passed

I added an uid to the one test that failed on CI.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 92.36%. Comparing base (292425c) to head (2cb718f). Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #557      +/-   ##
==========================================
+ Coverage   92.32%   92.36%   +0.03%     
==========================================
  Files          40       41       +1     
  Lines        1173     1179       +6     
  Branches       66       66              
==========================================
+ Hits         1083     1089       +6     
  Misses         65       65              
  Partials       25       25              

Flag	Coverage Δ
unittests	92.36% <100.00%> (+0.03%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[aleksanb]

Django main fails now because check has been renamed to condition, but that change doesn't happen until django 6.0.

[aleksanb]

Aha, pre-commit run ruff-format --all-files was the magic incantation i needed. Crazy that ruff tells me no changes are needed, and yet it formats when i run ruff-format.

[nijel]

Django main fails now because check has been renamed to condition, but that change doesn't happen until django 6.0.

Django main is tested against Django from Git. I don't mind dropping support for old Django versions if needed.

[aleksanb]

Check was replaced with condition in 5.1 it seems, https://docs.djangoproject.com/en/5.1/ref/models/constraints/#django.db.models.CheckConstraint.condition, and 4.2 is still in LTS. Unsure how to proceed, should i add an IF in there checking if the django version <6.0? But the migrations would need to differ as well.

[nijel]

That's unfortunate that it is impossible to write code which would work with both 4.2 and 6.0 as both will be supported at the same time by Django. For now, we can probably skip testing Django main (and replace it with 5.2b1), but we will need a solution for this later. That might be dropping support for Django < 5.1 once Django 5.0 is no longer supported.

[aleksanb]

So it seems like the migrations get generated with check regardless of you use check or condition, so as long as we set up the Metaclass to switch based on django-version.

Pushed up a version with this now.

[nijel]

In https://github.com/python-social-auth/social-app-django/pull/674 I'm dropping support for the oldest Django versions, but this will not help with this. Can you try discussing this compatibility issue at Django? Whether it is expected that the same code cannot work for both 4.2 and 6.0, while they will be supported at the same time.

[stianjensen]

I actually did ask a related question to this previously and their answer then was 'squash old migrations or edit them': https://code.djangoproject.com/ticket/35234 That wasn't in context of how to publish a library supporting several Django versions in parallel, though, but seems like they broke migrations on purpose at least.

So I think the solution must be to add a version switch to the migration file as well, to make the kwargs to CheckConstraint dynamic.

[nijel]

I've posted this at https://forum.djangoproject.com/t/reusable-library-and-incompatible-django-core-changes/40097, let's see if we get some feedback. I'm not really sure if it's better to include additional code for compatibility with Django 4.2 or drop support for it (we can disregard 5.0 as it is out of support as of April 2025).

[nijel]

As Django 5.0 is now out of support, I think it's time to drop support for 4.2 as well. LTS users can still use the older releases and it will be easier to move forward. See https://github.com/python-social-auth/social-app-django/pull/697

[aleksanb]

updated

[nijel]

Merged, thanks for your contribution!