Gaelyk + sitemesh

[Richard Vowles]

Hi guys,

I tried folding sitemesh in filter only into gaelyk as a precursor to
knowing how it might fold into grails. "Not working" really helps focus
one's understanding :-)

Must say I was surprised to find java.util.logging being used instead of
slf4j (which seems the industry standard now), but that aside - gaelyk has
the controller  servlet forward to the view servlet and this seems to cause
sitemesh to fail to process the request. I put some of my own logging in,
and found that the "should i buffer" is being asked twice, but haven't yet
figured out what the problem is.

I do prefer the mechanism grails uses to specify the decorator (the element
in the head tag at the top), so I am going to need to delve a bit deeper
into understanding how it all fits together in any case.

Richard

--
---
Richard Vowles,
Grails, Groovy, Java
Consistency is the last refuge of the unimaginative - Oscar Wilde
ph: +64275467747, linkedin, skype:rvowles
get 2Gb shared disk space in the cloud - Dropbox, its incredibly useful! - http://tinyurl.com/cmcceh
podcast: http://www.illegalargument.com

[Joe Walnes]

Hi Richardm

On Sun, Oct 25, 2009 at 2:54 PM, Richard Vowles <richard...@gmail.com> wrote:

Hi guys,

I tried folding sitemesh in filter only into gaelyk as a precursor to
knowing how it might fold into grails. "Not working" really helps focus
one's understanding :-)

Thanks for the feedback. It's good to have the tires kicked a little and this is good timing as I'd like to get a new version pushed out in the next 2 weeks.

I've never heard of Gaelyk - I'll check it out.

Must say I was surprised to find java.util.logging being used instead of
slf4j (which seems the industry standard now), 

The core of SM is currently dependency-less and I'd need to have good justification for changing this (particularly as logging is only done in one class). To be honest, I'd rather remove the logging altogether, than add a new dep.

but that aside - gaelyk has
the controller  servlet forward to the view servlet and this seems to cause
sitemesh to fail to process the request. I put some of my own logging in,
and found that the "should i buffer" is being asked twice, but haven't yet
figured out what the problem is.

So, is the problem caused when a servlet dispatches to another servlet?

I can create a test for this and have a go at fixing (unless you're already on the case).

I do prefer the mechanism grails uses to specify the decorator (the element
in the head tag at the top), so I am going to need to delve a bit deeper
into understanding how it all fits together in any case.

SM3 has the extension points to do this. At the moment, you have to write some code, but I'd like this to work out of the box before the final release - now would be a good opportunity to get your name in the code ;)

Here's the approach:

1. Define a custom TagRuleBundle (used by the ContentProcessor) that will extract the decorator name from the markup and add it as a property to the Content model. 

If you are getting this from the <meta> tag, you don't need to do anything as the default (CoreHtmlTagRuleBundle) already extracts <meta> tags.

2. Define a custom DecoratorSelector implementation, that reads the property from the Content model and tells SM to use that as the decorator.

3. Install the TagRuleBundle and DecoratorSelector by adding them to SiteMeshBuilder.

thanks

-Joe

[Richard Vowles]

On Mon, Oct 26, 2009 at 10:54 AM, Joe Walnes <j...@walnes.com> wrote:

but that aside - gaelyk has
the controller  servlet forward to the view servlet and this seems to cause
sitemesh to fail to process the request. I put some of my own logging in,
and found that the "should i buffer" is being asked twice, but haven't yet
figured out what the problem is.

So, is the problem caused when a servlet dispatches to another servlet?

I can create a test for this and have a go at fixing (unless you're already on the case).

It does appear to be - and I'd like to try and fix it :-)

I do prefer the mechanism grails uses to specify the decorator (the element
in the head tag at the top), so I am going to need to delve a bit deeper
into understanding how it all fits together in any case.

SM3 has the extension points to do this. At the moment, you have to write some code, but I'd like this to work out of the box before the final release - now would be a good opportunity to get your name in the code ;)

Here's the approach:

1. Define a custom TagRuleBundle (used by the ContentProcessor) that will extract the decorator name from the markup and add it as a property to the Content model. 

If you are getting this from the <meta> tag, you don't need to do anything as the default (CoreHtmlTagRuleBundle) already extracts <meta> tags.

2. Define a custom DecoratorSelector implementation, that reads the property from the Content model and tells SM to use that as the decorator.

3. Install the TagRuleBundle and DecoratorSelector by adding them to SiteMeshBuilder.

Fantastic! Was going to use the meta tag, so I will get onto this as well :-)

Thanks again!

Richard 

[Richard Vowles]

Ok, so the first possible problem I have found appears when the servlet is returning an error code and has data in the buffer. bufferAndPostProcess in ContentBufferingFilter.java appears to have just this:

if (buffer != null ) {

when it needs to have this (maybe?):

if (buffer != null && !responseBuffer.isCommitted()) {

Putting that line in stops  the Jetty "IllegalStateException: Committed" errors when errors occur...

Now, back to the forwarding issue...

[Richard Vowles]

And the problem I am encountering is that the BasicSelector appears to be exactly backwards in its status code checking?

    public boolean shouldAbortBufferingForHttpStatusCode(int statusCode) {

        return (statusCode == 200 || includeErrorPages && statusCode >= 400;

    }

This appears to say "abort the buffering if I get a 200 response or I should include error pages and the status code is above 400". Changing it to return the exact opposite:

    public boolean shouldAbortBufferingForHttpStatusCode(int statusCode) {

        return !(statusCode == 200 || includeErrorPages && statusCode >= 400);

    }

Seemed to do the trick. I will rollback all my logging additions and see if I can create tests for them, that sound sensible?

[Joe Walnes]

Yep. Fork the project on GitHub and push your changes there so we can review and integrate them as you go.

[Richard Vowles]

I just did one (a fork), I have another test I am trying out now.

A question: To me, if shouldAbortBufferingForHttpStatusCode returns TRUE, then we should not post process the file. Is this a correct assumption?

The problem occuring is that in the HttpServletResponseBuffer, when a status code of (say) 404 is set then a BasicSelector that has includeErrorPages set to false (which all of the contentbufferingfilter tests do) causes the buffer to be aborted. So far no problem.

*but* if someone's code then goes and writes anything else to the buffer, the buffer gets recreated. So something like 

setStatusCode(404);

setContentType("text/html"); <--- this line causes the buffer to be enabled again

Since it is re-enabled, the buffer automatically gets post processed, which IMHO isn't what is expected? 

IMHO, the Selector needs to be re-checked for its criteria when it drops out of the filter chain - so even if you have a proper buffer, you may not wish to post process it. Yes/no?