Determining subscribers

Determining subscribers

[Thomas.J.Waldmann, 2013-07-14 08:47:13]

https://codereview.appspot.com/10950045/diff/1/MoinMoin/util/subscriptions.py
File MoinMoin/util/subscriptions.py (right):

https://codereview.appspot.com/10950045/diff/1/MoinMoin/util/subscriptions.py...
MoinMoin/util/subscriptions.py:42: pattern_results = list()
results = []

https://codereview.appspot.com/10950045/diff/1/MoinMoin/util/subscriptions.py...
MoinMoin/util/subscriptions.py:54: :param user_items: an iterable that contains
user profile objects
they are rather user documents from the whoosh index, right?

https://codereview.appspot.com/10950045/diff/1/MoinMoin/util/subscriptions.py...
MoinMoin/util/subscriptions.py:55: :return: a set of users
a set of Subscriber objects

https://codereview.appspot.com/10950045/diff/1/MoinMoin/util/subscriptions.py...
MoinMoin/util/subscriptions.py:57: # store just the first name
# note: simply use the first one of all user names for now

https://codereview.appspot.com/10950045/diff/1/MoinMoin/util/subscriptions.py...
MoinMoin/util/subscriptions.py:71: item_namespace = item.meta[NAMESPACE]
meta = ...
namespace = ...

https://codereview.appspot.com/10950045/diff/1/MoinMoin/util/subscriptions.py...
MoinMoin/util/subscriptions.py:74: if subscription.startswith(NAME) or
subscription.startswith(NAMERE):
can it start with NAME?

also, be careful, you rather should check for starting with x + ':' or use
split.

https://codereview.appspot.com/10950045/diff/1/MoinMoin/util/subscriptions.py...
MoinMoin/util/subscriptions.py:78: if [name for name in item.meta[NAME] if
name.startswith(pattern)]:
if any(name.startswith(pattern) for name in meta[NAME]):

https://codereview.appspot.com/10950045/diff/1/MoinMoin/util/subscriptions.py...
MoinMoin/util/subscriptions.py:85: if [name for name in item.meta[NAME] if
pattern.search(name)]:
if any(pattern.search(name) for name in meta[NAME]):

and btw, it even short-circuits. :)

[Thomas.J.Waldmann, 2013-07-17 13:14:07]

https://codereview.appspot.com/10950045/diff/5001/MoinMoin/util/_tests/test_s...
File MoinMoin/util/_tests/test_subscriptions.py (right):

https://codereview.appspot.com/10950045/diff/5001/MoinMoin/util/_tests/test_s...
MoinMoin/util/_tests/test_subscriptions.py:39: meta = dict(locale=u'en')
hmm, why first build a dict ...

https://codereview.appspot.com/10950045/diff/5001/MoinMoin/util/_tests/test_s...
MoinMoin/util/_tests/test_subscriptions.py:40: user.create_user(username=name,
password=password, email=email, validate=False, **meta)
... and then explode it?

you can just give locale=u'en' here.

having such values in a separate variable only make sense if you need to test
against that value later in the test.

https://codereview.appspot.com/10950045/diff/5001/MoinMoin/util/_tests/test_s...
MoinMoin/util/_tests/test_subscriptions.py:59: expected_name =
user_.profile._meta[NAME][0]
don't we have user.name(s) for this?

[Thomas.J.Waldmann, 2013-07-17 13:48:34]

https://codereview.appspot.com/10950045/diff/19001/MoinMoin/util/_tests/test_...
File MoinMoin/util/_tests/test_subscriptions.py (right):

https://codereview.appspot.com/10950045/diff/19001/MoinMoin/util/_tests/test_...
MoinMoin/util/_tests/test_subscriptions.py:60: assert expected_name in
subscribers_names
shouldn't it be even

[expected_name] == subscribers_names

?

https://codereview.appspot.com/10950045/diff/19001/MoinMoin/util/_tests/test_...
MoinMoin/util/_tests/test_subscriptions.py:70: meta = dict(locale=u'en')
see the other place where you already fixed this

https://codereview.appspot.com/10950045/diff/19001/MoinMoin/util/_tests/test_...
MoinMoin/util/_tests/test_subscriptions.py:81: users_itemids = set([u.name for u
in users])
rather looks like user_names not users_itemids.

https://codereview.appspot.com/10950045/diff/19001/MoinMoin/util/_tests/test_...
MoinMoin/util/_tests/test_subscriptions.py:83: expected_users_names =
set([u.name0 for u in expected_users])
expected_user_names = set(u.name0 for u in [u1, u2, ])

(and kill the line above)

https://codereview.appspot.com/10950045/diff/19001/MoinMoin/util/subscription...
File MoinMoin/util/subscriptions.py (right):

https://codereview.appspot.com/10950045/diff/19001/MoinMoin/util/subscription...
MoinMoin/util/subscriptions.py:44: # cast the generator object to list and
append elements
is this comment helpful?

https://codereview.appspot.com/10950045/diff/19001/MoinMoin/util/subscription...
MoinMoin/util/subscriptions.py:50: def extract_users_info(user_items):
check if you call this more than once.

if not: is 1 line of rather trivial code enough justify an own function?

[ana.balica, 2013-07-17 15:17:44]

https://codereview.appspot.com/10950045/diff/19001/MoinMoin/util/subscription...
File MoinMoin/util/subscriptions.py (right):

https://codereview.appspot.com/10950045/diff/19001/MoinMoin/util/subscription...
MoinMoin/util/subscriptions.py:50: def extract_users_info(user_items):
On 2013/07/17 13:48:35, Thomas.J.Waldmann wrote:
> check if you call this more than once.
> 
> if not: is 1 line of rather trivial code enough justify an own function?

