Add to FileServlet getAttachmentName() #216

Closed
GithubJohn3031 opened this Issue Mar 9, 2016 · 6 comments

Projects

None yet

2 participants

@GithubJohn3031

Is it possible to add to FileServlet a possibilty to say from inherited class the attached file name.
Something like that:

        try {
            resource = new Resource(getFile(request));
                        resource.name = getAttachedName();
        }
        catch (IllegalArgumentException e) {
            response.sendError(HttpServletResponse.SC_BAD_REQUEST);
            return;
        }

or another idea were to make a protected method that modifies the resource.

        try {
            resource = new Resource(getFile(request));
                        resource = modifyResource (resource);
        }
        catch (IllegalArgumentException e) {
            response.sendError(HttpServletResponse.SC_BAD_REQUEST);
            return;
        }

EDIT: it would be cool to have a possibility to react to an error in derived class, for example we make a redirect, if some error occurs:

        try {
            resource = new Resource(getFile(request)); 
        }
        catch (IllegalArgumentException e) {                        
            response.sendError(HttpServletResponse.SC_BAD_REQUEST);
            return;
        } catch (IllegalStateException e){
                 //No action, errors should be set in derived class
                return;
       }

I should make an example of FileServlet with proposed changes.

@BalusC
Member
BalusC commented Mar 9, 2016

All is possible, I like changing the attachment name. Others not so.

But what exactly is the business requirement which you tried to solve? I feel there might be a better solution.

@GithubJohn3031

Use case 1:

protected File getFile(HttpServletRequest request) {
  File file = find file somehow
   if (file type is image && file not found){
       redirect to special error image on external server
       return null;
   }

   return file
}

Use Case 2:

protected File getFile(HttpServletRequest request) {
  File file = find file somehow
  response = Get HttpServletResponse somehow // I don't know how :)
  if (file == null)
    get file error somehow
  if (file error of type 1)
     response.sendError (SC_INTERNAL_SERVER_ERROR)
     return
  else if (file error of type 2)
     response.sendError (SC_NO_CONTENT)
     return

     //SC_UNSUPPORTED_MEDIA_TYPE
     //SC_NOT_IMPLEMENTED

   return file
}

protected String getAttachmentName (){
  String attachmentFile =  get file name to download
   return attachmentFile;
}

In Use Case 2 I don't know if there are rules for error messages in file downloads, that prohibit to use this error codes.

@BalusC
Member
BalusC commented Mar 10, 2016

How about below method which you can override?

    /**
     * Handles the case when the file is not found.
     * <p>
     * The default implementation sends a HTTP 404 error.
     * @param request The involved HTTP servlet request.
     * @param response The involved HTTP servlet response.
     * @throws IOException When something fails at I/O level.
     * @since 2.3
     */
    protected void handleFileNotFound(HttpServletRequest request, HttpServletResponse response) throws IOException {
        response.sendError(HttpServletResponse.SC_NOT_FOUND);
    }
@BalusC BalusC added a commit that referenced this issue Mar 10, 2016
@BalusC BalusC #216: fix javadoc error 311fe9e
@BalusC
Member
BalusC commented Mar 10, 2016

I added handleFileNotFound() and getAttachmentName() as per above commit. It's available in today's 2.3 snapshot. Can you give it a try and let me know if no more things/changes are needed?

@GithubJohn3031

Thank you very much. We've tested implementation with new methods. It fits all. The implementation of our servlet with omnifaces.FileServlet is simple and easy to understand. Better as before.
We don't need changes any more.

P.s. Theoreticaly I could me imagine the situation where the resource is not a file but a stream. But it will be StreamerServlet then :) And you can save the stream in a file and use FileServlet.

@BalusC
Member
BalusC commented Mar 11, 2016

Great.

With streams, supporting Range requests is not trivial. Hence Files only. You could always have a temp file directory.

@BalusC BalusC closed this Mar 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment