Return 400 for invalid instance registration payloads#5125
Return 400 for invalid instance registration payloads#5125amacharla15 wants to merge 2 commits intocodecentric:masterfrom
Conversation
|
Hi @amacharla15, thanks for your PR! I very much like that you have provided backend tests. I have got some questions, though:
|
|
Hi @SteKoe — Apologies and thanks for the review and the questions. Purpose of the frontend test Purpose of mappings.json Why I didn’t use spring-boot-starter-validation + @Valid/@notblank Additionally, I will rebase the branch onto the current codecentric:master so the PR is no longer out-of-date and CI can run cleanly. |
| @ExceptionHandler(ServerWebInputException.class) | ||
| public ResponseEntity<Void> handleWebInput(ServerWebInputException ex) { | ||
| Throwable cause = Exceptions.unwrap(ex); | ||
| if (cause instanceof IllegalArgumentException) { |
There was a problem hiding this comment.
I personally fail to understand the need of this if if, outside of it, you're returning exactly the same value.
I think it could be dropped.
| if (cause instanceof IllegalArgumentException) { | ||
| return ResponseEntity.badRequest().build(); | ||
| } | ||
| return ResponseEntity.badRequest().build(); |
There was a problem hiding this comment.
Just returning 400 with no details will leave users lost.
| import org.springframework.web.server.ServerWebInputException; | ||
| import reactor.core.Exceptions; | ||
|
|
||
| @RestControllerAdvice(annotations = AdminController.class) |
There was a problem hiding this comment.
Will this work also for the reactive-based part of SBA?
/cc @SteKoe
|
I'm not part of the team, therefore I shouldn't intervene in this, but as a consumer of SBA I'm afraid of potential side-effects and that's why I'm sticking my noise in this. :) If I may ask, what's the use case/motivation that triggered this change? Of course, I totally agree with @SteKoe's suggestion to use the bean validation. |
3 participants
Summary:
Map invalid /instances registration payloads to 400 BAD_REQUEST instead of surfacing as 500.
Add integration tests covering invalid JSON, missing name, and missing healthUrl.
How to run:
./mvnw -pl spring-boot-admin-server -Dtest=InstancesControllerIntegrationTest test
Evidence:
Before: invalid registration could surface as 500.
Now: tests fail if invalid payloads don’t return 400.