[PATCH 1 of 2] model: check for invalid users centrally
mads at kiilerich.com
Tue Jun 23 09:59:42 EDT 2015
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
>>> in pullrequest context, while it could have been done in
>> 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
> 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
>>> --- 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,
>>> 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,
>>> from kallithea.lib.utils import get_repo_slug
>>> -from kallithea.lib.exceptions import UserCreationError
>>> +from kallithea.lib.exceptions import UserCreationError,
>>> 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 =
>>> + try:
>>> + c.unread_notifications = NotificationModel()\
>>> + .get_unread_cnt_for_user(c.authuser.user_id)
>>> + c.my_pr_count =
>>> + 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.
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.
More information about the kallithea-general