Item12364: Func::saveTopic is non-functional

While researching a unit test fail in MailerContrib I discovered that Foswiki::Func::saveTopic is broken.

Discussion with Micha follows:

(08:17:51) CDot: I discovered late last night after a long debug that Foswiki::Func::saveTopic is seriously SNAFU
(08:19:06) CDot: anyways, saveTopic. The problem is that an extension that calls saveTopic on an existing topic with new content will never correctly save
(08:19:33) CDot: the reason is that Meta->save has a call to getVersionInfo at the start
(08:19:49) MichaelDaum: ... which loads the thing from disk again
(08:19:58) CDot: if the topic is pre-existing, then the version info is recovered by reading the topic
(08:20:10) CDot: this blows away any content the extension has provided
(08:20:32) ***CDot is puzzled why no-one noticed this. What am I missing?
(08:20:54) MichaelDaum: have you got example code that fails?
(08:21:01) CDot: the MailerContrib unit tests must have been failing for a long time because of this
(08:21:19) CDot: the test that SvenDowideit added to the MailerContrib shows it - though it's non-obvious
(08:21:29) CDot: I had to dig quite deep to find the cause
(08:21:56) CDot: I will generate a unit test for Foswiki::Func that should demonstrate it
(08:22:09) CDot: the problem is in Store::VC::Store
(08:22:27) CDot: where you added a line to read the topic to get the version info
(08:27:41) MichaelDaum: problem is that the plugin gets to see a meta obj that isnt loaded yet. if it was loaded getVersionInfo wouldnt overwrite changed content.
(08:29:03) CDot: exactly
(08:29:18) MichaelDaum: or putting it the other way: the conflict arises as the code does not know how to fully load the obj. does it keep whats in memory and potentially has been changed by some user, or does it load it from disk.
(08:29:27) CDot: I did consider setting _loadedRev or _isLatest in Func, but that feels dangerous
(08:29:57) CDot: I also wondered about marking objects as "untouchable" when ->text($) is called
(08:30:24) CDot: but the probem with that is that the same applies to %META, and the hash that stores it is exposed
(08:30:33) CDot: so there is no opportunity to mark if it is changed
(08:30:40) MichaelDaum: y
(08:31:17) MichaelDaum: so how comes the (plugin) code gets his hands on ameta obj stub not yet loaded?
(08:31:38) CDot: read Foswiki::Func::saveTopic and the answer will smack you in the teeth
(08:33:34) MichaelDaum: okay. got it.
(08:33:42) MichaelDaum: well not too wrong. almost right.
(08:33:50) CDot: very close to perfect
(08:34:04) CDot: just inperfect enough to be totally SNAFU
(08:34:11) CDot: imperfect
(08:34:47) MichaelDaum: the problem is more in copyFrom()
(08:35:08) MichaelDaum: well ->text() as well
(08:35:29) MichaelDaum: a one line addition to Func would cure the symptoms
(08:35:31) CDot: maybe; or maybe that's looking at it upside down
(08:35:47) MichaelDaum: making sure the target obj is loaded before altering it
(08:35:58) CDot: I worry that curing symptoms isn't often curing the disease
(08:36:27) ***MichaelDaum not sure we already agree on what the disease is
(08:37:29) CDot: ok; there are two scenarios here. New Meta object on an existing topic, and for a non-existing topic
(08:37:45) MichaelDaum: hm hm hm
(08:38:00) CDot: for efficiency, new meta on an existing topic should not need to load until the content is actually required
(08:38:17) CDot: new meta on a new topic has to behave like a loaded object
(08:38:30) CDot: so conflicting requirements
(08:39:26) CDot: the core is actually fine; it's the Store::VC::Store that is fucking up
(08:39:44) CDot: because of the version info recovery, and storing version meta in the topic
(08:40:05) CDot: otherwise it would not need to read the topic.
(08:40:29) MichaelDaum: the distinction between "loaded" and "unloaded" isn't that unique to foswiki. I've seen it on distributed document systems as well. although they tend to separate the two into Classes of their own: DocumentStub vs Document.
(08:40:34) CDot: One solution would be for Store::VC::Store to load the version info into a *new* meta-obj, and only copy the meta-info from there

-- CrawfordCurrie - 22 Jan 2013

Fixed it, I hope.

-- CrawfordCurrie - 22 Jan 2013

Commenting is disabled while not logged in