2
\$\begingroup\$

I made a Spring RESTful api as backend for a website. I would like to get feedback on coverage of errors and the response I return accordingly, am I using the status codes correctly? Basically anything that could be better I'd like to know.

Here's a few methods from the rest controller:

    @PostMapping("/customers")
    public ResponseEntity<Void> registerCustomer(@RequestBody Map<String, Object> customerData) {
        HttpHeaders headers = new HttpHeaders();
        try {
            CustomerDto registeredCustomer = CustomerService.getCustomerDto(customerData);
            Long insertId = this.service.addCustomer(registeredCustomer);
            headers.add("location", "customers/" + insertId);
            return new ResponseEntity<>(headers, HttpStatus.CREATED);
        } catch (SQLException | ConnectionFailedException e) {
            return new ResponseEntity<>(HttpStatus.INTERNAL_SERVER_ERROR);
        } catch (ClassCastException e) {
            return new ResponseEntity<>(HttpStatus.FORBIDDEN);
        }
    }

    @PostMapping("/customers/authenticate")
    public ResponseEntity<Map<String, Object>> authenticate(@RequestBody Map<String, String> credentials) {
        try {
            Map<String, Object> response = new HashMap<>();
            response.put("emailValid", false);
            response.put("passwordValid", false);

            Long id = this.service.getId(credentials.get("email"));
            if (id == 0L) {
                return new ResponseEntity<>(response, HttpStatus.NOT_FOUND);
            }
            response.put("emailValid", true);

            boolean passwordValid = this.service.authenticate(credentials.get("password"), id);
            if (!passwordValid) {
                return new ResponseEntity<>(response, HttpStatus.NOT_FOUND);
            }

            response.put("passwordValid", true);
            response.put("id", id);
            return new ResponseEntity<>(response, HttpStatus.FOUND);
        } catch (SQLException | ConnectionFailedException | IdDoesNotExistException e) {
            return new ResponseEntity<>(HttpStatus.INTERNAL_SERVER_ERROR);
        }
    }

    @PatchMapping("/customers/{id}/city")
    public ResponseEntity<String> updateCity(@PathVariable("id") Long id, @RequestBody String city) {
        try {
            if (city.length() > 50 || city.matches("^\\d+$")) {
                return new ResponseEntity<>(HttpStatus.FORBIDDEN);
            }
            boolean success = this.service.updateCity(id, city);
            if (!success) {
                return new ResponseEntity<>(HttpStatus.NOT_FOUND);
            }
            return new ResponseEntity<>(city, HttpStatus.ACCEPTED);
        } catch (SQLException | ConnectionFailedException e) {
            return new ResponseEntity<>(HttpStatus.INTERNAL_SERVER_ERROR);
        }
    }

    @GetMapping("/customers/{id}")
    public ResponseEntity<Map<String, String>> getCustomer(@PathVariable("id") Long id) {
        try {
            Map<String, String> response = new HashMap<>();
            CustomerDto customerDto = this.service.getCustomer(id);
            if (customerDto == null) {
                return new ResponseEntity<>(HttpStatus.NOT_FOUND);
            }

            ObjectMapper mapper = new ObjectMapper();
            try {
                String customerDtoJson = mapper.writeValueAsString(customerDto);
                response.put("customer", customerDtoJson);
            } catch (JsonProcessingException e) {
                e.printStackTrace();
                return new ResponseEntity<>(HttpStatus.INTERNAL_SERVER_ERROR);
            }
            return new ResponseEntity<>(response, HttpStatus.OK);
        } catch (SQLException | ConnectionFailedException e) {
            return new ResponseEntity<>(HttpStatus.INTERNAL_SERVER_ERROR);
        } catch (IdDoesNotExistException e) {
            return new ResponseEntity<>(HttpStatus.NOT_FOUND);
        }
    }

