[rfc] commons fileupload replacement

[Jorg Heymans <jh...@domek.be>]

Hi,

I have provided a patch [1] that removes cocoon's multipart upload
parser with one based on commons-fileupload.

My approach breaks existing client code however, so Antonio suggested to
get a few more opinions before continueing.

1) Part, PartOnDisk, PartInMemory are not necessary anymore. They are
replaced by the FileItem interface of commons fileupload. It should be
doable to wrap the FileItems in a Part class as suggested by Reinhard
(with a slight change to the contract, see  [1] ) .

2) web.xml parameter "overwrite-uploads" is gone. Instead, we need a
parameter like "where-to-store-uploads" with values "disk" or "memory".
In addition to this (not included in the patch) commons-fileupload
offers a "threshold" for memory based storage, files larger than the
threshold will get written to disk anyway - we don't *have* to use this
however.

3) There is no replacement for getHeaders() in Part.java.

The nice thing to do would be to deprecate the Part classes, and wrap
the FileItem interface inside Part for 2.1.6. , remove Part in 2.1.7?

Or is 2.1.6 too soon for this?


Thoughts?
Jorg


[1] http://nagoya.apache.org/bugzilla/show_bug.cgi?id=32102

Re: [rfc] commons fileupload replacement

[Sylvain Wallez <sy...@apache.org>]

Jorg Heymans wrote:

> Hi,
>
> I have provided a patch [1] that removes cocoon's multipart upload
> parser with one based on commons-fileupload.
>
> My approach breaks existing client code however, so Antonio suggested to
> get a few more opinions before continueing.
>
> 1) Part, PartOnDisk, PartInMemory are not necessary anymore. They are
> replaced by the FileItem interface of commons fileupload. It should be
> doable to wrap the FileItems in a Part class as suggested by Reinhard
> (with a slight change to the contract, see  [1] ) .


PartOnDisk and PartInMemory are particular implementations of the 
general Part contract. An application can rely on obtaining a Part, but 
cannot expect to have a particular implementation of it if it aims to be 
truly independent of the deployment environment.

However, testing for "instanceof PartOnDisk" may allow for performance 
improvements by allowing the application to move around a file rather 
than reading it and writing it in another location. Commons upload takes 
a different approach for this with the "write(File)" method that 
abstracts this. We could add this convenience method to Part to avoid 
the above-mentioned "instanceof" test.

So my opinion is that we should have a "CommonsUploadPart" that 
implements the original Part contract. Especially important is the 
"disposeWithRequest" stuff that is used by CForms: the default is to 
automatically trash uploaded content when a request ends, but the CForms 
upload widget can "detach" a Part from the requests so that uploaded 
content survives across continuation calls.

> 2) web.xml parameter "overwrite-uploads" is gone. Instead, we need a
> parameter like "where-to-store-uploads" with values "disk" or "memory".
> In addition to this (not included in the patch) commons-fileupload
> offers a "threshold" for memory based storage, files larger than the
> threshold will get written to disk anyway - we don't *have* to use this
> however.
>
> 3) There is no replacement for getHeaders() in Part.java.


Mmmh... let's keep it and map it the headers provided as methods in 
FileItem (content-type, length, etc).

> The nice thing to do would be to deprecate the Part classes, and wrap
> the FileItem interface inside Part for 2.1.6. , remove Part in 2.1.7?


Rather than deprecating Part, we should maybe move it to 
o.a.c.environment, as multipart requests can exist in environments other 
than servlets (i.e. SMTP).

Sylvain

-- 
Sylvain Wallez                                  Anyware Technologies
http://www.apache.org/~sylvain           http://www.anyware-tech.com
{ XML, Java, Cocoon, OpenSource }*{ Training, Consulting, Projects }

Re: [rfc] commons fileupload replacement

[Geoff Howard <ge...@gmail.com>]

On Mon, 8 Nov 2004 14:16:45 -0600 (CST), Luigi Bai
<lp...@focalpoint.com> wrote:
> 
> On Mon, 8 Nov 2004, Geoff Howard wrote:
> 
> > On Mon, 8 Nov 2004 08:20:56 -0600 (CST), Luigi Bai
> > <lp...@focalpoint.com> wrote:
...

