Skip to content

Commit

Permalink
SAK-32398 Remove Apache Commons FileItem#get() use (sakaiproject#4189)
Browse files Browse the repository at this point in the history 
This returns the file upload as a byte array which if you are allowing large uploads would all get read onto the heap. Instead it uses an input stream.
  • Loading branch information
@buckett @ottenhoff
buckett authored and ottenhoff committed Apr 13, 2017
1 parent a1d1c47 commit 1b76de4
Show file tree
Hide file tree
Showing 5 changed files with 90 additions and 54 deletions.
17 changes: 11 additions & 6 deletions access/access-impl/impl/src/java/org/sakaiproject/access/tool/WebServlet.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
package org.sakaiproject.access.tool;

import java.io.IOException;
import java.io.InputStream;
import java.util.Enumeration;

import javax.servlet.ServletConfig;
Expand Down Expand Up @@ -196,14 +197,18 @@ protected void postUpload(HttpServletRequest req, HttpServletResponse res)
if (o != null && o instanceof FileItem)
{
FileItem fi = (FileItem) o;
// System.out.println("found file " + fi.getName());
if (!writeFile(fi.getName(), fi.getContentType(), fi.get(), path, req, res, true)) return;
try (InputStream inputStream = fi.getInputStream())
{
if (!writeFile(fi.getName(), fi.getContentType(), inputStream, path, req, res, true)) return;
} catch (IOException ioe) {
M_log.warn("Problem getting InputStream", ioe);
}
}
}
}

