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

[Mads Kiilerich]

On 06/22/2015 10:09 PM, Thomas De Schampheleire wrote:
> 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?

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