A proposal for content upload ( http://docs.google.com/View?docid=dpbz5zc_4f34msgc4 ) is being incorporated opensocial spec 0.9.
This CL makes changes to support multipart/form-data based uploads in shindigs java Restful servlet.
On 2009/01/20 08:33:21, Sachin Shenoy wrote:
>
It has been pointed out to me that there needs to me some changes to be
complaint with the spec.
Especially the multipart/form-data has to go to JsonRpcServlet and
RestfulServlet should take the content from post body.
You can take a look at the changes, but this will change in any case.
Thanks,
Sachin
I have incorporated the comments on this issue into the JsonRpc issue here http://codereview.appspot.com/21068 This ...
16 years, 11 months ago
(2009-02-25 03:59:51 UTC)
#5
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.
Issue 11936: Support for multipart/form-data for Restful servlet for media item uploads
(Closed)
Created 17 years ago by Sachin Shenoy
Modified 16 years, 6 months ago
Reviewers: awiner, louiscryan
Base URL: http://svn.apache.org/repos/asf/incubator/shindig/trunk
Comments: 47
