[PATCH 1 of 2] model: check for invalid users centrally

[Thomas De Schampheleire]

On Mon, Jun 22, 2015 at 7:05 PM, Mads Kiilerich <mads at kiilerich.com> wrote:
> On 06/22/2015 06:45 PM, Thomas De Schampheleire wrote:
>>
>> # HG changeset patch
>> # User Thomas De Schampheleire <thomas.de.schampheleire at gmail.com>
>> # 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?

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.

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

/Thomas