Code review - need suggestions/help to improve the following code

[Jack Tauson]

Hi All,

I would like to get the following code reviewed by the experts here and wanted to figure out if there is a possibility of not using the servlet APIs which is kind of old I believe?

The following code works fine as far as downloading a file is concerned.

public void download (HttpServletRequest request, HttpServletResponse response, @PathVariable String filename, @PathVariable String user) throws IOException{
	          
		    String param1 = filename;
	        String param2 = user;

	        //The following URL will need to be updated with "prod" or "dev" or "test" depending upon the environment
	        final String FILE_LOCATION = "/mnt/nfs/myfolder/prod/documents/custom_documents/"+param2;
	        
	        
	        if(param1 == null || param2 == null) {
	            // The Request Parameters Were Not Present In The Query String. Do Something Or Exception Handling !!
	            System.out.println("Request Parameter Not Found in first if!");
	        } else if ("".equals(param1) || "".equals(param2)) {
	            // The Request Parameters Were Present In The Query String But Has No Value. Do Something Or  Exception Handling !!
	            System.out.println("Request Parameter Not Found in second if!");
	        } else {

	        
	            String fileName = (String) param1;
	            String contentType = getContentType(fileName.split("\\.")[1]);
	            File file = new File(FILE_LOCATION + "/" +fileName);
	            response.setContentType(contentType);
	            response.addHeader("Content-Disposition","attachment; filename=" + fileName);
	            response.setContentLength((int) file.length());
	            ServletOutputStream servletOutputStream = response.getOutputStream();
	            BufferedInputStream bufferedInputStream = new BufferedInputStream(new FileInputStream(file));
	            int bytesRead = bufferedInputStream.read();
	            while (bytesRead != -1) {
	                servletOutputStream.write(bytesRead);
	                bytesRead = bufferedInputStream.read();
	            }
	            if (servletOutputStream != null) servletOutputStream.close();
	            if(bufferedInputStream != null) bufferedInputStream.close();

	        }
	        
	        
	        
	}
	
	
	private String getContentType(String fileType) {
        String returnType = null;
        for(int i = 0; i < contentTypes.length; i++) {
            if(fileType.equals(contentTypes[i][0])) returnType = contentTypes[i][1];
        }
        return returnType;
    }

[Rob Spoor]

You can return Resource, wrapped in ResponseEntity if needed. The Resource implementation could be InputStreamResource, FileSystemResource or something else.

One note about your input - make sure to limit what the filename and user path variables can contain. For instance, make sure that they cannot contain ... If you do it will become possible to get access to files outside of the user's directory. For instance, if filename was ../../../../../../../etc/passwd, it would go all the way up to the root, then get etc/passwd. It would also be wise to prevent / in file names; right now that's not a risk, but if you ever switch to Path then using resolve or Paths.get will accept absolute files (File does not, I've checked when I ran into an issue because I expected it would).

[Jack Tauson]

Rob Spoor wrote:You can return Resource, wrapped in ResponseEntity if needed. The Resource implementation could be InputStreamResource, FileSystemResource or something else.

One note about your input - make sure to limit what the filename and user path variables can contain. For instance, make sure that they cannot contain ... If you do it will become possible to get access to files outside of the user's directory. For instance, if filename was ../../../../../../../etc/passwd, it would go all the way up to the root, then get etc/passwd. It would also be wise to prevent / in file names; right now that's not a risk, but if you ever switch to Path then using resolve or Paths.get will accept absolute files (File does not, I've checked when I ran into an issue because I expected it would).

Thank you for your valuable inputs. I have one more question.

Is there something in the newer version of spring boot framework that I could use instead of using older servlet APIs in my code?

[Junilu Lacar]

Why create nondescript variable names like param1 and param2 and messages that leak the structure of your implementation code ("Parameter not found in first if!"). It's almost as if you're purposely trying to obscure the intent of the perfectly fine names of filename and user. This carries over to lines 10 and 13. Wouldn't this be easier to understand instead:
if (filename == null || filename.isEmpty()) {    System.out.println("Missing parameter: filename is required."); } if (user == null || user.isEmpty()) {    System.out.println("Missing parameter: user is required."); }
Also, why not use Spring Validation instead? https://docs.spring.io/spring/docs/4.1.x/spring-framework-reference/html/validation.html

This comment screams "Inject the value instead of hard-coding it!":
//The following URL will need to be updated with "prod" or "dev" or "test" depending upon the environment final String FILE_LOCATION = "/mnt/nfs/myfolder/prod/documents/custom_documents/"+param2;
And the intent of that code is obfuscated by your nondescript variable name, param2.

[Jack Tauson]

Junilu Lacar wrote:Why create nondescript variable names like param1 and param2 and messages that leak the structure of your implementation code ("Parameter not found in first if!"). It's almost as if you're purposely trying to obscure the intent of the perfectly fine names of filename and user. This carries over to lines 10 and 13. Wouldn't this be easier to understand instead:
if (filename == null || filename.isEmpty()) {    System.out.println("Missing parameter: filename is required."); } if (user == null || user.isEmpty()) {    System.out.println("Missing parameter: user is required."); }
Also, why not use Spring Validation instead? https://docs.spring.io/spring/docs/4.1.x/spring-framework-reference/html/validation.html

This comment screams "Inject the value instead of hard-coding it!":
//The following URL will need to be updated with "prod" or "dev" or "test" depending upon the environment final String FILE_LOCATION = "/mnt/nfs/myfolder/prod/documents/custom_documents/"+param2;
And the intent of that code is obfuscated by your nondescript variable name, param2.

Thanks for your valuable inputs. Yes, I should get rid of those variables.It was a servlet code before and then I converted to an end point and it was left over there it seems like.

I'll have to take a look at the Spring Validation to see where I could use it.

When you say "This comment screams "Inject the value instead of hard-coding it!":",could you elaborate what you mean? Sorry, I didn't get it.

Also, for the following line, is it possible that if I define something in application.properties and then change the environment to prod,dev,test there instead of modifying the following path every time? If yes, what would be the best way to go about it?

final String FILE_LOCATION = "/mnt/nfs/myfolder/prod/documents/custom_documents/"+param2;

I will also get rid of "param2" and use "user" variable instead.

[Junilu Lacar]

Well, the comment says "This will need to be updated..." -- the value is a configuration item and as such you shouldn't need to change code. Instead of a value that is hard-coded in the program, you can take advantage of Spring and dependency injection to set the value of a field. That way, you can just write the code once. When you deploy to a different environment, Spring will take care of reading in the appropriate value and setting it in that class.

Read up on Spring dependency injection and environment variables

[Rob Spoor]

Jack Tauson wrote:Is there something in the newer version of spring boot framework that I could use instead of using older servlet APIs in my code?

I wouldn't call it new, but Spring MVC (which you're already using) can be used in almost all cases without needing access to HttpServletRequest and/or HttpServletResponse.

[Jack Tauson]

Rob Spoor wrote:

Jack Tauson wrote:Is there something in the newer version of spring boot framework that I could use instead of using older servlet APIs in my code?

I wouldn't call it new, but Spring MVC (which you're already using) can be used in almost all cases without needing access to HttpServletRequest and/or HttpServletResponse.

Thanks. Could you share some examples or any documentation where I can read more about replacing HttpServletRequest and/or HttpServletResponse ?

[Rob Spoor]

The official documentation is always a good start: https://docs.spring.io/spring/docs/current/spring-framework-reference/web.html#spring-web