IResourceChangeListener listener interface

[Andrej Golcov]

Hi everyone,

We are implementing a free-text search plugin as a part of  Apache™ Bloodhound. The approach we use is based on AdvancedSearch [1] and uses similar approach as TracAdvancedSearchPlugin [2] and FullTextSearchPlugin [3] plugins. In short: listening for ITicketChangeListener events and updating nonDb free-text-search index.

However,  there is a problem common for such plugins - renaming of a ticket related entity such as Version, Component Type, Severity etc. is not covered by the event and the plugin cannot reflect this change in its nonDb free-text-search index. 

As a part of Apache™ Bloodhound Trac fork, we added a new IResourceChangeListener interface, which is common for all resources (see full code in attached patch).  The main idea of the IResourceChangeListener interface is that it is similar to existing I*ChangeListener interfaces (existing interfaces are working as before), it is common for all resources (resource is sent as parameter) and that there is no need to introduce interfaces such as IVersionChangeListener, IComponentChangeListener etc. Listener may specificy resource types for which it has to be notified via method get_subscribed_resources. What do you think about such approach?

The IResourceChangeListener implementation was tested in Apache™ Bloodhound for a couple of months and it looks like it is stable enough and covers, at least, our requirements. I think that support of the IResourceChangeListener interface in Trac can be useful for Trac community and would like to suggest this patch for Trac. Please find the attached patch rebased against the Trac trunk.

Is the Trac community interested in applying something like IResourceChangeListener into the Trac codebase?

Cheers, Andrej

[1]  http://trac.edgewall.org/wiki/AdvancedSearch

[2] http://trac-hacks.org/wiki/TracAdvancedSearchPlugin

[3] http://trac-hacks.org/wiki/FullTextSearchPlugin

[Christopher Nelson]

> ...

> As a part of Apache™ Bloodhound Trac fork, we added a new
> IResourceChangeListener interface, which is common for all resources (see
> full code in attached patch). The main idea of the IResourceChangeListener
> interface is that it is similar to existing I*ChangeListener interfaces
> (existing interfaces are working as before), it is common for all resources
> (resource is sent as parameter) and that there is no need to introduce
> interfaces such as IVersionChangeListener, IComponentChangeListener etc.
> Listener may specificy resource types for which it has to be notified via
> method get_subscribed_resources. What do you think about such approach?

>...

It seems a fine idea. After a quick skim, the code seems fine. I
wonder thought why you need to notify about ticket changes; surely the
ticket change listener already covers that.

[Christian Boos]

Hello Andrej,

On 4/10/2013 2:36 PM, Andrej Golcov wrote:
> Hi everyone,
>
> We are implementing a free-text search plugin as a part

> of Apache� Bloodhound. The approach we use is based on AdvancedSearch

> [1] and uses similar approach as TracAdvancedSearchPlugin [2]
> and FullTextSearchPlugin [3] plugins. In short: listening for
> ITicketChangeListener events and updating nonDb free-text-search index.

Interesting indeed.

> However, there is a problem common for such plugins - renaming of a
> ticket related entity such as Version, Component Type, Severity etc. is
> not covered by the event and the plugin cannot reflect this change in
> its nonDb free-text-search index.
>

> As a part of Apache� Bloodhound Trac fork, we added a

> new IResourceChangeListener interface, which is common for all
> resources (see full code in attached patch). The main idea of
> the IResourceChangeListener interface is that it is similar to existing
> I*ChangeListener interfaces (existing interfaces are working as before),
> it is common for all resources (resource is sent as parameter) and that
> there is no need to introduce interfaces such as IVersionChangeListener,
> IComponentChangeListener etc. Listener may specificy resource types for
> which it has to be notified via method get_subscribed_resources. What do
> you think about such approach?

Yes, it's nice. The "context" however in the IResourceChangeListener can
be confused with a RenderingContext which is often passed as "context"
as well. Maybe changecontext? or changeinfo?

>
> The IResourceChangeListener implementation was tested

> in Apache� Bloodhound for a couple of months and it looks like it is

> stable enough and covers, at least, our requirements. I think that
> support of the IResourceChangeListener interface in Trac can be useful
> for Trac community and would like to suggest this patch for Trac. Please
> find the attached patch rebased against the Trac trunk.
> Is the Trac community interested in applying something
> like IResourceChangeListener into the Trac codebase?

Very much, thanks for thinking about contributing this one back.
You should open a ticket with that patch (against 1.1.2), so that we can
follow the progress of the integration, and discuss this further if
needed (discussion here on trac-dev is welcome as well).

-- Christian

[Steffen Hoffmann]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10.04.2013 20:01, Christopher Nelson wrote:
>> ...

>> As a part of Apache� Bloodhound Trac fork, we added a new

I've already been thinking lately, that it would be nice to implement
only one generic change listener instead of one per Trac realm,
effectively standardizing the divergent change listener APIs.

This is a nice enhancement indeed, and interesting for a number of Trac
plugins, that hold additional, complementary information for different
Trac resource realm. Just from the top of my head BreadCrumbsNavPlugin,
TagsPlugin and TracFormsPlugin might be candidates to profit from such a
generic interface.

Steffen Hoffmann
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAlFls2UACgkQ31DJeiZFuHdLXgCeK2FGPpUoa2HXZb6Oth/lCrmR
ULYAoKOBWkIoTRYQwz1DVuGb/1dWXSGr
=1suw
-----END PGP SIGNATURE-----

[Peter Suter]

Hi

On 10.04.2013 14:36, Andrej Golcov wrote:
> Is the Trac community interested in applying something like
> IResourceChangeListener into the Trac codebase?

Looks interesting.

Just in case you are not aware of it: There's an old open ticket #8834
"Add a generic IResourceChangeListener".
http://trac.edgewall.org/ticket/8834

While I think your patch deserves a new ticket, reviewing the comments
there might be interesting.

Summary of comments 1-8:
* Resources vs. Model instances: Passing model instances as parameters
to the listeners was preferred. (As in your patch.) Comment 2 suggests
not calling them resources to avoid confusion.

* old_values vs. alternatives: Passing old_values was preferred. (As in
your patch.)

