DO NOT REPLY [Bug 31012] New: - CachingPipeline validity and new IncludeTransformer

[bu...@apache.org]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG 
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=31012>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND 
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=31012

CachingPipeline validity and new IncludeTransformer

           Summary: CachingPipeline validity and new IncludeTransformer
           Product: Cocoon 2
           Version: 2.1.5
          Platform: PC
        OS/Version: Linux
            Status: NEW
          Severity: Normal
          Priority: Other
         Component: general components
        AssignedTo: dev@cocoon.apache.org
        ReportedBy: oliver.powell@tvnz.co.nz
                CC: unico@hippo.nl


This bug is related to an earlier closed bug #30356.

We are now using Unico's new
org.apache.cocoon.transformation.IncludeTransformer, which is working
fantastically for us. In a nutshell, the key feature of this new class for us is
that it 'takes on' its included source's validity as its own validity after
processing the included source/pipeline. This means the validities of included
content 'bubble up'. However, I believe this new Transformer has exposed a bug
with the way the CachingProcessingPipeline returns its validity in its
getValidityForEventPipeline method. 

The problem occurs when a pipeline that contains an IncludeTransformer step is
invoked and a valid CachedResponse is found for it. (All this action happens
primarily in the AbstractCachingProcessingPipeline class). The cached response
is validated and returned fine, but the problem is later when
getValidityForEventPipeline is called on the pipeline processing instance. 

This method fetches the validity objects from all of its cacheable components in
the pipeline (which in this case have only just been initialised but not
executed, because a cached response was found), chucks them all in an
AggregatedValidity and sends this back representing the validity of the entire
pipeline. This method is used by the SitemapSource class to set its own validity.

The problem here is that the IncludeTransformer's validity doesn't get populated
until it is executed, as naturally it doesn't know what the validity of the
included source is until is resolves it. So calling
IncludeTransformer.getValidity when it hasn't been executed (which is what
getValidityForEventPipeline does when a valid CachedResponse is found) will
always return an empty MultiSourceValidity, which is of course invalid.

Therefore, the Source validity for this pipeline will always be invalid because
the IncludeTransformer step validity is not correct.

My fix:
========
The thing is, when you have a valid CachedResponse, you actually already have
all the correct validities for the pipeline inside it. There is no need to
re-fetch them by calling getValidity on each pipeline component. So, I altered
getValidityForEventPipeline in AbstractCachingProcessingPipeline to use the
validities that were inside the CachedResponse, if one exists and is valid. The
method now looks like this:

    public SourceValidity getValidityForEventPipeline() {

        //***** This if block was added by OliverP ******
        if (this.cachedResponse != null) {

            final AggregatedValidity validity = new AggregatedValidity();
            for (int i=0; i < this.toCacheSourceValidities.length; i++) {
                validity.add(this.toCacheSourceValidities[i]);                
            }          

            if (this.getLogger().isDebugEnabled()) {
                this.getLogger().debug("OliverP: getValidityForEventPipeline:
returning cached response validity: " + validity);
            }

            return validity;

        } else {

            int vals = 0;
            
            if ( null != this.toCacheKey
                 && !this.cacheCompleteResponse 
                 && this.firstNotCacheableTransformerIndex ==
super.transformers.size()) {
                 vals = this.toCacheKey.size();
            } else if ( null != this.fromCacheKey
                         && !this.completeResponseIsCached
                         && this.firstProcessedTransformerIndex ==
super.transformers.size()) {
                 vals = this.fromCacheKey.size();
            }
            if ( vals > 0 ) {
                final AggregatedValidity validity = new AggregatedValidity();
                for(int i=0; i < vals; i++) {
                    validity.add(this.getValidityForInternalPipeline(i));
                    //validity.add(new DeferredPipelineValidity(this, i));
                }
                return validity;
            }
            return null;
        }
    }

The CachedResponse's validities have been stored in the toCacheSourceValidities
member variable. I altered the validatePipeline method in
AbstractCachingProcessingPipeline, near the end, to update this variable when it
has been determined that the CachedResponse is valid - before we lose the handle
on the CachedResponse object. This is the code block (about line #563) from the
validatePipeline method:

                if (responseIsValid) {
                    if (this.getLogger().isDebugEnabled()) {
                        this.getLogger().debug("validatePipeline: using valid
cached content for '" + environment.getURI() + "'.");
                    }
                    // we are valid, ok that's it
                    this.cachedResponse = response.getResponse();
                    this.cachedLastModified = response.getLastModified();
                    
                    //*****  Added by OliverP ***** 
                    this.toCacheSourceValidities = fromCacheValidityObjects;
                    
                } else {
                    if (this.getLogger().isDebugEnabled()) {
                        this.getLogger().debug("validatePipeline: cached content
is invalid for '" + environment.getURI() + "'.");
                    }
                    // we are not valid!
                    ......

After this, it worked great! (so far...)

I haven't got subversion going yet, so I'm not able to submit a diff format to
make these changes clearer. If anyone wants this, let me know and I'll try and
whizz one up. 

Any comments? Any pitfalls with this fix? If this is a valid bug, it would be
great if a fix could be added to the next release.

Thanks in advance,
Oliver Powell