Merge lp:~robru/gwibber/locked-login-refactor into lp:~barry/gwibber/py3

[Robert Bruce Park]

Barry, I split this into a separate mp just for you. No rush, Facebook won't be ready today anyway. ;-)

[Barry Warsaw (barry)]

On Oct 09, 2012, at 08:03 PM, Robert Bruce Park wrote:

>Barry, I split this into a separate mp just for you. No rush, Facebook won't
>be ready today anyway. ;-)

This looks great, and I'm ready to merge it, but I have one question.

> - data = get_json(SELF_URL.format(access_token=token)) or {}

I see this pattern a lot, but I wonder why. Is it expected that get_json()
would return a false value that *wasn't* an empty dictionary?

I'm very tempted to take out the "or {}" unless there's a concrete reason
to add it back (read: failing test :).

[Robert Bruce Park (robru)]

On 12-10-10 09:38 AM, Barry Warsaw wrote:
> This looks great, and I'm ready to merge it, but I have one question.
>
>> - data = get_json(SELF_URL.format(access_token=token)) or {}
>
> I see this pattern a lot, but I wonder why. Is it expected that get_json()
> would return a false value that *wasn't* an empty dictionary?

Well, just like the millions of places where I write .get('foo') instead
of ['foo'], it's written that way to deliberately be fault-tolerant
against *unknown* error conditions.

> I'm very tempted to take out the "or {}" unless there's a concrete reason
> to add it back (read: failing test :).

Well, if you want to make the code *more* brittle, I think it's up to
you to point at some API reference documentation that indicates that the
return value will *always* be some JSON dict regardless of any error
condition that might arise. I think the onus is on you to prove that
your change is safe, not on me to prove that this clearly-harmless edge
case won't save our butts at some point.

[Barry Warsaw (barry)]

On Oct 10, 2012, at 04:34 PM, Robert Bruce Park wrote:

>On 12-10-10 09:38 AM, Barry Warsaw wrote:
>> This looks great, and I'm ready to merge it, but I have one question.
>>
>>> - data = get_json(SELF_URL.format(access_token=token)) or {}
>>
>> I see this pattern a lot, but I wonder why. Is it expected that get_json()
>> would return a false value that *wasn't* an empty dictionary?
>
>Well, just like the millions of places where I write .get('foo') instead
>of ['foo'], it's written that way to deliberately be fault-tolerant
>against *unknown* error conditions.

That's different. In this latter case, you can be sure of the data type of
object you're accessing, but you can't be sure of the contents of the keys.
Of course, there are infinite number of unknown error conditions, so you can't
possibly defend against them all. Defending against missing keys is a
reasonable trade-off IMHO.

>> I'm very tempted to take out the "or {}" unless there's a concrete reason
>> to add it back (read: failing test :).
>
>Well, if you want to make the code *more* brittle, I think it's up to
>you to point at some API reference documentation that indicates that the
>return value will *always* be some JSON dict regardless of any error
>condition that might arise. I think the onus is on you to prove that
>your change is safe, not on me to prove that this clearly-harmless edge
>case won't save our butts at some point.

I think that's akin to trying to prove a negative. Taken to the extreme,
potentially any data type or value could be returned from get_json(), but it's
impossible to defend against them. As an example, it's probably entirely
possible that a non-empty list, or the integer 7 is returned from get_json(),
and in either of those cases, the original code would *still* fail because the
'or' clause wouldn't get triggered. It's only in the narrow case of a
non-true value (e.g. 0 or None, or the empty string) that the 'or' case would
get triggered.

In situations like this, I prefer to react to bugs so that we know exactly
what situation can cause the non-true return value to occur. In any event, a
test case would be useful.

You could argue that maybe in the .get() vs [] situation above we should do
the same - wait until a real-world bug arrives before we write defensive
code. That could be convincing, but the cost of adding defensive code in
those cases is so much higher because of the number of call sites that would
have to be fixed, so it seems like a better trade-off.
y

[Robert Bruce Park (robru)]

On 12-10-10 12:40 PM, Barry Warsaw wrote:
> On Oct 10, 2012, at 04:34 PM, Robert Bruce Park wrote:
>> Well, if you want to make the code *more* brittle, I think it's up to
>> you to point at some API reference documentation that indicates that the
>> return value will *always* be some JSON dict regardless of any error
>> condition that might arise. I think the onus is on you to prove that
>> your change is safe, not on me to prove that this clearly-harmless edge
>> case won't save our butts at some point.
>
> I think that's akin to trying to prove a negative. Taken to the extreme,
> potentially any data type or value could be returned from get_json(), but it's
> impossible to defend against them. As an example, it's probably entirely
> possible that a non-empty list, or the integer 7 is returned from get_json(),
> and in either of those cases, the original code would *still* fail because the
> 'or' clause wouldn't get triggered. It's only in the narrow case of a
> non-true value (e.g. 0 or None, or the empty string) that the 'or' case would
> get triggered.

