3
\$\begingroup\$

I'm writing a simple online shop using spring boot, for learning purposes. Right now I have purchasing-service and a product-service. The purchasing-service makes requests to the product-service via REST APIs for querying for product information. All of this is working, but I'm unclear on if I'm using exceptions correctly within the product-service.

I have an endpoint in my controller called retrieveProductById that retrieves information about a product given the product id. This id is passed to a service, which then uses a repository to look up the product in the database and return the product information. If the product id does not exist in the database, it throws a ProductDoesNotExistException. This is then handled by an ErrorHandlerControllerAdvice class that simply returns a 400 bad request response with the erorr message to the caller.

My question is: should my ProductDoesNotExistException be checked, or unchecked? To me, it makes sense for it to be checked, because as a developer or even a consumer of the API, you can clearly infer that if you give an invalid product id to the API, it will throw a ProductDoesNotExistException. If I change the exception to be unchecked, this is not as clear, as the IDE does not enforce me to explicitly add a throws ProductDoesNotExistException to the method signature.

So what would be the correct approach here? I do feel like it almost doesn't matter which I choose, as the controller advice class will handle the exception regardless of the type when it is thrown, so is the decision just a matter of preference on the developer part? See code below:

Controller

@RestController
@RequestMapping("/products")
@Slf4j
public class ProductController {

    private final ProductService productService;
    private final RestApiResponseFactory restApiResponseFactory;

    public ProductController(final ProductService productService,
            final RestApiResponseFactory restApiResponseFactory) {
        this.productService = productService;
        this.restApiResponseFactory = restApiResponseFactory;
    }


    @GetMapping("/{productId}")
    public ResponseEntity<RestApiResponse<ProductVO>> retrieveProductById(
            @PathVariable final String productId) throws ProductDoesNotExistException {
        log.info("Request made to find product {}", productId);
        final ProductVO productVO = this.productService.retrieveProductById(
                Integer.parseInt(productId));
        return ResponseEntity.ok(this.restApiResponseFactory.createSuccessResponse(productVO));
    }
}

Service

@Service
@Slf4j
public class ProductService {

    private final ProductRepository productRepository;

    public ProductService(final ProductRepository productRepository) {
        this.productRepository = productRepository;
    }

    public ProductVO retrieveProductById(final int productId) throws ProductDoesNotExistException {
        final Product product = findProductById(productId).orElseThrow(
                () -> new ProductDoesNotExistException(productId));
        return new ProductVO(product);
    }

    private Optional<Product> findProductById(final int productId) {
        return this.productRepository.findById(productId);
    }

}

Exception

public class ProductDoesNotExistException extends Exception {

    public ProductDoesNotExistException(final int productId) {
        super("No product with Product ID " + productId + " exists");
    }
}

Error handler

@ControllerAdvice
public class ErrorHandlerControllerAdvice {

    private final RestApiResponseFactory restApiResponseFactory;

    public ErrorHandlerControllerAdvice(final RestApiResponseFactory restApiResponseFactory) {
        this.restApiResponseFactory = restApiResponseFactory;
    }

    /**
     * Return 400 if a product does not exist
     */
    @ExceptionHandler(ProductDoesNotExistException.class)
    public ResponseEntity<RestApiResponse> handleProductDoesNotExistException(
            final ProductDoesNotExistException e) {
        return ResponseEntity.badRequest()
                .body(restApiResponseFactory.createErrorResponse(e.getMessage()));
    }

    /**
     * Catch all exception handler. In case any exceptions slip through the cracks, we want to
     * return a 500
     *
     * @param e the exception
     * @return
     */
    @ExceptionHandler(Exception.class)
    public ResponseEntity<RestApiResponse> handleGeneralException(final Exception e) {
        return ResponseEntity.internalServerError()
                .body(restApiResponseFactory.createErrorResponse(e.getMessage()));
    }

}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Please read or re-read How do I ask a good question?. for suggestions on how to title the question. \$\endgroup\$
    – pacmaninbw
    Jan 22, 2023 at 19:12
  • \$\begingroup\$ I've updated the title in line with the question conventions \$\endgroup\$
    – Eoin
    Jan 22, 2023 at 22:48

2 Answers 2

1
\$\begingroup\$

So what would be the correct approach here? I do feel like it almost doesn't matter which I choose, as the controller advice class will handle the exception regardless of the type when it is thrown, so is the decision just a matter of preference on the developer part?

It is a preference supported by software development principles. When the main concern is the usual business then treating exceptions between layers / tiers of the application is something "you aren't gonna needed" (YAGNI principle) instead treat exception where is needed, e.g. to send user readable messages. When the main concern is controlling each and every step of the business, perhaps each layer / tier having available different solutions to recover from exceptions, then treating exceptions between layers / tiers of the application could follow the principle of least knowledge:

Each unit should have only limited knowledge about other units: only units "closely" related to the current unit.
Each unit should only talk to its friends; don't talk to strangers.
Only talk to your immediate friends.

\$\endgroup\$
0
\$\begingroup\$

Some of my points:

ProductService can be declared as an interface Then another service ProductServiceImpl can have the concrete implementation.

@GetMapping("/{productId}")
public ResponseEntity<RestApiResponse<ProductVO>> retrieveProductById(
        @PathVariable final String productId) throws ProductDoesNotExistException {
    log.info("Request made to find product {}", productId);
    final ProductVO productVO = this.productService.retrieveProductById(
            Integer.parseInt(productId)); // You don't really need to specify this keyword here
    return ResponseEntity.ok(this.restApiResponseFactory.createSuccessResponse(productVO));
}

You might want to extend RTE instead of Exception in ProductDoesNotExistException class.

\$\endgroup\$
1
  • \$\begingroup\$ Extending the interface would just add un-needed verbosity. \$\endgroup\$ May 30, 2023 at 2:18

Your Answer

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

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