> I read your reference and Stefano's proposal. 

Excellent.

> I agree with the concept of
> having request interceptors, but I am not sure I agree with his syntactic
> proposal. In particular, it requires the creation of yet another type of
> sitemap component, meaning that it is heavily integrated into the Cocoon
> core. Why is that necessary?

Well, not really.  While sitemap statements are often also components
I don't think they always are or need to be IIUC.  At the end of the
day, they are nodes recognized by the tree processor.  We have no
construct (including actions) which allow the raw request object
(besides attributes) to be modified, or re-interpreted in any way.  To
grant this power to existing sitemap elements would be dangerous (in
the sense of ripe for abuse or confusion).

> Despite your characterization of Actions (below), I think they are a valid
> method for providing "cross cutting" functionality in a lightweight
> fashion. They don't require a user to implement PipelineNodes and all that
> jazz. And if the functionality proves to be stable and useful, then you
> can "harden" it into more formal syntax.

To clarify, once the proposed "adapt request" support exists, we would
almost certainly bundle and configure by default the very few
capabilities which would require it - multipart file handling would
almost certainly be part of this.  So, users wouldn't have to
implement PipelineNodes unless they had some unique new requirement or
wanted to write an alternative.  Further, this would likely be a
fairly simple contract anyway.

> The problem with Actions is that while they're flexible and lightweight,
> they cannot have access to all the parts of a pipeline that a Node can. I
> lean toward allowing an Action to tell the Environment to replace its
> Request object, allowing it to do the SOAP/DAV/upload processing in the
> Action. Yes, this can allow people to shoot themselves in the foot. But I
> think the benefits outweigh the risk.

:) Most would say that the problem is that Actions have access to too much!

I think though that we're just on some semantics based on the way you
have mentally categorized actions.

Again, the problem as I understand it with actions is that they: 
1) Can perform back-end "actions".
2) Can decide between alternate paths/steps in the sitemap.  
3) Can also completely redirect the request
4) Can also return values back to the sitemap which can trigger later
decision points using any other sitemap directive.

None of these are bad in them selves - it's the mixing of all of them.
 Adding the ability to modify the way in which the request is made
available to later steps in processing would only muddy things
further.

> Otherwise, a valid "hack" would be to have an Action that processed the
> multipart stream (like StreamGenerator) but which put its Parts under the
> RequestAttributes (the only part of the Request it's allowed to
> manipulate). Downstream components would then have to get the parts from
> the Attributes, not Params, but that'd be a contract to observe, nothing
> special.

I think this is why this needs to be handled deeper in the core (the
request adapting, not necessarily the multipart parsing).  And to
reiterate, would then not need to be re-done by people (just as it is
not now re-done by everyone needing to use multipart uploads) but
rather just utilized.

Anyone care to chip in with a path forward?  Can we do "adapt-request" now?

Geoff

> >> By the way, I'm not clear on why Multipart support can't be implemented as
> >> an Action? If it's an Action, then it's clearly pluggable, and all this
> >> goes away. Why is it so early in the process, hard coded in CocoonServlet?
> >
> > - Actions were a muddy concept that have been or can be at least
> > partially if not fully replaced by other items.
>

Re: [rfc] commons fileupload replacement

[Luigi Bai <lp...@focalpoint.com>]

On Mon, 8 Nov 2004, Geoff Howard wrote:

> On Mon, 8 Nov 2004 08:20:56 -0600 (CST), Luigi Bai
> <lp...@focalpoint.com> wrote:
>> I'd like to point out that the Multipart support in Cocoon is one of the
>> few things that isn't pluggable or replaceable. I'd like it to be possible
>> to configure web.xml/CocoonServlet to take a class name that implements
>> some MultipartParser interface, and keep Part as an interface (perhaps
>> letting it evolve as different implementations are available).
>
> Some historical/background points (with the caveat that some changes
> have happened due to CForms that I have not paid full attention to):
> - We also have tried to avoid anti-patterns like "over
> componentization" i.e., making things pluggable that really don't need
> to be, which was probably judged to be the case here (though see
> below).
> - There were other grander plans for request handling.  In particular,
> see the "Pluggable Request Factories" of Stefano's RT here:
> http://marc.theaimsgroup.com/?l=xml-cocoon-dev&m=106131747919501&w=2
> - You'll notice a third point in the RT above: our "pluggable" problem
> extends beyond multipart uploads.  It includes things like WebDAV and
> SOAP.
>

