Win a copy of Modern JavaScript for the Impatient this week in the Server-Side JavaScript and NodeJS forum!
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Ron McLeod
  • Paul Clapham
  • Bear Bibeault
  • Junilu Lacar
Sheriffs:
  • Jeanne Boyarsky
  • Tim Cooke
  • Henry Wong
Saloon Keepers:
  • Tim Moores
  • Stephan van Hulst
  • Tim Holloway
  • salvin francis
  • Frits Walraven
Bartenders:
  • Scott Selikoff
  • Piet Souris
  • Carey Brown

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

 
Ranch Hand
Posts: 226
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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.


 
Sheriff
Posts: 21997
107
Eclipse IDE Spring VI Editor Chrome Java Ubuntu Windows
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Ranch Hand
Posts: 226
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

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?
 
Marshal
Posts: 15870
265
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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:

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!":

And the intent of that code is obfuscated by your nondescript variable name, param2.
 
Jack Tauson
Ranch Hand
Posts: 226
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

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:

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!":

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?



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


 
Junilu Lacar
Marshal
Posts: 15870
265
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Sheriff
Posts: 21997
107
Eclipse IDE Spring VI Editor Chrome Java Ubuntu Windows
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

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
Ranch Hand
Posts: 226
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

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
Sheriff
Posts: 21997
107
Eclipse IDE Spring VI Editor Chrome Java Ubuntu Windows
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
The official documentation is always a good start: https://docs.spring.io/spring/docs/current/spring-framework-reference/web.html#spring-web
 
Beware the other head of science - it bites! Nibble on this message:
Thread Boost feature
https://coderanch.com/t/674455/Thread-Boost-feature
    Bookmark Topic Watch Topic
  • New Topic