Shed light on how PageManager saves pages?

[Andrew Jaquith <an...@mac.com>]

I have a few questions about the innards of JSPWiki and how it saves  
pages. I suspect Janne and Murray know the answer to these.

As you know, I am investigating how to make workflows serializable.  
For a given workflow to be serializable, it will likely require that  
*everything* we stash in a Workflow also be serializable. In the  
current PreSaveWikiPageTask, we stash a bunch of Strings plus the  
WikiContext, then retrieve it later in SaveWikiPageTask. The  
"SaveWikiPageTask" code is below.

      // Retrieve attributes
      WikiContext context = (WikiContext)  
getWorkflow().getAttribute( PRESAVE_WIKI_CONTEXT );
      String proposedText = (String)  
getWorkflow().getAttribute( FACT_PROPOSED_TEXT );

      WikiEngine engine = context.getEngine();
      WikiPage page = context.getPage();

      // Let the rest of the engine handle actual saving.
      engine.getPageManager().putPageText( page, proposedText );

      // Refresh the context for post save filtering.
      engine.getPage( page.getName() );
      engine.textToHTML( context, proposedText );
      engine.getFilterManager().doPostSaveFiltering( context,  
proposedText );

As you can see, we expect to be able to retrieve WikiContext as a  
Workflow attribute called PRESAVE_WIKI_CONTEXT, which was stashed by  
PreSaveTask.

My question is, is there anything special about the WikiContext that  
we retrieve, which is passed to textToHTML() and  
doPostSaveFiltering()? Could we create a synthetic one instead? In  
other words, I am proposing that in PreSaveTask we stash the WikiPage  
name as a string, then pick it up in SaveTask and use it to  
manufacture a new, fake WikiContext with the correct WikiPage.

Why this could be important: if the WikiContext does not necessarily  
need to be the original one (created by the user), that would mean we  
wouldn't need to stick it in the Workflow. Which would make the "save  
wiki page" workflow serializable.

Conversely, if the WikiContext used by textToHTML() and  
doPostSaveFiltering() *has to be* the same WikiContext (and not  
created synthetically), then we've got to keep stashing it as a  
workflow attribute, and thus (here's the punchline)... we will never  
to be able to persist workflows.

So, what do you think?

One more thing. Why do we call textToHTML() as part of the **save**  
process? Here is what textToHTML() does (lightly edited):

     boolean runFilters =  
"true 
".equals(m_variableManager.getValue(context,PROP_RUNFILTERS,"true"));
     if( runFilters )
         pagedata = m_filterManager.doPreTranslateFiltering( context,  
pagedata );

     result = m_renderingManager.getHTML( context, pagedata );

     if( runFilters )
         result = m_filterManager.doPostTranslateFiltering( context,  
result );

I seem to recall something about needing to run textToHTML() so that  
access rules and other meta-data were re-read correctly...


Andrew

Re: Shed light on how PageManager saves pages?

[Janne Jalkanen <Ja...@ecyrd.com>]

> Thinking out loud: Might I recommend that we encourage plugin  
> developers NOT to assume the presence of an HTTP request in 3.0?  
> It's going to be much cleaner simply to grab whatever parameters  
> Stripes binds to the ActionBean via their accessors (they will  
> correctly typed, among other things). E.g., if Stripes binds the  
> "page" request parameter to an ActionBean's property of type  
> WikiPage, why not just use getPage() to get it, rather than go  
> through trouble of getting the request from the context, calling  
> getParameter(), looking for nulls and misspellings etc...? So much  
> could go wrong, and Stripes takes care of this for you. So, in 3.0  
> a developer commandment might be:

It's already the norm - check out the javadocs.  For example,  
embedders would not see a HttpServletRequest in there.  We have a  
getHttpParameter() which checks for HTTP parameters safely.

The point why it exists is that certain plugins (like WikiForms) can  
this way communicate with each other across invocations.

> Aside: one of the things that pleases me the most about Stripes is  
> that its parameter-binding glue allows us to decouple the request  
> from the WikiContext. Likewise, the Resolution and related  
> UrlBuilder classes decouple it from the response.

Ah, but Stripes parameter binding is static.  You can't add new  
parameters at runtime without resorting to reading the request.

>> Looking at the issues involved, I think it probably makes more  
>> sense to stuff this to 3.0 and see if we can clarify the role of  
>> the WikiContext a bit more.  If WikiContext is going to be a  
>> Stripes ActionBeanContext in the future, we've got to make sure  
>> that is serializable too, of which I am not too sure about.
>
> I do not completely understand what you mean here, but I do not  
> think that WikiContext should be serializable. Today, it contains  
> references to too several things that have time-dependent states  
> (HttpServletRequests go out of scope; HttpSessions expire, etc.).  
> In the future, it would be extremely difficult to make WikiContexts/ 
> ActionBeans serializable because that would mean all of their  
> properties would have to be serializable too. Not too hard with a  
> concrete class, but probably impossible with an interface, which is  
> what ActionBean is... would impede flexibility way too much.

Well, if you do want to serialize the state of the request across  
workflows, then you might need to do that.  We could add custom  
serialization/deserialization code, which could go and create a  
synthetic WikiContext when deserialized.  What it would mean though  
is global variables (i.e. singletons, like WikiEngine) which could be  
accessed without knowledge of the context itself.  So when  
WikiContext is deserialized, it'll go and hydrate itself using  
WikiEngine.getInstance(), etc.  You will lose some information, but  
that probably does not matter in the contexts that we will be using  
it for (like workflows).

/Janne

Re: Shed light on how PageManager saves pages?

[Andrew Jaquith <an...@mac.com>]

> Yup, we've got a problem.  Most things in the WikiContext are such  
> that they do not survive serialization - WikiEngine,  
> HttpServletRequest, even WikiPage (due to interactions with the  
> CachingManager).
>
> It's a good question whether they even do survive leaving the  
> WikiSession and being executed in somebody else's context (the  
> HttpServletRequest comes to mind).  Now, since  
> SavePageTask.execute() invokes the parser, that means that user  
> plugins are also invoked, and if you've got someone relying on  
> HttpServletRequest parameters at the time of save parsing - well,  
> boom for them. (Not that I would see that anyone would do that.)