I read your reference and Stefano's proposal. I agree with the concept of 
having request interceptors, but I am not sure I agree with his syntactic 
proposal. In particular, it requires the creation of yet another type of 
sitemap component, meaning that it is heavily integrated into the Cocoon 
core. Why is that necessary?

Despite your characterization of Actions (below), I think they are a valid 
method for providing "cross cutting" functionality in a lightweight 
fashion. They don't require a user to implement PipelineNodes and all that 
jazz. And if the functionality proves to be stable and useful, then you 
can "harden" it into more formal syntax.

The problem with Actions is that while they're flexible and lightweight, 
they cannot have access to all the parts of a pipeline that a Node can. I 
lean toward allowing an Action to tell the Environment to replace its 
Request object, allowing it to do the SOAP/DAV/upload processing in the 
Action. Yes, this can allow people to shoot themselves in the foot. But I 
think the benefits outweigh the risk.

Otherwise, a valid "hack" would be to have an Action that processed the 
multipart stream (like StreamGenerator) but which put its Parts under the 
RequestAttributes (the only part of the Request it's allowed to 
manipulate). Downstream components would then have to get the parts from 
the Attributes, not Params, but that'd be a contract to observe, nothing 
special.

>
>> By the way, I'm not clear on why Multipart support can't be implemented as
>> an Action? If it's an Action, then it's clearly pluggable, and all this
>> goes away. Why is it so early in the process, hard coded in CocoonServlet?
>
> - Actions were a muddy concept that have been or can be at least
> partially if not fully replaced by other items.

Re: [rfc] commons fileupload replacement

[Luigi Bai <lp...@focalpoint.com>]

On Mon, 8 Nov 2004, Geoff Howard wrote:
> On Mon, 8 Nov 2004 08:20:56 -0600 (CST), Luigi Bai
> <lp...@focalpoint.com> wrote:
>> I'd like to point out that the Multipart support in Cocoon is one of the
>> few things that isn't pluggable or replaceable. I'd like it to be possible
>> to configure web.xml/CocoonServlet to take a class name that implements
>> some MultipartParser interface, and keep Part as an interface (perhaps
>> letting it evolve as different implementations are available).
>
> Some historical/background points (with the caveat that some changes
> have happened due to CForms that I have not paid full attention to):
> - We also have tried to avoid anti-patterns like "over
> componentization" i.e., making things pluggable that really don't need
> to be, which was probably judged to be the case here (though see
> below).
> - There were other grander plans for request handling.  In particular,
> see the "Pluggable Request Factories" of Stefano's RT here:
> http://marc.theaimsgroup.com/?l=xml-cocoon-dev&m=106131747919501&w=2
> - You'll notice a third point in the RT above: our "pluggable" problem
> extends beyond multipart uploads.  It includes things like WebDAV and
> SOAP.
>

Good points.

>> Keeping the integration at the level of Interface means we don't run into
>> the same kind of rigamarole that is going on with ECM etc.
>
> True, although another major lesson learned from ECM was that external
> communities can also let you down.  The last time I monitored the
> commons list, I didn't get warm fuzzies about the politics and health
> there, for what that's worth.
>

That's precisely what I mean...

