On 06/22/2015 10:09 PM, Thomas De Schampheleire wrote:
On Mon, Jun 22, 2015 at 7:05 PM, Mads Kiilerich <[email protected]> wrote:
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?
Would you translate the EntityNotFound in some other more specific
exception class, for example from _get_user, or would EntityNotFound
be the final exception?
I think that would be fine.
In some cases, there will be multiple users involved anyway and it will
not be possible to distinguish _which_ of them failed. The description
should of course explain what it was that failed.
This user part is an evolution from my initial introduction of
UserInvalidException, but as I hinted below I think it makes sense to
expand it. Doing it directly in get_instance is indeed more logical, I
can do that at the same time.
Ok - so you will rework this patch to do that?
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.)
One important reason why exceptions seem better to me in getObject(x),
is that you're sure that the possible nonexistence of the object will
not go unnoticed.
I agree that makes sense. That reasoning will be nice to have in the
commit message ... and perhaps also elsewhere, to establish a general
style in the code base.
When getObject(x) returns None but the caller does
not check the return value but simply start using it, at best the
caller itself will cause another exception because it accesses an
element of None. But worse, the caller may just need to pass the
object to another function, which also passes it through, and so on,
until at some point a real exception is thrown and the immediate cause
is no longer obvious.
With an exception, either the caller envisions a possible nonexistence
of the object and catches the exception, or the caller thinks/assumes
that the object does really exist -- the opposite will immediately
cause an exception and stop further code execution for that request.
The code below in lib/base.py, which is always executed even if there
is no authenticated user, is one example where Kallithea is apparently
juggling with invalid data. The exception code I added made the issue
clear in the tests.
Other reasons are that it can make the core logic more apparent
(moving out the exceptions a bit further down, although there are
advocates in making the try-block as short as possible and using the
else clause for the rest of the non-throwing code) and that it is more
pythonic ("Easier to Ask for Forgiveness than Permission." versus
"Look Before You Leap")
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 ...
You mean that this should be handled in get_unread_cnt_for_user and
get_pullrequest_cnt_for_user instead?
That makes sense.
If you mean something else, then please clarify. I don't think the
above changes propagates the weirdness _more_ than before, rather it
is the same amount of propagation.
Ok, agreed.
This new location for the weirdness might however deserve a comment,
admitting it is weird (if it is or why not), why it is there, and
perhaps what should be done to make it less weird.
/Mads
_______________________________________________
kallithea-general mailing list
[email protected]
http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
