Bug 28230 - Should query bare JID to check if PEP is supported
Should query bare JID to check if PEP is supported
Status: RESOLVED FIXED
Product: Telepathy
Classification: Unclassified
Component: gabble
git master
Other All
: medium normal
Assigned To: Guillaume Desmottes
Telepathy bugs list
http://git.collabora.co.uk/?p=user/ca...
review+
: patch
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-05-24 06:09 UTC by Guillaume Desmottes
Modified: 2010-06-09 02:39 UTC (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Guillaume Desmottes 2010-05-24 06:09:38 UTC
The PEP XEP says ( http://xmpp.org/extensions/xep-0163.html#support ):
"client SHOULD determine whether the account owner's server supports PEP; to do so, it MUST send a Service Discovery [19] information request to its own bare JID"

Currently Gabble doesn't do that but query the *server* features to check if PEP is supported.
This used to work fine but it seems that latest ejabberd doesn't advertise PEP in the server features any more.

We should then disco our own bare jid as well. This lead to a question: should we block the transition to the CONNECTED state until we received the reply of this disco (as we do for the reply of the server disco?).

If we don't then there is a risk of race: a client could try to use a PEP feature and wrongly assume that the server doesn't support PEP because it hasn't been discovered yet.
Also, if we don't we can't add OLPC specific interfaces once we discovered PEP because we can't change interfaces of a connected connection.
On the other hand, we could say that we don't care because those interfaces are about to die. Another solution could be to always claim that we support those interfaces because it can still make sense to get OLPC info from your contacts even if you can't publish them yourself (that's what we do with the location interface).
Comment 1 Guillaume Desmottes 2010-05-24 06:23:49 UTC
Here is a start of a branch fixing that:
http://git.collabora.co.uk/?p=user/cassidy/telepathy-gabble;a=shortlog;h=refs/heads/location

It doesn't block the transition state and doesn't add OLPC interfaces for now.

Note that with this branch it still don't work with my server :(
According to the XEP, the server is supposed to return:
    <identity category='pubsub' type='pep'/>
but I receive instead:
    <identity category='pubsub' type='service'/>

I guess that's an ejabberd bug so I opened https://support.process-one.net/browse/EJAB-1238
Comment 2 Will Thompson 2010-05-28 04:50:34 UTC
(In reply to comment #1)
> Here is a start of a branch fixing that:
> http://git.collabora.co.uk/?p=user/cassidy/telepathy-gabble;a=shortlog;h=refs/heads/location
> 
> It doesn't block the transition state and doesn't add OLPC interfaces for now.
> 
> Note that with this branch it still don't work with my server :(
> According to the XEP, the server is supposed to return:
>     <identity category='pubsub' type='pep'/>
> but I receive instead:
>     <identity category='pubsub' type='service'/>
> 
> I guess that's an ejabberd bug so I opened
> https://support.process-one.net/browse/EJAB-1238

+  if (!_gabble_connection_send_with_reply (conn, msg, set_location_sent_cb,
+        G_OBJECT (conn), NULL, NULL))

if you pass NULL as the cb to send_with_reply it squashes the unhandled IQ warning without needing a no-op function.

Given that you do actually use the callback in the next commit, just squash those two commits together?

Any particular reason for the bonus underscore in bare_jid__disco_cb ?

MyXmppStream is a terrible name.

+    try:
+        conn.Location.SetLocation({'lat': 0.0, 'lon': 0.0})
+    except dbus.DBusException:
+        pass
+    else:
+        assert False, "Should have had an error!"

call_async(...)
q.expect('dbus-error', error_name=cs.NOT_IMPLEMENTED)

would be clearer and test more and be shorter.

+    _, _, e = q.expect_many(
+        EventPattern('stream-iq', iq_type='set',
+            query_ns=ns.PUBSUB),
+        EventPattern('dbus-signal', signal='StatusChanged',
+            args=[cs.CONN_STATUS_CONNECTED, cs.CSR_REQUESTED]),
+        EventPattern('stream-iq', iq_type='get', to='test@localhost',
+            query_ns=ns.DISCO_INFO)
+        )

We only care about the last one, so why expect the others? (Particularly given that we shouldn't move to Connected before discoing ourself: see below.)

(In reply to comment #0)
> We should then disco our own bare jid as well. This lead to a question: should
> we block the transition to the CONNECTED state until we received the reply of
> this disco (as we do for the reply of the server disco?).

Yes we should. It's not just the OLPC interfaces that depend on this: we recently added a property to the Location interface specifying whether or not SetLocation is supported (http://telepathy.freedesktop.org/spec/org.freedesktop.Telepathy.Connection.Interface.Location.html#org.freedesktop.Telepathy.Connection.Interface.Location.SupportedLocationFeatures). It's not implemented yet, but in order to implement it correctly we need to wait until we've discoed ourself.
Comment 3 Guillaume Desmottes 2010-05-28 06:15:54 UTC
(In reply to comment #2)
> (In reply to comment #1)
> > Here is a start of a branch fixing that:
> > http://git.collabora.co.uk/?p=user/cassidy/telepathy-gabble;a=shortlog;h=refs/heads/location
> > 
> > It doesn't block the transition state and doesn't add OLPC interfaces for now.
> > 
> > Note that with this branch it still don't work with my server :(
> > According to the XEP, the server is supposed to return:
> >     <identity category='pubsub' type='pep'/>
> > but I receive instead:
> >     <identity category='pubsub' type='service'/>
> > 
> > I guess that's an ejabberd bug so I opened
> > https://support.process-one.net/browse/EJAB-1238
> 
> +  if (!_gabble_connection_send_with_reply (conn, msg, set_location_sent_cb,
> +        G_OBJECT (conn), NULL, NULL))
> 
> if you pass NULL as the cb to send_with_reply it squashes the unhandled IQ
> warning without needing a no-op function.
> 
> Given that you do actually use the callback in the next commit, just squash
> those two commits together?

squased.

> Any particular reason for the bonus underscore in bare_jid__disco_cb ?

oops. Fixed.

> MyXmppStream is a terrible name.

renamed.

> +    try:
> +        conn.Location.SetLocation({'lat': 0.0, 'lon': 0.0})
> +    except dbus.DBusException:
> +        pass
> +    else:
> +        assert False, "Should have had an error!"
> 
> call_async(...)
> q.expect('dbus-error', error_name=cs.NOT_IMPLEMENTED)

Oh didn't know (I didn't gabble since a while). Done.

> would be clearer and test more and be shorter.
> 
> +    _, _, e = q.expect_many(
> +        EventPattern('stream-iq', iq_type='set',
> +            query_ns=ns.PUBSUB),
> +        EventPattern('dbus-signal', signal='StatusChanged',
> +            args=[cs.CONN_STATUS_CONNECTED, cs.CSR_REQUESTED]),
> +        EventPattern('stream-iq', iq_type='get', to='test@localhost',
> +            query_ns=ns.DISCO_INFO)
> +        )
> 
> We only care about the last one, so why expect the others? (Particularly given
> that we shouldn't move to Connected before discoing ourself: see below.)
> 
> (In reply to comment #0)
> > We should then disco our own bare jid as well. This lead to a question: should
> > we block the transition to the CONNECTED state until we received the reply of
> > this disco (as we do for the reply of the server disco?).
> 
> Yes we should. It's not just the OLPC interfaces that depend on this: we
> recently added a property to the Location interface specifying whether or not
> SetLocation is supported
> (http://telepathy.freedesktop.org/spec/org.freedesktop.Telepathy.Connection.Interface.Location.html#org.freedesktop.Telepathy.Connection.Interface.Location.SupportedLocationFeatures).
> It's not implemented yet, but in order to implement it correctly we need to
> wait until we've discoed ourself.

Good point. I've done that.
Comment 4 Simon McVittie 2010-06-08 04:27:40 UTC
> +  /* FIXME: add OLPC iface if PEP is supported? */

Please do :-)

> +              DEBUG ("Server advertise PEP support in our jid features");

"Server advertises PEP ..."

> +/**
> + * connection_disco_cb
> + *
> + * Stage 3 of connecting
...
> + */
>  static void
>  set_status_to_connected (GabbleConnection *conn)

Wrong function name in the doc-comment

> +class PEPinJidDiscoXmppStream(BaseXmlStream):

Please be consistent about whether you capitalize acronyms in camel-case or not, at least within a class's name (PEPInJIDDiscoXMPPStream vs. PepInJidDiscoXmppStream).

I'd be inclined to change the behaviour of BaseXmlStream to be correct (like PEPinJidDiscoXmppStream is now), and make the special stream class in pep-support.py behave like the old BaseXmlStream - call it PepInServerDiscoXmlStream, or OldEjabberdXmlStream, or something. I think it's better if most of our tests are realistic, and legacy behaviour is the special case.
Comment 5 Simon McVittie 2010-06-08 04:28:23 UTC
(In reply to comment #4)
> > +  /* FIXME: add OLPC iface if PEP is supported? */
> 
> Please do :-)

Oh, you did later. Ignore that one.
Comment 6 Guillaume Desmottes 2010-06-09 02:35:22 UTC
I fixed all your comments:
http://git.collabora.co.uk/?p=user/cassidy/telepathy-gabble;a=shortlog;h=refs/heads/location
Comment 7 Guillaume Desmottes 2010-06-09 02:39:50 UTC
Merged to master.