Opened 11 years ago

Closed 9 years ago

Last modified 9 years ago

#2178 enhancement closed fixed (fixed)

Generalize XMPP subprotocol containment

Reported by: ralphm Owned by:
Priority: high Milestone: Twisted-8.2
Component: words Keywords:
Cc: ralphm, Glyph, therve Branch: branches/xmpp-subprotocols-2178-5
branch-diff, diff-cov, branch-cov, buildbot
Author: ralphm

Description

In twisted.words.protocols.jabber.component an interface IService, and accompanying implementation Service are defined. Together with ServiceManager they allow for implementing a server side XMPP component as a set of subcomponents that each implement parts of the (sub)protocols the component understands.

Other XMPP entities (clients, servers) could also benefit from such an abstraction, so a more general form would be useful. Also, in relation to #1995, the question was raised if this should be derived from twisted.application.service.Service.

Change History (15)

comment:1 Changed 11 years ago by Glyph

The description of this ticket says "subprotocol", but then the classes involved are named "service". Given the confusion with other interfaces and classes named "service", maybe it would better be named something like "Subprotocol"? :)

I don't think that subclassing from t.a.a.S is a good idea, because their purposes seem completely different.

comment:2 Changed 11 years ago by ralphm

Cc: ralphm Glyph added

That was basically the point of the discussion. The way I use this functionality thought up by dizzyd doesn't really use the t.a.s.Service semantics.

What I do in my projects is split up the handling of the incoming and outgoing traffic over several objects. E.g. one that handles service discovery, one that handles presence and one that handles publish-subscribe. It is very nice to compose an XMPP entity in this way.

comment:3 Changed 11 years ago by ralphm

Milestone: Words-0.5

I've been experimenting with an implementation as part of my work on Mimír. It involves a StreamManager service that holds protocol handler objects. I called the interface IEnhancementProtocol, but I am contemplating IXMPPHandler.

The nice thing about this abstraction is that a StreamManager can be extended to something called XMPPClient or XMPPComponent.

I'll create a branch and move the experiment in there. It would be nice to have this in Words 0.5 (and therefore the Twisted 2.5 bundle?).

comment:4 Changed 9 years ago by ralphm

author: ralphm
Branch: branches/xmpp-subprotocols-2178-5

(In [23198]) Branching to 'xmpp-subprotocols-2178-5'

comment:5 Changed 9 years ago by ralphm

Keywords: review added
Owner: ralphm deleted
Priority: normalhighest

Finally submitting this for review, after a short excursion to Wokkel, with more to move to Words shortly.

comment:6 Changed 9 years ago by therve

Keywords: review removed
Owner: set to ralphm
Priority: highesthigh
  • StreamManager should call XMPPHandlerCollection.__init__, and not duplicate attributes
  • please use zope.interface.verify.verifyClass/verifyObject to check that the classes/objects correctly implements/provides the interfaces
  • the test methods names with 2 '_' is ugly, one is enough.
  • the logTraffic options of StreamManager is not tested
  • I don't see the point of the manager attribute of IXMPPHandler. Isn't that redundant with the parent attribute?
  • lots of undocumented methods in XMPPHandler. Generally it's just to explain why it's empty (not put redundant information with the interface)
  • Isn't the send method part of the interface?
  • 2 blanks lines between methods/3 blank lines at module level
  • please update copyright notice

Thanks!

comment:7 in reply to:  6 Changed 9 years ago by ralphm

Owner: changed from ralphm to therve

Replying to therve:

  • StreamManager should call XMPPHandlerCollection.__init__, and not duplicate attributes

Fixed.

  • please use zope.interface.verify.verifyClass/verifyObject to check that the classes/objects correctly implements/provides the interfaces

Where should these be used exactly?

  • the test methods names with 2 '_' is ugly, one is enough.
  • the logTraffic options of StreamManager is not tested

Fixed.

  • I don't see the point of the manager attribute of IXMPPHandler. Isn't that redundant with the parent attribute?

The parent attribute was called manager in previous incarnations. I just forgot to change the interface. Fixed.

  • lots of undocumented methods in XMPPHandler. Generally it's just to explain why it's empty (not put redundant information with the interface)

Should I just put a note to that effect in the class docstring?

  • Isn't the send method part of the interface?

I wasn't sure if all IXMPPHandlers should have to implement this method. So for now, I didn't want to make it part of the interface. Also, this method is usually only called from within the class.

  • 2 blanks lines between methods/3 blank lines at module level
  • please update copyright notice

Done.

Thanks!

Thank you!

comment:8 Changed 9 years ago by ralphm

Cc: therve added
Keywords: review added

comment:9 Changed 9 years ago by therve

Keywords: review removed
Owner: changed from therve to ralphm
  • You can see an example of verifyObject in twisted.test.test_process. It should be used in tests to check interfaces
  • DummyFactory and DummyXMPPHandler are missing documentation.
  • XMPPHandlerTest and StreamManagerTest are missing class docstrings
  • I don't know what's the best strategy to document XMPPHandler, but it's clearly missing some. I think the empty methods should say 'Called when XXX. Override in subclasses'.
  • In XMPPHandlerCollection addHandler and removeHandler, you still refer to the manager attribute
  • the _initialized of XMPPHandlerCollection is not documented
  • several undocumented method in StreamManager

Back to you.

comment:10 Changed 9 years ago by ralphm

Keywords: review added
Owner: changed from ralphm to therve

I addressed the above issues. Thanks for showing me verifyObject, I never used it before and found a small problem with attribute initialization. The references to the manager attribute are really gone now.

comment:11 Changed 9 years ago by therve

(In [23337]) Some blank lines tweaks.

Refs #2178

comment:12 Changed 9 years ago by therve

Keywords: review removed
Owner: changed from therve to ralphm

I've made a small cleanup on blank lines. This is good for me, please merge.

comment:13 Changed 9 years ago by ralphm

Resolution: fixed
Status: newclosed

(In [23376]) Add abstraction for implementing XMPP protocol extensions.

Author: ralphm Reviewer: therve Fixes #2178.

This abstraction allows for implementing (parts of) XMPP protocol extensions in self-contained handlers of which instances can be added to an object that acts as a manager for a particular XML stream.

comment:14 Changed 9 years ago by Jean-Paul Calderone

Milestone: Words-8.0.0+1Twisted-8.2

Milestone Words-8.0.0+1 deleted

comment:15 Changed 7 years ago by <automation>

Owner: ralphm deleted
Note: See TracTickets for help on using tickets.