>> By the way, I'm not clear on why Multipart support can't be implemented as
>> an Action? If it's an Action, then it's clearly pluggable, and all this
>> goes away. Why is it so early in the process, hard coded in CocoonServlet?
>
> - Actions were a muddy concept that have been or can be at least
> partially if not fully replaced by other items.
> - Why so early in the process is another good question.  Two reasons I
> remember are:
> 1) Cocoon is not only a CocoonServlet but also runs in different
> environments (Command Line for example) where multipart requests do
> not have exact parallel if they exist at all.  We have tried to avoid
> dependencies on Servlet specific APIs in part due to this issue.
> Combined with the fact that Cocoon's uri abstraction (all uris are
> mapped to the CocoonServlet, making total control in sitemap possible)
> makes it possible that relying on Commons upload may turn out to be a
> bad idea in the end: they do not have the same complexity of concerns.
> In fact, they've had some recent refactoring there to accomodate
> portlet environments.  If someone can give an analysis of whether this
> helps us or hurts us with respect to our complex needs, that would be
> a big plus.
> 2) The cocoon framework as many frameworks pre-parses the request to
> make all parameters available to the pipeline semantics.  The plan of
> adaptRequest was to allow this to take place for most parameters up
> front, but the multipart bits later.  The work to do this was never
> done.
>

Ah, a generic request adaptation framework.

> Any patch that addresses all these concerns would probably get some
> good attention.
>
> Short of that, a patch that provides backwards compatibility by
> wrapping the commons library is next best in my opinion.  Short of
> full pluggable request adaptors, encapsulation is as good as
> configurability in this case IMO.  In this case, I think people will
> be fine with one solution for the project, as long as it's easy to rip
> out and replace if necessary.
>

That's why I'm suggesting having an Interface and an init-param that 
allows you to load a class implementing that Interface. Right now if 
multipart is not fixed/patched (see below) in 2.1.6 then we need to 
"carry" this patch forward to the new source tree if/when we use 2.1.6. 
This is in contrast to almost anything else (Generators, Serializers, 
Transformers) that we just fork off into our own namespace, patch or 
extend, and pull out of our own jar at run time.

I suppose we could fork the whole CocoonServlet.

> Earlier someone noted the comment in the code about multipart/mixed.
> I am not an expert here, but when I looked into this some time ago, it
> was not supported by browsers.  We do support multiple files per form
> using multiple input elements, so full conformance with the spec would
> be academic.  Am I wrong here?
>
> Geoff
>

Not academic; see http://issues.apache.org/bugzilla/show_bug.cgi?id=31067

We are using a script with "curl" to upload multiple files on one input 
element (this is for a list of images to a slide show style viewer).

Re: [rfc] commons fileupload replacement

[Geoff Howard <ge...@gmail.com>]

On Mon, 8 Nov 2004 08:20:56 -0600 (CST), Luigi Bai
<lp...@focalpoint.com> wrote:
> I'd like to point out that the Multipart support in Cocoon is one of the
> few things that isn't pluggable or replaceable. I'd like it to be possible
> to configure web.xml/CocoonServlet to take a class name that implements
> some MultipartParser interface, and keep Part as an interface (perhaps
> letting it evolve as different implementations are available).

Some historical/background points (with the caveat that some changes
have happened due to CForms that I have not paid full attention to):
- We also have tried to avoid anti-patterns like "over
componentization" i.e., making things pluggable that really don't need
to be, which was probably judged to be the case here (though see
below).
- There were other grander plans for request handling.  In particular,
see the "Pluggable Request Factories" of Stefano's RT here:
http://marc.theaimsgroup.com/?l=xml-cocoon-dev&m=106131747919501&w=2
- You'll notice a third point in the RT above: our "pluggable" problem
extends beyond multipart uploads.  It includes things like WebDAV and
SOAP.

> Keeping the integration at the level of Interface means we don't run into
> the same kind of rigamarole that is going on with ECM etc.

True, although another major lesson learned from ECM was that external
communities can also let you down.  The last time I monitored the
commons list, I didn't get warm fuzzies about the politics and health
there, for what that's worth.

> By the way, I'm not clear on why Multipart support can't be implemented as
> an Action? If it's an Action, then it's clearly pluggable, and all this
> goes away. Why is it so early in the process, hard coded in CocoonServlet?

