4
\$\begingroup\$

My project is supposed to be used as back-end for a simple to-do web app that lets you create an account, add, edit, complete, and delete todos.

It could be used with some front end consumer like AngularJS (I'm not doing that because I don't like front-end). I used Spring Boot + H2 in-memory database (Hibernate) + Spring Security (Basic Authorization). A GitHub repository can be found here.

Domain package: (ommited getters and setters)

BaseEntity.java:

@MappedSuperclass
public abstract class BaseEntity {

@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
@JsonIgnore
private Long id;
}

ToDoItem.java:

@Entity
@Table(name = "TO_DO_ITEMS")
public class ToDoItem extends BaseEntity {

@Column(name = "TITLE", nullable = false)
private String title;

@Column(name = "COMPLETED")
private boolean completed;

@Column(name = "DUE_DATE", nullable = false)
@Convert(converter = LocalDateAttributeConverter.class)
@JsonDeserialize(using = LocalDateDeserializer.class)
@JsonSerialize(using = LocalDateSerializer.class)
private LocalDate dueDate;

// a ToDoItem is only associated with one user
@ManyToOne(cascade = CascadeType.PERSIST)
@JsonIgnore
@JoinColumn(name = "USER_ID")
private User user;

// JPA demands empty constructor
public ToDoItem() {
}

public ToDoItem(User user, String title, LocalDate dueDate) {
    this.user = user;
    this.title = title;
    this.dueDate = dueDate;
}

User.java:

@Entity
@Table(name = "USERS")
public class User extends BaseEntity {

@Column(name = "USERNAME")
private String username;

@Column(name = "PASSWORD")
private String password;

@Column(name = "EMAIL")
private String email;

// user can have many ToDoItems
@JsonIgnore
@OneToMany(cascade = CascadeType.ALL, mappedBy = "user")
private Set<ToDoItem> toDoItems = new HashSet<>();

// JPA demands empty constructor
public User() {
}

public User(String username, String password, String email) {
    this.username = username;
    this.password = password;
    this.email = email;
}

Repositories package: (using Spring to the fullest here, just plain interfaces)

UserRepository.java:

public interface UserRepository extends CrudRepository<User, Long> {
User findByUsername(String username);
}

ToDoItemRepository.java:

public interface ToDoItemRepository extends CrudRepository<ToDoItem, Long> {}

Services package:

ToDoItemServiceBean.java:

@Service
@Transactional
public class ToDoItemServiceBean {
private final ToDoItemRepository toDoItemRepository;

@Autowired
public ToDoItemServiceBean(ToDoItemRepository toDoItemRepository) {
    this.toDoItemRepository = toDoItemRepository;
}

public ToDoItem addToDo(ToDoItem toDoItem, User user) {
    String toDoTitle = toDoItem.getTitle();
    LocalDate toDoDueDate = toDoItem.getDueDate();
    ToDoItem newToDo = new ToDoItem(user, toDoTitle, toDoDueDate);
    return toDoItemRepository.save(newToDo);
}

public ToDoItem editToDo(ToDoItem newToDoItem, ToDoItem oldToDoItem) {
    String newTitle = newToDoItem.getTitle();
    LocalDate newDueDate = newToDoItem.getDueDate();
    oldToDoItem.setTitle(newTitle);
    oldToDoItem.setDueDate(newDueDate);
    return oldToDoItem;
}

public void deleteToDo(Long id) {
    toDoItemRepository.delete(id);
}

public void completeToDo(ToDoItem toDoItem) {
    toDoItem.setCompleted(true);
}

public boolean toDoExists(Long id) {
    if (toDoItemRepository.findOne(id) != null) {
        return true;
    }
    return false;
}

public boolean canUserAccessToDo(ToDoItem toDoItem, User user) {
    if (toDoItem.getUser() == user) {
        return true;
    }
    return false;
}

public ToDoItem findToDoItemById(Long id) {
    return toDoItemRepository.findOne(id);
}
}

UserServiceBean.java:

@Service
@Transactional
public class UserServiceBean {

private final UserRepository userRepository;
private final PasswordEncoder passwordEncoder;

@Autowired
public UserServiceBean(UserRepository userRepository, PasswordEncoder passwordEncoder) {
    this.userRepository = userRepository;
    this.passwordEncoder = passwordEncoder;
}

public User saveUser(User user) {
    User newUser = new User();
    newUser.setUsername(user.getUsername());
    newUser.setEmail(user.getEmail());
    newUser.setPassword(passwordEncoder.encode(user.getPassword()));
    return userRepository.save(newUser);
}

public boolean userExists(User user) {
    if (userRepository.findByUsername(user.getUsername()) == null) {
        return false;
    }
    return true;
}

public Iterable<ToDoItem> getAllToDoItems(User user) {
    return user.getToDoItems();
}

public boolean deleteUser(Principal principal) {
    if (findLoggedInUser(principal) != null) {
        userRepository.delete(findLoggedInUser(principal));
        return true;
    }
    return false;
}

private User findUserbyUsername(String username) {
    return userRepository.findByUsername(username);
}

public User findLoggedInUser(Principal principal) {
    return findUserbyUsername(principal.getName());
}
}

Controller package: (EDITED)

ToDoItemController.java:

@RestController
public class ToDoItemController {
private final ToDoItemServiceBean toDoItemService;
private final UserServiceBean userService;

@Autowired
ToDoItemController(ToDoItemServiceBean toDoItemService, UserServiceBean userService) {
    this.toDoItemService = toDoItemService;
    this.userService = userService;
}

@GetMapping("/todos")
public ResponseEntity viewToDos(Principal principal) {
    User currentUser = userService.findLoggedInUser(principal);
    return new ResponseEntity<>(userService.getAllToDoItems(currentUser), HttpStatus.OK);
}


// CREATE NEW TODOITEM FROM SENT JSON
@PostMapping("/todos")
@ResponseStatus(HttpStatus.CREATED)
public ToDoItem newToDo(
        @RequestBody ToDoItem toDoItem,
        Principal principal
) {
    User currentUser = userService.findLoggedInUser(principal);
    return toDoItemService.addToDo(toDoItem, currentUser);
}

@DeleteMapping("/todos/{id}")
public ResponseEntity deleteToDo(
        @PathVariable("id") Long itemId,
        Principal principal
) {
    User currentUser = userService.findLoggedInUser(principal);
    if (!toDoItemService.toDoExists(itemId)) throw new ToDoItemNotFoundException();
    ToDoItem toDoFromDb = toDoItemService.findToDoItemById(itemId);
    if (!toDoItemService.canUserAccessToDo(toDoFromDb, currentUser)) throw new UserAccessException();
    toDoItemService.deleteToDo(itemId);
    return new ResponseEntity(HttpStatus.NO_CONTENT);
}

@PutMapping("/todos/{id}")
public ResponseEntity editToDo(
        @PathVariable("id") Long itemId,
        @RequestBody ToDoItem newToDoItem,
        Principal principal
) {
    User currentUser = userService.findLoggedInUser(principal);
    if (!toDoItemService.toDoExists(itemId)) throw new ToDoItemNotFoundException();
    ToDoItem toDoFromDb = toDoItemService.findToDoItemById(itemId);
    if (!toDoItemService.canUserAccessToDo(toDoFromDb, currentUser)) throw new UserAccessException();
    toDoItemService.editToDo(newToDoItem, toDoFromDb);
    return new ResponseEntity<>(newToDoItem, HttpStatus.OK);
}

@PatchMapping("/todos/{id}/complete")
public ResponseEntity editToDo(
        @PathVariable("id") Long itemId,
        Principal principal
) {
    User currentUser = userService.findLoggedInUser(principal);
    if (!toDoItemService.toDoExists(itemId)) throw new ToDoItemNotFoundException();
    ToDoItem toDoFromDb = toDoItemService.findToDoItemById(itemId);
    if (!toDoItemService.canUserAccessToDo(toDoFromDb, currentUser)) throw new UserAccessException();
    toDoItemService.completeToDo(toDoFromDb);
    return new ResponseEntity<>(toDoFromDb, HttpStatus.OK);
}
}

UserController.java:

@RestController
public class UserController {
private final UserServiceBean userService;
private final ObjectMapper mapper;

@Autowired
UserController(UserServiceBean userService, ObjectMapper objectMapper) {
    this.userService = userService;
    this.mapper = objectMapper;
}

// CREATE A USER
@PostMapping("/users")
public ResponseEntity<ObjectNode> createUser(@RequestBody User user) {
    ObjectNode jsonObject = mapper.createObjectNode();
    if (userService.userExists(user)) throw new UsernameAlreadyExistsException();
    userService.saveUser(user);
    jsonObject.put("status", "User created.");
    return new ResponseEntity<>(jsonObject, HttpStatus.CREATED);
}

// DELETE YOUR ACCOUNT - deletes logged in user
@DeleteMapping("/users")
public ResponseEntity deleteUser(Principal principal) {
    if (userService.deleteUser(principal)) {
        return new ResponseEntity(HttpStatus.NO_CONTENT);
    }
    return new ResponseEntity(HttpStatus.BAD_REQUEST);
}
}

And my CustomException package:

@ResponseStatus(value = HttpStatus.NOT_FOUND, reason = "No such ToDoItem")
public class ToDoItemNotFoundException extends RuntimeException {}

@ResponseStatus(value = HttpStatus.FORBIDDEN, reason = "You can only edit your ToDos")
public class UserAccessException extends RuntimeException {}

@ResponseStatus(value = HttpStatus.BAD_REQUEST, reason = "User with that username already exists")
public class UsernameAlreadyExistsException extends RuntimeException {}

I didn't add my security configuration classes because this post is already very long - they are in my repo linked above. If you find anything that I could do better just let me know.

\$\endgroup\$
1
\$\begingroup\$

The method UserServiceBean#userExists can be simplified by return userRepository.findByUsername(user.getUsername()) == null.

I'm not convinced that having a one to many relation form the user to his todo is a good idea. If you want to display the user details only or preferences you will load all your todos for nothing. I would have a ToDoServiceBean#getTodos(Principal) instead.

For your controllers, Http 204 should not have a body :

he 204 response MUST NOT include a message-body, and thus is always terminated by the first empty line after the header fields.

-- https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.2.5

You should instead returns 200 with a empty list. It is much easir for the client of your Api. Because it has to deal with one success status code and just use a Json array that can be empty.

Also instead of duplicating the if ( ...exists(..) ) code, your services can throw en exception when the item does not exists and you can use the spring exception handlers to catch it and return your 404 response.

Regarding those errors messages, it is far easier to work with a code. You can still provide an explanation and it is recommended but having a code is much easier and safe for your clients if they want to handle or translate them.

|improve this answer|||||
\$\endgroup\$
  • 1
    \$\begingroup\$ Concerning the @OneToMany relationship: the items are not necessarily loaded unless you set it explicitly, because fetch = FetchType.LAZY by default. \$\endgroup\$ – Antot Feb 8 '17 at 8:16
  • \$\begingroup\$ Of course but that lead to another problem : dzone.com/articles/lazy-loading-is-a-code-smell \$\endgroup\$ – gervais.b Feb 8 '17 at 8:24
  • \$\begingroup\$ I changed all of my controllers so that now they throw exceptions. Could you check out my code? I posted edited controllers. \$\endgroup\$ – ohwelppp Feb 8 '17 at 12:12

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.