diff --git a/.github/workflows/ci-minimal.yml b/.github/workflows/ci-minimal.yml new file mode 100644 index 000000000..453248706 --- /dev/null +++ b/.github/workflows/ci-minimal.yml @@ -0,0 +1,19 @@ +name: Java CI (minimal tests) + +on: + push: + pull_request: + +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - name: Set up JDK 17 + uses: actions/setup-java@v4 + with: + java-version: '17' + distribution: 'temurin' + cache: maven + - name: Run Maven tests + run: ./mvnw -B test diff --git a/.github/workflows/maven-build-main.yml b/.github/workflows/maven-build-main.yml index daa566a7a..5eed44e00 100644 --- a/.github/workflows/maven-build-main.yml +++ b/.github/workflows/maven-build-main.yml @@ -11,6 +11,8 @@ jobs: build: runs-on: ubuntu-latest + env: + SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} strategy: matrix: java: [ '17', '21' ] @@ -23,8 +25,10 @@ jobs: java-version: ${{matrix.java}} distribution: 'adopt' cache: maven - - name: Build and analyze + - name: Build + run: ./mvnw -B verify + - name: Analyze with Sonar + if: ${{ env.SONAR_TOKEN != '' }} env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} - run: ./mvnw -B verify org.sonarsource.scanner.maven:sonar-maven-plugin:sonar -Dsonar.projectKey=spring-petclinic_spring-framework-petclinic -Dsonar.organization=spring-petclinic + run: ./mvnw -B org.sonarsource.scanner.maven:sonar-maven-plugin:sonar -Dsonar.projectKey=spring-petclinic_spring-framework-petclinic -Dsonar.organization=spring-petclinic diff --git a/docs/leases/petclinic-autonomous-readability-refactor-batch-invocation-template.md b/docs/leases/petclinic-autonomous-readability-refactor-batch-invocation-template.md new file mode 100644 index 000000000..7db4f4d8d --- /dev/null +++ b/docs/leases/petclinic-autonomous-readability-refactor-batch-invocation-template.md @@ -0,0 +1,116 @@ +# PetClinic Autonomous Readability Refactor Batch Invocation Template + +Use this template to start a governed autonomous refactor batch from the current +repository state. + +```yaml +startLease: petclinic-autonomous-readability-refactor-batch-lease-v1 + +repository: C:\dev\spring-framework-petclinic +branch: +startHead: +initialWorktree: clean + +budget: + maxCandidatesThisRun: 10 + maxCommitsThisRun: 10 + maxFilesPerCandidate: 1 + maxChangedLinesPerCandidate: 80 + maxNewPrivateMethodsPerCandidate: 2 + maxRepairAttemptsPerCandidate: 1 + +scope: + primaryAllowedPaths: + - src/main/java/org/springframework/samples/petclinic/web/**/*.java + - src/main/java/org/springframework/samples/petclinic/service/**/*.java + optionalAllowedPaths: + - src/main/java/org/springframework/samples/petclinic/model/**/*.java + - src/main/java/org/springframework/samples/petclinic/util/**/*.java + - src/main/java/org/springframework/samples/petclinic/repository/**/*.java + repositoryScopeRules: + - readability-only changes + - no repository interface signature changes + - no SQL/HQL/JPQL string changes + - no query parameter name, value, or order changes + - no row mapper, result extractor, association, or persistence behavior changes + - no transaction or cache annotation changes + - no exception type or exception message changes + +validation: + fullMavenTestRequiredPerCandidate: true + commands: + - git diff --name-only + - git diff --check + - $env:JAVA_HOME='C:\Program Files\Java\jdk-17'; .\mvnw.cmd test + +authorization: + allowed: + - inspect repository state + - discover and rank candidates + - admit one candidate at a time + - edit only the admitted file + - exact-revert current candidate on validation failure + - one bounded repair attempt per candidate + - one local commit per successful candidate + - continue until budget exhausted or no candidate qualifies + forbidden: + - push + - PR creation or update + - upstream interaction + - fetch or pull + - force push + - merge + - release + - deploy + - pom.xml or dependency changes + - src/test changes + - src/main/resources changes + - src/main/webapp changes + - request mapping/view/model/validation/repository/persistence behavior changes + - SQL/HQL/JPQL changes + - repository interface changes + +stopConditions: + - branch mismatch + - start HEAD mismatch + - dirty worktree before candidate + - unexpected changed path + - no candidate score >= 75 + - ambiguity after tie-breaks + - behavior preservation cannot be argued mechanically + - changed-line or helper-method budget exceeded + - git diff --check fails + - Maven test fails after one bounded repair + - exact revert fails + - commit fails + - worktree not clean after commit + - maxCandidatesThisRun reached + - maxCommitsThisRun reached + +finalReport: + include: + - processed candidates + - rejected candidate summary + - commits created + - final HEAD + - final git status + - tests run and results + - stopped reason + - remaining likely candidates + - explicit non-claims +``` + +Example invocation: + +```text +Start lease petclinic-autonomous-readability-refactor-batch-lease-v1. + +Repository: C:\dev\spring-framework-petclinic +Branch: threshold-governed-refactor-demo-3 +Start HEAD: +Initial worktree: clean + +Run autonomously until budget exhaustion or ready_no_candidates. +Do not push, open or update PRs, merge, release, deploy, fetch, pull, or +interact with upstream. +``` diff --git a/docs/leases/petclinic-autonomous-readability-refactor-batch-lease-v1.md b/docs/leases/petclinic-autonomous-readability-refactor-batch-lease-v1.md new file mode 100644 index 000000000..e6bc2eded --- /dev/null +++ b/docs/leases/petclinic-autonomous-readability-refactor-batch-lease-v1.md @@ -0,0 +1,485 @@ +# PetClinic Autonomous Readability Refactor Batch Lease v1 + +This lease defines the governed workflow for autonomous, low-to-medium-risk +readability refactoring in the owned Spring PetClinic repository. + +It is intentionally split into authority phases. A refactor batch lease may +create local commits, but it must not publish, open or update a PR, merge, +release, deploy, or interact with upstream unless a later phase explicitly +authorizes that action. + +## Lease Identity + +```yaml +leaseName: petclinic-autonomous-readability-refactor-batch-lease-v1 +repository: C:\dev\spring-framework-petclinic +defaultBranchPattern: threshold-governed-refactor-demo* +requiredInitialWorktree: clean +``` + +## Purpose + +Continue PetClinic refactoring autonomously over a bounded candidate pool. +The lease is designed for mechanical readability work only: + +- private helper extraction +- duplicate literal constant extraction +- redundant local variable simplification +- small controller branch readability decomposition +- validator and formatter readability cleanup +- repository/JDBC/JPA readability cleanup without query or persistence behavior + changes + +The lease does not authorize behavior changes or public claims about readiness, +correctness, security, or compliance. + +## Authority Levels + +```yaml +allowedRiskClasses: + - R0_READ_ONLY + - R1_READOUT + - R2_REVERSIBLE_LOCAL_MUTATION + - R3_LOCAL_COMMIT + +notAuthorizedByThisLease: + - R4_OWNED_REPO_BRANCH_PUSH + - R4_OWNED_REPO_DRAFT_PR_CREATE_OR_UPDATE + - R4_CI_STABILIZATION + - R5_PR_MERGE + - release + - deploy +``` + +## Batch Budget + +```yaml +maxCandidatesThisRun: 10 +maxCommitsThisRun: 10 +maxFilesPerCandidate: 1 +maxChangedLinesPerCandidate: 80 +maxNewPrivateMethodsPerCandidate: 2 +maxRepairAttemptsPerCandidate: 1 +fullMavenTestRequiredPerCandidate: true +requireCleanWorktreeBeforeEachCandidate: true +requireCleanWorktreeAfterEachCommit: true +stopOnUnexpectedChangedPath: true +``` + +## Allowed Paths + +```yaml +allowedPaths: + - src/main/java/org/springframework/samples/petclinic/web/**/*.java + - src/main/java/org/springframework/samples/petclinic/service/**/*.java + - src/main/java/org/springframework/samples/petclinic/model/**/*.java + - src/main/java/org/springframework/samples/petclinic/util/**/*.java + - src/main/java/org/springframework/samples/petclinic/repository/**/*.java + +forbiddenPaths: + - pom.xml + - src/test/** + - src/main/resources/** + - src/main/webapp/** + - .github/** + - target/** + - hidden/config files +``` + +The default starting scope should be `web` and `service`. `model`, `util`, and +`repository` may be admitted only when the candidate is mechanically local and +covered by the same validation protocol. + +Repository work is restricted to readability-only changes in the existing JDBC, +JPA, and repository adapter classes. It must not change SQL/HQL/JPQL strings, +query parameters, result mapping semantics, save/merge/persist behavior, +transaction behavior, exception behavior, or repository interface contracts. + +## Candidate Classes + +### 1. Private Helper Extraction For Readability + +Allowed when all conditions hold: + +- same class only +- helper method is private +- at most two new private methods +- extracted code is contiguous or logically identical to an existing local block +- no public API change +- no request mapping change +- no validation annotation change +- no repository call change +- no persistence behavior change +- no returned view name or model attribute name change + +### 2. Duplicate Literal Constant Extraction + +Allowed when all conditions hold: + +- same class only +- new constant is `private static final String` +- literal is repeated in the same file +- literal represents a view name, redirect, model key, route path, validation + field, or validation code +- replacement preserves the exact string value +- at most two constants per candidate + +### 3. Redundant Local Variable Simplification + +Allowed when all conditions hold: + +- local variable is assigned exactly once +- initializer is side-effect free or already required immediately +- simplification does not reduce readability +- no behavior change + +### 4. Controller Branch Readability Decomposition + +Allowed when all conditions hold: + +- only splits existing branch bodies into private methods +- returned view names and redirects are identical strings or constants +- model keys are identical +- BindingResult behavior is identical +- branch order is unchanged +- no request mapping, HTTP method, validation, repository, or persistence change + +### 5. Validator Or Formatter Readability Cleanup + +Allowed when all conditions hold: + +- same class only +- no supported type change +- no validation field/code/message change +- no parse/print semantics change +- no exception message change unless explicitly authorized + +### 6. Repository/JDBC/JPA Readability Cleanup + +Allowed when all conditions hold: + +- same class only +- helper method is private +- at most two new private methods +- no repository interface signature change +- no SQL/HQL/JPQL string change +- no query parameter name, value, or order change +- no row mapper, result extractor, or entity association semantic change +- no persist, merge, insert, generated-key, or update behavior change +- no transaction annotation or cache annotation change +- no exception type or exception message change +- existing repository call order is preserved unless the candidate is purely a + local helper extraction that calls the same operations in the same order + +Examples that may qualify: + +- remove an unnecessary `else` after an immediate `return` +- extract a contiguous loop body into a private helper when query and mapping + behavior remain identical +- extract a local private helper for setting already-loaded associations on + already-loaded entities + +Examples that must be rejected: + +- rewriting SQL/HQL/JPQL +- changing joins, ordering, filters, aliases, or selected columns +- changing mapper classes or result extractor contracts +- changing save/update support or generated-key handling +- changing repository interfaces + +### 7. Micro-Format Cleanup + +Allowed only inside a method already admitted for one of the candidate classes +above. Broad file formatting is not authorized. + +## Explicitly Forbidden Changes + +```yaml +forbiddenChanges: + - request mapping path changes + - HTTP method changes + - validation annotation semantic changes + - repository query semantic changes + - persistence behavior changes + - transaction behavior changes unless separately authorized + - cache annotation changes + - returned view name changes + - model attribute name changes + - dependency or plugin updates + - pom.xml edits + - test edits + - resource edits + - broad formatting + - feature work +``` + +## Discovery Protocol + +Before each candidate: + +```powershell +git status -sb +git rev-parse HEAD +git branch --show-current +git diff --name-only +``` + +Required: + +- worktree is clean +- branch matches the lease invocation +- changed path list is empty + +Discovery must scan only allowed paths. Exclude candidates already committed in +the current branch lineage unless the new candidate is a strictly local +continuation in a different method. + +## Scoring + +```yaml +positive: + singleFile: 30 + noBehaviorChangeExpected: 30 + fullMavenTestAvailable: 20 + existingControllerOrServiceCoverage: 10 + existingRepositoryCoverage: 10 + privateHelperImprovesReadability: 15 + duplicateLiteralConstantRemovesRepetition: 10 + redundantLocalVariableSimplification: 12 + repositoryReadabilityWithoutSemanticChange: 10 + diffUnder40ChangedLines: 10 + diff40To80ChangedLines: 5 + noPublicSignatureChange: 10 + +negative: + controllerBranchDecomposition: -10 + repositoryAdapterTouch: -10 + jdbcOrJpaQueryAdjacentCode: -15 + helperNameRequiresSemanticJudgment: -10 + changesMoreThanOneBranch: -15 + extractionCrossesNonContiguousLogic: -20 + +reject: + - changes public method signature + - changes request mapping, view, model, validation, repository, or persistence behavior + - requires test changes + - requires pom.xml or dependency changes + - requires upstream, push, PR, merge, release, or deploy +``` + +Admission threshold: + +```yaml +minimumScore: 75 +``` + +## Tie-Break Rules + +Apply in order: + +1. Prefer smaller diff. +2. Prefer one private helper over two. +3. Prefer already covered controller or service methods. +4. Prefer `OwnerController` over `PetController` over `VisitController` over + `VetController` over `PetValidator` over `PetTypeFormatter` over service + over model over util over repository. +5. Within repository scope, prefer non-query-adjacent cleanup over JDBC helper + extraction, then JDBC helper extraction over JPA query-adjacent cleanup. +6. Prefer lexical file path. +7. Prefer lexical method name. +8. Stop with an ambiguity packet if still indistinguishable. + +## Candidate Packet + +For every admitted candidate, record: + +```yaml +candidateId: +candidateClass: +baseHead: +allowedFile: +beforeSha256: +expectedDiffSummary: +score: +tieBreakReason: +behaviorPreservation: +validationCommands: + - git diff --name-only + - git diff --check + - $env:JAVA_HOME='C:\Program Files\Java\jdk-17'; .\mvnw.cmd test +revertStrategy: exact restore of admitted file before commit +nonClaims: + - no push + - no PR + - no upstream interaction + - no merge + - no release + - no deploy + - no readiness/correctness/security/compliance claim +``` + +## Patch Protocol + +Rules: + +- edit only the admitted file +- stay within changed-line and helper-method budgets +- preserve all forbidden behavior-change checks +- use repository style +- avoid unrelated formatting + +## Validation Protocol + +Run after every candidate patch: + +```powershell +git diff --name-only +git diff --check +$env:JAVA_HOME='C:\Program Files\Java\jdk-17'; .\mvnw.cmd test +``` + +Required: + +- changed paths contain only the admitted file +- `git diff --check` passes +- full Maven test passes + +## Failure And Repair Protocol + +If validation fails: + +1. Determine whether the failure is clearly caused by the current candidate. +2. If yes, perform at most one bounded repair inside the admitted file. +3. Rerun `git diff --check` and full Maven test. +4. If still failing, exact-revert the admitted file. +5. Require clean worktree after revert. +6. Stop with a failure receipt. + +If the failure requires forbidden files, upstream interaction, dependency +changes, test changes, force push, merge, release, or deploy, stop without +repair. + +## Commit Protocol + +After successful validation: + +```powershell +Get-FileHash -Algorithm SHA256 +git add +git commit -m "Refactor PetClinic " +git status -sb +git log -1 --oneline +git rev-parse HEAD +``` + +Required: + +- only admitted file staged +- exactly one local commit per successful candidate +- worktree clean after commit + +## Batch Continuation + +Continue to the next candidate only when: + +- budget remains +- worktree is clean +- previous commit succeeded +- another candidate reaches the admission threshold + +Stop with `ready_no_candidates` when no candidate qualifies. + +## Terminal Receipt + +At batch end, report: + +```yaml +processedCandidates: [] +commitsCreated: [] +finalHead: +finalGitStatus: +testsRun: +stoppedReason: +remainingLikelyCandidates: [] +nonClaims: + - no push + - no PR + - no upstream interaction + - no fetch/pull + - no pom.xml/dependency change + - no src/test change + - no request mapping/view/model/validation/repository/persistence behavior change + - no merge/release/deploy + - no public readiness/correctness/security/compliance claim +``` + +## Downstream Authority Phases + +The following leases must be separate invocations. + +### Owned Branch Publish And Draft PR Lease + +Purpose: + +- run final `git diff --check` +- run full Maven test +- push the current branch to owned origin +- create or update a draft PR in the owned repository only + +Not allowed: + +- upstream interaction +- force push +- ready-for-review +- reviewer request +- merge +- release +- deploy + +### Draft PR CI Stabilization Lease + +Purpose: + +- inspect existing owned draft PR +- read CI status +- if green, stop +- if pending, stop +- if failing, perform at most bounded branch-local repairs inside allowed paths + +Not allowed: + +- new feature work +- broad refactoring +- dependency or test changes +- ready-for-review +- reviewer request +- merge +- release +- deploy + +### Owned Repo PR Merge Lease + +Purpose: + +- merge exactly one owned-repository PR using the authorized merge method + +Preconditions: + +- worktree clean +- PR open +- PR not draft +- PR head matches required SHA +- PR targets owned repository, not third-party upstream +- no conflicts +- all required checks pass +- full local Maven test passes + +Not allowed: + +- branch protection bypass +- force push +- code changes +- tag creation +- release +- deploy +- upstream interaction diff --git a/pom.xml b/pom.xml index 831084563..bacef40a4 100644 --- a/pom.xml +++ b/pom.xml @@ -62,7 +62,7 @@ 2.10.0 5.23.0 3.0 - 6.1.0 + 6.1.1 8.1.0 @@ -81,7 +81,7 @@ 3.5.5 3.5.1 0.8.15 - 3.6.2 + 3.6.3 0.3.4 diff --git a/src/main/java/org/springframework/samples/petclinic/model/Owner.java b/src/main/java/org/springframework/samples/petclinic/model/Owner.java index e662aeaa3..d51a493aa 100644 --- a/src/main/java/org/springframework/samples/petclinic/model/Owner.java +++ b/src/main/java/org/springframework/samples/petclinic/model/Owner.java @@ -43,6 +43,10 @@ @Entity @Table(name = "owners") public class Owner extends Person { + + private static final Comparator PET_NAME_COMPARATOR = + Comparator.comparing(Pet::getName, String.CASE_INSENSITIVE_ORDER); + @Column(name = "address") @NotEmpty private String address; @@ -97,7 +101,7 @@ protected void setPetsInternal(Set pets) { public List getPets() { List sortedPets = new ArrayList<>(getPetsInternal()); - sortedPets.sort(Comparator.comparing(Pet::getName, String.CASE_INSENSITIVE_ORDER)); + sortedPets.sort(PET_NAME_COMPARATOR); return Collections.unmodifiableList(sortedPets); } @@ -125,17 +129,17 @@ public Pet getPet(String name) { public Pet getPet(String name, boolean ignoreNew) { name = name.toLowerCase(); for (Pet pet : getPetsInternal()) { - if (!ignoreNew || !pet.isNew()) { - String compName = pet.getName(); - compName = compName.toLowerCase(); - if (compName.equals(name)) { - return pet; - } + if (isMatchingPet(pet, name, ignoreNew)) { + return pet; } } return null; } + private boolean isMatchingPet(Pet pet, String name, boolean ignoreNew) { + return (!ignoreNew || !pet.isNew()) && pet.getName().toLowerCase().equals(name); + } + @Override public String toString() { return new ToStringCreator(this) diff --git a/src/main/java/org/springframework/samples/petclinic/model/Pet.java b/src/main/java/org/springframework/samples/petclinic/model/Pet.java index 562620473..8725c913c 100644 --- a/src/main/java/org/springframework/samples/petclinic/model/Pet.java +++ b/src/main/java/org/springframework/samples/petclinic/model/Pet.java @@ -45,6 +45,8 @@ @Table(name = "pets") public class Pet extends NamedEntity { + private static final Comparator VISIT_DATE_DESCENDING = Comparator.comparing(Visit::getDate).reversed(); + @Column(name = "birth_date") @DateTimeFormat(pattern = "yyyy/MM/dd") private LocalDate birthDate; @@ -98,7 +100,7 @@ protected void setVisitsInternal(Set visits) { public List getVisits() { List sortedVisits = new ArrayList<>(getVisitsInternal()); - sortedVisits.sort(Comparator.comparing(Visit::getDate).reversed()); + sortedVisits.sort(VISIT_DATE_DESCENDING); return Collections.unmodifiableList(sortedVisits); } diff --git a/src/main/java/org/springframework/samples/petclinic/model/Vet.java b/src/main/java/org/springframework/samples/petclinic/model/Vet.java index a8af064b3..c98acbb6a 100644 --- a/src/main/java/org/springframework/samples/petclinic/model/Vet.java +++ b/src/main/java/org/springframework/samples/petclinic/model/Vet.java @@ -43,6 +43,9 @@ @Table(name = "vets") public class Vet extends Person { + private static final Comparator SPECIALTY_NAME_COMPARATOR = + Comparator.comparing(Specialty::getName, String.CASE_INSENSITIVE_ORDER); + @ManyToMany(fetch = FetchType.EAGER) @JoinTable(name = "vet_specialties", joinColumns = @JoinColumn(name = "vet_id"), inverseJoinColumns = @JoinColumn(name = "specialty_id")) @@ -62,7 +65,7 @@ protected void setSpecialtiesInternal(Set specialties) { @XmlElement public List getSpecialties() { List sortedSpecs = new ArrayList<>(getSpecialtiesInternal()); - sortedSpecs.sort(Comparator.comparing(Specialty::getName, String.CASE_INSENSITIVE_ORDER)); + sortedSpecs.sort(SPECIALTY_NAME_COMPARATOR); return Collections.unmodifiableList(sortedSpecs); } diff --git a/src/main/java/org/springframework/samples/petclinic/repository/jdbc/JdbcOwnerRepositoryImpl.java b/src/main/java/org/springframework/samples/petclinic/repository/jdbc/JdbcOwnerRepositoryImpl.java index 65403977f..4b471f3fe 100644 --- a/src/main/java/org/springframework/samples/petclinic/repository/jdbc/JdbcOwnerRepositoryImpl.java +++ b/src/main/java/org/springframework/samples/petclinic/repository/jdbc/JdbcOwnerRepositoryImpl.java @@ -69,14 +69,7 @@ public JdbcOwnerRepositoryImpl(DataSource dataSource, JdbcClient jdbcClient) { */ @Override public Collection findByLastName(String lastName) { - List owners = this.jdbcClient.sql(""" - SELECT id, first_name, last_name, address, city, telephone - FROM owners - WHERE last_name like :lastName - """) - .param("lastName", lastName + "%") - .query(BeanPropertyRowMapper.newInstance(Owner.class)) - .list(); + List owners = findOwnersByLastNamePrefix(lastName); loadOwnersPetsAndVisits(owners); return owners; } @@ -87,9 +80,25 @@ public Collection findByLastName(String lastName) { */ @Override public Owner findById(int id) { - Owner owner; + Owner owner = findOwnerById(id); + loadPetsAndVisits(owner); + return owner; + } + + private List findOwnersByLastNamePrefix(String lastName) { + return this.jdbcClient.sql(""" + SELECT id, first_name, last_name, address, city, telephone + FROM owners + WHERE last_name like :lastName + """) + .param("lastName", lastName + "%") + .query(BeanPropertyRowMapper.newInstance(Owner.class)) + .list(); + } + + private Owner findOwnerById(int id) { try { - owner = this.jdbcClient.sql(""" + return this.jdbcClient.sql(""" SELECT id, first_name, last_name, address, city, telephone FROM owners WHERE id = :id """) @@ -99,18 +108,23 @@ public Owner findById(int id) { } catch (EmptyResultDataAccessException ex) { throw new ObjectRetrievalFailureException(Owner.class, id); } - loadPetsAndVisits(owner); - return owner; } public void loadPetsAndVisits(final Owner owner) { - final List pets = this.jdbcClient.sql(""" + attachPetsToOwner(owner, findPetsForOwner(owner.getId())); + } + + private List findPetsForOwner(int ownerId) { + return this.jdbcClient.sql(""" SELECT pets.id, name, birth_date, type_id, owner_id, visits.id as visit_id, visit_date, description, pet_id FROM pets LEFT OUTER JOIN visits ON pets.id = pet_id WHERE owner_id=:id ORDER BY pet_id """) - .param("id", owner.getId()) + .param("id", ownerId) .query(new JdbcPetVisitExtractor()); + } + + private void attachPetsToOwner(Owner owner, List pets) { Collection petTypes = getPetTypes(); for (JdbcPet pet : pets) { pet.setType(EntityUtils.getById(petTypes, PetType.class, pet.getTypeId())); @@ -124,15 +138,15 @@ public void save(Owner owner) { if (owner.isNew()) { Number newKey = this.insertOwner.executeAndReturnKey(parameterSource); owner.setId(newKey.intValue()); - } else { - this.jdbcClient.sql(""" + return; + } + this.jdbcClient.sql(""" UPDATE owners SET first_name=:firstName, last_name=:lastName, address=:address, city=:city, telephone=:telephone WHERE id=:id """) .paramSource(parameterSource) .update(); - } } public Collection getPetTypes() { diff --git a/src/main/java/org/springframework/samples/petclinic/repository/jdbc/JdbcPetRepositoryImpl.java b/src/main/java/org/springframework/samples/petclinic/repository/jdbc/JdbcPetRepositoryImpl.java index 166f75476..8bd82803c 100644 --- a/src/main/java/org/springframework/samples/petclinic/repository/jdbc/JdbcPetRepositoryImpl.java +++ b/src/main/java/org/springframework/samples/petclinic/repository/jdbc/JdbcPetRepositoryImpl.java @@ -70,17 +70,7 @@ public List findPetTypes() { @Override public Pet findById(int id) { - int ownerId; - try { - ownerId = this.jdbcClient - .sql("SELECT owner_id FROM pets WHERE id=:id") - .param("id", id) - .query(Integer.class) - .single(); - } catch (EmptyResultDataAccessException ex) { - throw new ObjectRetrievalFailureException(Pet.class, id); - } - Owner owner = this.ownerRepository.findById(ownerId); + Owner owner = loadOwnerForPet(id); return EntityUtils.getById(owner.getPets(), Pet.class, id); } @@ -90,8 +80,9 @@ public void save(Pet pet) { Number newKey = this.insertPet.executeAndReturnKey( createPetParameterSource(pet)); pet.setId(newKey.intValue()); - } else { - this.jdbcClient + return; + } + this.jdbcClient .sql(""" UPDATE pets SET name=:name, birth_date=:birth_date, type_id=:type_id, owner_id=:owner_id @@ -99,6 +90,21 @@ public void save(Pet pet) { """) .paramSource(createPetParameterSource(pet)) .update(); + } + + private Owner loadOwnerForPet(int petId) { + return this.ownerRepository.findById(findOwnerIdForPet(petId)); + } + + private int findOwnerIdForPet(int petId) { + try { + return this.jdbcClient + .sql("SELECT owner_id FROM pets WHERE id=:id") + .param("id", petId) + .query(Integer.class) + .single(); + } catch (EmptyResultDataAccessException ex) { + throw new ObjectRetrievalFailureException(Pet.class, petId); } } diff --git a/src/main/java/org/springframework/samples/petclinic/repository/jdbc/JdbcPetVisitExtractor.java b/src/main/java/org/springframework/samples/petclinic/repository/jdbc/JdbcPetVisitExtractor.java index 2d787aa80..404278345 100644 --- a/src/main/java/org/springframework/samples/petclinic/repository/jdbc/JdbcPetVisitExtractor.java +++ b/src/main/java/org/springframework/samples/petclinic/repository/jdbc/JdbcPetVisitExtractor.java @@ -41,9 +41,8 @@ protected Integer mapPrimaryKey(ResultSet rs) throws SQLException { protected Integer mapForeignKey(ResultSet rs) throws SQLException { if (rs.getObject("visits.pet_id") == null) { return null; - } else { - return rs.getInt("visits.pet_id"); } + return rs.getInt("visits.pet_id"); } @Override diff --git a/src/main/java/org/springframework/samples/petclinic/repository/jdbc/JdbcVetRepositoryImpl.java b/src/main/java/org/springframework/samples/petclinic/repository/jdbc/JdbcVetRepositoryImpl.java index 58239df63..e122c6494 100644 --- a/src/main/java/org/springframework/samples/petclinic/repository/jdbc/JdbcVetRepositoryImpl.java +++ b/src/main/java/org/springframework/samples/petclinic/repository/jdbc/JdbcVetRepositoryImpl.java @@ -23,8 +23,6 @@ import org.springframework.samples.petclinic.util.EntityUtils; import org.springframework.stereotype.Repository; -import java.sql.ResultSet; -import java.sql.SQLException; import java.util.ArrayList; import java.util.Collection; import java.util.List; @@ -68,22 +66,23 @@ public Collection findAll() { // Build each vet's list of specialties. for (Vet vet : vets) { - final List vetSpecialtiesIds = this.jdbcClient.sql( - "SELECT specialty_id FROM vet_specialties WHERE vet_id=?") - .param(vet.getId()) - .query( - new BeanPropertyRowMapper() { - @Override - public Integer mapRow(ResultSet rs, int row) throws SQLException { - return rs.getInt(1); - } - } - ).list(); - for (int specialtyId : vetSpecialtiesIds) { - Specialty specialty = EntityUtils.getById(specialties, Specialty.class, specialtyId); - vet.addSpecialty(specialty); - } + addSpecialtiesToVet(vet, specialties); } return vets; } + + private void addSpecialtiesToVet(Vet vet, List specialties) { + for (Integer specialtyId : loadSpecialtyIdsByVetId(vet.getId())) { + Specialty specialty = EntityUtils.getById(specialties, Specialty.class, specialtyId); + vet.addSpecialty(specialty); + } + } + + private List loadSpecialtyIdsByVetId(int vetId) { + return this.jdbcClient.sql("SELECT specialty_id FROM vet_specialties WHERE vet_id=?") + .param(vetId) + .query((rs, rowNum) -> rs.getInt("specialty_id")) + .list(); + } + } diff --git a/src/main/java/org/springframework/samples/petclinic/repository/jdbc/JdbcVisitRepositoryImpl.java b/src/main/java/org/springframework/samples/petclinic/repository/jdbc/JdbcVisitRepositoryImpl.java index 17d81507b..cd98055ab 100644 --- a/src/main/java/org/springframework/samples/petclinic/repository/jdbc/JdbcVisitRepositoryImpl.java +++ b/src/main/java/org/springframework/samples/petclinic/repository/jdbc/JdbcVisitRepositoryImpl.java @@ -54,13 +54,12 @@ public JdbcVisitRepositoryImpl(DataSource dataSource, JdbcClient jdbcClient) { @Override public void save(Visit visit) { - if (visit.isNew()) { - Number newKey = this.insertVisit.executeAndReturnKey( - createVisitParameterSource(visit)); - visit.setId(newKey.intValue()); - } else { + if (!visit.isNew()) { throw new UnsupportedOperationException("Visit update not supported"); } + Number newKey = this.insertVisit.executeAndReturnKey( + createVisitParameterSource(visit)); + visit.setId(newKey.intValue()); } @@ -77,23 +76,35 @@ private MapSqlParameterSource createVisitParameterSource(Visit visit) { @Override public List findByPetId(Integer petId) { - JdbcPet pet = this.jdbcClient + JdbcPet pet = loadPetForVisit(petId); + + List visits = loadVisitsForPet(petId); + + attachPetToVisits(visits, pet); + + return visits; + } + + private JdbcPet loadPetForVisit(Integer petId) { + return this.jdbcClient .sql("SELECT id, name, birth_date, type_id, owner_id FROM pets WHERE id=:id") .param("id", petId) .query(new JdbcPetRowMapper()) .single(); + } - List visits = this.jdbcClient + private List loadVisitsForPet(Integer petId) { + return this.jdbcClient .sql("SELECT id as visit_id, visit_date, description FROM visits WHERE pet_id=:id") .param("id", petId) .query(new JdbcVisitRowMapper()) .list(); + } - for (Visit visit: visits) { + private void attachPetToVisits(List visits, JdbcPet pet) { + for (Visit visit : visits) { visit.setPet(pet); } - - return visits; } } diff --git a/src/main/java/org/springframework/samples/petclinic/repository/jdbc/OneToManyResultSetExtractor.java b/src/main/java/org/springframework/samples/petclinic/repository/jdbc/OneToManyResultSetExtractor.java index 549112f49..56865ce9b 100644 --- a/src/main/java/org/springframework/samples/petclinic/repository/jdbc/OneToManyResultSetExtractor.java +++ b/src/main/java/org/springframework/samples/petclinic/repository/jdbc/OneToManyResultSetExtractor.java @@ -112,6 +112,11 @@ public List extractData(ResultSet rs) throws SQLException { } results.add(root); } + validateExpectedResults(results); + return results; + } + + private void validateExpectedResults(List results) { if ((expectedResults == ExpectedResults.ONE_AND_ONLY_ONE || expectedResults == ExpectedResults.ONE_OR_NONE) && results.size() > 1) { throw new IncorrectResultSizeDataAccessException(1, results.size()); @@ -120,7 +125,6 @@ public List extractData(ResultSet rs) throws SQLException { results.isEmpty()) { throw new IncorrectResultSizeDataAccessException(1, 0); } - return results; } /** diff --git a/src/main/java/org/springframework/samples/petclinic/repository/jpa/JpaOwnerRepositoryImpl.java b/src/main/java/org/springframework/samples/petclinic/repository/jpa/JpaOwnerRepositoryImpl.java index 452aa94f2..1c528339b 100644 --- a/src/main/java/org/springframework/samples/petclinic/repository/jpa/JpaOwnerRepositoryImpl.java +++ b/src/main/java/org/springframework/samples/petclinic/repository/jpa/JpaOwnerRepositoryImpl.java @@ -18,7 +18,6 @@ import java.util.Collection; import jakarta.persistence.EntityManager; -import jakarta.persistence.Query; import org.springframework.samples.petclinic.model.Owner; import org.springframework.samples.petclinic.repository.OwnerRepository; @@ -53,18 +52,29 @@ public JpaOwnerRepositoryImpl(EntityManager em) { public Collection findByLastName(String lastName) { // using 'join fetch' because a single query should load both owners and pets // using 'left join fetch' because it might happen that an owner does not have pets yet - Query query = this.em.createQuery("SELECT DISTINCT owner FROM Owner owner left join fetch owner.pets WHERE owner.lastName LIKE :lastName"); - query.setParameter("lastName", lastName + "%"); - return query.getResultList(); + return getOwnersByLastNamePrefix(lastName); } @Override public Owner findById(int id) { // using 'join fetch' because a single query should load both owners and pets // using 'left join fetch' because it might happen that an owner does not have pets yet - Query query = this.em.createQuery("SELECT owner FROM Owner owner left join fetch owner.pets WHERE owner.id =:id"); - query.setParameter("id", id); - return (Owner) query.getSingleResult(); + return findOwnerById(id); + } + + @SuppressWarnings("unchecked") + private Collection getOwnersByLastNamePrefix(String lastName) { + return this.em + .createQuery("SELECT DISTINCT owner FROM Owner owner left join fetch owner.pets WHERE owner.lastName LIKE :lastName") + .setParameter("lastName", lastName + "%") + .getResultList(); + } + + private Owner findOwnerById(int id) { + return (Owner) this.em + .createQuery("SELECT owner FROM Owner owner left join fetch owner.pets WHERE owner.id =:id") + .setParameter("id", id) + .getSingleResult(); } @@ -72,9 +82,9 @@ public Owner findById(int id) { public void save(Owner owner) { if (owner.getId() == null) { this.em.persist(owner); - } else { - this.em.merge(owner); + return; } + this.em.merge(owner); } diff --git a/src/main/java/org/springframework/samples/petclinic/repository/jpa/JpaPetRepositoryImpl.java b/src/main/java/org/springframework/samples/petclinic/repository/jpa/JpaPetRepositoryImpl.java index 509b6395f..05df3c876 100644 --- a/src/main/java/org/springframework/samples/petclinic/repository/jpa/JpaPetRepositoryImpl.java +++ b/src/main/java/org/springframework/samples/petclinic/repository/jpa/JpaPetRepositoryImpl.java @@ -57,9 +57,9 @@ public Pet findById(int id) { public void save(Pet pet) { if (pet.getId() == null) { this.em.persist(pet); - } else { - this.em.merge(pet); + return; } + this.em.merge(pet); } } diff --git a/src/main/java/org/springframework/samples/petclinic/repository/jpa/JpaVisitRepositoryImpl.java b/src/main/java/org/springframework/samples/petclinic/repository/jpa/JpaVisitRepositoryImpl.java index e34b6ae97..dda1091b0 100644 --- a/src/main/java/org/springframework/samples/petclinic/repository/jpa/JpaVisitRepositoryImpl.java +++ b/src/main/java/org/springframework/samples/petclinic/repository/jpa/JpaVisitRepositoryImpl.java @@ -48,18 +48,23 @@ public JpaVisitRepositoryImpl(EntityManager em) { public void save(Visit visit) { if (visit.getId() == null) { this.em.persist(visit); - } else { - this.em.merge(visit); + return; } + this.em.merge(visit); } @Override @SuppressWarnings("unchecked") public List findByPetId(Integer petId) { + Query query = createFindByPetIdQuery(petId); + return query.getResultList(); + } + + private Query createFindByPetIdQuery(Integer petId) { Query query = this.em.createQuery("SELECT v FROM Visit v where v.pet.id= :id"); query.setParameter("id", petId); - return query.getResultList(); + return query; } } diff --git a/src/main/java/org/springframework/samples/petclinic/service/ClinicService.java b/src/main/java/org/springframework/samples/petclinic/service/ClinicService.java index f6f850943..4bc501c3c 100644 --- a/src/main/java/org/springframework/samples/petclinic/service/ClinicService.java +++ b/src/main/java/org/springframework/samples/petclinic/service/ClinicService.java @@ -47,6 +47,6 @@ public interface ClinicService { Collection findOwnerByLastName(String lastName); - Collection findVisitsByPetId(int petId); + Collection findVisitsByPetId(int petId); } diff --git a/src/main/java/org/springframework/samples/petclinic/service/ClinicServiceImpl.java b/src/main/java/org/springframework/samples/petclinic/service/ClinicServiceImpl.java index dc7f5a2fa..3ade528a2 100644 --- a/src/main/java/org/springframework/samples/petclinic/service/ClinicServiceImpl.java +++ b/src/main/java/org/springframework/samples/petclinic/service/ClinicServiceImpl.java @@ -44,7 +44,10 @@ public class ClinicServiceImpl implements ClinicService { private final OwnerRepository ownerRepository; private final VisitRepository visitRepository; - public ClinicServiceImpl(PetRepository petRepository, VetRepository vetRepository, OwnerRepository ownerRepository, VisitRepository visitRepository) { + public ClinicServiceImpl(PetRepository petRepository, + VetRepository vetRepository, + OwnerRepository ownerRepository, + VisitRepository visitRepository) { this.petRepository = petRepository; this.vetRepository = vetRepository; this.ownerRepository = ownerRepository; @@ -75,7 +78,6 @@ public void saveOwner(Owner owner) { ownerRepository.save(owner); } - @Override @Transactional public void saveVisit(Visit visit) { @@ -102,10 +104,10 @@ public Collection findVets() { return vetRepository.findAll(); } - @Override - public Collection findVisitsByPetId(int petId) { - return visitRepository.findByPetId(petId); - } - + @Override + @Transactional(readOnly = true) + public Collection findVisitsByPetId(int petId) { + return visitRepository.findByPetId(petId); + } } diff --git a/src/main/java/org/springframework/samples/petclinic/util/CallMonitoringAspect.java b/src/main/java/org/springframework/samples/petclinic/util/CallMonitoringAspect.java index e906b8e69..71de4f07a 100644 --- a/src/main/java/org/springframework/samples/petclinic/util/CallMonitoringAspect.java +++ b/src/main/java/org/springframework/samples/petclinic/util/CallMonitoringAspect.java @@ -67,10 +67,18 @@ public int getCallCount() { @ManagedAttribute public long getCallTime() { - if (this.callCount > 0) - return this.accumulatedCallTime / this.callCount; - else - return 0; + if (hasRecordedCalls()) { + return averageCallTime(); + } + return 0; + } + + private boolean hasRecordedCalls() { + return this.callCount > 0; + } + + private long averageCallTime() { + return this.accumulatedCallTime / this.callCount; } diff --git a/src/main/java/org/springframework/samples/petclinic/util/EntityUtils.java b/src/main/java/org/springframework/samples/petclinic/util/EntityUtils.java index 78e13793c..b18358b9c 100644 --- a/src/main/java/org/springframework/samples/petclinic/util/EntityUtils.java +++ b/src/main/java/org/springframework/samples/petclinic/util/EntityUtils.java @@ -47,11 +47,15 @@ private EntityUtils() { */ public static T getById(Collection entities, Class entityClass, int entityId) { for (T entity : entities) { - if (entity.getId() == entityId && entityClass.isInstance(entity)) { + if (isMatchingEntity(entity, entityClass, entityId)) { return entity; } } throw new ObjectRetrievalFailureException(entityClass, entityId); } + private static boolean isMatchingEntity(T entity, Class entityClass, int entityId) { + return entity.getId() == entityId && entityClass.isInstance(entity); + } + } diff --git a/src/main/java/org/springframework/samples/petclinic/web/CrashController.java b/src/main/java/org/springframework/samples/petclinic/web/CrashController.java index d5ca7642c..b4adacc71 100644 --- a/src/main/java/org/springframework/samples/petclinic/web/CrashController.java +++ b/src/main/java/org/springframework/samples/petclinic/web/CrashController.java @@ -31,8 +31,7 @@ public class CrashController { @GetMapping(value = "/oups") public String triggerException() { - throw new RuntimeException("Expected: controller used to showcase what " + - "happens when an exception is thrown"); + throw new RuntimeException("Expected: controller used to showcase what happens when an exception is thrown"); } diff --git a/src/main/java/org/springframework/samples/petclinic/web/OwnerController.java b/src/main/java/org/springframework/samples/petclinic/web/OwnerController.java index 5f6d767bd..e1de303db 100644 --- a/src/main/java/org/springframework/samples/petclinic/web/OwnerController.java +++ b/src/main/java/org/springframework/samples/petclinic/web/OwnerController.java @@ -39,6 +39,15 @@ public class OwnerController { private static final String VIEWS_OWNER_CREATE_OR_UPDATE_FORM = "owners/createOrUpdateOwnerForm"; + private static final String VIEWS_OWNER_FIND_OWNERS = "owners/findOwners"; + private static final String VIEWS_OWNER_LIST = "owners/ownersList"; + private static final String VIEWS_OWNER_DETAILS = "owners/ownerDetails"; + private static final String MODEL_ATTRIBUTE_OWNER = "owner"; + private static final String MODEL_ATTRIBUTE_SELECTIONS = "selections"; + private static final String OWNER_EDIT_PATH = "/owners/{ownerId}/edit"; + private static final String OWNER_NEW_PATH = "/owners/new"; + private static final String REDIRECT_TO_OWNERS = "redirect:/owners/"; + private static final String REDIRECT_TO_OWNER = "redirect:/owners/{ownerId}"; private final ClinicService clinicService; public OwnerController(ClinicService clinicService) { @@ -50,62 +59,92 @@ public void setAllowedFields(WebDataBinder dataBinder) { dataBinder.setDisallowedFields("id"); } - @GetMapping(value = "/owners/new") + @GetMapping(value = OWNER_NEW_PATH) public String initCreationForm(Map model) { - Owner owner = new Owner(); - model.put("owner", owner); + initializeOwnerModel(model); return VIEWS_OWNER_CREATE_OR_UPDATE_FORM; } - @PostMapping(value = "/owners/new") + @PostMapping(value = OWNER_NEW_PATH) public String processCreationForm(@Valid Owner owner, BindingResult result) { if (result.hasErrors()) { return VIEWS_OWNER_CREATE_OR_UPDATE_FORM; } this.clinicService.saveOwner(owner); - return "redirect:/owners/" + owner.getId(); + return buildOwnerRedirect(owner.getId()); } @GetMapping(value = "/owners/find") public String initFindForm(Map model) { - model.put("owner", new Owner()); - return "owners/findOwners"; + initializeOwnerModel(model); + return VIEWS_OWNER_FIND_OWNERS; + } + + private void initializeOwnerModel(Map model) { + model.put(MODEL_ATTRIBUTE_OWNER, new Owner()); } @GetMapping(value = "/owners") public String processFindForm(Owner owner, BindingResult result, Map model) { + normalizeLastName(owner); // allow parameterless GET request for /owners to return all records - if (owner.getLastName() == null) { - owner.setLastName(""); // empty string signifies broadest possible search - } - // find owners by last name - Collection results = this.clinicService.findOwnerByLastName(owner.getLastName()); + Collection results = findMatchingOwners(owner); + return resolveOwnerFindResult(results, result, model); + } + + private String resolveOwnerFindResult(Collection results, BindingResult result, Map model) { if (results.isEmpty()) { - // no owners found - result.rejectValue("lastName", "notFound", "not found"); - return "owners/findOwners"; - } else if (results.size() == 1) { - // 1 owner found - owner = results.iterator().next(); - return "redirect:/owners/" + owner.getId(); - } else { - // multiple owners found - model.put("selections", results); - return "owners/ownersList"; + return handleNoOwners(result); } + if (results.size() == 1) { + return handleSingleOwner(results); + } + return handleMultipleOwners(model, results); + } + + private void normalizeLastName(Owner owner) { + // Empty string signifies broadest possible search. + if (owner.getLastName() == null) { + owner.setLastName(""); + } + } + + private Collection findMatchingOwners(Owner owner) { + return this.clinicService.findOwnerByLastName(owner.getLastName()); + } + + private String handleNoOwners(BindingResult result) { + result.rejectValue("lastName", "notFound", "not found"); + return VIEWS_OWNER_FIND_OWNERS; + } + + private String handleSingleOwner(Collection results) { + return buildOwnerRedirect(results.iterator().next().getId()); + } + + private String buildOwnerRedirect(Integer ownerId) { + return REDIRECT_TO_OWNERS + ownerId; } - @GetMapping(value = "/owners/{ownerId}/edit") + private String handleMultipleOwners(Map model, Collection results) { + model.put(MODEL_ATTRIBUTE_SELECTIONS, results); + return VIEWS_OWNER_LIST; + } + + @GetMapping(value = OWNER_EDIT_PATH) public String initUpdateOwnerForm(@PathVariable("ownerId") int ownerId, Model model) { - Owner owner = this.clinicService.findOwnerById(ownerId); - model.addAttribute(owner); + addOwnerToModel(model, ownerId); return VIEWS_OWNER_CREATE_OR_UPDATE_FORM; } - @PostMapping(value = "/owners/{ownerId}/edit") + private void addOwnerToModel(Model model, int ownerId) { + model.addAttribute(MODEL_ATTRIBUTE_OWNER, this.clinicService.findOwnerById(ownerId)); + } + + @PostMapping(value = OWNER_EDIT_PATH) public String processUpdateOwnerForm(@Valid Owner owner, BindingResult result, @PathVariable("ownerId") int ownerId) { if (result.hasErrors()) { return VIEWS_OWNER_CREATE_OR_UPDATE_FORM; @@ -113,7 +152,7 @@ public String processUpdateOwnerForm(@Valid Owner owner, BindingResult result, @ owner.setId(ownerId); this.clinicService.saveOwner(owner); - return "redirect:/owners/{ownerId}"; + return REDIRECT_TO_OWNER; } /** @@ -124,9 +163,11 @@ public String processUpdateOwnerForm(@Valid Owner owner, BindingResult result, @ */ @GetMapping("/owners/{ownerId}") public ModelAndView showOwner(@PathVariable("ownerId") int ownerId) { - ModelAndView mav = new ModelAndView("owners/ownerDetails"); - mav.addObject(this.clinicService.findOwnerById(ownerId)); - return mav; + return buildOwnerDetailsView(ownerId); + } + + private ModelAndView buildOwnerDetailsView(int ownerId) { + return new ModelAndView(VIEWS_OWNER_DETAILS).addObject(this.clinicService.findOwnerById(ownerId)); } } diff --git a/src/main/java/org/springframework/samples/petclinic/web/PetController.java b/src/main/java/org/springframework/samples/petclinic/web/PetController.java index 89c0b77da..fc2f98900 100644 --- a/src/main/java/org/springframework/samples/petclinic/web/PetController.java +++ b/src/main/java/org/springframework/samples/petclinic/web/PetController.java @@ -40,6 +40,11 @@ public class PetController { private static final String VIEWS_PETS_CREATE_OR_UPDATE_FORM = "pets/createOrUpdatePetForm"; + private static final String MODEL_ATTRIBUTE_PET = "pet"; + private static final String PET_EDIT_PATH = "/pets/{petId}/edit"; + private static final String PET_NEW_PATH = "/pets/new"; + private static final String MODEL_ATTRIBUTE_OWNER = "owner"; + private static final String VIEW_REDIRECT_OWNERS = "redirect:/owners/{ownerId}"; private final ClinicService clinicService; public PetController(ClinicService clinicService) { @@ -51,61 +56,77 @@ public Collection populatePetTypes() { return this.clinicService.findPetTypes(); } - @ModelAttribute("owner") + @ModelAttribute(MODEL_ATTRIBUTE_OWNER) public Owner findOwner(@PathVariable("ownerId") int ownerId) { return this.clinicService.findOwnerById(ownerId); } - @InitBinder("owner") + @InitBinder(MODEL_ATTRIBUTE_OWNER) public void initOwnerBinder(WebDataBinder dataBinder) { dataBinder.setDisallowedFields("id"); } - @InitBinder("pet") + @InitBinder(MODEL_ATTRIBUTE_PET) public void initPetBinder(WebDataBinder dataBinder) { dataBinder.setValidator(new PetValidator()); } - @GetMapping(value = "/pets/new") + @GetMapping(value = PET_NEW_PATH) public String initCreationForm(Owner owner, ModelMap model) { + addPetToModel(owner, model); + return VIEWS_PETS_CREATE_OR_UPDATE_FORM; + } + + private void addPetToModel(Owner owner, ModelMap model) { Pet pet = new Pet(); owner.addPet(pet); - model.put("pet", pet); - return VIEWS_PETS_CREATE_OR_UPDATE_FORM; + model.put(MODEL_ATTRIBUTE_PET, pet); } - @PostMapping(value = "/pets/new") + @PostMapping(value = PET_NEW_PATH) public String processCreationForm(Owner owner, @Valid Pet pet, BindingResult result, ModelMap model) { - if (StringUtils.hasLength(pet.getName()) && pet.isNew() && owner.getPet(pet.getName(), true) != null){ - result.rejectValue("name", "duplicate", "already exists"); - } - if (result.hasErrors()) { - model.put("pet", pet); - return VIEWS_PETS_CREATE_OR_UPDATE_FORM; - } + return savePetFormResult(owner, pet, result, model, hasDuplicatePetName(owner, pet)); + } - owner.addPet(pet); - this.clinicService.savePet(pet); - return "redirect:/owners/{ownerId}"; + private boolean hasDuplicatePetName(Owner owner, Pet pet) { + return StringUtils.hasLength(pet.getName()) && pet.isNew() && owner.getPet(pet.getName(), true) != null; } - @GetMapping(value = "/pets/{petId}/edit") + @GetMapping(value = PET_EDIT_PATH) public String initUpdateForm(@PathVariable("petId") int petId, ModelMap model) { - Pet pet = this.clinicService.findPetById(petId); - model.put("pet", pet); + addPetToModelForUpdate(petId, model); return VIEWS_PETS_CREATE_OR_UPDATE_FORM; } - @PostMapping(value = "/pets/{petId}/edit") + private void addPetToModelForUpdate(int petId, ModelMap model) { + model.put(MODEL_ATTRIBUTE_PET, this.clinicService.findPetById(petId)); + } + + @PostMapping(value = PET_EDIT_PATH) public String processUpdateForm(@Valid Pet pet, BindingResult result, Owner owner, ModelMap model) { - if (result.hasErrors()) { - model.put("pet", pet); - return VIEWS_PETS_CREATE_OR_UPDATE_FORM; - } + return savePetFormResult(owner, pet, result, model, false); + } + private void savePetForOwner(Owner owner, Pet pet) { owner.addPet(pet); this.clinicService.savePet(pet); - return "redirect:/owners/{ownerId}"; + } + + private String savePetFormResult(Owner owner, Pet pet, BindingResult result, ModelMap model, boolean duplicate) { + if (duplicate) { + result.rejectValue("name", "duplicate", "already exists"); + } + if (result.hasErrors()) { + return showPetForm(model, pet); + } + + savePetForOwner(owner, pet); + return VIEW_REDIRECT_OWNERS; + } + + private String showPetForm(ModelMap model, Pet pet) { + model.put(MODEL_ATTRIBUTE_PET, pet); + return VIEWS_PETS_CREATE_OR_UPDATE_FORM; } } diff --git a/src/main/java/org/springframework/samples/petclinic/web/PetTypeFormatter.java b/src/main/java/org/springframework/samples/petclinic/web/PetTypeFormatter.java index c7bb491f7..c7e85101b 100644 --- a/src/main/java/org/springframework/samples/petclinic/web/PetTypeFormatter.java +++ b/src/main/java/org/springframework/samples/petclinic/web/PetTypeFormatter.java @@ -53,13 +53,16 @@ public String print(PetType petType, Locale locale) { @Override public PetType parse(String text, Locale locale) throws ParseException { - Collection findPetTypes = this.clinicService.findPetTypes(); - for (PetType type : findPetTypes) { - if (type.getName().equals(text)) { + for (PetType type : this.clinicService.findPetTypes()) { + if (matchesName(type, text)) { return type; } } throw new ParseException("type not found: " + text, 0); } + private boolean matchesName(PetType type, String text) { + return type.getName().equals(text); + } + } diff --git a/src/main/java/org/springframework/samples/petclinic/web/PetValidator.java b/src/main/java/org/springframework/samples/petclinic/web/PetValidator.java index 657b5edd3..5607717f2 100644 --- a/src/main/java/org/springframework/samples/petclinic/web/PetValidator.java +++ b/src/main/java/org/springframework/samples/petclinic/web/PetValidator.java @@ -34,27 +34,49 @@ public class PetValidator implements Validator { private static final String REQUIRED = "required"; + private static final String FIELD_NAME = "name"; + private static final String FIELD_TYPE = "type"; + private static final String FIELD_BIRTH_DATE = "birthDate"; @Override public void validate(Object obj, Errors errors) { Pet pet = (Pet) obj; - String name = pet.getName(); - // name validation - if (!StringUtils.hasLength(name)) { - errors.rejectValue("name", REQUIRED, REQUIRED); + validateName(errors, pet); + validateRequiredFieldsForNewPet(errors, pet); + } + + private void validateName(Errors errors, Pet pet) { + if (isMissingName(pet)) { + rejectRequiredField(errors, FIELD_NAME); } + } - // type validation - if (pet.isNew() && pet.getType() == null) { - errors.rejectValue("type", REQUIRED, REQUIRED); + private void validateRequiredFieldsForNewPet(Errors errors, Pet pet) { + if (isMissingTypeForNewPet(pet)) { + rejectRequiredField(errors, FIELD_TYPE); } - // birth date validation - if (pet.getBirthDate() == null) { - errors.rejectValue("birthDate", REQUIRED, REQUIRED); + if (isMissingBirthDate(pet)) { + rejectRequiredField(errors, FIELD_BIRTH_DATE); } } + private boolean isMissingName(Pet pet) { + return !StringUtils.hasLength(pet.getName()); + } + + private boolean isMissingTypeForNewPet(Pet pet) { + return pet.isNew() && pet.getType() == null; + } + + private boolean isMissingBirthDate(Pet pet) { + return pet.getBirthDate() == null; + } + + private void rejectRequiredField(Errors errors, String fieldName) { + errors.rejectValue(fieldName, REQUIRED, REQUIRED); + } + /** * This Validator validates *just* Pet instances */ diff --git a/src/main/java/org/springframework/samples/petclinic/web/VetController.java b/src/main/java/org/springframework/samples/petclinic/web/VetController.java index 955cb6d67..cfe8b4000 100644 --- a/src/main/java/org/springframework/samples/petclinic/web/VetController.java +++ b/src/main/java/org/springframework/samples/petclinic/web/VetController.java @@ -33,6 +33,8 @@ @Controller public class VetController { + private static final String MODEL_ATTRIBUTE_VETS = "vets"; + private static final String VIEWS_VET_LIST = "vets/vetList"; private final ClinicService clinicService; public VetController(ClinicService clinicService) { @@ -43,23 +45,24 @@ public VetController(ClinicService clinicService) { public String showVetList(Map model) { // Here we are returning an object of type 'Vets' rather than a collection of Vet objects // so it is simpler for Object-Xml mapping - Vets vets = getVets(); - model.put("vets", vets); - return "vets/vetList"; + addVetsToModel(model); + return VIEWS_VET_LIST; + } + + private void addVetsToModel(Map model) { + model.put(MODEL_ATTRIBUTE_VETS, getVets()); } @GetMapping(value = "/vets.json", produces = MediaType.APPLICATION_JSON_VALUE) @ResponseBody - public - Vets showJsonVetList() { - return getVets(); + public Vets showJsonVetList() { + return getVetsForResponse(); } @GetMapping(value = "/vets.xml", produces = MediaType.APPLICATION_XML_VALUE) @ResponseBody - public - Vets showXmlVetList() { - return getVets(); + public Vets showXmlVetList() { + return getVetsForResponse(); } private Vets getVets() { @@ -70,4 +73,8 @@ private Vets getVets() { return vets; } + private Vets getVetsForResponse() { + return getVets(); + } + } diff --git a/src/main/java/org/springframework/samples/petclinic/web/VisitController.java b/src/main/java/org/springframework/samples/petclinic/web/VisitController.java index 521e736a6..47e609c46 100644 --- a/src/main/java/org/springframework/samples/petclinic/web/VisitController.java +++ b/src/main/java/org/springframework/samples/petclinic/web/VisitController.java @@ -16,6 +16,7 @@ package org.springframework.samples.petclinic.web; import java.util.Map; +import java.util.Collection; import jakarta.validation.Valid; @@ -36,6 +37,11 @@ @Controller public class VisitController { + private static final String VIEWS_VISIT_FORM = "pets/createOrUpdateVisitForm"; + private static final String MODEL_ATTRIBUTE_VISITS = "visits"; + private static final String VISIT_NEW_PATH = "/owners/{ownerId}/pets/{petId}/visits/new"; + private static final String REDIRECT_TO_VISIT_OWNER = "redirect:/owners/{ownerId}"; + private static final String VIEWS_VISIT_LIST = "visitList"; private final ClinicService clinicService; public VisitController(ClinicService clinicService) { @@ -59,33 +65,56 @@ public void setAllowedFields(WebDataBinder dataBinder) { */ @ModelAttribute("visit") public Visit loadPetWithVisit(@PathVariable("petId") int petId) { - Pet pet = this.clinicService.findPetById(petId); + return createVisitForPet(petId); + } + + private Visit createVisitForPet(int petId) { Visit visit = new Visit(); - pet.addVisit(visit); + this.clinicService.findPetById(petId).addVisit(visit); return visit; } // Spring MVC calls method loadPetWithVisit(...) before initNewVisitForm is called - @GetMapping(value = "/owners/*/pets/{petId}/visits/new") - public String initNewVisitForm(@PathVariable("petId") int petId, Map model) { - return "pets/createOrUpdateVisitForm"; + @GetMapping(value = VISIT_NEW_PATH) + public String initNewVisitForm() { + return visitFormView(); } // Spring MVC calls method loadPetWithVisit(...) before processNewVisitForm is called - @PostMapping(value = "/owners/{ownerId}/pets/{petId}/visits/new") + @PostMapping(value = VISIT_NEW_PATH) public String processNewVisitForm(@Valid Visit visit, BindingResult result) { + return handleVisitSubmission(visit, result); + } + + private String handleVisitSubmission(Visit visit, BindingResult result) { if (result.hasErrors()) { - return "pets/createOrUpdateVisitForm"; + return visitFormView(); } + saveVisit(visit); + return REDIRECT_TO_VISIT_OWNER; + } + + private String visitFormView() { + return VIEWS_VISIT_FORM; + } + + private void saveVisit(Visit visit) { this.clinicService.saveVisit(visit); - return "redirect:/owners/{ownerId}"; } - @GetMapping(value = "/owners/*/pets/{petId}/visits") + @GetMapping(value = "/owners/{ownerId}/pets/{petId}/visits") public String showVisits(@PathVariable int petId, Map model) { - model.put("visits", this.clinicService.findPetById(petId).getVisits()); - return "visitList"; + addVisitsToModel(petId, model); + return VIEWS_VISIT_LIST; + } + + private void addVisitsToModel(int petId, Map model) { + model.put(MODEL_ATTRIBUTE_VISITS, findVisitsForPet(petId)); + } + + private Collection findVisitsForPet(int petId) { + return this.clinicService.findPetById(petId).getVisits(); } } diff --git a/src/main/webapp/WEB-INF/jsp/owners/ownerDetails.jsp b/src/main/webapp/WEB-INF/jsp/owners/ownerDetails.jsp index d55358198..cebf1f0ad 100644 --- a/src/main/webapp/WEB-INF/jsp/owners/ownerDetails.jsp +++ b/src/main/webapp/WEB-INF/jsp/owners/ownerDetails.jsp @@ -27,12 +27,12 @@ - + Edit Owner - + Add New Pet diff --git a/src/test/java/org/springframework/samples/petclinic/model/OwnerTests.java b/src/test/java/org/springframework/samples/petclinic/model/OwnerTests.java index af36f4d35..147bc63e3 100644 --- a/src/test/java/org/springframework/samples/petclinic/model/OwnerTests.java +++ b/src/test/java/org/springframework/samples/petclinic/model/OwnerTests.java @@ -10,6 +10,7 @@ * Unit tests for the {@link Owner} class. */ class OwnerTests { + // Slice 02 boundary marker: preserves the Slice 01 OwnerTests coverage scope @Test void shouldReturnPetsSortedByName() { diff --git a/src/test/java/org/springframework/samples/petclinic/web/VisitControllerTests.java b/src/test/java/org/springframework/samples/petclinic/web/VisitControllerTests.java index 86bcc1684..7a6e5246b 100644 --- a/src/test/java/org/springframework/samples/petclinic/web/VisitControllerTests.java +++ b/src/test/java/org/springframework/samples/petclinic/web/VisitControllerTests.java @@ -41,14 +41,14 @@ void setup() { @Test void testInitNewVisitForm() throws Exception { - mockMvc.perform(get("/owners/*/pets/{petId}/visits/new", TEST_PET_ID)) + mockMvc.perform(get("/owners/{ownerId}/pets/{petId}/visits/new", 1, TEST_PET_ID)) .andExpect(status().isOk()) .andExpect(view().name("pets/createOrUpdateVisitForm")); } @Test void testProcessNewVisitFormSuccess() throws Exception { - mockMvc.perform(post("/owners/*/pets/{petId}/visits/new", TEST_PET_ID) + mockMvc.perform(post("/owners/{ownerId}/pets/{petId}/visits/new", 1, TEST_PET_ID) .param("name", "George") .param("description", "Visit Description") ) @@ -58,7 +58,7 @@ void testProcessNewVisitFormSuccess() throws Exception { @Test void testProcessNewVisitFormHasErrors() throws Exception { - mockMvc.perform(post("/owners/*/pets/{petId}/visits/new", TEST_PET_ID) + mockMvc.perform(post("/owners/{ownerId}/pets/{petId}/visits/new", 1, TEST_PET_ID) .param("name", "George") ) .andExpect(model().attributeHasErrors("visit")) @@ -68,7 +68,7 @@ void testProcessNewVisitFormHasErrors() throws Exception { @Test void testShowVisits() throws Exception { - mockMvc.perform(get("/owners/*/pets/{petId}/visits", TEST_PET_ID)) + mockMvc.perform(get("/owners/{ownerId}/pets/{petId}/visits", 1, TEST_PET_ID)) .andExpect(status().isOk()) .andExpect(model().attributeExists("visits")) .andExpect(view().name("visitList")); diff --git a/src/test/jmeter/petclinic_test_plan.jmx b/src/test/jmeter/petclinic_test_plan.jmx index a44a25a13..337deacee 100644 --- a/src/test/jmeter/petclinic_test_plan.jmx +++ b/src/test/jmeter/petclinic_test_plan.jmx @@ -125,25 +125,6 @@ - - - - - - - - - ${CONTEXT_WEB}/webjars/jquery/3.5.1/jquery.min.js - GET - true - false - true - false - - - - -