-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Global exception dev #4
Conversation
and validate some field in books entity + import settings fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, Great job. I approved but left some comments.
pom.xml
Outdated
<dependency> | ||
<groupId>org.hibernate</groupId> | ||
<artifactId>hibernate-validator</artifactId> | ||
<version>8.0.1.Final</version> | ||
</dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<dependency> | |
<groupId>org.hibernate</groupId> | |
<artifactId>hibernate-validator</artifactId> | |
<version>8.0.1.Final</version> | |
</dependency> | |
<dependency> | |
<groupId>org.springframework.boot</groupId> | |
<artifactId>spring-boot-starter-validation</artifactId> | |
</dependency> |
you could use spring-boot-starter-validation as it contains all needed dependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree with @ystankevych - it is better to use spring-boot-starter-validation
import jakarta.validation.constraints.NotNull; | ||
import java.math.BigDecimal; | ||
|
||
public record CreateBookRequestDto(@NotNull String title, @NotNull String author, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe NotBlank will be more suitable for String validation. As NotNull not allow only null values, but field can be empty. As for NotBlank -> a String field constrained with @notblank must be not null, and the trimmed length must be greater than zero
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job! Let's improve your solution
pom.xml
Outdated
<dependency> | ||
<groupId>org.hibernate</groupId> | ||
<artifactId>hibernate-validator</artifactId> | ||
<version>8.0.1.Final</version> | ||
</dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree with @ystankevych - it is better to use spring-boot-starter-validation
|
||
@GetMapping(value = "/{id}") | ||
@ResponseStatus(HttpStatus.OK) | ||
public BookDto getBookById(@PathVariable Long id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just FYI
you may also validate such things as @PathVariable, @RequestParams etc. It looks like:
@PathVariable @Positive Long id
In order to be able to perform such validation you need to annotate your controller class with @validated annotation
again, this is just FYI - apply it or not :)
public record CreateBookRequestDto(@NotBlank String title, | ||
@NotBlank String author, | ||
@NotBlank String isbn, | ||
@NotBlank @Min(0) BigDecimal price, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Min(0)
looks weird :)
could you use @Positive
or @PositiveOrZero
?
body.put("timestampt", LocalDateTime.now()); | ||
body.put("status", HttpStatus.BAD_REQUEST); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
time is pretty useless in response body - it does not bring any value to the client
status is obvious as well - no sense to have it in body
@ControllerAdvice | ||
public class CustomGlobalExceptionHandler extends ResponseEntityExceptionHandler { | ||
@Override | ||
protected ResponseEntity<Object> handleMethodArgumentNotValid( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you have any other exceptions except for MethodArgumentNotValidException
?
does it make sense to handle them as well?
Optional<Book> bookOptional = bookRepository.findById(id); | ||
if (bookOptional.isPresent()) { | ||
Book book = bookMapper.toBook(bookRequestDto); | ||
book.setId(id); | ||
bookRepository.save(book); | ||
return bookMapper.toDto(book); | ||
} else { | ||
throw new EntityNotFoundException("Book with ID " + id + " not found"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's improve this a bit :)
first thing is that you can leverage Mapstruct to perform the update:
void updateBook(CreateBookRequestDto dto, @MappingTarget Book book);
then:
Book bookToUpdate = bookRepository.findById(id).orElseThrow(...);
bookMapper.updateBook(dto, bookToUpdate);
bookRepository.save(book);
return bookMapper.toDto(book);
|
||
void deleteById(Long id); | ||
|
||
public BookDto update(CreateBookRequestDto bookRequestDto, Long id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public
is redundant here
# Conflicts: # pom.xml # src/main/java/com/example/springbootbookshop/controller/BookController.java # src/main/java/com/example/springbootbookshop/dto/CreateBookRequestDto.java # src/main/java/com/example/springbootbookshop/service/impl/BookServiceImpl.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
<dependency> | ||
<groupId>org.springframework.boot</groupId> | ||
<artifactId>spring-boot-starter-validation</artifactId> | ||
<version>3.1.4</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is no need to specify the version here
…l-exception-dev # Conflicts: # src/main/java/com/example/springbootbookshop/controller/BookController.java
No description provided.