Oy.

Thinking out loud: Might I recommend that we encourage plugin  
developers NOT to assume the presence of an HTTP request in 3.0? It's  
going to be much cleaner simply to grab whatever parameters Stripes  
binds to the ActionBean via their accessors (they will correctly  
typed, among other things). E.g., if Stripes binds the "page" request  
parameter to an ActionBean's property of type WikiPage, why not just  
use getPage() to get it, rather than go through trouble of getting the  
request from the context, calling getParameter(), looking for nulls  
and misspellings etc...? So much could go wrong, and Stripes takes  
care of this for you. So, in 3.0 a developer commandment might be:

Thy plugins shalt not assume that HttpServletRequests are available,  
and shalt not parse its parameters.

In 2.8, clearly we **can't** make a "don't depend on the request"  
recommendation...

Aside: one of the things that pleases me the most about Stripes is  
that its parameter-binding glue allows us to decouple the request from  
the WikiContext. Likewise, the Resolution and related UrlBuilder  
classes decouple it from the response.

> The reason we call textToHTML() is to make sure the access rules and  
> metadata are correct after save.  This is needed because of the  
> CachingProvider, and is one of the reasons why our metadata system  
> is so crappy and kludgy and I can't wait to start fixing it in 3.0...

Hah! I knew it was something like that. It's never good to call a  
method just because it has magic "side effects"; ideally we'd have a  
non-kluge-y way to do thus... But there is no point in extracting the  
magic bits from the renderer code that (in essence) do the metadata  
refreshing. I completely agree that we can't solve this until 3.0.

> Looking at the issues involved, I think it probably makes more sense  
> to stuff this to 3.0 and see if we can clarify the role of the  
> WikiContext a bit more.  If WikiContext is going to be a Stripes  
> ActionBeanContext in the future, we've got to make sure that is  
> serializable too, of which I am not too sure about.

I do not completely understand what you mean here, but I do not think  
that WikiContext should be serializable. Today, it contains references  
to too several things that have time-dependent states  
(HttpServletRequests go out of scope; HttpSessions expire, etc.). In  
the future, it would be extremely difficult to make WikiContexts/ 
ActionBeans serializable because that would mean all of their  
properties would have to be serializable too. Not too hard with a  
concrete class, but probably impossible with an interface, which is  
what ActionBean is... would impede flexibility way too much.

Re: Shed light on how PageManager saves pages?

[Janne Jalkanen <Ja...@ecyrd.com>]

> My question is, is there anything special about the WikiContext  
> that we retrieve, which is passed to textToHTML() and  
> doPostSaveFiltering()? Could we create a synthetic one instead? In  
> other words, I am proposing that in PreSaveTask we stash the  
> WikiPage name as a string, then pick it up in SaveTask and use it  
> to manufacture a new, fake WikiContext with the correct WikiPage.

Yup, we've got a problem.  Most things in the WikiContext are such  
that they do not survive serialization - WikiEngine,  
HttpServletRequest, even WikiPage (due to interactions with the  
CachingManager).

It's a good question whether they even do survive leaving the  
WikiSession and being executed in somebody else's context (the  
HttpServletRequest comes to mind).  Now, since SavePageTask.execute()  
invokes the parser, that means that user plugins are also invoked,  
and if you've got someone relying on HttpServletRequest parameters at  
the time of save parsing - well, boom for them. (Not that I would see  
that anyone would do that.)

> Conversely, if the WikiContext used by textToHTML() and  
> doPostSaveFiltering() *has to be* the same WikiContext (and not  
> created synthetically), then we've got to keep stashing it as a  
> workflow attribute, and thus (here's the punchline)... we will  
> never to be able to persist workflows.

Yes.

The reason we call textToHTML() is to make sure the access rules and  
metadata are correct after save.  This is needed because of the  
CachingProvider, and is one of the reasons why our metadata system is  
so crappy and kludgy and I can't wait to start fixing it in 3.0...

Looking at the issues involved, I think it probably makes more sense  
to stuff this to 3.0 and see if we can clarify the role of the  
WikiContext a bit more.  If WikiContext is going to be a Stripes  
ActionBeanContext in the future, we've got to make sure that is  
serializable too, of which I am not too sure about.

/Janne