fix: owner not exists validation addPetToOwner#292
Conversation
| if (owner == null) | ||
| return new ResponseEntity<>(HttpStatus.NOT_FOUND); | ||
|
|
||
| owner.setId(ownerId); |
There was a problem hiding this comment.
I guess this assignement doesn't make sense now
There was a problem hiding this comment.
why? it is a validation if owner doesnt exist ...
There was a problem hiding this comment.
we already fetched owner, so no reason to set owner Id again in
owner.setId(ownerId);
There was a problem hiding this comment.
ahhh got you, this line was already there, i will remove
| Owner owner = new Owner(); | ||
| Owner owner = clinicService.findOwnerById(ownerId); | ||
|
|
||
| if (owner == null) |
There was a problem hiding this comment.
pls add/update tests for this case
There was a problem hiding this comment.
how can i do this ? you mean add this validation in update endpoint?
There was a problem hiding this comment.
check/fix tests in OwnerRestControllerTests.java
There was a problem hiding this comment.
i found the archive but i dont now how this works, i am learning spring boot
There was a problem hiding this comment.
i added the tests , check it pls
There was a problem hiding this comment.
it is the result of my test
There was a problem hiding this comment.
Is that correct? Or do you need something else?
There was a problem hiding this comment.
I said “tests,” not “a test.” The CI/CD is failing because one of the tests started failing after your changes. It would be good to check why this happened and fix it.
P.S.
It’s good practice to run mvn test locally before creating PRs - may save you some time.
There was a problem hiding this comment.
ok, There's an error in the test from four years ago, what i need to do?
There was a problem hiding this comment.
yep. this test was fine before you changed controller logic. so think about it - how this test was verify previous logic, what is changed, how you can fix it etc.
| PetDto petDto = petMapper.toPetDto(pet); | ||
| headers.setLocation(UriComponentsBuilder.newInstance().path("/api/pets/{id}") | ||
| .buildAndExpand(pet.getId()).toUri()); | ||
| .buildAndExpand(pet.getId()).toUri()); |
There was a problem hiding this comment.
I’d suggest keeping changes to a minimum and preserving the existing formatting (here and in other places)
2 participants
This PR fixes issue #287.
The previous error handling exposed SQL and constraint details in API responses by using ex.getLocalizedMessage().
Changes:
This prevents leakage of internal persistence details while maintaining useful logging.