-
Notifications
You must be signed in to change notification settings - Fork 375
Fixed issue in create_user
#370
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
Conversation
The function assumed that `User.get_by_type_and_id` returns a User instance or None. However, this called method raises a `helios_auth.models.User.DoesNotExist` exception. This commit changes the behavior of `create_user` to match the behavior of `User.get_by_type_and_id`.
Example$ ./manage.py shell
Python 3.10.6 (main, Nov 14 2022, 16:10:14) [GCC 11.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
(InteractiveConsole)
>>> import django
>>> from helios_auth.models import User
>>> from helios_auth.auth_systems.password import create_user
>>> django.VERSION
(2, 2, 24, 'final', 0)
>>> create_user("andi","very-secret-password", "Andi")
Traceback (most recent call last):
File "<console>", line 1, in <module>
File "/home/andi/workspace/helios-server/helios_auth/auth_systems/password.py", line 24, in create_user
user = User.get_by_type_and_id('password', username)
File "/home/andi/workspace/helios-server/helios_auth/models.py", line 48, in get_by_type_and_id
return cls.objects.get(user_type = user_type, user_id = user_id)
File "/home/andi/virtualenvs/helios/lib/python3.10/site-packages/django/db/models/manager.py", line 82, in manager_method
return getattr(self.get_queryset(), name)(*args, **kwargs)
File "/home/andi/virtualenvs/helios/lib/python3.10/site-packages/django/db/models/query.py", line 406, in get
raise self.model.DoesNotExist(
helios_auth.models.User.DoesNotExist: User matching query does not exist.Note: I only tested this with Python 3.10 and Django 2.21 Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a bug in the create_user function that incorrectly assumed User.get_by_type_and_id returns None when a user doesn't exist. The method actually raises a User.DoesNotExist exception (as confirmed by the Django ORM's objects.get() behavior), which would have caused the function to crash instead of properly handling existing users.
Key Changes:
- Replaced conditional check with try/except/else block to properly handle
User.DoesNotExistexception - Aligned error handling pattern with other usages in the same file (lines 65, 90)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if user: | ||
|
|
||
| try: | ||
| User.get_by_type_and_id('password', username) |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The result of User.get_by_type_and_id is not assigned to any variable. While the current code works correctly for checking existence, it would be more idiomatic to assign it to a variable (e.g., user = User.get_by_type_and_id('password', username)) for consistency with other usages in the codebase (see lines 65 and 90).
| User.get_by_type_and_id('password', username) | |
| user = User.get_by_type_and_id('password', username) |
Co-authored-by: Copilot <[email protected]>
The function assumed that
User.get_by_type_and_idreturns a User instance or None. However, this called method raises ahelios_auth.models.User.DoesNotExistexception.This commit changes the behavior of
create_userto match the behavior ofUser.get_by_type_and_id.