And here some DAO methods:

    public Long findIdByEmail(String email) throws SQLException, ConnectionFailedException {
        try (Connection conn = MySqlConn.getConn()) {
            String sql = "SELECT id FROM customers WHERE email=?";
            PreparedStatement stmt = conn.prepareStatement(sql);
            stmt.setString(1, email);
            ResultSet rs = this.crud.readRow(stmt);
            if (rs.next()) {
                return rs.getLong("id");
            }
            return 0L;
        }
    }

    public Long save(CustomerDto customerDto) throws SQLException, ConnectionFailedException {
        try (Connection conn = MySqlConn.getConn()) {
            String sql = "INSERT INTO customers(name, email, password, telNr, street, houseNr, city, dateOfLastAppointment)" +
                    " VALUES(?, ?, ?, ?, ?, ?, ?, ?)";
            PreparedStatement stmt = conn.prepareStatement(sql, Statement.RETURN_GENERATED_KEYS);
            stmt.setString(1, customerDto.getName());
            stmt.setString(2, customerDto.getEmail());
            stmt.setString(3, customerDto.getPassword());
            stmt.setString(4, customerDto.getTelNr());
            stmt.setString(5, customerDto.getStreet());
            stmt.setString(6, customerDto.getHouseNr());
            stmt.setString(7, customerDto.getCity());
            stmt.setString(8, customerDto.getDateOfLastAppointment());

            this.crud.insertRow(stmt);
            ResultSet rs = stmt.getGeneratedKeys();
            Long insertId = 0L;
            if (rs.next()) {
                insertId = rs.getLong(1);
            }
            conn.commit();
            return insertId;
        }
    }

    public String getHashedPassword(Long id) throws SQLException, ConnectionFailedException, IdDoesNotExistException {
        try (Connection conn = MySqlConn.getConn()) {
            String sql = "SELECT password FROM customers WHERE id=?";
            PreparedStatement stmt = conn.prepareStatement(sql);
            stmt.setLong(1, id);

            ResultSet rs = stmt.executeQuery();
            if (rs.next()) {
                return rs.getString("password");
            } else {
                throw new IdDoesNotExistException("invalid id");
            }
        }
    }

    public CustomerDto getCustomer(Long id) throws SQLException, ConnectionFailedException, IdDoesNotExistException {
        CustomerDto customerDto = new CustomerDto();
        try (Connection conn = MySqlConn.getConn()) {
            String sql = "SELECT name, email, telNr, street, houseNr, city, dateOfLastAppointment, admin " +
                    "FROM customers WHERE id=?";
            PreparedStatement stmt = conn.prepareStatement(sql);
            stmt.setLong(1, id);

            ResultSet rs = this.crud.readRow(stmt);
            if (rs.next()) {
                customerDto.setName(rs.getString("name"));
                customerDto.setEmail(rs.getString("email"));
                customerDto.setTelNr(rs.getString("telNr"));
                customerDto.setStreet(rs.getString("street"));
                customerDto.setHouseNr(rs.getString("houseNr"));
                customerDto.setCity(rs.getString("city"));
                customerDto.setDateOfLastAppointment(rs.getString("dateOfLastAppointment"));
                customerDto.setId(id);
                customerDto.setAdmin(rs.getBoolean("admin"));
            } else {
                throw new IdDoesNotExistException("invalid id");
            }
            return customerDto;
        }
    }

    public int updateName(Long id, String name) throws SQLException, ConnectionFailedException {
        try (Connection conn = MySqlConn.getConn()) {
            String sql = "UPDATE customers " +
                    "SET name=? " +
                    "WHERE id=?";
            PreparedStatement stmt = conn.prepareStatement(sql);
            stmt.setString(1, name);
            stmt.setLong(2, id);

            int result = this.crud.updateRow(stmt);
            conn.commit();
            return result;
        }
    }

And finally service.getCustomerDto(): (this might throw ClassCastException)

