On 06/22/2015 06:45 PM, Thomas De Schampheleire wrote:
# HG changeset patch
# User Thomas De Schampheleire <[email protected]>
# Date 1434984724 -7200
# Mon Jun 22 16:52:04 2015 +0200
# Node ID 68032f8bd7f52c676bb5143a03b0dfd5df242f0c
# Parent 7303d0035d82a20f93df1c78b3358403b7ff9827
model: check for invalid users centrally
Commit 9a23b444a7fefe5b67e57a42d26343787d992874 introduced
UserInvalidException. The raising of this exception, however, was done only
in pullrequest context, while it could have been done in
BaseModel._get_user().
Why not be more consistent and do it in _get_instance (using a more
generic EntityNotFound exception (or something like that)? Is that the
end goal and this just an intermediate step?
Note: the same 'use-exceptions-rather-than-explicit-checks-for-None'
approach could actually be introduced for the other _get_X methods in
model, like _get_repo.
Why do you think exceptions are better than None? Is it often one extra
line of code and usually slower (not that matters in most of these
cases). (I might agree but it would be nice to have reasoning/assumption
stated explicitly.)
diff --git a/kallithea/controllers/api/api.py b/kallithea/controllers/api/api.py
--- a/kallithea/controllers/api/api.py
+++ b/kallithea/controllers/api/api.py
@@ -37,6 +37,7 @@ from kallithea.lib.auth import (
PasswordGenerator, AuthUser, HasPermissionAllDecorator,
HasPermissionAnyDecorator, HasPermissionAnyApi, HasRepoPermissionAnyApi,
HasRepoGroupPermissionAnyApi, HasUserGroupPermissionAny)
+from kallithea.lib.exceptions import UserInvalidException
from kallithea.lib.utils import map_groups, repo2db_mapper
from kallithea.lib.utils2 import (
str2bool, time_to_datetime, safe_int, Optional, OAttr)
@@ -72,8 +73,9 @@ def get_user_or_error(userid):
:param userid:
"""
- user = UserModel().get_user(userid)
- if user is None:
+ try:
+ user = UserModel().get_user(userid)
+ except UserInvalidException, u:
raise JSONRPCError("user `%s` does not exist" % (userid,))
return user
diff --git a/kallithea/lib/base.py b/kallithea/lib/base.py
--- a/kallithea/lib/base.py
+++ b/kallithea/lib/base.py
@@ -50,7 +50,7 @@ from kallithea.lib.utils2 import str2boo
from kallithea.lib import auth_modules
from kallithea.lib.auth import AuthUser, HasPermissionAnyMiddleware,
CookieStoreWrapper
from kallithea.lib.utils import get_repo_slug
-from kallithea.lib.exceptions import UserCreationError
+from kallithea.lib.exceptions import UserCreationError, UserInvalidException
from kallithea.lib.vcs.exceptions import RepositoryError,
EmptyRepositoryError, ChangesetDoesNotExistError
from kallithea.model import meta
@@ -331,12 +331,16 @@ class BaseController(WSGIController):
c.repo_name = get_repo_slug(request) # can be empty
c.backends = BACKENDS.keys()
- c.unread_notifications = NotificationModel()\
- .get_unread_cnt_for_user(c.authuser.user_id)
self.cut_off_limit = safe_int(config.get('cut_off_limit'))
- c.my_pr_count = PullRequestModel().get_pullrequest_cnt_for_user(c.authuser.user_id)
+ try:
+ c.unread_notifications = NotificationModel()\
+ .get_unread_cnt_for_user(c.authuser.user_id)
+ c.my_pr_count =
PullRequestModel().get_pullrequest_cnt_for_user(c.authuser.user_id)
+ except UserInvalidException, u:
+ c.unread_notifications = 0
+ c.my_pr_count = 0
It seems like NotificationModel already quietly ignored invalid user
IDs. That seems weird and wrong ... but perhaps a consequence of the
"feature" of being able to delete users? It seems like the weirdness
propagates a bit here ...
/Mads
self.sa = meta.Session
self.scm_model = ScmModel(self.sa)
diff --git a/kallithea/model/__init__.py b/kallithea/model/__init__.py
--- a/kallithea/model/__init__.py
+++ b/kallithea/model/__init__.py
@@ -46,6 +46,7 @@ Original author and date, and relevant c
import logging
from kallithea.model import meta
+from kallithea.lib.exceptions import UserInvalidException
from kallithea.lib.utils2 import safe_str, obfuscate_url_pw
log = logging.getLogger(__name__)
@@ -110,8 +111,11 @@ class BaseModel(object):
:param user: UserID, username, or User instance
"""
from kallithea.model.db import User
- return self._get_instance(User, user,
- callback=User.get_by_username)
+ u = self._get_instance(User, user,
+ callback=User.get_by_username)
+ if u is None:
+ raise UserInvalidException(user)
+ return u
def _get_repo(self, repository):
"""
diff --git a/kallithea/model/notification.py b/kallithea/model/notification.py
--- a/kallithea/model/notification.py
+++ b/kallithea/model/notification.py
@@ -35,6 +35,7 @@ from sqlalchemy.orm import joinedload, s
import kallithea
from kallithea.lib import helpers as h
+from kallithea.lib.exceptions import UserInvalidException
from kallithea.lib.utils2 import safe_unicode
from kallithea.model import BaseModel
from kallithea.model.db import Notification, User, UserNotification
@@ -84,10 +85,10 @@ class NotificationModel(BaseModel):
recipients_objs = []
if recipients:
for u in recipients:
- obj = self._get_user(u)
- if obj:
+ try:
+ obj = self._get_user(u)
recipients_objs.append(obj)
- else:
+ except UserInvalidException:
# TODO: inform user that requested operation couldn't be
completed
log.error('cannot email unknown user %r', u)
recipients_objs = set(recipients_objs)
@@ -154,7 +155,7 @@ class NotificationModel(BaseModel):
try:
notification = self.__get_notification(notification)
user = self._get_user(user)
- if notification and user:
+ if notification:
obj = UserNotification.query()\
.filter(UserNotification.user == user)\
.filter(UserNotification.notification
@@ -192,7 +193,7 @@ class NotificationModel(BaseModel):
try:
notification = self.__get_notification(notification)
user = self._get_user(user)
- if notification and user:
+ if notification:
obj = UserNotification.query()\
.filter(UserNotification.user == user)\
.filter(UserNotification.notification
diff --git a/kallithea/model/pull_request.py b/kallithea/model/pull_request.py
--- a/kallithea/model/pull_request.py
+++ b/kallithea/model/pull_request.py
@@ -32,7 +32,6 @@ from pylons.i18n.translation import _
from kallithea.model.meta import Session
from kallithea.lib import helpers as h
-from kallithea.lib.exceptions import UserInvalidException
from kallithea.model import BaseModel
from kallithea.model.db import PullRequest, PullRequestReviewers,
Notification,\
ChangesetStatus, User
@@ -119,8 +118,6 @@ class PullRequestModel(BaseModel):
#members
for member in set(reviewers):
_usr = self._get_user(member)
- if _usr is None:
- raise UserInvalidException(member)
reviewer = PullRequestReviewers(_usr, pr)
Session().add(reviewer)
_______________________________________________
kallithea-general mailing list
[email protected]
http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
