restlet / restlet-framework-java

The first REST API framework for Java
https://restlet.talend.com
647 stars 284 forks source link

Binary File Upload using Restlet + Apache Commons FileUpload #1023

Open santiagopoli opened 9 years ago

santiagopoli commented 9 years ago

I have a REST API with Restlet 2.3 and need to implement a file-uploading functionality to it. The problem is that when someone uploads a file using a POST (with a multipart/form-data Content-Type), the file reaches the server with another encoding. To test this, I printed the contents of the original file in a Unix Terminal and then printed it again before parsing the requet with Apache Commons FileUpload (with almost the same code of this example http://restlet.com/technical-resources/restlet-framework/guide/2.2/extensions/fileupload). Both printed contents are very similar, but the original file has less characters, so i assume that my Java server is using the wrong encoding to interpret the file.

The file I sent is a PNG image. With text files the server works perfectly, but when I send photos or any binary file, the problem appears.

santiagopoli commented 9 years ago

Here is the method which implements this functionality.

private String getFileData(FileItemIterator fileIterator) throws IOException {
    String result = null;
    while (fileIterator.hasNext()) {
        FileItemStream fi = fileIterator.next();
        if (fi.getFieldName().equals(FILE_TO_UPLOAD)) {
            StringBuilder sb = new StringBuilder();
            BufferedReader br = new BufferedReader(new InputStreamReader(fi.openStream()));
            String line = null;
            while ((line = br.readLine()) != null) {
                sb.append(line);
            }
            sb.append("\n");
            result = sb.toString();
        }
    }
    return result;
}
santiagopoli commented 9 years ago

Actually solved it by using Google's ByteStreams class:

while (fileIterator.hasNext()) {
    FileItemStream fi = fileIterator.next();
    if (fi.getFieldName().equals(FILE_TO_UPLOAD)) {
        byte[] byteArray = ByteStreams.toByteArray(fi.openStream());
        result = new String(byteArray,Charset.forName("ISO-8859-1"));
    }
}
Tembrel commented 9 years ago

That's a little dangerous, since you aren't closing the InputStream.

How about:

    while (fileIterator.hasNext()) {
        FileItemStream fi = fileIterator.next();
        if (fi.getFieldName().equals(FILE_TO_UPLOAD)) {
            Optional<String> str = readAsString(fi);
            if (str.isPresent()) {
                result = str.get();
            } else {
                // ???
            }
        }
    }

where readAsString is defined using try-with-resources:

private Optional<String> readAsString(FileItemStream fi) {
    try (InputStream s = fi.openStream()) {
        return Optional.of(new String(ByteStreams.toByteArray(s), Charsets.ISO_8859_1));
    } catch (IOException ex) {
        System.err.printf("problem opening or reading %s: %s", fi.getName(), ex);
        return Optional.absent();
    }
}

--tim

On Thu, Feb 5, 2015 at 9:09 AM, Santiago Ignacio Poli < notifications@github.com> wrote:

Actually solved it using Google's ByteStreams class:

while (fileIterator.hasNext()) { FileItemStream fi = fileIterator.next(); if (fi.getFieldName().equals(FILE_TO_UPLOAD)) { byte[] byteArray = ByteStreams.toByteArray(fi.openStream()); result = new String(byteArray,Charset.forName("ISO-8859-1")); } }

— Reply to this email directly or view it on GitHub https://github.com/restlet/restlet-framework-java/issues/1023#issuecomment-73051116 .

santiagopoli commented 9 years ago

Thanks for your answer. Yes, I did notice that I didn't close the stream, so I changed my code to this:

while (fileIterator.hasNext()) {
    FileItemStream fi = fileIterator.next();
    if (fi.getFieldName().equals(FILE_TO_UPLOAD)) {
        InputStream attachmentStream = fi.openStream();
        byte[] attachmentBytes = ByteStreams.toByteArray(attachmentStream);
        attachmentStream.close();
        result = new String(attachmentBytes, Charset.forName("ISO-8859-1"));
    }
}

I didn't try your solution though, but it seems solid.

Tembrel commented 9 years ago

That's going to fail to close the stream if there is an exception thrown during toByteArray. If this happens enough you could start to leak connections.

Why not use try-with-resources? That's what it's for!

--tim

On Thu, Feb 5, 2015 at 9:48 AM, Santiago Ignacio Poli < notifications@github.com> wrote:

Thanks for your answer. Yes, I did notice that i didn't close the stream, so I changed my code to this:

while (fileIterator.hasNext()) { FileItemStream fi = fileIterator.next(); if (fi.getFieldName().equals(FILE_TO_UPLOAD)) { InputStream attachmentStream = fi.openStream(); byte[] attachmentBytes = ByteStreams.toByteArray(attachmentStream); attachmentStream.close(); result = new String(attachmentBytes, Charset.forName("ISO-8859-1")); } }

— Reply to this email directly or view it on GitHub https://github.com/restlet/restlet-framework-java/issues/1023#issuecomment-73057587 .

santiagopoli commented 9 years ago

I did modify my code to make it more understandable. The real method stands like this:

private AttachmentData getFileData(FileItemIterator fileIterator) throws FileUploadException, IOException {
    AttachmentData result = null;
    while (fileIterator.hasNext()) {
        FileItemStream fi = fileIterator.next();
        if (fi.getFieldName().equals(FILE_TO_UPLOAD)) {
            InputStream attachmentStream = fi.openStream();
            try {
                byte[] attachmentBytes = ByteStreams.toByteArray(attachmentStream);
                result = new AttachmentData(attachmentBytes);
            } finally {
                attachmentStream.close();
            }
        }
    }
    return result;
}
santiagopoli commented 9 years ago

I didn't knew about try-with-resources (http://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html). All of our projects use Java 7, so I've always used a try-catch-finally or try-finally statement. It seems that I learn something new everyday :)

thboileau commented 9 years ago

@santiagopoli , do you think we can close this issue?

I wonder if your initial issue is that you were reading binary content in decoding it into characters. I'm not sure to understand your last comment, butI think you can use try-with-resource with jdk7.