Item10961: calling Foswiki::Func::saveAttachment() can destroy the topic content

It seems Meta::atach() saves an empty topic object, that is there's a loadVersion() missing. But why didn't that show up before? Can anybody confirm http://trac.foswiki.org/changeset/12156 is the right thing to do?

-- MichaelDaum - 11 Jul 2011

Item10927 fixes a very similar problem. The issue is that some code "accidentally" worked as long as you weren't admin, because the ACL check caused the $topicObj to be loaded properly. If you're admin, it doesn't need to check ACLs, so it doesn't load the topic object.

Given that Foswiki::Meta::attach() fundamentally writes to the store, and needs a fully loaded $topicObj to do so, we definitely cannot do the attach operation with an unloaded meta object.

However, I think distro:033c435ef4f6 is not the right way - the perldoc implies that perhaps we should enforce a contract that the caller should supply a $topicObj that is loaded AND is latest revision.

So in that case, an error might be a better way to do it. That way the caller has an opportunity to fix their code, instead of silently ending up with a $topicObj that's been loaded with latest revision where they may not have expected that to happen.

-- PaulHarvey - 12 Jul 2011

Sure, adding a croak somewhere trying to write an unloaded topicObj to the store would uncover more of these kind of horrible errors.

Still the actual condition must be avoided in the calling code as that's where the error happens. For the store itself, even for saveAs(), it is too late to load the object in case it hasn't been loaded before. That's because you (1) create a topicObj (2) manipulate its data and (3) save it back to the store, and to make sure you are working on correct data you will have to "load" it between step (1) and (2).

As far as I have seen in Func and Meta, this kind of code flow has been moved out of Func and into the first-level API of Meta, as there aren't any calls to loadVersion() in Func but in Meta::moveAttachment() and the like. So having a loadVersion() in Meta::attach seems to be a natural thing to do, just before adding the new meta data / before changing attachment meta data.

Judging from the kind of errors we have seen for quite some time now, errors of the pattern "unloaded object causing loss of data", it seems as if there is a fundamental problem here. The concept loaded-vs-unloaded topicObj is probably complicating things too much for a questionable optimization that it was intended for. So it might be better to get rid of "unloaded topicObjs" for the sake of correctness of the code.

Even if we were able to fix all errors of this pattern, the concept seems to be too blurred or not well understood by all core programmers including me that I'd expect these kind of problems showing up again and again in regular intervals while the code changes over time.

Of course, if someone could explain why we ultimately and 4ever will have to deal with loaded-unloaded topicObjs and that it is an absolute must, I'll be ready to swallow that pill.

Maybe we just don't need that dichotomy anymore as Paul now invented Foswiki::Address.

-- MichaelDaum - 12 Jul 2011

Making this a release blocker to get people interested. :/

-- MichaelDaum - 26 Aug 2011

ew. And yes, I'm working on fixing the unloaded dichotomy (And Atm, working towards a Session-less store..) - but thats a foswiki 2.0 change :/ (and no, we don't need to support this unloaded meta thing forever, its a 1.1 invention that we've decided to do away with)

I'll poke around this for 1.1.4 - but as you point out, this is only one symptom of the problem, and its going to be extremely hard to fix it at its root, as the error becomes evident too late compared to where the UI should alert the user.

-- SvenDowideit - 26 Aug 2011

https://github.com/SvenDowideit/foswiki being where I'm working on this, as my time is sporadic, so it'll be a while before enough code is done for it to actually work.

-- SvenDowideit - 26 Aug 2011

What is it I have to do as an admin - in my browser - to trigger this.

I have never encountered nuking a topic by adding an attachment. It seems to me that this must be related to a few specific plugins.

What is it in this bug that prevents release of 1.1.4?

-- KennethLavrsen - 05 Sep 2011

It can happen when an offline change is made to a topic, e.g. by a cron job. The "head" gets nuked if it hasn't been checked in. This is what I'm working on at the moment - Item11091

-- CrawfordCurrie - 05 Sep 2011

If we're unable to find an elegant solution for 1.1.4, we can probably "just" ASSERT(defined $topicObj->loadedRev()) and put a big fat warning in Foswiki::Func::saveAttachment() perldoc to note that you must pass a "fully loaded" topic object, because saveAttachment() isn't going to load it for you.

-- PaulHarvey - 06 Sep 2011

On the other hand, that assert might break some stuff where you want to save an attachment to a non-existent topic. Perhaps ASSERT(defined $topicObject->loadedRev() or not Foswiki::Func::topicExists($topicObj->web() . '.' . $topicObject->topic()))

-- PaulHarvey - 06 Sep 2011

As far as I can say distro:033c435ef4f6 cures my problems on foswiki/trunk. So why not use this patch on the 1.1.x branch as long as we still have unloaded meta objs?

-- MichaelDaum - 06 Sep 2011

the commit cures one of many possible variations of the problem - ie, plasters over only one crack. We're looking into the root cause of the issue and trying to find if we can solve that

-- SvenDowideit - 06 Sep 2011

Right now for 1.1, there are these options:

• Merge over distro:033c435ef4f6, or
• Apply the fix in Foswiki::Func's call to the $metaObj->attach() (ensure $metaObj is loaded before trying to attach)
• Make Foswiki::Meta->attach() croak if it's unloaded
• Document this contract with callers of Foswiki::Meta->attach()

-- PaulHarvey - 07 Sep 2011

Imho, depending on the caller to make sure the meta obj is loaded is not really an option, because loadedness is a concept about to go away as soon as possible, maybe starting with foswiki-1.2.0. This all still is an intensive headache as plugins require to stay functional even on foswiki-1.1.x

-- MichaelDaum - 07 Sep 2011

or we could wait a little for Crawford to complete the root cause investigation that he says he's doing....

-- SvenDowideit - 07 Sep 2011

I believe this should now be OK from the fixes for Item11091. However I have not generated a unit test (I had more than enough other tests to write). This report should remain open (and urgent) until someone has added one.

-- CrawfordCurrie - 11 Sep 2011

I realised on inspection that verify_Inconsistent_saveAttachment covers the most obvious case. While I would still have liked more tests.I believe this covers the case described here.If you believe otherwise, PLEASE ADD A UNIT TEST

-- CrawfordCurrie - 22 Sep 2011

Commenting is disabled while not logged in