Except that some unforeseen error condition that prevents the dict from
being returned is *much* more likely to return None instead of the
integer 7 or a non-empty list, because None is what you get when you
call return with no values. So I don't think a simple test against the
false-ness of the return value is particularly unreasonable, especially
when it is only 5 bytes of source code.

> In situations like this, I prefer to react to bugs so that we know exactly
> what situation can cause the non-true return value to occur. In any event, a
> test case would be useful.

Ok, but you made the change, so you write the test :-P

[Barry Warsaw (barry)]

On Oct 10, 2012, at 06:40 PM, Robert Bruce Park wrote:

>Except that some unforeseen error condition that prevents the dict from
>being returned is *much* more likely to return None instead of the
>integer 7 or a non-empty list, because None is what you get when you
>call return with no values.

Except there's only one way out of get_json() and that's through json.loads().
So you *could* get None back, but you'd have to have gotten a response of just
the string 'null', since that's the only thing that results in None by
json.loads().

Yes, that's an error condition, but again, it's unlikely and you can't defend
against every possible response from a service.

>So I don't think a simple test against the false-ness of the return value is
>particularly unreasonable, especially when it is only 5 bytes of source code.
>
>> In situations like this, I prefer to react to bugs so that we know exactly
>> what situation can cause the non-true return value to occur. In any event,
>> a test case would be useful.
>
>Ok, but you made the change, so you write the test :-P

But in this case, there's nothing more to test. :)

=== modified file 'gwibber/gwibber/protocols/flickr.py'
--- gwibber/gwibber/protocols/flickr.py	2012-09-25 20:25:42 +0000
+++ gwibber/gwibber/protocols/flickr.py	2012-10-09 20:02:23 +0000
@@ -22,7 +22,6 @@
 import logging
 
 from gwibber.errors import AuthorizationError
-from gwibber.utils.authentication import Authentication
 from gwibber.utils.base import Base, feature
 from gwibber.utils.download import get_json
 from gwibber.utils.time import iso8601utc, parsetime
@@ -59,22 +58,11 @@
                 return None
         return self._account.user_id
 
-    def _locked_login(self, old_token):
-        if old_token is None:
-            log.debug('Logging into Flickr')
-        else:
-            log.debug('Re-authenticating to Flickr')
-        result = Authentication(self._account).login()
-        if result is None:
-            log.error('No Flickr authentication results received')
-        elif 'AccessToken' in result:
-            self._account.user_name = result.get('username')
-            self._account.user_id = result.get('user_nsid')
-            self._account.access_token = result.get('AccessToken')
-            self._account.secret_token = result.get('TokenSecret')
-            log.debug('Flickr AccessToken received')
-        else:
-            log.error('No AccessToken in Flickr session: {!r}'.format(result))
+    def _whoami(self, authdata):
+        """Identify the authenticating user."""
+        self._account.secret_token = authdata.get('TokenSecret')
+        self._account.user_id = authdata.get('user_nsid')
+        self._account.user_name = authdata.get('username')
 
 # http://www.flickr.com/services/api/flickr.photos.getContactsPublicPhotos.html
     @feature
 
=== modified file 'gwibber/gwibber/protocols/foursquare.py'
--- gwibber/gwibber/protocols/foursquare.py	2012-09-25 20:25:42 +0000
+++ gwibber/gwibber/protocols/foursquare.py	2012-10-09 20:02:23 +0000
@@ -22,7 +22,6 @@
 import logging
 
 from gwibber.errors import AuthorizationError
-from gwibber.utils.authentication import Authentication
 from gwibber.utils.base import Base, feature
 from gwibber.utils.download import get_json
 from gwibber.utils.time import iso8601utc
@@ -55,26 +54,14 @@
 
 
 class FourSquare(Base):