- Actions were a muddy concept that have been or can be at least
partially if not fully replaced by other items.
- Why so early in the process is another good question.  Two reasons I
remember are:
1) Cocoon is not only a CocoonServlet but also runs in different
environments (Command Line for example) where multipart requests do
not have exact parallel if they exist at all.  We have tried to avoid
dependencies on Servlet specific APIs in part due to this issue.
Combined with the fact that Cocoon's uri abstraction (all uris are
mapped to the CocoonServlet, making total control in sitemap possible)
makes it possible that relying on Commons upload may turn out to be a
bad idea in the end: they do not have the same complexity of concerns.
 In fact, they've had some recent refactoring there to accomodate
portlet environments.  If someone can give an analysis of whether this
helps us or hurts us with respect to our complex needs, that would be
a big plus.
2) The cocoon framework as many frameworks pre-parses the request to
make all parameters available to the pipeline semantics.  The plan of
adaptRequest was to allow this to take place for most parameters up
front, but the multipart bits later.  The work to do this was never
done.

Any patch that addresses all these concerns would probably get some
good attention.

Short of that, a patch that provides backwards compatibility by
wrapping the commons library is next best in my opinion.  Short of
full pluggable request adaptors, encapsulation is as good as
configurability in this case IMO.  In this case, I think people will
be fine with one solution for the project, as long as it's easy to rip
out and replace if necessary.

A third option:  The entire project at Commons as far as I can see is
about 15 classes and hasn't had a release in almost a year and a half.
 (I'm guessing this is because the RFC for file uploads has not
changed and probably isn't expected to?).  In this case, I think it's
entirely reasonable to just fix our existing implementation rather
than depend on another project.

Earlier someone noted the comment in the code about multipart/mixed. 
I am not an expert here, but when I looked into this some time ago, it
was not supported by browsers.  We do support multiple files per form
using multiple input elements, so full conformance with the spec would
be academic.  Am I wrong here?

Geoff

> On Mon, 8 Nov 2004, Jorg Heymans wrote:
> 
> >>
> >> Is posible to add support for both at the same time? If this is possible,
> >> then we can add it in 2.1.6 and deprecate the old approach.
> >
> > so you mean actually wrap a FileItem inside PartOnDisk and PartInMemory,
> > instead of creating our own FileItemPart subclassed from Part? Yes that would
> > work as well give or take a few bodges.
> >
> > Regards
> > Jorg

Re: [rfc] commons fileupload replacement

[Luigi Bai <lp...@focalpoint.com>]

On Mon, 8 Nov 2004, Jorg Heymans wrote:

>
>
> Luigi Bai wrote:
>> Hi,
>> 
>> I'd like to point out that the Multipart support in Cocoon is one of the 
>> few things that isn't pluggable or replaceable. I'd like it to be possible 
>> to configure web.xml/CocoonServlet to take a class name that implements 
>> some MultipartParser interface, and keep Part as an interface (perhaps 
>> letting it evolve as different implementations are available).
> Point taken, but how many different implementations does a multipartparser 
> really need ? Commons-fileupload is RFC 1867 compliant, has disk and memory 
> based storage and is a "known and trusted" commons library.
>

Not many different ones are "needed". However, the current one is buggy, 
and it would have been much easier to replace for an end-user if it 
werent' so heavily embedded into the Servlet. If some problem turns up in 
commons-upload, or in your code, as a non-commit level end user I'm back 
in the same boat; I have to rewrite CocoonServlet to just fix it or hack 
it out. I *could* then make a PATCH to fix it, which would be forever 
ignored in Bugzilla. :-)

I find it hard to believe that we just saw Avalon/Excalibur/etc. implode, 
Carsten is rewriting core to get around it - I don't think it's reasonable 
to hard code another dependency at the class level. Since you're rewriting 
it anyway, how hard is it to abstract one level out?

>> 
>> Keeping the integration at the level of Interface means we don't run into 
>> the same kind of rigamarole that is going on with ECM etc.
> True, but we're talking about 5 isolated classes that deal with file uploads, 
> nothing more.
>
> IMHO : YAGNI
>

Well, it's your code.

Re: [rfc] commons fileupload replacement