protected boolean writeFile(String name, String type, byte[] data, String dir, HttpServletRequest req,
HttpServletResponse resp, boolean mkdir)
protected boolean writeFile(String name, String type, InputStream inputStream, String dir, HttpServletRequest req,
HttpServletResponse resp, boolean mkdir)
{
try
{
Expand Down Expand Up @@ -283,7 +288,7 @@ protected boolean writeFile(String name, String type, byte[] data, String dir, H
resourceProperties.addProperty(ResourceProperties.PROP_DISPLAY_NAME, name);

// System.out.println("Trying Add " + path);
ContentResource resource = contentHostingService.addResource(path, type, data, resourceProperties,
ContentResource resource = contentHostingService.addResource(path, type, inputStream, resourceProperties,
NotificationService.NOTI_NONE);

}
Expand All @@ -295,7 +300,7 @@ protected boolean writeFile(String name, String type, byte[] data, String dir, H
try
{
ContentCollection collection = contentHostingService.addCollection(dir, resourceProperties);
return writeFile(name, type, data, dir, req, resp, false);
return writeFile(name, type, inputStream, dir, req, resp, false);
}
catch (Throwable ee)
{
Expand Down
45 changes: 45 additions & 0 deletions feedback/src/java/org/sakaiproject/feedback/util/FileItemDataSource.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package org.sakaiproject.feedback.util;

import org.apache.commons.fileupload.FileItem;
import org.apache.commons.io.FilenameUtils;

import javax.activation.DataSource;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;

/**
* Proxy to allow FileItems to be used as DataSources for attachments.
*/
public class FileItemDataSource implements DataSource {

private FileItem fileItem;

public FileItemDataSource(FileItem fileItem) {
this.fileItem = fileItem;
}

@Override
public InputStream getInputStream() throws IOException {
return fileItem.getInputStream();
}

@Override
public OutputStream getOutputStream() throws IOException {
throw new IOException("getOutputStream() isn't supported");
}

@Override
public String getContentType() {
return fileItem.getContentType();
}

@Override
public String getName() {
String fileName = fileItem.getName();
if (fileName != null) {
fileName = FilenameUtils.getName(fileName);
}
return fileName;
}
}
10 changes: 1 addition & 9 deletions feedback/src/java/org/sakaiproject/feedback/util/SakaiProxy.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,15 +177,7 @@ public void sendEmail(String fromUserId, String senderAddress, String toAddress,

if (fileItems.size() > 0) {
for (FileItem fileItem : fileItems) {
String name = fileItem.getName();

if (name.contains("/")) {
name = name.substring(name.lastIndexOf("/") + 1);
} else if (name.contains("\\")) {
name = name.substring(name.lastIndexOf("\\") + 1);
}

attachments.add(new Attachment(new ByteArrayDataSource(fileItem.get(), fileItem.getContentType()), name));
attachments.add(new Attachment(new FileItemDataSource(fileItem)));
}
}

Expand Down
24 changes: 13 additions & 11 deletions ...r/messageforums-app/src/java/org/sakaiproject/tool/messageforums/DiscussionForumTool.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
**********************************************************************************/
package org.sakaiproject.tool.messageforums;

import java.io.InputStream;
import java.text.DecimalFormat;
import java.text.DecimalFormatSymbols;
import java.text.NumberFormat;
Expand Down Expand Up @@ -62,6 +63,7 @@

import org.apache.commons.fileupload.FileItem;
import org.apache.commons.lang.StringUtils;
import org.sakaiproject.content.api.ContentResourceEdit;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.sakaiproject.api.app.messageforums.AnonymousManager;
Expand Down Expand Up @@ -9753,14 +9755,12 @@ public String processUpload(ValueChangeEvent event) {
if (newValue == null) {
return "";
}
try {
FileItem item = (FileItem) event.getNewValue();
if (!validateImageSize(item)) {
return null;
}

FileItem item = (FileItem) event.getNewValue();
if (!validateImageSize(item)) {
return null;
}
try (InputStream inputStream = item.getInputStream()) {
String fileName = item.getName();
byte[] fileContents = item.get();
ResourcePropertiesEdit props = contentHostingService.newResourceProperties();
String tempS = fileName;

Expand All @@ -9769,14 +9769,16 @@ public String processUpload(ValueChangeEvent event) {
fileName = tempS.substring(lastSlash + 1);
}
props.addProperty(ResourceProperties.PROP_DISPLAY_NAME, fileName);
ContentResource thisAttach = contentHostingService.addAttachmentResource(fileName, item.getContentType(), fileContents,
props);
ContentResourceEdit thisAttach = contentHostingService.addAttachmentResource(fileName);
thisAttach.setContentType(item.getContentType());
thisAttach.setContent(inputStream);
thisAttach.getPropertiesEdit().addAll(props);
contentHostingService.commitResource(thisAttach);
RankImage attachObj = rankManager.createRankImageAttachmentObject(thisAttach.getId(), fileName);
attachment = attachObj;

} catch (Exception e) {
LOG.error(this + ".processUpload() in DiscussionForumTool " + e);
e.printStackTrace();
LOG.error(this + ".processUpload() in DiscussionForumTool", e);
}
just_created = true;
return VIEW_RANK;
Expand Down
48 changes: 20 additions & 28 deletions syllabus/syllabus-app/src/java/org/sakaiproject/tool/syllabus/SyllabusTool.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
**********************************************************************************/
package org.sakaiproject.tool.syllabus;

import java.io.InputStream;
import java.net.MalformedURLException;
import java.net.URL;
import java.text.ParseException;
Expand All @@ -40,6 +41,8 @@
import javax.faces.event.ValueChangeEvent;

import org.apache.commons.fileupload.FileItem;
import org.apache.commons.io.FilenameUtils;
import org.sakaiproject.content.api.ContentResourceEdit;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.sakaiproject.api.app.syllabus.SyllabusAttachment;
Expand Down Expand Up @@ -2138,50 +2141,39 @@ public String processUpload(ValueChangeEvent event)
{
if(attachCaneled == false)
{
UIComponent component = event.getComponent();
Object newValue = event.getNewValue();
Object oldValue = event.getOldValue();
PhaseId phaseId = event.getPhaseId();
Object source = event.getSource();


if (newValue instanceof String) return "";
if (newValue == null) return "";

try

FileItem item = (FileItem) event.getNewValue();
try (InputStream inputStream = item.getInputStream())
{
FileItem item = (FileItem) event.getNewValue();
String fileName = item.getName();
byte[] fileContents = item.get();


ResourcePropertiesEdit props = contentHostingService.newResourceProperties();

String tempS = fileName;
//logger.info(tempS);
int lastSlash = tempS.lastIndexOf("/") > tempS.lastIndexOf("\\") ?
tempS.lastIndexOf("/") : tempS.lastIndexOf("\\");
if(lastSlash > 0)
fileName = tempS.substring(lastSlash+1);

ContentResource thisAttach = contentHostingService.addAttachmentResource(fileName, item.getContentType(), fileContents, props);

if (fileName != null) {
filename = FilenameUtils.getName(filename);
}

ContentResourceEdit thisAttach = contentHostingService.addAttachmentResource(fileName);
thisAttach.setContent(inputStream);
thisAttach.setContentType(item.getContentType());
thisAttach.getPropertiesEdit().addAll(props);
contentHostingService.commitResource(thisAttach);

SyllabusAttachment attachObj = syllabusManager.createSyllabusAttachmentObject(thisAttach.getId(), fileName);
////////revise syllabusManager.addSyllabusAttachToSyllabusData(getEntry().getEntry(), attachObj);
attachments.add(attachObj);

String ss = thisAttach.getUrl();
String fileWithWholePath = thisAttach.getUrl();

String s = ss;


if(entry.justCreated != true)
{
allAttachments.add(attachObj);
}
}
catch (Exception e)
{
logger.error(this + ".processUpload() in SyllabusTool " + e);
e.printStackTrace();
logger.error(this + ".processUpload() in SyllabusTool", e);
}
if(entry.justCreated == true)
{
Expand Down

0 comments on commit 1b76de4

Please sign in to comment.