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

Add non-empty constraint for UserSocialAuth uid #557

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aleksanb
Copy link

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)
  • 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)
  • 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.

Any empty uids are surely erroneous, and better to have integrity errors
than having users start sharing accounts when logging in.
@aleksanb
Copy link
Author

aleksanb commented Apr 3, 2024

Any thoughts on whether this could be accepted?

@nijel
Copy link
Member

nijel commented Apr 3, 2024

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
Copy link
Author

aleksanb commented Apr 3, 2024

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
Copy link
Member

nijel commented Apr 3, 2024

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

@stianjensen
Copy link

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).

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

Successfully merging this pull request may close these issues.

3 participants