-    def _locked_login(self, old_token):
-        log.debug('{} to FourSquare'.format(
-            'Re-authenticating' if old_token else 'Logging in'))
-
-        result = Authentication(self._account, log).login()
-        if result is None:
-            log.error('No FourSquare authentication results received.')
-            return
-
-        token = result.get('AccessToken')
-        if token is None:
-            log.error('No AccessToken in FourSquare session: {!r}', result)
-        else:
-            data = get_json(SELF_URL.format(access_token=token)) or {}
-            user = data.get('response', {}).get('user', {})
-            uid = user['id']
-            self._account.access_token = token
-            self._account.user_name = _full_name(user)
-            self._account.user_id = uid
-            log.debug('FourSquare UID is: {}', uid)
+    def _whoami(self, authdata):
+        """Identify the authenticating user."""
+        data = get_json(SELF_URL.format(
+                access_token=self._account.access_token)) or {}
+        user = data.get('response', {}).get('user', {})
+        self._account.secret_token = authdata.get('TokenSecret')
+        self._account.user_name = _full_name(user)
+        self._account.user_id = user.get('id')
 
     @feature
     def receive(self):
 
=== modified file 'gwibber/gwibber/protocols/twitter.py'
--- gwibber/gwibber/protocols/twitter.py	2012-10-05 17:58:23 +0000
+++ gwibber/gwibber/protocols/twitter.py	2012-10-09 20:02:23 +0000
@@ -28,7 +28,6 @@
 from urllib.error import HTTPError
 from urllib.parse import quote, urlsplit, urlunsplit
 
-from gwibber.utils.authentication import Authentication
 from gwibber.utils.base import Base, feature
 from gwibber.utils.download import RateLimiter as BaseRateLimiter, get_json
 from gwibber.utils.time import parsetime, iso8601utc
@@ -65,23 +64,11 @@
         super().__init__(account)
         self._rate_limiter = RateLimiter()
 
-    def _locked_login(self, old_token):
-        """Sign in without worrying about concurrent login attempts."""
-        result = Authentication(self._account, log).login()
-        if result is None:
-            log.error('No Twitter authentication results received.')
-            return
-
-        token = result.get('AccessToken')
-        if token is None:
-            log.error('No AccessToken in Twitter session: {!r}', result)
-        else:
-            self._account.access_token = token
-            self._account.secret_token = result.get('TokenSecret')
-            self._account.user_id = result.get('UserId')
-            self._account.user_name = result.get('ScreenName')
-            log.debug('{} UID: {}'.format(self.__class__.__name__,
-                                          self._account.user_id))
+    def _whoami(self, authdata):
+        """Identify the authenticating user."""
+        self._account.secret_token = authdata.get('TokenSecret')
+        self._account.user_id = authdata.get('UserId')
+        self._account.user_name = authdata.get('ScreenName')
 
     def _get_url(self, url, data=None):
         """Access the Twitter API with correct OAuth signed headers."""
 
=== modified file 'gwibber/gwibber/tests/test_flickr.py'
--- gwibber/gwibber/tests/test_flickr.py	2012-09-25 20:25:42 +0000
+++ gwibber/gwibber/tests/test_flickr.py	2012-10-09 20:02:23 +0000
@@ -163,7 +163,7 @@
         # AccessToken, but this fails.
         self.protocol('receive')
         self.assertEqual(self.log_mock.empty(), """\
-No Flickr authentication results received
+No Flickr authentication results received.
 Flickr: No NSID available
 Gwibber operation exception:
  Traceback (most recent call last):
 
=== modified file 'gwibber/gwibber/utils/base.py'
--- gwibber/gwibber/utils/base.py	2012-10-04 19:29:35 +0000
+++ gwibber/gwibber/utils/base.py	2012-10-09 20:02:23 +0000
@@ -26,6 +26,7 @@
 import logging
 import threading
 
+from gwibber.utils.authentication import Authentication
 from gwibber.utils.model import COLUMN_INDICES, SCHEMA, DEFAULTS, Model
 
 
@@ -289,11 +290,28 @@
             return self._account.access_token != old_token
 
     def _locked_login(self, old_token):
-        """Login operation stub. This must be implemented by subclasses."""
-        raise NotImplementedError
+        """Sign in without worrying about concurrent login attempts."""
+        cls = self.__class__.__name__
+
+        log.debug('{} to {}'.format(
+                'Re-authenticating' if old_token else 'Logging in', cls))
+
+        result = Authentication(self._account, log).login()
+        if result is None:
+            log.error('No {} authentication results received.'.format(cls))
+            return
+
+        token = result.get('AccessToken')
+        if token is None:
+            log.error('No AccessToken in {} session: {!r}'.format(cls, result))
+        else:
+            self._account.access_token = token
+            self._whoami(result)
+            log.debug('{} UID: {}'.format(cls, self._account.user_id))
 
     @classmethod
     def get_features(cls):
+        """Report what public operations we expose over DBus."""
         features = []
         for name in dir(cls):
             if getattr(getattr(cls, name), 'is_feature', False):