1
\$\begingroup\$

Category Service:

   public Boolean deleteCategory(int id) {
    try {
        Category category = categoryDao.getCategoryById(id).orElseThrow(NoSuchElementException::new);
        categoryDao.deleteCategory(id);
        categoryDao.moveCategoriesUp(category.getUserid(), category.getPosition());
        return true;
    } catch (DataAccessException | IllegalArgumentException | NoSuchElementException ex) {
        logger.error("Error log message" + ex.getMessage());
        return false;
    }
}

Explanation of what this does:

  1. It deletes the category
  2. It pushes all the other above categories down (by down I mean further closer to 0)
  3. Then I return to the controller "false" or "true" regarding the success status of the operation.

Here is the controller:

Delete Category API:

@DeleteMapping(path = "/deleteCat/{id}")
@ResponseBody
public Map<String, Boolean> deleteCategory(@PathVariable("id") int id) {
     return Collections.singletonMap("status", categoryService.deleteCategory(id));
}

What I want to achieve When the user deletes a category of tasks, I want that category to be deleted and push all the other relevant ones above one "position". What I am not sure about though is whether I am handling the errors correctly. If for some reason, the id is null, should I let the client know that the IllegalArgumentException was caught? Is it enough if I send back that the operation wasn't successful, for security reasons.

So, my questions are:

  1. Is it ok if I return just "false" or "true" to the controller and NOT let the client know what went wrong?
  2. Does my code break any other conventions or best practices?
  3. Would it be better if when I caught an exception I would respond with a response entity, let's say, a 400 Error code? So, the client would just receive the error from the http call and then perhaps just display the error message.

The reason I put Spring in the title is because I'm using the Spring framework now in this project. But I am asking about the most acceptable practice generally, for other frameworks or no frameworks too.

EDIT: My RequesFilter class:

@Component
public class JwtRequestFilter extends OncePerRequestFilter {

    @Qualifier("handlerExceptionResolver")
    private final HandlerExceptionResolver resolver;

    private final UserDetailsServiceImpl userService;

    private final ExceptionTranslator exceptionTranslator;

    private final JwtUtil jwtUtil;

    @Autowired
    public JwtRequestFilter(@Qualifier("handlerExceptionResolver") HandlerExceptionResolver resolver, UserDetailsServiceImpl userService, ExceptionTranslator exceptionTranslator, JwtUtil jwtUtil) {
        this.resolver = resolver;
        this.userService = userService;
        this.exceptionTranslator = exceptionTranslator;
        this.jwtUtil = jwtUtil;
    }

    @Override
    protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain chain)
            throws ServletException, IOException, ExpiredJwtException, MalformedJwtException {

        try {

            final String authorizationHeader = request.getHeader("Authorization");

            String username = null;
            String jwt = null;

            if (authorizationHeader != null && authorizationHeader.startsWith("Bearer ")) {
                jwt = authorizationHeader.substring(7);
                username = jwtUtil.extractUsername(jwt);
            }

            if (username != null && SecurityContextHolder.getContext().getAuthentication() == null) {
                UserDetails userDetails = userService.loadUserByUsername(username);

                boolean correct = jwtUtil.validateToken(jwt, userDetails);

                if (correct) {
                    UsernamePasswordAuthenticationToken usernamePasswordAuthenticationToken = new UsernamePasswordAuthenticationToken(
                            userDetails, null, userDetails.getAuthorities());

                    usernamePasswordAuthenticationToken
                            .setDetails(new WebAuthenticationDetailsSource().buildDetails(request));
                    SecurityContextHolder.getContext().setAuthentication(usernamePasswordAuthenticationToken);
                }
            }

            chain.doFilter(request, response);
        } catch (ExpiredJwtException | MalformedJwtException | AccessDeniedException | IOException ex) {
            ErrorDTO errorDto = exceptionTranslator.handleException(ex.getClass().getCanonicalName(), ex.getMessage().);
            response.getWriter().write(errorDto.getHandle());
            response.setStatus(HttpServletResponse.SC_UNAUTHORIZED);
            resolver.resolveException(request, response, null, ex);
        }
    }
\$\endgroup\$
4
  • 2
    \$\begingroup\$ Welcome to CodeReview@SE. Heed How do I ask a Good Question?: some code&context is necessary to give a useful review. \$\endgroup\$
    – greybeard
    Aug 31, 2020 at 10:15
  • \$\begingroup\$ @greybeard I read the link and improved the question. Is it better now? Can anyone give me feedback instead of just downvoting please? \$\endgroup\$ Aug 31, 2020 at 10:44
  • 3
    \$\begingroup\$ I find the question improved, especially the title. I wouldn't down-vote it in its current state. I probably would have down-voted it if you left it in its initial state. My mayor current worry: Java has no notion of *controller * that I know of, or annotations @DeleteMapping() and @ResponseBody (Spring Framework(?)) - you ask In Java …. \$\endgroup\$
    – greybeard
    Aug 31, 2020 at 12:05
  • 1
    \$\begingroup\$ Instead of adding revised code to your post, perhaps a better action would be Posting a new question. - see the section under heading I improved my code based on the reviews. What next? on What should I do when someone answers my question? as well as what you may and may not do after receiving answers. \$\endgroup\$ Sep 2, 2020 at 22:39

1 Answer 1

3
\$\begingroup\$

It depends on your context, but generally speaking it's good to return as much information as you can about the error to your caller. Unless it discloses security information (e.g., avoid returning error messages that contain names of DB tables or columns.)

The way I like to code in Spring MVC is to let the business logic throw an appropriate exception when something goes wrong. Then I use a @ControllerAdvice-annotated class that handles the exception by returning an appropriate error response to the client.

Getting to your questions:

  1. When you perform a business operation, the business logic should return nothing, or throw an appropriate exception to signal that something went wrong.

  2. The CategoryDao could treat the two operations, of deleting and "moving up", as a single operation and offer a method to do that. In any case, the two operations should be wrapped in a transaction

  3. On NoSuchElementException the usual thing is to either return 400 or 404, with an appropriate message in the entity body. I prefer 400 because 404 would be generated also in the case when, say, the path of the HTTP request is wrong. It's nice to be able to tell that the error was generated by your code, not by Spring. I know others disagree on this.

\$\endgroup\$
2
  • \$\begingroup\$ So in my request filter I try to catch the exceptions that are thrown there? should I remove the try/catch blocks and the "throws" from my code? \$\endgroup\$ Aug 31, 2020 at 15:01
  • 1
    \$\begingroup\$ Sometimes you want to keep the try-catch to transform a "technical" exception such as SQLException to a business exception. Other than that, I think that in your example the try-catch can be removed. Let the exception happen and use a @ControllerAdvice class to handle it. Shameless plug: matteo.vaccari.name/blog/archives/875 \$\endgroup\$
    – xpmatteo
    Sep 2, 2020 at 6:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.