Skip to content

fix: owner not exists validation addPetToOwner #292

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.

Sign up for GitHub

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

Open
LeonardoSantosBR wants to merge 5 commits into
base: master
Choose a base branch
from
Open

fix: owner not exists validation addPetToOwner #292

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
feb77c4
fix: owner validation addPetToOwner
LeonardoSantosBR Feb 6, 2026
e384bcb
style: identation
LeonardoSantosBR Feb 7, 2026
603583f
fix: setowner removed
LeonardoSantosBR Feb 7, 2026
05f8cb7
feat: test
LeonardoSantosBR Feb 8, 2026
6b200ed
style: comments removed
LeonardoSantosBR Feb 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions ...ain/java/org/springframework/samples/petclinic/rest/advice/ExceptionControllerAdvice.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@
package org.springframework.samples.petclinic.rest.advice;

import jakarta.servlet.http.HttpServletRequest;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.dao.DataIntegrityViolationException;
import org.springframework.http.HttpStatus;
import org.springframework.http.ProblemDetail;
Expand All @@ -42,6 +45,9 @@
@ControllerAdvice
public class ExceptionControllerAdvice {

private static final Logger log =
LoggerFactory.getLogger(ExceptionControllerAdvice.class);

/**
* Private method for constructing the {@link ProblemDetail} object passing the name and details of the exception
* class.
Expand All @@ -53,9 +59,12 @@ public class ExceptionControllerAdvice {
private ProblemDetail detailBuild(Exception ex, HttpStatus status, StringBuffer url) {
ProblemDetail detail = ProblemDetail.forStatus(status);
detail.setType(URI.create(url.toString()));
detail.setTitle(ex.getClass().getSimpleName());
detail.setDetail(ex.getLocalizedMessage());
detail.setTitle(status.getReasonPhrase());
detail.setDetail("An unexpected error occurred.");
detail.setProperty("timestamp", Instant.now());

log.error("Request failed. status={}, url={}, ex={}", status, url, ex.getLocalizedMessage(), ex);

return detail;
}

Expand Down
19 changes: 11 additions & 8 deletions src/main/java/org/springframework/samples/petclinic/rest/controller/OwnerRestController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,9 @@ public class OwnerRestController implements OwnersApi {
private final VisitMapper visitMapper;

public OwnerRestController(ClinicService clinicService,
OwnerMapper ownerMapper,
PetMapper petMapper,
VisitMapper visitMapper) {
OwnerMapper ownerMapper,
PetMapper petMapper,
VisitMapper visitMapper) {
this.clinicService = clinicService;
this.ownerMapper = ownerMapper;
this.petMapper = petMapper;
Expand Down Expand Up @@ -99,7 +99,7 @@ public ResponseEntity<OwnerDto> addOwner(OwnerFieldsDto ownerFieldsDto) {
this.clinicService.saveOwner(owner);
OwnerDto ownerDto = ownerMapper.toOwnerDto(owner);
headers.setLocation(UriComponentsBuilder.newInstance()
.path("/api/owners/{id}").buildAndExpand(owner.getId()).toUri());
.path("/api/owners/{id}").buildAndExpand(owner.getId()).toUri());
return new ResponseEntity<>(ownerDto, headers, HttpStatus.CREATED);
}

Expand Down Expand Up @@ -136,14 +136,18 @@ public ResponseEntity<OwnerDto> deleteOwner(Integer ownerId) {
public ResponseEntity<PetDto> addPetToOwner(Integer ownerId, PetFieldsDto petFieldsDto) {
HttpHeaders headers = new HttpHeaders();
Pet pet = petMapper.toPet(petFieldsDto);
Owner owner = new Owner();
Owner owner = clinicService.findOwnerById(ownerId);

if (owner == null)
Copy link
Copy Markdown
Collaborator

@vfedoriv vfedoriv Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls add/update tests for this case

Copy link
Copy Markdown
Author

@LeonardoSantosBR LeonardoSantosBR Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how can i do this ? you mean add this validation in update endpoint?

Copy link
Copy Markdown
Collaborator

@vfedoriv vfedoriv Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check/fix tests in OwnerRestControllerTests.java

Copy link
Copy Markdown
Author

@LeonardoSantosBR LeonardoSantosBR Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i found the archive but i dont now how this works, i am learning spring boot

Copy link
Copy Markdown
Author

@LeonardoSantosBR LeonardoSantosBR Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i added the tests , check it pls

Copy link
Copy Markdown
Author

@LeonardoSantosBR LeonardoSantosBR Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image it is the result of my test

Copy link
Copy Markdown
Author

@LeonardoSantosBR LeonardoSantosBR Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that correct? Or do you need something else?

Copy link
Copy Markdown
Collaborator

@vfedoriv vfedoriv Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

@LeonardoSantosBR LeonardoSantosBR Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, There's an error in the test from four years ago, what i need to do?
image

Copy link
Copy Markdown
Collaborator

@vfedoriv vfedoriv Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return new ResponseEntity<>(HttpStatus.NOT_FOUND);

owner.setId(ownerId);
Copy link
Copy Markdown
Collaborator

@vfedoriv vfedoriv Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this assignement doesn't make sense now

Copy link
Copy Markdown
Author

@LeonardoSantosBR LeonardoSantosBR Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why? it is a validation if owner doesnt exist ...

Copy link
Copy Markdown
Collaborator

@vfedoriv vfedoriv Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already fetched owner, so no reason to set owner Id again in
owner.setId(ownerId);

Copy link
Copy Markdown
Author

@LeonardoSantosBR LeonardoSantosBR Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahhh got you, this line was already there, i will remove

pet.setOwner(owner);
pet.getType().setName(null);
this.clinicService.savePet(pet);
PetDto petDto = petMapper.toPetDto(pet);
headers.setLocation(UriComponentsBuilder.newInstance().path("/api/pets/{id}")
.buildAndExpand(pet.getId()).toUri());
.buildAndExpand(pet.getId()).toUri());
Copy link
Copy Markdown
Collaborator

@vfedoriv vfedoriv Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d suggest keeping changes to a minimum and preserving the existing formatting (here and in other places)

return new ResponseEntity<>(petDto, headers, HttpStatus.CREATED);
}

Expand Down Expand Up @@ -175,11 +179,10 @@ public ResponseEntity<VisitDto> addVisitToOwner(Integer ownerId, Integer petId,
this.clinicService.saveVisit(visit);
VisitDto visitDto = visitMapper.toVisitDto(visit);
headers.setLocation(UriComponentsBuilder.newInstance().path("/api/visits/{id}")
.buildAndExpand(visit.getId()).toUri());
.buildAndExpand(visit.getId()).toUri());
return new ResponseEntity<>(visitDto, headers, HttpStatus.CREATED);
}


@PreAuthorize("hasRole(@roles.OWNER_ADMIN)")
@Override
public ResponseEntity<PetDto> getOwnersPet(Integer ownerId, Integer petId) {
Expand Down
Loading
⚡