* Comment 1 proposes interfaces based on experience from AnnouncerPlugin
(which has a similar generalization built on top of the existing
listener interfaces).
One detail I find interesting there is that the event types are generic
as well. You seem to be using similar event type identifiers, but only
internally when you call _notify('resource_created', ...) etc. Have you
considered also exposing these in the interface and allowing e.g.
plugins to define new event types? Bad idea? You seem to prefer handling
things like wiki_page_renamed or attachment_reparented using the
catch-all resource_changed event.

(Comments 9-30 are mainly about email notification and how perceived
performance might be improved with threads or multiprocessing.)

Thanks for sharing.

Peter

[Andrej Golcov]

It seems a fine idea.  After a quick skim, the code seems fine.  I
wonder thought why you need to notify about ticket changes; surely the
ticket change listener already covers that.

I see are a couple of reasons for this: 

 - There is a case when a listener has to receive change events from different entities, the ticket change event and the version change event. The implementation is much more clean if the listener have to implement only one interface IResourceChangeListener without additionally implementing ITicketChangeListener.

 - Once upon a time, ITicketChangeListener could be deprecated towards IResourceChangeListener that will simplify core code and decrease number of unit-tests.

Cheers, Andrej 

[Andrej Golcov]

> Yes, it's nice. The "context" however in the IResourceChangeListener can
> be confused with a RenderingContext which is often passed as "context"
> as well. Maybe changecontext? or changeinfo?

Interesting that Olemis pointed out the same issue on bloodhound-dev
mailing list. I hope he will join this discussion also.

I agree about possible confusion with parameter named "context". I'm
thinking towards "event_context" name but I'm aslo fine with suggested
"changecontext" or "changeinfo".

> Very much, thanks for thinking about contributing this one back.
> You should open a ticket with that patch (against 1.1.2), so that we can
> follow the progress of the integration, and discuss this further if
> needed (discussion here on trac-dev is welcome as well).

Sure, I'll do it.

Cheers, Andrej

[Olemis Lang]

On 4/10/13, Andrej Golcov <and...@digiverse.si> wrote:
>> Yes, it's nice. The "context" however in the IResourceChangeListener can
>> be confused with a RenderingContext which is often passed as "context"
>> as well. Maybe changecontext? or changeinfo?
> Interesting that Olemis pointed out the same issue on bloodhound-dev
> mailing list. I hope he will join this discussion also.
>
> I agree about possible confusion with parameter named "context". I'm
> thinking towards "event_context" name but I'm aslo fine with suggested
> "changecontext" or "changeinfo".
>

Like I just said before in d...@bloodhound.apache.org [1]_ [2]_ this
would be a point that would be noticed immediately by people familiar
with Trac code base . JFTR , in here I also suggested :

