GH#37 Link Visit with Vet

🤖 Review

9 open, worst first · 2 auto-applied · 4 coder assumptions to check

Requires human review

1. must look/code-review The new foreign key makes a vet with visits undeletable — DELETE /api/vets/{id} returns 500

   vet_id gets no ON DELETE clause and nothing on the JPA side nulls it out, so delete(vet) raises a foreign-key violation the advice turns into a 500. Verified live this run: DELETE /api/vets/4 → HTTP 500, violates foreign key constraint "visits_vet_id_fkey". The backfill gives every seeded vet a visit, so on a fresh database none of the six can be deleted.

   Three defensible answers: ON DELETE SET NULL, a guard in deleteVet answering 409, or “a vet who has attended is not deletable”. Returning 500 is not one of them.

   V9__link_visit_to_vet.sql:3-4new file

   3ALTER TABLE visits ADD COLUMN vet_id INT REFERENCES vets (id);
   4CREATE INDEX ON visits (vet_id);

   VetRestController.java:96-100unchanged

    96@DeleteMapping("{vetId}")
    97public void deleteVet(@PathVariable int vetId) {
    98    Vet vet = vetRepository.findById(vetId).orElseThrow();
    99    vetRepository.delete(vet);
    100}

2. worth a look/code-review The V9 backfill hardcodes vet ids 1–6, and the comment above it is not true of every database

   The UPDATE assumes the six V3-seeded vets still occupy ids 1–6. DELETE /api/vets/{id} is exposed, so in any long-lived database where a seeded vet was removed this violates visits_vet_id_fkey: Flyway aborts and the app does not start.

   Also false wherever a visit was booked through the UI. Selecting from the vets that actually exist costs one subquery and removes both problems.

   V9__link_visit_to_vet.sql:6-8new file

   6-- Backfill: every visit in the DB at this point comes from the V3 sample data,
   7-- spread round-robin over the 6 seeded vets so the UI has something to show.
   8UPDATE visits SET vet_id = 1 + (id % 6);

