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
