Item9715: Enable dynamic subclassing of the selected Store (expert feature for 1.2.0)

The Plugins API provides some rudimentary handlers that intercept store actions; the afterSaveHandler, afterMoveHandler and friends.

These handlers suffer from not being integrated closely enough with the store implementation, with the result that the are called in the wrong place, and someteims not at all (repRev is one of the obvious cases.

Because of legacy we can't move the handlers. We need a new level of "listener" that sits close to the store and triggers when a low level store event happens.

I think this should be part of the Store interface, but haven't worked out quite how to do that yet. The experimentation I have done implements listeners in the Store implementation - in the common case, Foswiki::Store::VC::Store.

-- CrawfordCurrie - 19 Sep 2010

rather than @{$Foswiki::cfg{Store}{Listeners}}, can we use %{$Foswiki::cfg{Store}{Listeners}}

eg, $Foswiki::cfg{Store}{Listeners}{Foswiki::Plugins::MongoDBPlugin::Listener} = 1;

that way, listeners can be really trivially added by Config.spec, their new() can return undef if not relevant, or missing dependencies, and configure could even order them using different values.

I don't think

# @ISA not required (base class is empty)
#our @ISA = ('Foswiki::Store::Listener');

is a good idea, as it prevents code from testing class relationships.

Also, I'd like to suggest thinking about merging Listeners and Loggers, and additionally, looking at replay.

(That way we could start using it as a transaction log we can replay on server failure, or as a redundant server syncing system (its basically what RDBMS's use)........which implies a need for before and after commit methods.

it could be part of an application level sync/transactin log, an interface to external data-systems (ie, I could sync the Tasks into an enterprise-wide project planning system) or an auto backup - ie, commit to a third repository)

-- SvenDowideit - 19 Oct 2010

Yes, to all the above. When I started I only had one listener; that spread to an array and, subject to an ordering being available, a hash is fine - an array can always be extracted from a hash. Regarding merging with loggers - ok, we can look at it, but I'm slightly nervous. Store listeners are (currently) a concept specific to VC stores, and I'd be unhappy if that started leaking into the rest of the core.

-- CrawfordCurrie - 19 Oct 2010

Smells like new API which is fine.

But where is the feature proposal? It is an old already accepted one that I cannot see the relevance of or have you just not yet raised a feature proposal on this?

-- KennethLavrsen - 19 Oct 2010

The main feature proposal is ImproveSupportForDatabaseSearches.

Other relevant feature proposals are:

• DatabaseStore
• MongoDBPlugin

I still see this as experimental; I really want Sven's feedback on application to the MongoDBPlugin (and bloody anyone's feedback on DBIStoreContrib) before we move on.

-- CrawfordCurrie - 23 Oct 2010

Crawford. I plan to try to add some Store listeners for the various attach related operations. Covered in the Brainstorming topic EnableCloudStorageForAttachments. Talking with SvenDowideit on IRC, seemed like this was the task to use. Here are the changes I have ready to go. Waiting for feedback and some testing before I commit.

• sub readTopic - auto-attach code:
  • $this->tellListeners('autoattach', $topicObject, $foundAttachment);

I think I needed a different verb (or an option) to indicate autoattach. Since auto-attach doesn't actually update store, it's possible that the listener will be told repeatedly that an attachment has been auto-attached. So it needs a clue to do other tests as to whether or not the attachment has changed.

• sub moveAttachment
  • $this->tellListeners('attach', $oldTopicObject, $oldAttachment, $newTopicObject, $newAttachment);
• sub copyAttachment
  • $this->tellListeners('attach', $newTopicObject, $newAttachment);
• sub saveAttachment
  • $this->tellListeners('attach', $newTopicObject, $name);

And I changed remove, and I think I fixed a bug. The Listeners were told the topic was removed, even if only an attachment was removed. So removing attachments could depopulate your DB Store.

• sub remove (Added $attachment)
  • $this->tellListeners('remove', $topicObject, $attachment);

-- GeorgeClark - 07 Dec 2010

And for some API feedback, wondering if the $newObject should always be first. rather than some operations using update ( oldObj, newObj ) vs. update ( newObj ) It would make the called routines more consistent.

-- GeorgeClark - 07 Dec 2010

I've never been very happy about auto-attach - it seems to me it ought to go through the same process as a normal attach. I don't understand why we'd want a specific handler for it? Note: I deliberately defined a listener API that worked for the callee and not so much for the caller. IMHO the API should hide store implementation details as far as possible, and "auto-attach" is one of those details that a callee shouldn't be forced to know about.

Good catch on the bug.

Regarding the API proposal; I think you are trying to overload three distinct verbs onto one name. Looking at the existing listener API:

insert($metaObject)
update($oldMetaObject[, $newMetaObject])
remove($metaObject)

You are looking for a way to express the same three verbs for attachments, are you not? Since an attachment is specified by a meta object plus a name, why not simply extend the existing interface:

insert($metaObject[, $attachment_name])
update($oldMetaObject[, $attachment_name][, $newMetaObject[, $attachment_name]])
remove($metaObject[, $attachment_name])

Since a meta-object can be easily distinguished from a string (using ref()) it's easy for the callee to determine which signature is being used.

I agree there may be an argument to add "move" and "copy" (instead of overloading "update") since some listeners will implement these operations atomically.

-- CrawfordCurrie - 07 Dec 2010

For autoattach, SvenDowideit and I were talking a bit about deprecating it and making his UpdateAttachmentsPlugin a default. The reason it needs to be treated differently I is that the API doesn't actually report an update of either an attachment or a topic. Each read of the topic will rediscover the attachments. So either a listener needs to test each attachment to determine if it really changed, or the API needs to reflect that it is potentially changed.

I agree about API, except that you did not implement insert. So I've been working backwards from the code.

-- GeorgeClark - 07 Dec 2010

I'm not ready to commit yet, but I have reworked most of the changes to your model of insert, update, remove. I'll change saveTopic to choose insert/update depending upon whether or not the topic exists. Latest changes:

This is a list of the tellListeners after my changes.

  readTopic:  $this->tellListeners('autoattach', $topicObject, $foundAttachment);
  moveAttach  $this->tellListeners('update', $oldTopicObject, $oldAttachment, $newTopicObject, $newAttachment);
  copyAttach  $this->tellListeners('insert', $newTopicObject, $newAttachment);
  moveTopic   $this->tellListeners('update', $oldTopicObject, $newTopicObject);
  moveWeb     $this->tellListeners('update', $oldWebObject, $newWebObject);
  saveAttach  $this->tellListeners('[insert/update]', $topicObject, $name);
  saveTopic   $this->tellListeners('[insert/update]', $topicObject);
  repRev      $this->tellListeners('update', $topicObject);
  delRev      $this->tellListeners('update', $topicObject);
  remove      $this->tellListeners('remove', $topicObject, $attachment);

The two changes that will impact your code is the attachment added to 'remove' and the 'insert' verb in saveTopic. I've left autoattach for now since it really isn't updating any topic or attachment directly.

-- GeorgeClark - 08 Dec 2010

OK. I also added the askListener interface, to allow the store to ask a cache if it has a version of the topic to load. This lets me run two stores in parallel over the same data i.e. a MySQL "cache" and an RCS "backup". Not sure if this is a good direction yet, but it does give me a way to implement a full store in the DBIStoreContrib without having to completely replace RCS.

-- CrawfordCurrie - 08 Dec 2010

The autoattach flow is:

• Attachment is stored into /pub/Web/Topic/somefile.txt from outside Foswiki with no API calls.
• Topic is read by Foswiki, and attachment is merged into MetaData, but not saved.
• Topic is later read again, attachment is merged again.
• Topic is saved, and auto attachment becomes permanent part of MetaData
  • update Listener is called with topic name.
  • no attachments are ever reported to a listener.

The purpose of the autoattach verb is to tell the Listener that there is an attachment that has not been seen by an insert or update. Because every read of the topic will "re-discover" the attachment, the same file can be notified many times.

We could call the insert verb instead, but it's really not a true insert because the topic itself has not bee updated. Or we could parse the metadata on topic save looking for any auto-attachments that had been added to the metadata. But both of them add complexity to the Store, and would miss attachments until the topic is saved.

Sven is right that this is probably better fixed by deprecating autoattach from core and using a cron task to correctly attach the files. By keeping the autoattach code distinct from the insert/update/remove it keeps this code isolated.

-- GeorgeClark - 08 Dec 2010

I suspect that the listeners should be triggered by configure also - when an extension is installed.

and how do we deal with the untar / copy over the top upgrades?

-- SvenDowideit - 09 Dec 2010

Crawford - your suggestion for using one verb is terrible.

suddenly, the meaning of the parameters changes based on parameter count - surely we don't need to make this so complicated

-- SvenDowideit - 09 Dec 2010

The configure installer (shell or web) will check-in topics & attachments, espec. if it finds an existing topic. In that case it ignores the noci option and does what's right for the rcs. But I don't know if the Foswiki env. will be complete enough to have the listeners function correctly. And the the untar/unzip crowd will have an issue.

-- GeorgeClark - 09 Dec 2010

having decided that i detested the count, test type and hope versin of the API, I'e changes it to named params - still need to test the params for definedness, but at least you know what they are intended to be.

-- SvenDowideit - 09 Dec 2010

Named params is fine. I fully expected we would go through several iterations of this interface.

Regarding startup; that's a tricky one. For a cache like DBIStoreContrib, you want to trigger the first load event as early as possible, but only after you are sure the listeners are registered and working. It would be terrible if you spent a day building a DB that ended up being unusable because one or more topic updates were missed. I'm less than convinced that configure is the right place to do that.

-- CrawfordCurrie - 09 Dec 2010

I have removed the listners and replaced it with a dynamic way to subclass the selected store - much more flexible

-- SvenDowideit - 02 Nov 2012

Commenting is disabled while not logged in