3. worth a look/code-review The vet dropdown calls a VET_ADMIN endpoint from two OWNER_ADMIN screens, and the 403 is swallowed

   The 403 is swallowed into an empty array, so the select renders with nothing but -- not assigned -- and no error.

   Masked today: petclinic.security.enable is false by default and the one seeded user holds all three roles, so no test catches it. PetTypeRestController sets the precedent — hasAnyRole(@roles.OWNER_ADMIN, @roles.VET_ADMIN) — but widening an authorization rule is a contract decision.

   VetRestController.java:27-70unchanged

    27@RestController
    28@RequestMapping("/api/vets")
    29@PreAuthorize("hasRole(@roles.VET_ADMIN)")
    30public class VetRestController {
    31
    32    private final VetMapper vetMapper;
    33    private final SpecialtyMapper specialtyMapper;
    34    private final VetRepository vetRepository;
    35    private final SpecialtyRepository specialtyRepository;
    36
    37    public VetRestController(
    38            VetMapper vetMapper,
    39            SpecialtyMapper specialtyMapper,
    40            VetRepository vetRepository,
    41            SpecialtyRepository specialtyRepository) {
    42        this.vetMapper = vetMapper;
    43        this.specialtyMapper = specialtyMapper;
    44        this.vetRepository = vetRepository;
    45        this.specialtyRepository = specialtyRepository;
    46    }
    47
    48    @GetMapping
    49    @ApiResponse(responseCode = "200", description = "OK",
    50            content = @Content(mediaType = "application/json",
    51                    array = @ArraySchema(schema = @Schema(implementation = VetDto.class)),
    52                    examples = @ExampleObject(name = "sample", value = ApiExamples.VETS)))
    53    public List<VetDto> listVets() {
    54        List<Vet> allVets = vetRepository.findAll();
    55        return vetMapper.toVetDtos(allVets);
    56    }
    57
    58    @GetMapping("{vetId}")
    59    public VetDto getVet(@PathVariable int vetId) {
    60        Vet vet = vetRepository.findById(vetId).orElseThrow();
    61        return vetMapper.toVetDto(vet);
    62    }
    63
    64    @PostMapping
    65    public ResponseEntity<Void> addVet(@RequestBody @Validated VetDto vetDto) {
    66        Vet vet = vetMapper.toVet(vetDto);
    67        updateSpecialties(vet);
    68        URI createdVetUri = UriComponentsBuilder.fromPath("/api/vets/{id}")
    69                .buildAndExpand(vet.getId()).toUri();
    70        return ResponseEntity.created(createdVetUri).build();

   vet.service.ts:21-26unchanged

    21getVets(): Observable<Vet[]> {
    22  return this.http.get<Vet[]>(this.entityUrl)
    23    .pipe(
    24      catchError(this.handlerError('getVets', []))
    25    );
    26}

4. worth a look/code-review @ManyToOne Vet is EAGER, and only one of the four read paths join-fetches it

   The other three — GET /api/owners/{ownerId} (which the new e2e exercises), GET /api/visits/{visitId} and PetClinicMcp.listVisits — issue one select … from vets where id=? per visit. On the MCP tool that is N+1 across the whole clinic.

   LAZY trades a query count for a LazyInitializationException on any path that maps outside the transaction. That trade is yours.

   Visit.java:31-33new code

   +31@ManyToOne
   +32@JoinColumn(name = "vet_id")
   +33private Vet vet;

   VisitRepository.java:18-191 line changed

   +18@Query("SELECT v FROM Visit v JOIN FETCH v.pet p JOIN FETCH p.owner LEFT JOIN FETCH v.vet")
    19List<Visit> findAllWithPetAndOwner();

5. nit/code-review An unknown vetId answers 404, for a resource the client is trying to create

   orElseThrow() throws NoSuchElementException, which the advice maps to 404 with a bare "Not found!" string body rather than the ProblemDetail every other handler returns. POST /api/visits {"vetId": 9999} answers 404 and the client cannot tell a bad field from a bad URL.

   Its Javadoc says “an unknown one is rejected”; a 400 naming the field would match that intent. Left alone because a status code is a contract.

   VetRepository.java:28-34new code

   +28@Nullable
   +29default Vet getByIdOrNull(@Nullable Integer vetId) {
   +30    if (vetId == null) {
   +31        return null;
   +32    }
   +33    return findByIdWithoutSpecialties(vetId).orElseThrow();
   +34}

6. nit/simplify The map now says a visit has exactly one vet, and the code says it may have none

   DomainModelExtractor reads the model by plain reflection and ignores JPA annotations on purpose, so it can only mark a * end — an unmarked end reads as exactly one.

   Optionality is the central design decision of this change and the one thing the living diagrams cannot show; no test will complain either — the extractor that feeds them cannot see it.

   Visit.java:31-33new code

   +31@ManyToOne
   +32@JoinColumn(name = "vet_id")
   +33private Vet vet;

7. nit/code-review The deny that protects the generated openapi.yaml covers only one tool

   Edit(./openapi.yaml) blocks Edit and not Write, MultiEdit or NotebookEdit; the shell-write denies added earlier were dropped again. CLAUDE.md says hand-editing the file is denied, so the rule and the documentation disagree.

   Raised, not fixed, on purpose: an agent should not widen its own permission configuration because a review suggested it — and the ./ prefix in that glob is worth confirming actually matches first.

   settings.json:10-17unchanged

    10"deny": [
    11  "Bash(git commit --no-verify*)",
    12  "Bash(git push --no-verify*)",
    13  "Edit(./openapi.yaml)"
    14],
    15"ask": [
    16  "Edit(.claude/settings.json)"
    17]

8. context/simplify The sequence deltas carry rendering noise

   This branch moved the diagram generator itself — trace-to-puml.ts alone changed 102 lines — so the committed base diagrams were rendered by the old generator and arrows that never changed still read as removed-and-re-added: 45 of 54 arrows red in add-visit, 34 of 64 in owner-search, a scenario this feature does not touch at all.

   Red means “rendered differently”, not “changed”. In add-visit, the listVets / GET /api/vets arrows are genuinely new. Re-recording the base side with the base generator needs the stack running on the base commit, and its ports are fixed.

   trace-to-puml.ts:1-20

9. context/simplify Which line should a diagram header point at — the comment, or the declaration?

   The existing test pins the opposite rule on purpose, at line 3 of its fixture: a comment naming the test is still where a reviewer wants to land. The applied fix was reverted; the file is untouched by this branch.

   Which rule you actually want is a call for whoever reads these diagrams.

   test-location.spec.ts:31-33unchanged

    31test('takes the first line that names the test, comment or declaration', () => {
    32  expect(lineOfTest(SPEC, 'Add a visit to an existing pet from the owner detail page')).toBe(3);
    33});

Decided by the coder, not by you

1. your callassumption The attending vet is optional at every layer — a visit nobody attended is a legal, permanent state

   Issue #37 asks that a visit “be linked to the vet that attended that consultation” and never says whether that link is mandatory. Six minutes in, the coder settled it as nullable everywhere — column, entity, both DTOs, no validation — and made -- not assigned -- the picker's first option. Nothing in the stack can now refuse a vet-less visit. “one design decision matters: vet should be optional/nullable since legacy rows and MCP-booked visits won't have one yet” — its own thinking, 13 Aug 23:00.

   Read the other way: Required on every new visit — @NotNull on VisitFieldsDto, NOT NULL after the backfill — with vet-less tolerated only for rows that predate the column.

   Whether a receptionist may save a visit naming no vet, and whether that is a legitimate resting state or a to-do something should chase.

   VisitFieldsDto.java:17-243 lines changed

    17@NotNull
    18@Size(min = 1, max = 255)
    19@Schema(example = "rabies shot", description = "The description for the visit.")
    20private String description;
    21
   +22@Min(0)
   +23@Schema(example = "1", description = "The ID of the vet that attended the visit.")
   +24private @Nullable Integer vetId;

   visit-add.component.html:54-62new code

   +54<div class="form-group">
   +55  <label for="vetId" class="col-sm-2 control-label">Vet</label>
   +56  <div class="col-sm-10">
   +57    <select id="vetId" name="vetId" class="form-control" [(ngModel)]="visit.vetId">
   +58      <option [ngValue]="null">-- not assigned --</option>
   +59      <option *ngFor="let vet of vets" [ngValue]="vet.id">{{ vet.firstName }} {{ vet.lastName }}</option>
   +60    </select>
   +61  </div>
   +62</div>

2. your callassumption A PUT that omits vetId silently unassigns the attending vet

   updateVisit assigns resolveVet(visitDto.getVetId()) unconditionally, and vetId is a plain Integer — so “field absent” and “explicitly null” are the same wire value. Any client still sending the pre-feature body wipes the vet on every save. “I'm adding a helper method that resolves a vet by id, returning null if the id is null and throwing otherwise, then using it in both bookVisit and updateVisit” — its own thinking, 13 Aug 23:02. The two paths were never separated after that.

   Read the other way: Leave the recorded vet untouched when vetId is absent, and require an explicit null to unassign.

   Whether an older integration, or a partial form post, is allowed to erase attribution as a side effect. No test clears a vet, so nothing pins today's answer either.

   VisitRestController.java:89-972 lines changed

   +89@Transactional
    90@PutMapping("{visitId}")
    91public void updateVisit(@PathVariable int visitId, @RequestBody @Validated VisitFieldsDto visitDto) {
    92    Visit currentVisit = visitRepository.findById(visitId).orElseThrow();
    93    currentVisit.setDate(visitDto.getDate());
    94    currentVisit.setDescription(visitDto.getDescription());
   +95    currentVisit.setVet(resolveVet(visitDto.getVetId()));
    96    visitRepository.save(currentVisit);
    97}

3. your callassumption The picker offers every vet — no specialty, no availability, no double-booking rule

   The form loads getVets() and lists the whole clinic, unfiltered and unordered. The domain already models vet_specialties, and a visit already carries a date and an exact time, so narrowing by specialty or by who is free at that slot was available and was never weighed. Nothing prevents booking one vet for two visits at the same minute. “adding a vets array to visit-add, loading it in ngOnInit, and binding a select dropdown to vetId using ngValue” — its own thinking, 13 Aug 23:03, straight from “add a dropdown” to binding it.

   Read the other way: Offer only vets whose specialty fits, or only those with no clashing visit at that date and time — and reject a clashing vetId server-side.

   Whether picking a vet is scheduling, with the constraints that implies, or only record-keeping about who was there.

   visit-add.component.ts:43-591 line changed

    43ngOnInit() {
   +44  this.vetService.getVets().subscribe(vets => this.vets = vets);
    45  console.log(this.route.parent);
    46  const petId = this.route.snapshot.params.id;
    47  this.petService.getPetById(petId).subscribe(
    48    pet => {
    49      this.currentPet = pet;
    50      this.visit.pet = this.currentPet;
    51      this.currentPetType = this.currentPet.type;
    52      this.ownerService.getOwnerById(pet.ownerId).subscribe(
    53        owner => {
    54          this.currentOwner = owner;
    55        }
    56      )
    57    },
    58    error => this.errorMessage = error as any);
    59}

4. your callassumption The chatbot can read the attending vet but can never set one

   “Display its vet everywhere throughout the app” was read as display-only for the MCP surface: list_visits gained a vet name, create_visit gained no vet parameter. Every visit an owner books through the assistant is therefore vet-less until somebody opens the web form. “This keeps things least disruptive and avoids breaking the MCP create_visit path, so I'll move on to checking the remaining files rather than asking for clarification” — its own thinking, 13 Aug 23:00. That constraint is also what forced the column to stay nullable, so this call and the first one are the same call.

   Read the other way: Give create_visit an optional vetId, so the chat booking path carries what the web form does.

   Whether owners booking by chat are meant to choose a vet at all, and who is expected to fill the gap on the visits they create.

   PetClinicMcp.java:119-153unchanged

    119@McpTool(
    120        name = "create_visit",
    121        description = "Create a new vet visit for one of the authenticated owner's pets "
    122                + "(date/time, pet, description). Books the visit directly — no confirmation prompt.",
    123        annotations = @McpAnnotations(destructiveHint = true))
    124@Transactional
    125public String createVisit(
    126        @McpToolParam(description = "Pet ID (must belong to the authenticated owner)", required = true) int petId,
    127        @McpToolParam(description = "Visit date (yyyy-MM-dd); must be today or in the future",
    128                required = true) LocalDate visitDate,
    129        @McpToolParam(description = "Exact local time of the appointment (HH:mm), e.g. 08:00",
    130                required = true) LocalTime visitTime,
    131        @McpToolParam(description = "Visit description (reason, diagnosis, notes...)",
    132                required = true) String description) {
    133    int ownerId = McpSecurity.currentOwnerId();
    134    Pet pet = petRepository.findById(petId)
    135            .orElseThrow(() -> new IllegalArgumentException("Pet not found: " + petId));
    136    if (pet.getOwner() == null || pet.getOwner().getId() != ownerId) {
    137        throw new IllegalArgumentException("Pet " + petId + " does not belong to owner " + ownerId);
    138    }
    139    requireFutureDate(visitDate);
    140    if (LocalDateTime.of(visitDate, visitTime).isBefore(LocalDateTime.now())) {
    141        throw new IllegalArgumentException("Visit time must be in the future: " + visitDate + " " + visitTime);
    142    }
    143    requireUnderUpcomingVisitCap(pet);
    144
    145    Visit v = new Visit();
    146    v.setDate(visitDate);
    147    v.setTime(visitTime);
    148    v.setDescription(description);
    149    pet.addVisit(v); // maintain both sides of the Pet<->Visit association
    150    Visit saved = visitRepository.save(v);
    151    return "Created visit id=" + saved.getId() + " for pet '" + pet.getName() + "' on " + visitDate
    152            + " at " + visitTime;
    153}

Already fixed for you

Diffs cut from bf26a0de alone. A third was applied and then reverted — it inverted a rule the tests pin deliberately; that is item 9 above.

1. auto-fixed/code-review Made the two remaining visit write paths one unit of work — on the public methods, not the private one

   @Transactional on the private, self-invoked method is a no-op — Spring AOP proxies the public entry point, which is where it now sits.

   VisitRestController.java+20 −2 vs bf26a0de

   		import io.swagger.v3.oas.annotations.media.Schema;
   8	8	import io.swagger.v3.oas.annotations.responses.ApiResponse;
   9	9	import jakarta.transaction.Transactional;
   10	10	import lombok.RequiredArgsConstructor;
   	11	+import lombok.extern.slf4j.Slf4j;
   11	12	import org.springframework.http.ResponseEntity;
   	13	+import victor.training.petclinic.domain.Vet;
   12	14	import victor.training.petclinic.mapper.VisitMapper;
   13	15	import victor.training.petclinic.domain.Visit;
   14	16	import victor.training.petclinic.repository.VetRepository;
   		import org.springframework.security.access.prepost.PreAuthorize;
   21	23	import org.springframework.web.util.UriComponentsBuilder;
   22	24
   23	25	import java.util.List;
   	26	+import java.util.NoSuchElementException;
   24	27
   	28	+@Slf4j
   25	29	@RestController
   26	30	@RequestMapping("/api/visits")
   27	31	@RequiredArgsConstructor
   		public class VisitRestController {
   47	51	return visitMapper.toVisitDto(visit);
   48	52	}
   49	53
   	54	+ // On the public entry point, not on bookVisit: that one is private and self-invoked,
   	55	+ // so a @Transactional there would be silently ignored by Spring AOP — the same reason
   	56	+ // the span below has to come from the bytecode agent.
   	57	+ @Transactional
   50	58	@PostMapping
   51	59	public ResponseEntity<Void> addVisit(@RequestBody @Validated VisitDto visitDto) {
   52	60	int id = bookVisit(visitDto);
   		public class VisitRestController {
   63	71	@WithSpan("book-visit")
   64	72	private int bookVisit(VisitDto visitDto) {
   65	73	Visit visit = visitMapper.toVisit(visitDto);
   66		- visit.setVet(vetRepository.getByIdOrNull(visitDto.getVetId()));
   	74	+ visit.setVet(resolveVet(visitDto.getVetId()));
   67	75	visitRepository.save(visit);
   68	76	return visit.getId();
   69	77	}
   70	78
   	79	+ @Transactional
   71	80	@PutMapping("{visitId}")
   72	81	public void updateVisit(@PathVariable int visitId, @RequestBody @Validated VisitFieldsDto visitDto) {
   73	82	Visit currentVisit = visitRepository.findById(visitId).orElseThrow();
   74	83	currentVisit.setDate(visitDto.getDate());
   75	84	currentVisit.setDescription(visitDto.getDescription());
   76		- currentVisit.setVet(vetRepository.getByIdOrNull(visitDto.getVetId()));
   	85	+ currentVisit.setVet(resolveVet(visitDto.getVetId()));
   77	86	visitRepository.save(currentVisit);
   78	87	}
   79	88
   	89	+ private Vet resolveVet(Integer vetId) {
   	90	+ try {
   	91	+ return vetRepository.getByIdOrNull(vetId);
   	92	+ } catch (NoSuchElementException e) {
   	93	+ log.warn("Rejecting visit: attending vet id {} does not exist", vetId);
   	94	+ throw e;
   	95	+ }
   	96	+ }
   	97	+
   80	98	@Transactional
   81	99	@DeleteMapping("{visitId}")
   82	100	public void deleteVisit(@PathVariable int visitId) {

2. auto-fixed/simplify Attaching a vet to a visit no longer drags its specialties along

   The vet-detail query was fetching specialties that three write endpoints throw away; the existing one is untouched for the paths that need them.

   VetRepository.java+5 −1 vs bf26a0de

   		public interface VetRepository extends Repository<Vet, Integer> {
   15	15	@Query("SELECT v FROM Vet v LEFT JOIN FETCH v.specialties WHERE v.id = :id")
   16	16	Optional<Vet> findById(int id);
   17	17
   	18	+ /** The row alone — attaching a vet to a visit needs its identity, not its specialties. */
   	19	+ @Query("SELECT v FROM Vet v WHERE v.id = :id")
   	20	+ Optional<Vet> findByIdWithoutSpecialties(int id);
   	21	+
   18	22	/**
   19	23	* The attending-vet rule, in one place: a null vetId means "no vet attended (yet)",
   20	24	* an unknown one is rejected. Both write paths that accept a vetId from a DTO
   		public interface VetRepository extends Repository<Vet, Integer> {
   26	30	if (vetId == null) {
   27	31	return null;
   28	32	}
   29		- return findById(vetId).orElseThrow();
   	33	+ return findByIdWithoutSpecialties(vetId).orElseThrow();
   30	34	}
   31	35
   32	36	void save(Vet vet);

Demo

API

Data

The concepts, as the team drew them

Tests

Legend:fully coveredpartiallyexecutedmissingN/A

victorrentea opened on Jun 13, 2026

The Visit should be linked to the vet that attended that consultation. Visit should display its vet everywhere throughout the app.

---

What it has to do

1. Booking a visit lets you choose the vet, and lets you not choose one. The vet is optional: half the time the appointment is booked before anyone knows who is taking it, and forcing a choice there produces bad data rather than information.
2. Editing a visit can change the vet, and can remove it. Clearing the field must persist as empty. We had this with pet types: the old value kept coming back.
3. Wherever a visit is shown with its details, the vet is shown too. Today that means the owner's page and the all-visits screen.
4. A visit with no vet reads as having none. Not "Unknown", not blank-because-broken, and never an error. Visits created before this change have no vet and will not get one.

Out of scope

Searching or filtering visits by vet.

UIclicks the screenAPIREST/MCPunitone isolated component

Sequence

 60@GenerateSequence
 61class AddVisitSequenceTest {
 ⋯8 lines not shown
 70    @Test
+71    @Order(1)
 72    void addsAVisitToAnExistingPet() throws Exception {
 73        given("an owner with at least one pet exists");
 74        JsonNode owner = anOwnerWithAPet();
 75        int ownerId = owner.path("id").asInt();
 76        int petId = owner.path("pets").get(0).path("id").asInt();
 77
 78        when("the owner detail page is opened");
 79        call(mockMvc, get("/api/owners/{ownerId}", ownerId)).andExpect(status().isOk());
 80
 81        and("a visit is added for the first pet");
 82        String description = "Annual check-up " + System.currentTimeMillis();
 83        call(mockMvc, post("/api/owners/{ownerId}/pets/{petId}/visits", ownerId, petId)
 84                .contentType(MediaType.APPLICATION_JSON)
 85                .content(mapper.writeValueAsString(Map.of("date", VISIT_DATE, "description", description))))
 86                .andExpect(status().isCreated());
 87
 88        then("the visit is listed under the pet");
 89        JsonNode reloaded = json(call(mockMvc, get("/api/owners/{ownerId}", ownerId))
 90                .andExpect(status().isOk()));
 91        assertThat(reloaded.path("pets").get(0).path("visits").toString())
 92                .contains(description)
 93                .contains(VISIT_DATE);
 94    }
 ⋯29 lines not shown
+124    @Test
+125    @Order(2)
+126    @WithMockUser(roles = {"OWNER_ADMIN", "VET_ADMIN"})
+127    void remembersTheVetWhoAttendedIt() throws Exception {
+128        given("an owner with at least one pet exists");
+129        JsonNode owner = anOwnerWithAPet();
+130        int ownerId = owner.path("id").asInt();
+131        int petId = owner.path("pets").get(0).path("id").asInt();
+132
+133        and("the clinic has a vet who can attend it");
+134        JsonNode vet = theFirstVet();
+135        int vetId = vet.path("id").asInt();
+136        String vetName = vet.path("firstName").asText() + " " + vet.path("lastName").asText();
+137
+138        when("a visit is booked for that pet with that vet attending");
+139        String description = "Annual check-up " + System.currentTimeMillis();
+140        call(mockMvc, post("/api/owners/{ownerId}/pets/{petId}/visits", ownerId, petId)
+141                .contentType(MediaType.APPLICATION_JSON)
+142                .content(mapper.writeValueAsString(
+143                        Map.of("date", VISIT_DATE, "description", description, "vetId", vetId))))
+144                .andExpect(status().isCreated());
+145
+146        then("that pet's history shows the visit was attended by that vet");
+147        JsonNode reloaded = json(call(mockMvc, get("/api/owners/{ownerId}", ownerId))
+148                .andExpect(status().isOk()));
+149        JsonNode visit = visitDescribed(reloaded, description);
+150        // The name, not only the id: the id proves the column was written, the two names
+151        // prove the read path joins the vet back in — which is the half the UI depends on.
+152        assertThat(visit.path("vetId").asInt()).isEqualTo(vetId);
+153        assertThat(visit.path("vetFirstName").asText() + " " + visit.path("vetLastName").asText())
+154                .isEqualTo(vetName);
+155    }