aggregate-fallback bug and improvement suggestions..

hi *!

unless i'm very much mistaken, aggregate-fallback does not
handle 
namespaces correctly. i saw a case where the new common
document element 
did not retain its namespace. i think this piece of code in

AggregatingSource.java might be wrong:

Element docElement = sourceDom.getDocumentElement();
if (this.dom == null) {
     String namespaceUri = docElement.getNamespaceURI();
     String prefix = docElement.getPrefix();
     String localName = docElement.getLocalName();

     if (namespaceUri == null || prefix == null) {
        this.dom = DocumentHelper.createDocument(null,
localName, null);
     } else {
         NamespaceHelper helper = new
NamespaceHelper(namespaceUri,
                                             
prefix,localName);
         this.dom = helper.getDocument();
     }

if the source document uses a default namespace, prefix will
probably be 
null. in that case, the namespace is wrongly discarded. i'm
not familiar 
enough with sax namespace handling to tackle that - anyone?
looks like a 
trivial fix, but i fear i might miss something.

while we're at it, i think the behaviour of
aggregate-fallback is not 
optimal. it would be a lot more flexible if it introduced a
new document 
element instead of squeezing everything into the document
element of the 
  first sourceURI.
as it is now, information is lost, as there is no way of
telling which 
of the nodes used to belong to the same file. (if anyone
wants to know 
why this is interesting, i can give examples.)
moreover, the cocoon aggregator does introduce a new
document element, 
and it would be nice if our AggregatingSource would behave
accordingly.
not being able to specify a document element name and
namespace is not a 
problem: just set it to <aggregation-wrapper/>, no
namespace, and xsls 
down the pipeline can match it with "/*" without
every having to care 
what the name actually is.


wdyt?


regards,

jörn


------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribelenya.apache.org
For additional commands, e-mail: dev-helplenya.apache.org

Andreas Hartmann wrote:
> Joern Nettingsmeier schrieb:
>> hi *!
>>
>> unless i'm very much mistaken, aggregate-fallback
does not handle
>> namespaces correctly. i saw a case where the new
common document element
>> did not retain its namespace.
> 
> Would you mind filing a bug and describe how to
reproduce the problem?

i will. if you have the time, would you mind checking out
the branch i 
created yesterday? there is a problem now with 
webapp/lenya/xslt/modules/modules2xinclude.xsl - it does not
work, and i 
thought it might be due to the fact that the document
element now has a 
namespace.

>> i think this piece of code in
>> AggregatingSource.java might be wrong:
>>
>> Element docElement =
sourceDom.getDocumentElement();
>> if (this.dom == null) {
>>     String namespaceUri =
docElement.getNamespaceURI();
>>     String prefix = docElement.getPrefix();
>>     String localName = docElement.getLocalName();
>>
>>     if (namespaceUri == null || prefix == null) {
>>        this.dom =
DocumentHelper.createDocument(null, localName, null);
>>     } else {
>>         NamespaceHelper helper = new
NamespaceHelper(namespaceUri,
>>                                             
prefix,localName);
>>         this.dom = helper.getDocument();
>>     }
>>
>> if the source document uses a default namespace,
prefix will probably be
>> null.
> 
> Actually it should be an emtpy string.

ah, ok. so my conclusion might be wrong. i need to
investigate further.

>> while we're at it, i think the behaviour of
aggregate-fallback is not
>> optimal. it would be a lot more flexible if it
introduced a new document
>> element instead of squeezing everything into the
document element of the
>>  first sourceURI.
> 
> IMO the purpose of aggregate-fallback is to aggregate
sources of the
> same kind, and they should have the same document
element. Maybe we
> should add a check for this?

i don't think we should anticipate the way a source will be
used and put 
artificial constraints on its functionality based on such
guesses.
especially not if it helps to simplify the code.
in this case, a special case handler could be omitted, which
is always a 
good thing.

>> as it is now, information is lost, as there is no
way of telling which
>> of the nodes used to belong to the same file. (if
anyone wants to know
>> why this is interesting, i can give examples.)
> 
> OK, this is a good point.
> Attributes of the document element are another source
of problems.

important point.

>> moreover, the cocoon aggregator does introduce a
new document element,
>> and it would be nice if our AggregatingSource would
behave accordingly.
> 
> OK
> 
>> not being able to specify a document element name
and namespace is not a
>> problem: just set it to
<aggregation-wrapper/>,
> 
> Or maybe just <aggregate>?

i don't care. it's just that i used aggregation-wrapper
whenever i had 
to use map:aggregate, to make it explicit that nobody should
care what 
the name of this element is.

>> no namespace,
> 
> -1, I'd rather use a distinct namespace.

i think the use of a namespace implies that it is properly
defined and 
documented. which means extra work, but ok.
but: what we really want to say is, ignore this element,
it's a zombie 
thing that came out of thin air. i really want to encourage
people to 
match it as "/*" in their xsls and not assume
anything about it. if it 
gets a namespace, people might assume it has actual meaning
and match it 
by name...
well, either way, i don't care too much.

>> wdyt?
> 
> +1 to introduce a wrapper element, but it should be
optional, so that
> you don't have to strip it if you don't need it. This
would mean that we
> need two protocols, or an URL parameter.

let's honor the KISS principle here and have only one
protocol and one 
behaviour, no options, no configurations. URL parameters are
really 
really painful imnsho...
i volunteer to wade through the source and fix all xslts to
match the 
new wrapper element (which is easy and does not muck up the
xslts that much)

> Maybe we should also add a src attribute to all
aggregated document
> elements:
> 
> <agg:aggregate>
>   <catalogue agg:src="file:///...">
>     ...
>   </catalogue>
>   ...
> </agg:aggregate>

very neat idea. ok, i see why a namespace is useful - you
win ;)

do you think you could implement this change? i'm still not
much into 
SAX... perhaps you could use the branch, so that i can fix
the xslts 
afterwards, without breaking trunk until it's done?



------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribelenya.apache.org
For additional commands, e-mail: dev-helplenya.apache.org

Joern Nettingsmeier wrote:
> Andreas Hartmann wrote:
>> Joern Nettingsmeier schrieb:
>>> hi *!
>>>
>>> unless i'm very much mistaken,
aggregate-fallback does not handle
>>> namespaces correctly. i saw a case where the
new common document element
>>> did not retain its namespace.
>>
>> Would you mind filing a bug and describe how to
reproduce the problem?
> 
> i will. if you have the time, would you mind checking
out the branch i 
> created yesterday? there is a problem now with 
> webapp/lenya/xslt/modules/modules2xinclude.xsl - it
does not work, and i 
> thought it might be due to the fact that the document
element now has a 
> namespace.
> 

here's what comes out of the aggregate-fallback:// source
when 
publication.xconf is moved to a namespace:

<publication>
   <name xmlns="http://apache.org/cocoon/lenya/publication/1.1&qu
ot;>Default 
Publication</name>
....

</publication>

as you can see, the document element does not retain its
namespace, and 
the inner elements all get individual namespace nodes.

to reproduce:

svn co https://svn.apache.org/repos/asf/lenya/branc
hes/trunk-rework-pubconf

build, run, go to default pub, import content. you will
notice that in 
the file menu, the "New <resource-type>
Document" items are missing.

check out 
http://localhost:8888/modules/profiling/profile.html 
(profiling is already enabled for the pipeline in
question).

------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribelenya.apache.org
For additional commands, e-mail: dev-helplenya.apache.org