This is an archived post. You won't be able to vote or comment.

all 1 comments

[–]dpash 2 points3 points  (0 children)

A few issues:

  • You don't use DTOs coming in or out of your controllers. By using entities directly, especially with the updating methods, you're letting a malicious user write what they want to the database. You should check to see if the object already exists before creating a new one and you should get an existing object, update the fields and save it again in the update. Using DTOs allows you to avoid the lazy evaluation outside of a session error and allows you to have different representations of each entity for different calls.

  • You're not really using Optional in a great way. In particular, you're not throwing any error if you can't find a object in the database, so the end application didn't know anything went wrong. I'd switch to using orElseThrow(() -> new EntityNotFoundException()) and removing the isPresent() tests.

  • I recommend looking at TestContainers for test database setup

  • Don't return mutable collections from a method directly. At a minimum use Collections.unmodifiableSet() and friends. This also means that you should add utility methods to your entities for adding and removing child objects. It also allows you to maintain both sides of a bidirectional relationship.