[Jorg Heymans <jh...@domek.be>]

Luigi Bai wrote:
> Hi,
> 
> I'd like to point out that the Multipart support in Cocoon is one of the 
> few things that isn't pluggable or replaceable. I'd like it to be 
> possible to configure web.xml/CocoonServlet to take a class name that 
> implements some MultipartParser interface, and keep Part as an interface 
> (perhaps letting it evolve as different implementations are available).
Point taken, but how many different implementations does a 
multipartparser really need ? Commons-fileupload is RFC 1867 compliant, 
has disk and memory based storage and is a "known and trusted" commons 
library.

> 
> Keeping the integration at the level of Interface means we don't run 
> into the same kind of rigamarole that is going on with ECM etc.
True, but we're talking about 5 isolated classes that deal with file 
uploads, nothing more.

IMHO : YAGNI

Regards
Jorg

Re: [rfc] commons fileupload replacement

[Luigi Bai <lp...@focalpoint.com>]

Hi,

I'd like to point out that the Multipart support in Cocoon is one of the 
few things that isn't pluggable or replaceable. I'd like it to be possible 
to configure web.xml/CocoonServlet to take a class name that implements 
some MultipartParser interface, and keep Part as an interface (perhaps 
letting it evolve as different implementations are available).

Keeping the integration at the level of Interface means we don't run into 
the same kind of rigamarole that is going on with ECM etc.

By the way, I'm not clear on why Multipart support can't be implemented as 
an Action? If it's an Action, then it's clearly pluggable, and all this 
goes away. Why is it so early in the process, hard coded in CocoonServlet?

On Mon, 8 Nov 2004, Jorg Heymans wrote:

>> 
>> Is posible to add support for both at the same time? If this is possible,
>> then we can add it in 2.1.6 and deprecate the old approach.
>
> so you mean actually wrap a FileItem inside PartOnDisk and PartInMemory, 
> instead of creating our own FileItemPart subclassed from Part? Yes that would 
> work as well give or take a few bodges.
>
> Regards
> Jorg
>
>

Re: [rfc] commons fileupload replacement

[Jorg Heymans <jh...@domek.be>]

> 
> Is posible to add support for both at the same time? If this is possible,
> then we can add it in 2.1.6 and deprecate the old approach.

so you mean actually wrap a FileItem inside PartOnDisk and PartInMemory, 
instead of creating our own FileItemPart subclassed from Part? Yes that 
would work as well give or take a few bodges.

Regards
Jorg

Re: [rfc] commons fileupload replacement

[Antonio Gallardo <ag...@agssa.net>]

Jorg Heymans dijo:
> Hi,
>
> I have provided a patch [1] that removes cocoon's multipart upload
> parser with one based on commons-fileupload.
>
> My approach breaks existing client code however, so Antonio suggested to
> get a few more opinions before continueing.
>
> 1) Part, PartOnDisk, PartInMemory are not necessary anymore. They are
> replaced by the FileItem interface of commons fileupload. It should be
> doable to wrap the FileItems in a Part class as suggested by Reinhard
> (with a slight change to the contract, see  [1] ) .
>
> 2) web.xml parameter "overwrite-uploads" is gone. Instead, we need a
> parameter like "where-to-store-uploads" with values "disk" or "memory".
> In addition to this (not included in the patch) commons-fileupload
> offers a "threshold" for memory based storage, files larger than the
> threshold will get written to disk anyway - we don't *have* to use this
> however.
>
> 3) There is no replacement for getHeaders() in Part.java.
>
> The nice thing to do would be to deprecate the Part classes, and wrap
> the FileItem interface inside Part for 2.1.6. , remove Part in 2.1.7?
>
> Or is 2.1.6 too soon for this?
>
> Thoughts?
> Jorg
>
> [1] http://nagoya.apache.org/bugzilla/show_bug.cgi?id=32102

Hi Jorg:

Thanks for posting about that! :-D

Is posible to add support for both at the same time? If this is possible,
then we can add it in 2.1.6 and deprecate the old approach.

WDYT?

Best Regards,

Antonio Gallardo