The function is called twice. But I am going to change get_subscribers() such as
to be called only once and therefore due to the only usage, function
extract_users_info() will be no longer be needed.

[Thomas.J.Waldmann, 2013-07-19 10:56:55]

https://codereview.appspot.com/10950045/diff/36001/MoinMoin/util/subscription...
File MoinMoin/util/subscriptions.py (right):

https://codereview.appspot.com/10950045/diff/36001/MoinMoin/util/subscription...
MoinMoin/util/subscriptions.py:38: with
flaskg.storage.indexer.ix[LATEST_REVS].searcher() as searcher:
add here:

# TODO: check ACL behaviour - is this avoiding the protection layer?

https://codereview.appspot.com/10950045/diff/36001/MoinMoin/util/subscription...
MoinMoin/util/subscriptions.py:39: results = searcher.search(query, limit=None)
result_iterators = [searcher...., ]

https://codereview.appspot.com/10950045/diff/36001/MoinMoin/util/subscription...
MoinMoin/util/subscriptions.py:42: documents =
[searcher.documents(subscription_patterns=pattern) for pattern in patterns]
result_iterators += [searcher.... for ...]

using that construct, you need one chain operation less.

https://codereview.appspot.com/10950045/diff/36001/MoinMoin/util/subscription...
MoinMoin/util/subscriptions.py:43: user_documents =
list(chain.from_iterable(documents))
why are you making a list out of the chain of iterators?

https://codereview.appspot.com/10950045/diff/36001/MoinMoin/util/subscription...
MoinMoin/util/subscriptions.py:44: results = list(chain(results,
user_documents))
same question

https://codereview.appspot.com/10950045/diff/36001/MoinMoin/util/subscription...
MoinMoin/util/subscriptions.py:46: for user in results}
... for user in chain.from_iterable(result_iterators)

https://codereview.appspot.com/10950045/diff/36001/MoinMoin/util/subscription...
MoinMoin/util/subscriptions.py:70: except re.error:
do you try to compile the regex in the validator to avoid that this happens
here?

[ana.balica, 2013-07-19 12:31:12]

https://codereview.appspot.com/10950045/diff/41001/MoinMoin/util/subscription...
File MoinMoin/util/subscriptions.py (right):

https://codereview.appspot.com/10950045/diff/41001/MoinMoin/util/subscription...
MoinMoin/util/subscriptions.py:67: pattern = re.compile(r'{0}'.format(pattern))
Assume the pattern will compile without errors, since it is checked for validity
in the validation middleware. The updated validator here:
https://codereview.appspot.com/11403044/

[waldi, 2013-07-19 12:45:16]

https://codereview.appspot.com/10950045/diff/41001/MoinMoin/util/subscription...
File MoinMoin/util/subscriptions.py (right):

https://codereview.appspot.com/10950045/diff/41001/MoinMoin/util/subscription...
MoinMoin/util/subscriptions.py:67: pattern = re.compile(r'{0}'.format(pattern))
On 2013/07/19 12:31:12, ana.balica wrote:
> Assume the pattern will compile without errors, since it is checked for
validity
> in the validation middleware. The updated validator here:
> https://codereview.appspot.com/11403044/

This is user-controlled input. If you assume this can never raise an exception,
document it in the code. Otherwise just catch and handle it.

In this case just check it.

[ana.balica, 2013-07-19 13:07:56]

https://codereview.appspot.com/10950045/diff/41001/MoinMoin/util/subscription...
File MoinMoin/util/subscriptions.py (right):

https://codereview.appspot.com/10950045/diff/41001/MoinMoin/util/subscription...
MoinMoin/util/subscriptions.py:67: pattern = re.compile(r'{0}'.format(pattern))
The user input will pass through validation and the validator's task is to catch
the exception. I am not sure, but I think the only way user input can surpass
that step is to edit manually the metadata in the filesystem.

[Thomas.J.Waldmann, 2013-07-20 12:29:31]

https://codereview.appspot.com/10950045/diff/41001/MoinMoin/util/subscription...
File MoinMoin/util/subscriptions.py (right):

https://codereview.appspot.com/10950045/diff/41001/MoinMoin/util/subscription...
MoinMoin/util/subscriptions.py:45: for user in
chain.from_iterable(result_iterators)}
looks much less complicated now :)

https://codereview.appspot.com/10950045/diff/41001/MoinMoin/util/subscription...
MoinMoin/util/subscriptions.py:67: pattern = re.compile(r'{0}'.format(pattern))
hmm, after thinking about it, I'ld say: catch the error and log it (together
with useful info for admin, so he can fix it).

even if the validator makes sure the user does not input invalid regexes, there
could be errors in filesystem or maybe some other way to input invalid regexes -
and we don't want to completely blow up while sending all notifications just
because of 1 bad regex.

[waldi, 2013-07-20 12:35:23]

On 2013/07/19 13:07:56, ana.balica wrote:
>
https://codereview.appspot.com/10950045/diff/41001/MoinMoin/util/subscription...
> File MoinMoin/util/subscriptions.py (right):
> 
>
https://codereview.appspot.com/10950045/diff/41001/MoinMoin/util/subscription...
> MoinMoin/util/subscriptions.py:67: pattern =
re.compile(r'{0}'.format(pattern))
> The user input will pass through validation and the validator's task is to
catch
> the exception. I am not sure, but I think the only way user input can surpass
> that step is to edit manually the metadata in the filesystem.

Please show proof that a regex that worked once (aka in the validator) will work
_any_ time in the future. This includes bugs in the lexer, differences between
python versions and maybe also overrun stack. (Hint: you can't.)

[Thomas.J.Waldmann, 2013-07-20 13:38:33]

ok