public static CustomerDto getCustomerDto(Map<String, Object> customerData) throws ClassCastException {
        CustomerDto dto = new CustomerDto();
        if (customerData.get("mPassword") != null) {
            dto.setPassword((String) customerData.get("mPassword"));
        }
        if (customerData.get("mId") != null) {
            dto.setId(new Long((Integer) customerData.get("mId")));
        }
        if (customerData.get("mName") != null) {
            dto.setName((String) customerData.get("mName"));
        }
        if (customerData.get("mEmail") != null) {
            dto.setEmail((String) customerData.get("mEmail"));
        }
        if (customerData.get("mTelNr") != null) {
            dto.setTelNr((String) customerData.get("mTelNr"));
        }
        if (customerData.get("mStreet") != null) {
            dto.setStreet((String) customerData.get("mStreet"));
        }
        if (customerData.get("mHouseNr") != null) {
            dto.setHouseNr((String) customerData.get("mHouseNr"));
        }
        if (customerData.get("mCity") != null) {
            dto.setCity((String) customerData.get("mCity"));
        }
        if (customerData.get("mDateOfLastAppointment") != null) {
            dto.setDateOfLastAppointment((String) customerData.get("mDateOfLastAppointment"));
        }
        if (customerData.get("mAdmin") != null) {
            dto.setAdmin((Boolean) customerData.get("mAdmin"));
        }
        return dto;
    }
```
\$\endgroup\$
  • 1
    \$\begingroup\$ Hello! Are you aware of Spring's ResponseEntityExceptionHandler? It is a convenient base class for ControllerAdvice classes that wish to provide centralized exception handling across all RequestMapping methods through ExceptionHandler methods. \$\endgroup\$ – TorbenPutkonen Aug 14 at 8:01
  • \$\begingroup\$ @TorbenPutkonen Hi, no I wasn't aware of that actually. I'll definitely look into it. Thank you! \$\endgroup\$ – Bas Velden Aug 14 at 8:15
1
\$\begingroup\$

While you check out the ResponseEntityExceptionHandler I'll take a look at the logical handling of exceptions.

Using classes for what they were not intended for

    } catch (SQLException | ConnectionFailedException e) {
        return new ResponseEntity<>(HttpStatus.INTERNAL_SERVER_ERROR);
    } catch (ClassCastException e) {
        return new ResponseEntity<>(HttpStatus.FORBIDDEN);

In this example, ClassCastException is used to transfer information about access rights. This is confusing as ClassCastException is meant to be used to transfer information about a specific and serious programming error. A specific exception type should be created for the purpose of denying access here and any occurrence of ClassCastException should result in an internal server error (and full stack trace logging on error-level).

Wrong result

    } catch (SQLException | ConnectionFailedException | IdDoesNotExistException e) {
        return new ResponseEntity<>(HttpStatus.INTERNAL_SERVER_ERROR);

If the IdDoesNotExistException is thrown as a result of something the user provided, the result is not an internal error. Internal errors should be reserved to things the client can not affect. It should thus return FORBIDDEN.

Duplicate error handling

It is a bit confusing that ID is being checked against value 0L in a code block that catches IdDoesNotExistException. It would seem that either returning 0L or throwing an IdDoesNotExistException would suffice, but not both. I would go for the exception approach, as they are more self documenting than magic numbers or null values.

\$\endgroup\$
  • \$\begingroup\$ The reason that I return HttpStatus.INTERNAL_SERVER_ERROR when a IdDoesNotExistException occurs in that specific case is because the id is fetched by the server and is not given by the client. Therefore I thought it would make more sense to return the server error in stead of e.g. NOT_FOUND or FORBIDDEN. What status code would be best to return in this case? \$\endgroup\$ – Bas Velden Aug 14 at 11:09
  • \$\begingroup\$ The check: if (id == 0L) is to check if the email is in the database. The line executed 6 lines later is what might throw IdDoesNotExistException if for whatever reason the record with that id is gone / deleted between getting the id and then using that id to authenticate the password. Is there a better way of handling this situation and is it even worth handling? \$\endgroup\$ – Bas Velden Aug 14 at 11:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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