1. evt (dojo)
2. eventObject (jquery)
3. EventArgs (C#)
4. ... <= which is where "event_context" , "changecontext"
or "changeinfo" would fit .

>> Very much, thanks for thinking about contributing this one back.
>> You should open a ticket with that patch (against 1.1.2), so that we can
>> follow the progress of the integration, and discuss this further if
>> needed (discussion here on trac-dev is welcome as well).
> Sure, I'll do it.
>

During some time I've noticed some things that have not been included
in the patch and I'd also like to put under your consideration as well
. I'll prepare a summary with references to previous discussions we've
had in d...@bloodhound.apache.org

.. [1] Re: Transaction resource changing events
(http://goo.gl/0gthX)

.. [2] Re: Transaction resource changing events
(http://goo.gl/R8kh8)

--
Regards,

Olemis.

Apache™ Bloodhound contributor
http://issues.apache.org/bloodhound

Blog ES: http://simelo-es.blogspot.com/
Blog EN: http://simelo-en.blogspot.com/

Featured article:

[Olemis Lang]

On 4/10/13, Olemis Lang <ole...@gmail.com> wrote:
[...]

>
> During some time I've noticed some things that have not been included
> in the patch and I'd also like to put under your consideration as well
> . I'll prepare a summary with references to previous discussions we've
> had in d...@bloodhound.apache.org
>

Firstly @peter even if not explicitly mentioned in the initial message
[1]_ suggesting this feature for Apache™ Bloodhound , the idea behind
`IResourceChangeListener` has been inspired on original
trac:ticket:8834 . The implementation is substantially different
though . In general , in Bloodhound we pay attention to previous Trac
proposals / ideas / tickets / ... especially if features will have an
impact on our copy of Trac core . We'd like to be in synch as much as
possible to make it easy for us to maintain our vendor branch .

Secondly , most of what I mention below is highly influenced by my
personal opinion .

Points that I think should be considered :

1. In order to completely replace Trac core listener interfaces
the interface is missing reparent notifications. As far as
I could see in Trac this is only used for attachments .
* Nonetheless in Apache™ Bloodhound itself (... so also possibly
in other plugins as well e.g. trachacks:SimpleMultiProjectPlugin,
even if implementation details will be a bit different)
reparent notifications will be needed more often .
* With the notion of product environments and resource neighborhoods
installed in place if a resource (e.g. a ticket) is moved
from product P1 to product P2 then two notifications have
to be broadcast (parent = Product X)
a. all listeners in product env P1 will be notified
of the fact that resource was moved somewhere
else
b. all listeners in product env P2 will be notified
of the fact that resource was added from somewhere
else (e.g. migrate and/or update search index data
gathered in P1 context)
* @andrej : this is something we are lacking in our
multi-product plugin and might also have some impact on
plugins like BH Search plugin
2. @christopher: IResourceChangeListener is aimed at unifying
existing I*ChangeListener interfaces so eventually the later
should be phased out .
3. @peter [2]_ if `old_values` is considered to be yet another
instance of event arguments then all methods will have
the same interface
* which makes me wander whether they should be
merged into a single method plus event name
included as argument or another field in event args
object (i.e. `context` in signatures above)
* @peter ... and , by doing so , subsystems would even be
able to define their own event types without
requiring major architectural refactoring . Similar to
actions in trac:comment:1:ticket:8834
4. There are three kinds of clients of this interface
a. Components focused on events for a single realm
(e.g. ticket relationships).
b. Components handling a well-known subset of available
realms e.g. ticket + attachment
c. Components sniffing into activity on every realm e.g.
search indexing , generic notification engines
5. In Apache™ Bloodhound 0.5 [3]_ only `match_resource` is
supported . It behaves like `self._changed_listeners_map[None]`
in the patch
a. That's not efficient O(l * r) , especially considering
(4a) and (4b)
- l : number of classes implementing IResourceListenerChange
- r : number of active resource realms
b. realm subscription might not appropriate for (4c)
since listener would have to be aware of (almost) all
kinds of resource realms .
c. That's the rationale for having the special case of
`self._changed_listeners_map[None]`
6. Proposed patch implies that listener component and resource
listener loops are tightly coupled to model classes since that's
what they'll return in `get_subscribed_resources`
* IMHO that might lead to some trouble
* not appropriate for (4c)
* ... and since the whole resource API relies on abstract
`Resource` concept it'd be nice to use it instead and
rename that method somehow like `get_subscribed_realms`
* ... not all models are associated to realms though
(e.g. ticket versions)

Finally , slightly off-topic , so JFYI :

A. We are discussing now the introduction of yet another extension point
for other kinds of notifications , quite similar to
IResourceChangeListener.
B. Something similar should be done for I*Manipulator interfaces.

{{{
#!sh

$ grep -r class trac | grep -v ".js" | grep Manipulator | more
trac/attachment.py:class IAttachmentManipulator(Interface):
trac/ticket/api.py:class ITicketManipulator(Interface):
trac/wiki/api.py:class IWikiPageManipulator(Interface):

}}}

Maybe there's something else to say along the way , but this is enough
for start .

.. [1] Re: [Apache Bloodhound] #404: Populate default schema on product addition
(http://goo.gl/o4QAK)

.. [2] Re: Transaction resource changing events

(http://goo.gl/0gthX)

.. [3] http://svn.apache.org/viewvc/bloodhound/branches/0.5/trac/trac/resource.py?view=markup

[Andrej Golcov]

Hi,

> Just in case you are not aware of it: There's an old open ticket #8834 "Add a generic
> IResourceChangeListener".
> http://trac.edgewall.org/ticket/8834

Very interesting discussion on similar subject. It's a pity, it was
not reflected in a code.

> Summary of comments 1-8:
> * Resources vs. Model instances: Passing model instances as parameters to
> the listeners was preferred. (As in your patch.) Comment 2 suggests not
> calling them resources to avoid confusion.

Correct, not all entities exposed via IResourceChangeListener are
actually resources e.g. Version, Component and AbstractEnum based
classes. As an alternative, IResourceChangeListener could be renamed
to something like IEntityChangeListener or any other more appropriate
name.

> * Comment 1 proposes interfaces based on experience from AnnouncerPlugin
> (which has a similar generalization built on top of the existing listener
> interfaces).
> One detail I find interesting there is that the event types are generic as
> well. You seem to be using similar event type identifiers, but only
> internally when you call _notify('resource_created', ...) etc. Have you
> considered also exposing these in the interface and allowing e.g. plugins to
> define new event types? Bad idea?

IMO, triggering of events could be improved and made in more generic
way. But we can start with simple current implementation and improve
it later. From the listener prospective, using of non-generic
interfaces is more convenient (let's say aspect oriented) and provides
documentation by code. While more generic events make developer to
rely on documentation (that is
not the strongest part of the open-source).

> You seem to prefer handling things like
> wiki_page_renamed or attachment_reparented using the catch-all
> resource_changed event.

A listener may subscribe for the specific entity type e.g. only
attachement and/or wiki page etc.
In current proposal, wiki_page_renamed and attachment_reparented are
mapped to IResourceChangeListener.resource_changed event. . We tried
to find some kind of common denominator in the suggested proposal -
not all entities support rename operation. I think, that "was_renamed"
flag can be part of the event context for those entities which have
rename operation.

[Olemis Lang]

On 4/11/13, Andrej Golcov <and...@digiverse.si> wrote:
> Hi,
>
>> Just in case you are not aware of it: There's an old open ticket #8834
>> "Add a generic
>> IResourceChangeListener".
>> http://trac.edgewall.org/ticket/8834
> Very interesting discussion on similar subject. It's a pity, it was
> not reflected in a code.
>
>> Summary of comments 1-8:
>> * Resources vs. Model instances: Passing model instances as parameters to
>> the listeners was preferred. (As in your patch.) Comment 2 suggests not
>> calling them resources to avoid confusion.
> Correct, not all entities exposed via IResourceChangeListener are
> actually resources e.g. Version, Component and AbstractEnum based
> classes. As an alternative, IResourceChangeListener could be renamed
> to something like IEntityChangeListener or any other more appropriate
> name.
>

The fact that they are not resources is just because they are not
associated to a resource manager . I see two options (maybe more ...)
:

1. as stated above to relax the scope of the interface
and rename it to `IEntityChangeListener`
2. Upgrade those entities and transform them into
resources .

IMO , if an entity is so relevant and there's an interest in
broadcasting events to listener components this way then

a. there should be associated state (not limited to DB
e.g. VCS backend)
b. there should be something (request handler, admin panel,
admin command ... but not limited to this) action upon
that state
c. there should be a way to keep track of operations in (b)
e.g. repository hooks

Consequently IMHO such entity deserves to be treated as a resource .
If those conditions are not met it's just either a transient object ,
a field value , ... IMO Resources should be the minimal unit of event
dispatching at this level of abstraction .

>> * Comment 1 proposes interfaces based on experience from AnnouncerPlugin
>> (which has a similar generalization built on top of the existing listener
>> interfaces).
>> One detail I find interesting there is that the event types are generic
>> as
>> well. You seem to be using similar event type identifiers, but only
>> internally when you call _notify('resource_created', ...) etc. Have you
>> considered also exposing these in the interface and allowing e.g. plugins
>> to
>> define new event types? Bad idea?
>
> IMO, triggering of events could be improved and made in more generic
> way. But we can start with simple current implementation and improve
> it later. From the listener prospective, using of non-generic
> interfaces is more convenient (let's say aspect oriented) and provides
> documentation by code. While more generic events make developer to
> rely on documentation (that is
> not the strongest part of the open-source).
>

Like I said in a message sent to d...@bloodhound.apache.org (<=
citation missing) if I had to choose among an architecture that would
have to be changed every time a new event type is needed and another
more stable in time I'd rather choose the later . Practicality beats
purity . There are no such things like aspects in Trac .

However that does not mean that the methods in interface signature are
all wrong . Indeed that's the barely minimal set of event types that
every resource should support . Nevertheless by doing *only* that ,
plugins are not able to reuse the event processing loops to dispatch
other kinds of events they might want to deliver to some listener
classes .

>> You seem to prefer handling things like
>> wiki_page_renamed or attachment_reparented using the catch-all
>> resource_changed event.
> A listener may subscribe for the specific entity type e.g. only
> attachement and/or wiki page etc.
> In current proposal, wiki_page_renamed and attachment_reparented are
> mapped to IResourceChangeListener.resource_changed event. . We tried
> to find some kind of common denominator in the suggested proposal -
> not all entities support rename operation. I think, that "was_renamed"
> flag can be part of the event context for those entities which have
> rename operation.
>

The way I see it the fact that a given resource is reparented not
necessarily means that its state changed . What really changed was a
relationship external to object state .

For instance in the case I mentioned in a previous message when parent
is a given product (say it in Bloodhound or in another plugin like
trachacks:SimpleMultiProductPlugin) and resource is moved from P1 to
P2 then reparenting is another thing substantially different as
compared to what happens by just editing some fields and keep it where
it is . In the case of attachments it works exactly the same . Editing
means new description , etc ... e.g. a single DB operation whereas
reparenting involves moving files to a different folder , a new whole
set of authz permission rules , ...

[Andrej Golcov]

> 3. @peter [2]_ if `old_values` is considered to be yet another
> instance of event arguments then all methods will have
> the same interface
> * which makes me wander whether they should be
> merged into a single method plus event name
> included as argument or another field in event args
> object (i.e. `context` in signatures above)

In this case, we are moving dispatching logic from the Trac core to
event consumers.
Current proposal already brings some dispatching logic on the event
consumer side - when a consumer subscribes for events from multiple
types (e.g. ticket and milestone). Adding event name as a parameter
will complicate the event consumer dispatching even more.

Stepping back, I'll try to summarize which problems the proposal
should to solve:
- Introduce common denominator for a entity changed events (in
future, new events can be added such as manipulator/before-change and
during-changing events).
- Avoid manual maintenance and documentation of I*ChangeListener
interface per entity (currently, there is 11 such entities in Trac).
- Extendibility for plugin entities
- Provide simple subscription for events of specific entity (e.g.
only ticket changed events) with possibility to listent to multiple
types events.
- Do not re-lay dispatching logic on event listeners

Is the summary more or less correct?

Based on the received feedback, I'll try prepare alternative to
IResourceChangeListener implementation that we can compare pros and
cons of different approach.

Cheers, Andrej

[Andrej Golcov]

> Like I said in a message sent to d...@bloodhound.apache.org (<=
> citation missing) if I had to choose among an architecture that would
> have to be changed every time a new event type is needed and another
> more stable in time I'd rather choose the later . Practicality beats
> purity . There are no such things like aspects in Trac .

Adding new methods does not mean architecture changing. IMO, practical
approach is leveraging infrastructure for repeatable work (events
dispatching in out case) and allowing plugins to concentrate on
business logic. For example, let's assume a component that listens to
changed events for Ticket and Version entities. In more generic
approach the code could look like:
{{{
class MultipleEntitiesEventsConsumer(Component):
implements(IEventListener)

#components some how define that it wants receive change events for
Ticket and Version

def event(event_name, entity, event_args):
if event_name == "created":
if isinstance(Ticket, entity):
self.ticket_created(entity, event_args["context"])
if isinstance(Version, entity):
self.version_created(entity, event_args["context"])
elif event_name == "changed":
...
#you've got the point

def ticket_created(self, entity, context):
pass

def version_created(self, entity, context):
pass
}}}

Compare this with alternative:
{{{
class MultipleEntitiesEventsConsumer(Component):
implements(IVersionPostChangeListener, TicketPostChangeListener)

def ticket_created(self, entity, context):
pass

# other "ticket_" prefixed ChangeListener methods

def version_created(self, entity, context):
pass

# other "version_" prefixed ChangeListener methods
}}}

Inspired by this discussion, I prepared a bit different proposal that
leverages ComponentManager for event dispatching and tries to solve
the problem with manual creation and maintenance of several (at least
11) I*ChangeListener interfaces per each entity:
http://trac.edgewall.org/ticket/11148#comment:2

I think the alternative proposal solves previously discussed requirements.
What do you think?

Cheers, Andrej

[Olemis Lang]

On 4/12/13, Andrej Golcov <and...@digiverse.si> wrote:
>> Like I said in a message sent to d...@bloodhound.apache.org (<=
>> citation missing) if I had to choose among an architecture that would
>> have to be changed every time a new event type is needed and another
>> more stable in time I'd rather choose the later . Practicality beats
>> purity . There are no such things like aspects in Trac .
>

[...]

> For example, let's assume a component that listens to
> changed events for Ticket and Version entities. In more generic
> approach the code could look like:
> {{{
> class MultipleEntitiesEventsConsumer(Component):
> implements(IEventListener)
>
> #components some how define that it wants receive change events for
> Ticket and Version
>
> def event(event_name, entity, event_args):
> if event_name == "created":
> if isinstance(Ticket, entity):
> self.ticket_created(entity, event_args["context"])
> if isinstance(Version, entity):
> self.version_created(entity, event_args["context"])
> elif event_name == "changed":
> ...

Not necessarily ...

> #you've got the point
>

... I do understand what you mean

> def ticket_created(self, entity, context):
> pass
>
> def version_created(self, entity, context):
> pass
> }}}
>

[...]

--
Regards,

Olemis.

[osimons]

On 12 apr, 18:47, Andrej Golcov <and...@digiverse.si> wrote:
> > Like I said in a message sent to d...@bloodhound.apache.org (<=
> > citation missing) if I had to choose among an architecture that would
> > have to be changed every time a new event type is needed and another
> > more stable in time I'd rather choose the later . Practicality beats
> > purity . There are no such things like aspects in Trac .
>
> Adding new methods does not mean architecture changing. IMO, practical
> approach is leveraging infrastructure for repeatable work (events
> dispatching in out case) and allowing plugins to concentrate on
> business logic.

I like the Trac approach. Each component / sub-system declaring its
own resources and providing its own manipulator and lister signatures.
I can obviously appreciate where you are coming from trying to add
some form of event notifications for various ticket system data as
pseudo-resources. Many of which I'm sure share a lot of similarities
being based on the same simple enumeration type. Of course, also bear
in mind that the Ticket system is just one part of Trac and plugin
ecosystem.

I see that the natural instinct is to try to rationalise it by lumping
everything together to share code. But by sharing the code you have to
abstract the data, and in my opinion this is not desirable. It is a
design philosophy we have discussed many times in the Trac project.

The complex objects passed around with generic no-meaning names line
'context' immediately makes the API non-obvious. You have to look in
the implementation and introspect the code to determine exactly what
data is part of 'context' and what form it takes. And unlike a method
signature it is no binding 'contract' - it is just data that is
subject to change at will, either in new versions of Trac or plugins,
or just by circumstances from local setup and stored data.

Wiki and Ticket were the initial Trac event models. Recently there has
been various repository-related events added, and likely more will be
added either in Trac or plugins if features such as fork-handling,
pull requests, changeset and source line comments and so on gets
added. For myself I do resources and listener handling in various
unrelated plugins with completely different internals and data
structures, like for instance: Bitten (builds and build
configurations), FullBlog (posts and comments), Talk (rooms, lines).
Having all data hidden in a 'context' argument makes all data non-
obvious to say the least. Without an api.py with interface definitions
and clear documentation of expected arguments and usage, where are we
going to find this documentation?

The API should be explicit and binding. Changes should be obvious and
documented to the API user. Update the signature, add a new method to
denote a new version, or just elaborate in the documentation. Fully
generic and hidden implementation has often been described as a
'feature' and a 'benefit' as it allows to modify API without breaking
existing code. In theory, not practice. The painful reality is that it
just gets much harder to maintain an implementation. The commonly used
'fix' to improve documentation for generic calling is of course to
have 'context' refer to some sort of data structure description and
version. Like perhaps providing a versioned schema to describe the
data passed? This 'XML way' is really not how I would like us to do
thing in Trac...

Regardless of perceived ease of code sharing, you should not be
listening to events that you aren't prepared to handle. You need
domain knowledge to know what you get and what to do with the data.
And to do more useful work you may even need to be able to use the
data to initiate actual model objects or call domain-specific API
methods. It all requires know-how that cannot and should not be
abstracted and generalized. There can be no meaningful sharing across
unrelated domains - at least nothing that can compensate for managing
and developing against this new complexity. I see no great benefit of
rewriting all implementations to do 'if .. elif .. else' to first
check for resource realms, and then calling out to internal helpers to
_do_wiki_update_event() for decoding of hidden data and handling of
the event.

The generic API may look simpler and cleaner, but there is nothing
simpler about the actual work and know-how involved in using it. Some
things stay the same, some things will just require doing things in a
different way. To return to the original post in this thread, how
would a search engine listening to events be expected to make sense of
the blog, build and chat data it receives in order to update its
index? Listeners and Manipulators are not generic problems, and hiding
that fact just makes it worse.


:::simon

https://www.coderesort.com
http://trac-hacks.org/wiki/osimons

[Christian Boos]

On 4/15/2013 12:09 PM, osimons wrote:
> [...] The commonly used

> 'fix' to improve documentation for generic calling is of course to
> have 'context' refer to some sort of data structure description and
> version. Like perhaps providing a versioned schema to describe the
> data passed? This 'XML way' is really not how I would like us to do
> thing in Trac...

I think that something like this is indeed necessary to make the
`IResourceChangeNotificationListener` really useful. Describing the
model fields in a way close to what we *already* do in
`TicketSystem.get_ticket_fields` should be enough.

> Regardless of perceived ease of code sharing, you should not be
> listening to events that you aren't prepared to handle. You need
> domain knowledge to know what you get and what to do with the data.
> And to do more useful work you may even need to be able to use the
> data to initiate actual model objects or call domain-specific API
> methods. It all requires know-how that cannot and should not be
> abstracted and generalized. There can be no meaningful sharing across
> unrelated domains - at least nothing that can compensate for managing
> and developing against this new complexity. I see no great benefit of
> rewriting all implementations to do 'if .. elif .. else' to first
> check for resource realms, and then calling out to internal helpers to
> _do_wiki_update_event() for decoding of hidden data and handling of
> the event.

You don't necessarily need to oppose the two ways. They also can be seen
as complementary. See http://trac.edgewall.org/ticket/11148#comment:2
which precisely address your concerns above.
We can keep the existing interfaces (or something close), plus reuse the
proposed dispatching machinery (`ResourceManager.notify`) for
specialized events. And at the same time have the "generic" events (the
tip of the iceberg) made available for the generic listeners.

> The generic API may look simpler and cleaner, but there is nothing
> simpler about the actual work and know-how involved in using it. Some
> things stay the same, some things will just require doing things in a
> different way. To return to the original post in this thread, how
> would a search engine listening to events be expected to make sense of
> the blog, build and chat data it receives in order to update its
> index?

Because those plugins will present a minimal description of what can be
found in the corresponding "model" element. We can also expect to have a
model.resource property for the identity of the object. The "changeinfo"
will be standardized (e.g. author, timestamp, reason) and "oldvalues" is
the backup data for the modified fields in "model". With that, you'll be
able to index the content of your objects in a generic search system.

The overall problem we're trying to address here is the following.
If you want to propose a service that tries to address a cross-component
concern (typically the timeline, the search), for now it better has to
be an interface (`ITimelineEventProvider`, `ISearchSource`) coming from
trac core, which have the privileged position of being "well-known"
interfaces, otherwise if you're just a plugin among others, you can be
sure that only at best a small fraction of the other plugins will be
interested in implementing that interface. Or you could implement the
glue yourself, again for a limited fraction of the existing plugins.
Either way, you can't expect to have more than partial "coverage" in
this N x N situation. Or you make it possible to have a middle ground (N
-> m -> N), N plugins producing data for a restricted set of m
well-known generic interfaces, which in turn can be consumed by N
plugins. Again, I don't say that *all* interfaces have to follow this
model, but that having this is a nice addition to the system, opening
new possibilities.

And to echo a past discussion, the above can be transposed to data
*representation* as well, where producing some intermediate data format
can also be a solution for producing rich content in various different
end user formats (http://trac.edgewall.org/ticket/6492#comment:34).

-- Christian

[Olemis Lang]

Thanks osimons and cboos for your replies .

On 4/16/13, Christian Boos <christi...@free.fr> wrote:
> On 4/15/2013 12:09 PM, osimons wrote:
>> [...] The commonly used
>> 'fix' to improve documentation for generic calling is of course to
>> have 'context' refer to some sort of data structure description and
>> version. Like perhaps providing a versioned schema to describe the
>> data passed? This 'XML way' is really not how I would like us to do
>> thing in Trac...
>
> I think that something like this is indeed necessary to make the
> `IResourceChangeNotificationListener` really useful. Describing the
> model fields in a way close to what we *already* do in
> `TicketSystem.get_ticket_fields` should be enough.
>

Well , osimons message enlightened me to actually think of another way
to get this done . I'll try to work on a patch towards this alternate
implementation and upload as soon as I could get it done .

>> Regardless of perceived ease of code sharing, you should not be
>> listening to events that you aren't prepared to handle. You need
>> domain knowledge to know what you get and what to do with the data.
>> And to do more useful work you may even need to be able to use the
>> data to initiate actual model objects or call domain-specific API
>> methods. It all requires know-how that cannot and should not be
>> abstracted and generalized. There can be no meaningful sharing across
>> unrelated domains - at least nothing that can compensate for managing
>> and developing against this new complexity. I see no great benefit of
>> rewriting all implementations to do 'if .. elif .. else' to first
>> check for resource realms, and then calling out to internal helpers to
>> _do_wiki_update_event() for decoding of hidden data and handling of
>> the event.
>
> You don't necessarily need to oppose the two ways.
> They also can be seen
> as complementary. See http://trac.edgewall.org/ticket/11148#comment:2
> which precisely address your concerns above.
> We can keep the existing interfaces (or something close), plus reuse the
> proposed dispatching machinery (`ResourceManager.notify`) for
> specialized events. And at the same time have the "generic" events (the
> tip of the iceberg) made available for the generic listeners.
>

I agree .

>> The generic API may look simpler and cleaner, but there is nothing
>> simpler about the actual work and know-how involved in using it. Some
>> things stay the same, some things will just require doing things in a
>> different way. To return to the original post in this thread, how
>> would a search engine listening to events be expected to make sense of
>> the blog, build and chat data it receives in order to update its
>> index?
>
> Because those plugins will present a minimal description of what can be
> found in the corresponding "model" element. We can also expect to have a
> model.resource property for the identity of the object. The "changeinfo"
> will be standardized (e.g. author, timestamp, reason) and "oldvalues" is
> the backup data for the modified fields in "model". With that, you'll be
> able to index the content of your objects in a generic search system.
>

I think I should mention that Bloodhound Search plugin implements
extension points for custom indexing strategies . This would be useful
to have a default indexing strategy and fall back to it if no indexing
enhancements are found <= @andrej cmiiw

Besides this I was thinking of other use cases as well that could be
benefited by the introduction of IResourceChangeListener interface in
a generic way .

For instance let's think of a component implementing XMPP or other
kinds of notifications of anything that's taking place in an
environment .Considering the preconditions mentioned above by cboos .
Provided that there is consistent MIME types for models (e.g.
'x-trac-<resource realm>') then by using `resource` it would be
possible to look for available conversions from 'x-trac-<resource
realm>' onto 'application/atom+xml' (i.e. Atom <entry />) wrap it
inside XMPP pubsub envelop (i.e. XEP-0060) and notify interested
parties .

> The overall problem we're trying to address here is the following.
> If you want to propose a service that tries to address a cross-component
> concern (typically the timeline, the search),

... notification engines ...

> for now it better has to
> be an interface (`ITimelineEventProvider`, `ISearchSource`) coming from
> trac core, which have the privileged position of being "well-known"
> interfaces, otherwise if you're just a plugin among others, you can be
> sure that only at best a small fraction of the other plugins will be
> interested in implementing that interface. Or you could implement the
> glue yourself, again for a limited fraction of the existing plugins.
> Either way, you can't expect to have more than partial "coverage" in
> this N x N situation. Or you make it possible to have a middle ground (N
> -> m -> N), N plugins producing data for a restricted set of m
> well-known generic interfaces, which in turn can be consumed by N
> plugins. Again, I don't say that *all* interfaces have to follow this
> model, but that having this is a nice addition to the system, opening
> new possibilities.
>

The way I see it using interfaces also means that there are import
dependencies to anything that could be notified . Therefore , a
generic notifications solution (like the one mentioned above) would
have to :

a. Know of all imaginable resource-specific listener interfaces
b. Import each one of them
c. Subscribe itself (somehow) to resource-specific notifications
d. Keep track of all other new I*ChangeListener interfaces
implemented by plugins installed at run time .

Option (d) is the tough one

In my previous messages I mentioned that clients of
IResourceChangeListener interface would may be classified in one of
the following groups:

1. components subscribing to notifications in a single realm
e.g. I*ChangeListener as we know them today .
2. components subscribing to notifications in a well-known
set of realms e.g. `ticket` + `attachment`
3. components interested in all sorts of notifications no matter
what realm it is about .

Grups (1) and (2) would be ok with the realm-specific interface
approach , but there's no hope for components in group (3) .

[...]

Summirizing : I'll be working on a new patch for you to review based
on osimons comments .

[osimons]

On 16 apr, 17:56, Olemis Lang <ole...@gmail.com> wrote:
> Thanks osimons and cboos for your replies .
>

> On 4/16/13, Christian Boos <christian.b...@free.fr> wrote:
>
> > On 4/15/2013 12:09 PM, osimons wrote:
> >> Regardless of perceived ease of code sharing, you should not be
> >> listening to events that you aren't prepared to handle. You need
> >> domain knowledge to know what you get and what to do with the data.
> >> And to do more useful work you may even need to be able to use the
> >> data to initiate actual model objects or call domain-specific API
> >> methods. It all requires know-how that cannot and should not be
> >> abstracted and generalized. There can be no meaningful sharing across
> >> unrelated domains - at least nothing that can compensate for managing
> >> and developing against this new complexity. I see no great benefit of
> >> rewriting all implementations to do 'if .. elif .. else' to first
> >> check for resource realms, and then calling out to internal helpers to
> >> _do_wiki_update_event() for decoding of hidden data and handling of
> >> the event.
>
> > You don't necessarily need to oppose the two ways.
> > They also can be seen

> > as complementary. Seehttp://trac.edgewall.org/ticket/11148#comment:2

> > which precisely address your concerns above.
> > We can keep the existing interfaces (or something close), plus reuse the
> > proposed dispatching machinery (`ResourceManager.notify`) for
> > specialized events. And at the same time have the "generic" events (the
> > tip of the iceberg) made available for the generic listeners.
>
> I agree .

Just to avoid misunderstanding: I don't object to something simple
derived from cboos' comment in 11145#comment:2. In my mind this would
drop the whole idea of a 'context' and just pass the 'resource'.
Resources can be complex enough, but they would still have predictable
signatures and content - even with one or more parents or children
attached. You can ask for more standard data via resource manager API
- making links, titles, description and more. A resource including a
'version' may possibly be requested to provide some details of its
change. The IResourceManager methods can naturally be extended if more
common ground is needed (and found).

Your module publishing the interface would of course have to call out
to both to the 'native' listeners, and to the 'resource' listeners. It
won't look so neat in code with double work, but that's really no big
concern.

> >> The generic API may look simpler and cleaner, but there is nothing
> >> simpler about the actual work and know-how involved in using it. Some
> >> things stay the same, some things will just require doing things in a
> >> different way. To return to the original post in this thread, how
> >> would a search engine listening to events be expected to make sense of
> >> the blog, build and chat data it receives in order to update its
> >> index?
>
> > Because those plugins will present a minimal description of what can be
> > found in the corresponding "model" element. We can also expect to have a
> > model.resource property for the identity of the object. The "changeinfo"
> > will be standardized (e.g. author, timestamp, reason) and "oldvalues" is
> > the backup data for the modified fields in "model". With that, you'll be
> > able to index the content of your objects in a generic search system.

See, this is just the kind of generalization that will not hold water.
We cannot make any assumptions about 'model'. Or some 'changeinfo'
data structure or its possible 'oldvalues'. There may not even be a
model. Bitten passes a 'build' as only parameter to listeners, but you
need to know about how the 'build' consists of a collection of 'steps'
with output and errors stored in log files on disk. Are you perhaps
suggesting that every 'object' presented through Trac should conform
to the same pre-defined data structure? Defining an API by data
structure instead of callable methods?

I have no problem understanding the problem with imports. Or with
mapping all 'listeners' (however we decide to define them). What I
fail to understand is how you are supposed in a generic manner to make
sense of a 'build' being passed to you (or rather 'context.build'
following the code samples)? Would you not need to know that a 'build'
is provided by Bitten, and make explicit 'if .. else' conditionals in
your code to make sense of the specific data you find? And know where
in Bitten to look for more features if needed? How is it better to
bury all these domain requirements as implementation details inside a
"generic" method - rather than making it explicit and in-your-face
with predictable outcomes when problems arise?

I just fail to see what non-trivial problems you expect to solve in a
generic manner. Or how moving to generic methods would somehow be a
better trade-off between features & problems.

[Andrej Golcov]

I figured out that I forgot to share ticket id that was created to
reflect to this discussion: #11148
(http://trac.edgewall.org/ticket/11148)

I'm preparing a prototype based on
http://trac.edgewall.org/ticket/11148#comment:3 feedback and will get
back when it will be ready for sharing.

Cheers, Andrej

[Andrej Golcov]

I've just added an alternative proposal as comment on the ticket:
http://trac.edgewall.org/ticket/11148#comment:5

Please share what do you think on this.

Cheers, Andrej

[osimons]

I've added a brief comment, and as far as I can see this works exactly
like Listeners do today?

Anyway, let's take a step back first:

I'm not saying the current *Listener interfaces are perfect. Far from
it. The main problem in my mind is that they are already way to
limited due to the limited data they get passed. Like no request
context, no user, no permissions. The things you can do in and around
Trac without user and permission information is very, very limited.
Really restricted to just doing 'bookkeeping' in plugins that create
related data structures behind the scenes - like how deleting a ticket
could also delete tags pointing to it, for instance.

The need for 'generic' notification and similar user-facing needs has
been mentioned many times in this thread, but that seems to not quite
see the whole picture as without user and permission you cannot
notify. If you change a ticket that is marked as 'private' through
some security policy, no listener (generic or otherwise) have the
required information to enforce security before passing the
information along to users. What you can safely do with generic
notifications is very limited.

This is main reason why our notification system (trac.notification)
for the large part have retained its pre-0.9 implementation. Not much
in the way of API-based thinking in the deep corners of that sub-
system... The lack of better internal notification/listener interfaces
is the number one reason why. It really should have been completely
redone to only use *Listener interfaces instead of the direct call
architecture we still use.

To further add to the perspective I'd also like to mention our
*Manipulators as their issues are somewhat related - and yet
completely different. These are usually much more 'powerful'
interfaces that allows us to do much more - we usually have a request
with perm, user and all we need. The problem then is that it actually
becomes hard to use when not accessed through the web interface. Which
is why for instance the ticket-commit-updater component that gets
called from repository hooks (via 'trac-admin changeset added') can't
call manipulators, and only calls out to listeners inside ticket
save_changes() as that don't depend on a 'req'. This can all be found
in http://trac.edgewall.org/ticket/10125 for those interested.

Complex non-obvious objects with unbounded storage options should not
be passed as part of APIs. Even 'req' fails this test because we
cannot faithfully recreate all parts and variations outside a request
context. So let's instead gather the bits we all should depend on and
that would be the minimum set needed to do useful work in all
contexts. perm, user, resource and href is a good start, and pass each
as named arguments and set to None if unavailable. In a pinch I would
settle for using a well-defined OfflineRequest with essential data and
passed as 'req'.

I appreciate the effort taken to discuss our Listener architecture
(and by extension Manipulators), but personally I would like them to
be able to do MORE instead of LESS. That's where I think we should be
spending the effort, and this is the perspective we should have when
trying to come up with better ways of doing it. Please let us continue
this discussion!

[Olemis Lang]

On 4/18/13, osimons <odds...@gmail.com> wrote:
> On 18 apr, 15:18, Andrej Golcov <and...@digiverse.si> wrote:
>> I've just added an alternative proposal as comment on the
>> ticket:http://trac.edgewall.org/ticket/11148#comment:5
>>
>> Please share what do you think on this.
>>
>> Cheers, Andrej
>
> I've added a brief comment, and as far as I can see this works exactly
> like Listeners do today?
>
> Anyway, let's take a step back first:
>
> I'm not saying the current *Listener interfaces are perfect. Far from
> it. The main problem in my mind is that they are already way to
> limited due to the limited data they get passed. Like no request
> context,

what if there's no such context e.g. trac admin command ?

> no user,

maybe usefull yes .

> no permissions.

what would these be for ?

> The things you can do in and around
> Trac without user and permission information is very, very limited.
> Really restricted to just doing 'bookkeeping' in plugins that create
> related data structures behind the scenes - like how deleting a ticket
> could also delete tags pointing to it, for instance.
>
> The need for 'generic' notification and similar user-facing needs has
> been mentioned many times in this thread, but that seems to not quite
> see the whole picture as without user and permission you cannot
> notify.
> If you change a ticket that is marked as 'private' through
> some security policy, no listener (generic or otherwise) have the
> required information to enforce security before passing the
> information along to users. What you can safely do with generic
> notifications is very limited.
>

I disagree especially considering your example. On ticket change
you'll only be able to figure out the permissions context of the actor
performing such action , which is guaranteed to be allowed before
actually changing the entity . Now that it was successfully changed ,
as it's private what needs to be considered is the permissions of
*other* interested (subscribed) parties .

The way I see it now (but I might be missing something) it's
impossible to design the listeners API based by cluttering it with
data reflecting external influences . In your example ticket ID is
well known by listener (be it in model , resource, ...), access rules
may be retrieved somehow . So just let the listener put all the pieces
together to get thins done .

> This is main reason why our notification system (trac.notification)
> for the large part have retained its pre-0.9 implementation. Not much
> in the way of API-based thinking in the deep corners of that sub-
> system... The lack of better internal notification/listener interfaces
> is the number one reason why. It really should have been completely
> redone to only use *Listener interfaces instead of the direct call
> architecture we still use.
>

+1 ... somehow we've been decoupling handling of certain things in
Bloodhound multi-product plugin by implementing product listeners .
What you mention is much more complex though as concerns involved
impact across multiple realms

> To further add to the perspective I'd also like to mention our
> *Manipulators as their issues are somewhat related - and yet
> completely different.

+1

> These are usually much more 'powerful'
> interfaces that allows us to do much more - we usually have a request
> with perm, user and all we need. The problem then is that it actually
> becomes hard to use when not accessed through the web interface.

Yes , such a headache .

> Which
> is why for instance the ticket-commit-updater component that gets
> called from repository hooks (via 'trac-admin changeset added') can't
> call manipulators, and only calls out to listeners inside ticket
> save_changes() as that don't depend on a 'req'.
> This can all be found
> in http://trac.edgewall.org/ticket/10125 for those interested.
>
> Complex non-obvious objects with unbounded storage options should not
> be passed as part of APIs. Even 'req' fails this test because we
> cannot faithfully recreate all parts and variations outside a request
> context. So let's instead gather the bits we all should depend on and
> that would be the minimum set needed to do useful work in all
> contexts. perm, user, resource and href is a good start, and pass each
> as named arguments and set to None if unavailable.

I do not agree . This is similar to rendering context . It's a single
object , so related changes are encapsulated in that single class (as
much as possible ;) . Even different contexts may be represented by
different classes satisfying the same contract under different
constraints . However changing method signatures is much more complex

> In a pinch I would
> settle for using a well-defined OfflineRequest with essential data and
> passed as 'req'.
>

We do that in Bloodhound (i.e. `bhdashboard.util.dummy_request` ) to
reuse Trac code in Bloodhound widgets .

[Andrej Golcov]

> Complex non-obvious objects with unbounded storage options should not
> be passed as part of APIs. Even 'req' fails this test because we
> cannot faithfully recreate all parts and variations outside a request
> context. So let's instead gather the bits we all should depend on and
> that would be the minimum set needed to do useful work in all
> contexts. perm, user, resource and href is a good start, and pass each
> as named arguments and set to None if unavailable. In a pinch I would
> settle for using a well-defined OfflineRequest with essential data and
> passed as 'req'.

IMO, passing context data (such as perm, user, resource and href) as events args brings a few problems:
 - That will bloat interfaces, complicates logic and introduce a lot of repeatable code because this parameter(s) have to be sent almost to each method and be part of almost each interface.
 -  As you said, it defines a minimum set. What if a plugin requires some data out of this minimum?
 - That breaks Single Responsibility Principle (http://en.wikipedia.org/wiki/Single_responsibility_principle) - a class have to be aware of context data even if it is not required by the class functionality.
 - obtaining backward compatibility brings additional effort

I suggest the different approach:
Trac ComponentManager already provides component resolving functionality with the only supported visibility scope - singleton per environment. ComponentManager could go a step forward and support additional scopes (lifestyles) e.g. per request, transient etc.

Let's take an example: a plugin wants to get a source IP (usename, authentication type... you name it) on ticket change event - that is currently not a simple trick because the request parameter is not a part of event method.

What can be done: at the begging of web request handling, Trac obtains an instance of a WebRequest component from ComponentManager (OfflineRequest for trac-admin tool) and fill the appropriate data. The WebRequest class should be marked as PerRequest:
{{{
@lifestyle(PerRequest)
class WebRequest(Components):
  implements(IRequest)

  #request data is here
  ....
}}}

ComponentManager should know how to track components with PerRequest lifestyle. For examle, ComponentManager can use thread local context for this (thread local context should be cleared at the end of the request).

The plugin implementation could look like:
{{{
class SampleTicketChangeListener(Component):
  implements(ITicketChangeListener)
 
  def ticket_created(ticket): 

     #get_component returns an instance of a component that implements IRequest interface. Because WebRequest implements IRequest declares and it's
     # lifestyle is PerRequest the result will be the instance of WebRequest bounded to the current request.
     source_ip = self.env.get_component(IRequest).source_ip

     #... plugin's logic here

}}}

Does it all make sense?
This is not a small change but functionality can be added incrementally without breaking backward compatibility.

Cheers, Andrej