I have incorporated the comments on this issue into the JsonRpc issue
here http://codereview.appspot.com/21068
This issue itself is blocked until spec gets resolved.
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) {
As per the spec, the request will be present in a field named "request".
That gets removed from this list and merged into base parameters.
As you pointed I will document that clearly on the Servlet, where it is
done.
On 2009/02/19 18:45:46, awiner wrote:
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) {
On 2009/02/19 18:45:46, awiner wrote:
you can't use constructor injection on servlets - it doesn't work with
WEB-INF/web.xml. Use method injection.
Done.
http://codereview.appspot.com/11936/diff/401/1411#newcode63
Line 63: this(new DefaultMultipartFormParser());
On 2009/02/19 18:45:46, awiner wrote:
don't add a default value
Done.
http://codereview.appspot.com/11936/diff/401/1411#newcode125
Line 125: boolean isMultipart =
ServletFileUpload.isMultipartContent(servletRequest);
On 2009/02/19 18:45:46, awiner wrote:
move isMultipartContent() onto MultipartFormParser
Added this as a util method here itself as the implementation will not
change.
http://codereview.appspot.com/11936/diff/401/1411#newcode127
Line 127: Iterator<FormDataItem> iter =
formParser.parse(servletRequest).iterator();
On 2009/02/19 18:45:46, awiner wrote:
why not just:
while (FormDataItem item : formParser.parse(servletRequest)) {
Done.
http://codereview.appspot.com/11936/diff/401/1411#newcode130
Line 130: String contentType = "application/json";
On 2009/02/19 18:45:46, awiner wrote:
This variable doesn't need to be defined here; it can be inside the
"if" (and
doesn't need a default value)
Done.
http://codereview.appspot.com/11936/diff/401/1411#newcode133
Line 133: if (REQUEST_PARAM.equals(item.getFieldName())) {
On 2009/02/19 18:45:46, awiner wrote:
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
Done.
http://codereview.appspot.com/11936/diff/401/1411#newcode198
Line 198: //this happens while testing
ok.
On 2009/02/19 18:45:46, awiner wrote:
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 {
On 2009/02/19 18:45:46, awiner wrote:
Javadoc
Done.
http://codereview.appspot.com/11936/diff/401/1418#newcode35
Line 35: throws UnknownServiceException {
On 2009/02/19 18:45:46, awiner wrote:
indent 4
Done.
http://codereview.appspot.com/11936/diff/401/1418#newcode42
Line 42: throw new UnknownServiceException("File upload error : " +
e.getMessage());
On 2009/02/19 18:45:46, awiner wrote:
never do + e.getMessage() in constructing new exceptions; always use
the
original exception as a cause
Done.
http://codereview.appspot.com/11936/diff/401/1418#newcode46
Line 46: private List<FormDataItem> convertToFormData(List fileItems) {
On 2009/02/19 18:45:46, awiner wrote:
List<FileItem>. Perform the cast in parse (with @SuppressWarnings on
the local
variable)
Done.
http://codereview.appspot.com/11936/diff/401/1418#newcode47
Line 47: ArrayList<FormDataItem> formDataItems = new
ArrayList<FormDataItem>();
On 2009/02/19 18:45:46, awiner wrote:
List<FormDataItem>
Done.
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 {
On 2009/02/19 18:45:46, awiner wrote:
Javadoc for all methods.
Done.
http://codereview.appspot.com/11936/diff/401/1416#newcode26
Line 26: String getContentType();
ContentType will be needed as we may want to know what is the type of
the file that was uploaed, like image/gif or video/mpeg.
On 2009/02/19 18:45:46, awiner wrote:
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();
hmm... it provides for easy and faster access in cases where the content
is already in memory (that would be the case where content is small).
Also, underlying implementation can cache the result.
On 2009/02/19 18:45:46, awiner wrote:
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();
On 2009/02/19 18:45:46, awiner wrote:
what's the difference between getName() and getFieldName()?
for file upload, one could give a file name. getName() returns the file
Name.
While getFieldName returns name to identify a particular field in
formdata.
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 {
On 2009/02/19 18:45:46, awiner wrote:
really not "Impl". It's really CommonsFormDataItem
Done.
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;
On 2009/02/19 18:45:46, awiner wrote:
move this (and all the impl, and FormDataItem, etc.) to
o.a.s.protocol.multipart
Done.
http://codereview.appspot.com/11936/diff/401/1417#newcode27
Line 27: public interface MultipartFormParser {
On 2009/02/19 18:45:46, awiner wrote:
Add Javadoc.
Done.
http://codereview.appspot.com/11936/diff/401/1417#newcode28
Line 28: List<FormDataItem> parse(HttpServletRequest request) throws
UnknownServiceException;
On 2009/02/19 18:45:46, awiner wrote:
why not just IOException?
Done.
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);
On 2009/02/19 18:45:46, awiner wrote:
Want FileItem getFileItem(String partName) if this is only for files
FormDataItem represents both, File upload as well as simple Form Field.
As per spec (RPC), Form field with field name "request" will contain the
actual json-rpc request, others may contain any media that is attached
with the request.
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);
On 2009/02/19 18:45:46, awiner wrote:
shouldn't be part of this patch. I've just fixed this, though.
removed.
http://codereview.appspot.com/11936
