Re: Support for multipart/form-data for Restful servlet for media item uploads

[aw...@gmail.com]

http://codereview.appspot.com/11936/diff/401/1412
File
java/common/src/main/java/org/apache/shindig/protocol/BaseRequestItem.java
(right):

http://codereview.appspot.com/11936/diff/401/1412#newcode276
Line 276: public FormDataItem getFormItem(String partName) {
multipart handling has to fold in any non-file items into the base
parameters.  All there should be access to is file items, not general
FormDataItems.

http://codereview.appspot.com/11936/diff/401/1411
File
java/common/src/main/java/org/apache/shindig/protocol/DataServiceServlet.java
(right):

http://codereview.appspot.com/11936/diff/401/1411#newcode58
Line 58: DataServiceServlet(MultipartFormParser formParser) {
you can't use constructor injection on servlets - it doesn't work with
WEB-INF/web.xml.  Use method injection.

http://codereview.appspot.com/11936/diff/401/1411#newcode63
Line 63: this(new DefaultMultipartFormParser());
don't add a default value

http://codereview.appspot.com/11936/diff/401/1411#newcode125
Line 125: boolean isMultipart =
ServletFileUpload.isMultipartContent(servletRequest);
move isMultipartContent() onto MultipartFormParser

http://codereview.appspot.com/11936/diff/401/1411#newcode127
Line 127: Iterator<FormDataItem> iter =
formParser.parse(servletRequest).iterator();
why not just:
   while (FormDataItem item : formParser.parse(servletRequest)) {

http://codereview.appspot.com/11936/diff/401/1411#newcode130
Line 130: String contentType = "application/json";
This variable doesn't need to be defined here;  it can be inside the
"if" (and doesn't need a default value)

http://codereview.appspot.com/11936/diff/401/1411#newcode133
Line 133: if (REQUEST_PARAM.equals(item.getFieldName())) {
comments please!  this is difficult to understand;  why REQUEST_PARAM?
why ignore all other fields?  I suspect some unfortunate wackiness in
the spec, but please describe waht this code is doing

http://codereview.appspot.com/11936/diff/401/1411#newcode198
Line 198: //this happens while testing
Inherited code,  but:

never catch Throwable.  It means you're catching Error, which you
shouldn't ever be doing.

And this is clearly debugging code, which doesn't belong in here

http://codereview.appspot.com/11936/diff/401/1418
File
java/common/src/main/java/org/apache/shindig/protocol/DefaultMultipartFormParser.java
(right):

http://codereview.appspot.com/11936/diff/401/1418#newcode32
Line 32: public class DefaultMultipartFormParser implements
MultipartFormParser {
Javadoc

http://codereview.appspot.com/11936/diff/401/1418#newcode35
Line 35: throws UnknownServiceException {
indent 4

http://codereview.appspot.com/11936/diff/401/1418#newcode42
Line 42: throw new UnknownServiceException("File upload error : " +
e.getMessage());
never do  + e.getMessage() in constructing new exceptions;  always use
the original exception as a cause

http://codereview.appspot.com/11936/diff/401/1418#newcode46
Line 46: private List<FormDataItem> convertToFormData(List fileItems) {
List<FileItem>.  Perform the cast in parse (with @SuppressWarnings on
the local variable)

http://codereview.appspot.com/11936/diff/401/1418#newcode47
Line 47: ArrayList<FormDataItem> formDataItems = new
ArrayList<FormDataItem>();
List<FormDataItem>

http://codereview.appspot.com/11936/diff/401/1416
File
java/common/src/main/java/org/apache/shindig/protocol/FormDataItem.java
(right):

http://codereview.appspot.com/11936/diff/401/1416#newcode24
Line 24: public interface FormDataItem {
Javadoc for all methods.

http://codereview.appspot.com/11936/diff/401/1416#newcode26
Line 26: String getContentType();
do we need all of these fields for file uploads?  Consider using a
super-interface that's just for file items, so we can stick just those
in RequestItem

http://codereview.appspot.com/11936/diff/401/1416#newcode32
Line 32: byte[] get();
do we need byte[] get?  getInputStream() should be sufficient.

In general, don't duplicate the Apache FileItem API.  Only add the
methods that we actually need in Shindig.

http://codereview.appspot.com/11936/diff/401/1416#newcode36
Line 36: String getName();
what's the difference between getName() and getFieldName()?

http://codereview.appspot.com/11936/diff/401/1419
File
java/common/src/main/java/org/apache/shindig/protocol/FormDataItemImpl.java
(right):

http://codereview.appspot.com/11936/diff/401/1419#newcode26
Line 26: class FormDataItemImpl implements FormDataItem {
really not "Impl".  It's really CommonsFormDataItem

http://codereview.appspot.com/11936/diff/401/1417
File
java/common/src/main/java/org/apache/shindig/protocol/MultipartFormParser.java
(right):

http://codereview.appspot.com/11936/diff/401/1417#newcode18
Line 18: package org.apache.shindig.protocol;
move this (and all the impl, and FormDataItem, etc.) to
o.a.s.protocol.multipart

http://codereview.appspot.com/11936/diff/401/1417#newcode27
Line 27: public interface MultipartFormParser {
Add Javadoc.

http://codereview.appspot.com/11936/diff/401/1417#newcode28
Line 28: List<FormDataItem> parse(HttpServletRequest request) throws
UnknownServiceException;
why not just IOException?

http://codereview.appspot.com/11936/diff/401/1413
File
java/common/src/main/java/org/apache/shindig/protocol/RequestItem.java
(right):

http://codereview.appspot.com/11936/diff/401/1413#newcode81
Line 81: FormDataItem getFormItem(String partName);
Want FileItem getFileItem(String partName) if this is only for files

http://codereview.appspot.com/11936/diff/401/1410
File
java/common/src/test/java/org/apache/shindig/common/util/Matcher.java
(right):

http://codereview.appspot.com/11936/diff/401/1410#newcode26
Line 26: public class Matcher<T> {
This class is unnecessary.  Use EasyMock's built-in Capture<T> class.

http://codereview.appspot.com/11936/diff/401/1421
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/http/AbstractHttpCacheTest.java
(right):

http://codereview.appspot.com/11936/diff/401/1421#newcode432
Line 432: TimeSource fakeClock = new FakeTimeSource(expiration - 60L);
shouldn't be part of this patch.  I've just fixed this, though.

http://codereview.appspot.com/11936