[Springframework-developer] Multipart committed

[Trevor C. <pr...@se...>]

I've just committed the multipart stuff to cvs.  It currently should NOT
change any existing projects.  In order to "activate" it you will need to
explicitly turn it on as outlined below.
<WARNING>Note that these procedures for enabling it are subject to change,
as is the underlying implementation.  Please consider the entire multipart
implementation highly volatile until further notice.</WARNING>

With that out of the way, I'll detail how everything works.  I've seperated
it into 3 parts, the "multipart api", changes to Spring, and the jakarta
commons fileupload implementation.  Note that I've provided what I consider
a sufficient level of documentation in the javadocs, so if anyone referring
to those finds them lacking, please let me know (since I think improving our
docs is/should be a big push right now).  I'd also appreciate any
help/suggestions on how to unit test these, since I'm not exactly sure how
to proceed with them (due to how to make mock request, etc.).


Multipart API
-------------
The api contains 3 classes and 1 exception:
- org.springframework.web.servlet.MultipartResolver
- org.springframework.web.servlet.MultipartHttpServletRequest
- org.springframework.web.servlet.MultipartFile
- org.springframework.web.servlet.MultipartException

Most of the details for these are in the javadocs.  The interfaces are in
the org.springframework.web.servlet package to follow the pattern set by the
LocaleResolver/ThemeResolver.

I made the content for the MultipartFile a byte array (byte[]) and avoided
using File/InputStream.  This seems to make it simpler and more consistent.
If you need to retrieve the original filename, you will have to use a
MultipartFile object in your bean.  Based on my personal feelings and
comments from others, I think using a File is inappropriate (except "under
the covers" when necessary), and the only other alternative is to create
some form of "NamedByteArray".  Since any special class containing a
filename will introduce a Spring dependancy, I think MultipartFile is as
good as creating an additional class.

The methods in the MultipartHttpServletRequest were designed to mimic their
HttpServletRequest parameter counterparts.  The only one not matched
currently is "getFileValues()" to cover the possiblity of a MultipartFile
array.  What are the opinions on supporting a MultipartFile array.  I
personally have never used them because they add a ton of extra complexity.
With Spring, it might not be so bad now, but I'd have to see a need.  If we
do see a potential for using them later, we could create the interface
declaration now, and use a nop exception in the implementation until we get
it working.  Personally,  I'd leave it out, I'm mentioning it in case others
see/have real uses for it.


Changes to Spring
-----------------
Just to reiterate, none of these changes should mess up any existing
Spring-based apps.  Also, all the Spring code is based on the above api.
None of it references any of the "commons" implementation, so it should be
fully pluggable for other implementations.

org.springframework.web.servlet.DispatcherServlet looks for a bean named
"multipartResolver" and if it doesn't find one, it does NOT create a default
(it uses null).  The reason for this is because we don't want to wrap normal
request's so that we can use "request instanceof
MultipartHttpServletRequest".  If it contains a multipartResolver, it wraps
the request ONLY if "multipartResolver.isMultipart(request)" is true.  This
wrapped request is then used for the balance of the doService method, and
then cleanup is performed.  This means that if no MultipartResolver is set,
no multipart wrapping/processing will occur.

org.springframework.web.bind.ServletRequestDataBinder has a new constructor
which takes the request as a parameter.  This is to allow special multipart
property editors to be assigned.  I tried various methods of having the
binder simply use the normal getParameter method to parse the content, but
it proved very clumsy and didn't meet all needs (like getting filename).  In
the end it appears that the property editor needs access to the request so
it can map "getParamater(String)" results to "getFile(String)" results.
This behaviour is documented in the
MultipartResolver.resolveMultipart(HttpServletRequest)" javadocs.  This
means that wherever you currently create a ServletRequestDataBinder you need
to use the new constructor if you want it to be able to use the multipart
handling.

Unless somebody can see major flaws in using this approach or has a more
elegant solution, I would recommend deleting the current constructor and
have all methods use the new constructor which receives the request.  The
way the constructor is written, if a multipartresolver isn't set, things
occur the same way they do now.  Basically, using the new constructor won't
change anything unless multipart handling is used (in which case we need
this constructor anyway).

"getMultipartResolver(HttpServletRequest)" was added to
org.springframework.web.servlet.support.RequestContextUtils.  This is just a
utility method and is only used (so far) by ServletRequestDataBinder.

I created a few "general use" multipart classes (all of which are in the
org.springframework.web.servlet.multipart package).  The main one is
MultipartHttpServletRequestImpl.  It should work for most multipart
implementations unless someone has very special requirements (since most
"real work" occurs in the resolver).  I also created 3 property editors
(which are all used by the commons implementation, but can be used by others
as well).
MultipartBytePropertyEditor supports "byte[]" and maps the content
(MultipartFile.getBytes()) as the value.  MultipartFilePropertyEditor
supports "MultipartFile" and maps the whole MultipartFile object as the
value.  MultipartStringPropertyEditor supports String and maps the filename
(MultipartFile.getFileName()) as the value.  All 3 are reusable for any
other multipart implementations, and it should be simple to create custom
property editors if desired (like File, etc.).

The String editor is the one I question a little, so I'd appreciate
feedback.  Basically, if you don't have a String editor, then the
"key=value" requirement means you will get weird values in your objects.
Placing the file content in a String doesn't seem to make sense either.  So
basically, this seemed like the "better of evils" (a great design
justification :) ).


Jakarta Commons FileUpload implementation
-----------------------------------------
The implementation only requires 2 special classes, CommonsMultipartFile and
CommonsMultipartResolver (both in org.springframework.web.servlet.multipart
package).  CommonsMultipartFile is very basic, just a simple wrapper around
org.apache.commons.fileupload.FileItem .

CommonsMultipartResolver uses commons to process isMultipart requests, and
it registers the 3 editors mentioned above.  The actual resolveMultipart
contains special handling to check each FileItem in order to ensure "regular
field" arrays are handled correctly.  It provides 4 configuration settings
which can be set in the context file, etc. which are specific to how the
commons package works.  The default values are outlined in the javadocs, but
I'll list them here for discussion:

maximumFileSize = -1 (no file size limit)
maximumInMemorySize = 1024 (1 kb)
temporaryFilePath = System.getProperty("java.io.tmpdir")

These are all consistent with the actual commons defaults.  The only
questionable one is the "maximumFileSize" being unlimited (since this could
be a DOS/DDOS hole as Colin mentioned earlier).  However, I don't think we
can choose an arbitrary default that will work for a "majority" of uses.  I
would recommend that we simply leave it as is and let the user set it, but
I'm open to suggestions.


Conclusion/Questions
--------------------
I have already converted our big app and everything works fine with these
classes, including validation (we actually check the byte[] to make sure it
is an image in one case).  Once we finalize the api and default
implementation, what should Spring's default be, either no multipart
handling or the commons implementation?  Personally, I think having it on by
default makes sense, since you'll never use it unless you receive a form
which is multipart (in which case you need it anyway).  What does everyone
else think?


Trevor D. Cook