Conversation
WalkthroughThe pull request establishes a Spring Boot 3.5.7 application from scratch, replacing a basic Java project. Introduces MySQL database persistence, Spring Security with dev/prod profiles, web controllers with Thymeleaf templates, JPA entities (Patient/Admission), repositories, DTOs, and corresponding integration tests. Includes Docker Compose and SQL schema migration. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Browser/Client
participant Controller as Spring Controller
participant Security as SecurityConfig
participant UserSvc as UserDetailsService
participant Repository as Repository Layer
participant Database as MySQL Database
rect rgb(200, 220, 240)
note over Client,Database: Authentication Flow (Dev Profile)
Client->>Controller: GET /login
Controller->>Client: Return login.html template
Client->>Controller: POST /login (username: admin, password: password)
Controller->>Security: Validate credentials via SecurityFilterChain
Security->>UserSvc: Load user details (userDetailsService bean)
UserSvc->>Security: Return in-memory admin user + role ADMIN
Security->>Security: BCrypt password encoder validates password
Security->>Client: Authenticated session created (redirect to /)
end
rect rgb(220, 240, 200)
note over Client,Database: Authorized Request Flow
Client->>Controller: GET /admissions (with admin session)
Controller->>Security: `@PreAuthorize` checks role ADMIN
Security->>Client: Authorization granted
Controller->>Repository: admissionRepository.findAll()
Repository->>Database: SELECT * FROM admission
Database->>Repository: Return Admission rows (with patient references)
Repository->>Controller: Return List<Admission>
Controller->>Controller: Map Admission to AdmissionDTO
Controller->>Client: Render admissions.html with DTOs
end
rect rgb(240, 230, 200)
note over Client,Database: Unauthorized Request Flow
Client->>Controller: GET /api/patients/new (without ADMIN role)
Controller->>Security: `@PreAuthorize`("hasRole('ADMIN')") check fails
Security->>Client: HTTP 403 Forbidden (access denied)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (32)
.gitignore (1)
1-3: Reconsider ignoringdocker-compose.ymlIgnoring
docker-compose.ymlmeans each developer may run with a different, unversioned Docker setup; consider committing a default ordocker-compose.example.ymland keeping only the local override ignored instead.src/main/resources/static/style.css (1)
6-9: Global 300px width on all<div>s is very restrictive
div { width: 300px; margin: 0 auto; }constrains every container in the app to a narrow column and will look cramped on wider screens; consider moving this rule to a specific class (e.g..form-container) and applying it only where needed.src/main/resources/templates/error.html (1)
1-16: Error template message is 403-specific but file name is genericSince
error.htmlis usually rendered for many error statuses, always saying “You are not authorized to access this resource” may be confusing on 404/500 etc.; consider either making the message generic or adding status-specific views like403.htmlfor authorization failures.src/main/resources/db/migration/V1__.sql (1)
1-24: Tighten nullability onadmission.pat_idif every admission must have a patientCurrently
pat_idis nullable even though there’s an FK topatient(pat_id); if the domain doesn’t allow “orphan” admissions, consider declaringpat_id BIGINT NOT NULLto enforce that invariant at the database level.src/main/java/org/example/config/JavaMigrationsFlywayCustomizer.java (1)
1-15: Remove or implement the fully commented-out Flyway customizerThe whole
JavaMigrationsFlywayCustomizerclass is wrapped in a block comment, so it never compiles or runs but still clutters the config package; either delete this file for now or uncomment and implement it once you actually need custom Flyway configuration.pom.xml (1)
12-31: Verifyjava.versionand clean up placeholder POM metadataSetting
<java.version>25</java.version>assumes your build and runtime JDK are 25; if the exercise environment (or CI) is still on 21, you’ll want to align this property to avoid compiler/bytecode-level issues. Also consider either filling in or removing the empty<url>,<licenses>,<developers>, and<scm>sections so the POM reflects the project more accurately.src/main/resources/templates/index.html (1)
2-2: Remove unused namespace declaration.The
secnamespace is declared but never used in this template.-<html lang="en" xmlns:th="http://www.thymeleaf.org" xmlns:sec="http://www.w3.org/1999/xhtml"> +<html lang="en" xmlns:th="http://www.thymeleaf.org">src/main/resources/templates/login.html (1)
5-5: Use consistent path format for stylesheet reference.The CSS reference lacks a leading slash, which could cause path resolution issues. Use a Thymeleaf URL expression or add a leading slash for consistency with other templates.
- <link rel="stylesheet" href="login.css"> + <link rel="stylesheet" th:href="@{/login.css}">src/main/java/org/example/dto/NewAdmissionDTO.java (1)
4-5: Consider adding validation annotations.As a DTO that accepts user input for creating admissions, this record should include validation annotations (e.g.,
@NotBlank,@Patternfor SSN format) to ensure data integrity and provide meaningful error messages.package org.example.dto; +import jakarta.validation.constraints.NotBlank; +import jakarta.validation.constraints.Pattern; -public record NewAdmissionDTO(String patSsn, String diagnosis, String department) { +public record NewAdmissionDTO( + @NotBlank(message = "Patient SSN is required") + @Pattern(regexp = "\\d{8}-\\d{4}", message = "Invalid SSN format") + String patSsn, + @NotBlank(message = "Diagnosis is required") + String diagnosis, + @NotBlank(message = "Department is required") + String department +) {src/main/resources/templates/patients/patients.html (1)
12-16: Use semantic HTML structure for patient names.Using
<p>tags with inline display to create horizontal layout is semantically incorrect. Consider using<span>elements or proper CSS layout techniques.<ul th:each="patient : ${patients}"> <li> - <p th:text="${patient.firstName()}" style="display: inline"></p> - <p style="display: inline"></p> - <p th:text="${patient.lastName()}" style="display: inline"></p> + <span th:text="${patient.firstName()}"></span> + <span th:text="' ' + ${patient.lastName()}"></span> </li> </ul>src/main/java/org/example/dto/PatientDTO.java (1)
5-6: Use explicit field name instead of abbreviation.The field name
dobis abbreviated. Consider usingdateOfBirthfor better code readability and maintainability.-public record PatientDTO(String firstName, String lastName, String address, LocalDate dob) { +public record PatientDTO(String firstName, String lastName, String address, LocalDate dateOfBirth) {src/main/resources/templates/patients/newPatient.html (1)
9-17: Add CSRF token (if enabled) and label the submit actionIf Spring Security CSRF protection is enabled, this POST will need a CSRF token. Also, giving the submit control an explicit label improves clarity.
Suggested change:
- <form method="post" th:action="@{/api/patients/new}" th:object="${patient}"> + <form method="post" th:action="@{/api/patients/new}" th:object="${patient}"> @@ - <label for="ssn">Social Security Number:</label><input type="text" id="ssn" th:field="*{ssn}"> - <input type="submit"> + <label for="ssn">Social Security Number:</label><input type="text" id="ssn" th:field="*{ssn}"> + <!-- Only needed if CSRF is enabled in Spring Security --> + <input type="hidden" th:name="${_csrf.parameterName}" th:value="${_csrf.token}"/> + <input type="submit" value="Create patient">Please verify how CSRF is configured; if it is disabled for these endpoints, the hidden field can be omitted.
src/main/java/org/example/Exercise2025Application.java (1)
6-11: Spring Boot application entry point looks good (scanBasePackages optional)The main class is a standard Spring Boot bootstrap and should work as expected. Since this class is already in
org.example, the explicitscanBasePackages = "org.example"is redundant but not harmful—feel free to drop it if you prefer the default component scan.src/main/java/org/example/controller/WebController.java (1)
7-23: MVC mappings are fine; consider removing unusedModelparameters and align/logoutwith security configThe three GET handlers cleanly route to
index,login, andlogoutviews. TheModelparameter isn’t used in any of them, so it can be removed to reduce noise. Also, ensure your Spring Security configuration’s logout handling (usually POST/logoutby default) is consistent with serving a GET/logoutview.Example cleanup:
- @GetMapping("/") - public String index(Model model) { + @GetMapping("/") + public String index() { return "index"; } @@ - @GetMapping("/login") - public String login(Model model) { + @GetMapping("/login") + public String login() { return "login"; } @@ - @GetMapping("/logout") - public String logout(Model model) { + @GetMapping("/logout") + public String logout() { return "logout"; }Please double‑check that your security configuration and templates work together as intended for the logout flow.
src/main/java/org/example/repository/AdmissionRepository.java (2)
11-14: Redundant query method.
getAllAdmissions()duplicates the inheritedfindAll()method fromListCrudRepository. Consider removing this custom query and using the inherited method directly.
16-19: Consider using derived query methods or inheritedfindById.
getAdmissionByIdreplicates functionality already provided byListCrudRepository.findById(Long id). If custom return type (non-Optional) is intentional, consider using Spring Data's derived query:Admission findByAdmission_id(Long id)— though note this returnsnullwhen not found rather thanOptional.src/main/java/org/example/repository/PatientRepository.java (3)
7-12: Simplify type references and consider using inherited method.
findPatientByIdreplicatesfindById()fromListCrudRepository. Also,java.lang.Longcan be simplified toLong.-public interface PatientRepository extends ListCrudRepository<Patient, java.lang.Long> { +public interface PatientRepository extends ListCrudRepository<Patient, Long> { - @Query(""" - select pat from Patient pat where pat.pat_id = :id - """) - public Patient findPatientById(java.lang.Long id);If you need a non-Optional return, use derived query naming:
Patient findByPatId(Long id).
14-17: Fragile name matching with CONCAT.The CONCAT-based comparison is sensitive to exact spacing and case. Consider matching first and last name separately for robustness:
@Query(""" - select pat from Patient pat where concat(pat.firstName, ' ', pat.lastName) = concat(:first, ' ', :last) + select pat from Patient pat where pat.firstName = :first and pat.lastName = :last """) public Patient findByFirstAndLastName(String first, String last);Or use Spring Data's derived query:
Patient findByFirstNameAndLastName(String firstName, String lastName).
19-22: Consider using derived query method.Spring Data JPA can auto-generate this query from the method name. The explicit
@Queryis unnecessary.- @Query(""" - select pat from Patient pat where pat.ssn = :ssn - """) - public Patient findBySsn(String ssn); + Patient findBySsn(String ssn);src/test/java/org/example/ControllerTest.java (1)
40-52: Consider adding response body assertions.The test verifies status but doesn't assert the response content. For the
/admissionsendpoint serving a Thymeleaf view, consider verifying the model attribute or view name.mockMvc.perform(get("/admissions")) - .andExpect(status().isOk()); + .andExpect(status().isOk()) + .andExpect(view().name("admissions/admissions")) + .andExpect(model().attributeExists("admissions"));You'll need to import:
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.view; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.model;src/main/java/org/example/controller/AdmissionsController.java (2)
21-24: Misleading method name.
displayPatientMenuhandles the admissions menu, not patients. Rename for clarity.@GetMapping("/admissions-menu") - public String displayPatientMenu(Model model) { + public String displayAdmissionsMenu(Model model) { return "admissions/admissionMenu"; }
16-17: Prefer constructor injection over field injection.Field injection with
@Autowiredmakes testing harder and hides dependencies. Constructor injection is the recommended approach in Spring.@Controller @PreAuthorize("hasRole('ADMIN')") public class AdmissionsController { - @Autowired - AdmissionRepository admissionRepository; + private final AdmissionRepository admissionRepository; + + public AdmissionsController(AdmissionRepository admissionRepository) { + this.admissionRepository = admissionRepository; + }This pattern is already used in
HospitalController(see lines 17-21 in that file).src/main/java/org/example/entity/Admission.java (4)
11-11: Consider Java naming conventions for the ID field.
admission_iduses snake_case which is unconventional in Java. ConsideradmissionIdwith an explicit@Column(name = "admission_id")annotation to maintain database column mapping.@Id @GeneratedValue(strategy = GenerationType.IDENTITY) - private java.lang.Long admission_id; + @Column(name = "admission_id") + private Long admissionId;This would require updating the getter and JPQL queries accordingly.
19-21: Make the patient field private.The
patientfield lacks an access modifier, making it package-private. For proper encapsulation, it should beprivate.@ManyToOne @JoinColumn(name = "pat_id") - Patient patient; + private Patient patient;
25-30:LocalDateTime.now()in constructor reduces testability.Calling
LocalDateTime.now()directly makes unit testing time-dependent behavior difficult. Consider accepting aClockor the date as a constructor parameter for testability.For improved testability:
public Admission(Patient patient, String diagnosis, String department, LocalDateTime dateIn) { this.patient = patient; this.diagnosis = diagnosis; this.department = department; this.dateIn = dateIn; }Then callers can pass
LocalDateTime.now()in production and fixed values in tests.
59-61: Unconventional setter signature.
setDateOut()with no parameter is unexpected for a setter. Consider either renaming tomarkDischarged()orwriteOut()to reflect the action, or accepting aLocalDateTimeparameter for standard setter behavior.- public void setDateOut() { - this.dateOut = LocalDateTime.now(); + public void markDischarged() { + this.dateOut = LocalDateTime.now(); } + + public void setDateOut(LocalDateTime dateOut) { + this.dateOut = dateOut; + }src/main/java/org/example/config/SecurityConfig.java (2)
51-63: Avoid putting the encoded password into theAuthenticationcredentialsThe custom
AuthenticationManagercurrently sets the principal tousernameand the credentials to the stored (encoded) password. Best practice is to use theUserDetailsas principal and set credentials tonullafter verification so secrets are not kept around in theSecurityContext.- UserDetails user = userDetailsService.loadUserByUsername(username); - - if (passwordEncoder.matches(rawPassword, user.getPassword())) { - return new UsernamePasswordAuthenticationToken(username, user.getPassword(), user.getAuthorities()); - } + UserDetails user = userDetailsService.loadUserByUsername(username); + + if (passwordEncoder.matches(rawPassword, user.getPassword())) { + // Principal is the full UserDetails; credentials cleared after successful auth + return new UsernamePasswordAuthenticationToken(user, null, user.getAuthorities()); + } throw new BadCredentialsException("Bad credentials");You can also drop the unused
AuthenticationManager authenticationManagerparameter from thewebbean method to reduce noise.
41-48: Hard‑coded in‑memory admin user should be clearly dev‑onlyDefining an
admin/passworduser inInMemoryUserDetailsManageris fine for an exercise or dev profile, but it’s unsafe in any shared or production‑like environment. At minimum, restrict this bean to a dev profile or externalize the username/password into configuration.Example:
-@Configuration -public class SecurityConfig { +@Configuration +@Profile({"dev", "default"}) +public class SecurityConfig {Or create a separate dev‑only config for the in‑memory user and rely on a real user store elsewhere.
src/main/java/org/example/controller/PatientsController.java (1)
12-18: Prefer constructor injection andprivate finalrepository fieldThe controller logic looks fine, but the field injection with package‑private visibility is non‑idiomatic compared to your other controllers.
Consider aligning with
HospitalControllerby using constructor injection and making the dependencyprivate final:-@Controller -@PreAuthorize("hasRole('ADMIN')") -public class PatientsController { - - @Autowired - PatientRepository patientRepository; +@Controller +@PreAuthorize("hasRole('ADMIN')") +public class PatientsController { + + private final PatientRepository patientRepository; + + public PatientsController(PatientRepository patientRepository) { + this.patientRepository = patientRepository; + }This improves testability and avoids reflection‑based field injection.
src/main/java/org/example/config/DataInitializer.java (1)
16-43: Seed behavior tied todevanddefaultprofiles—confirm this matches your environmentsThe initializer correctly guards against reseeding by checking
admissionRepository.count()and supports a--force-initflag. However, annotating it with@Profile({"dev", "default"})means it will also run whenever no profile is explicitly set (Spring’s default profile).If you intend the sample data strictly for local development, consider narrowing to just
dev(and maybetest) or ensuring production runs with a distinct profile:-@Profile({"dev", "default"}) +@Profile({"dev"}) public class DataInitializer implements ApplicationRunner {Alternatively, standardize on a dedicated seeding profile (e.g.,
local) to avoid surprises.src/main/java/org/example/entity/Patient.java (1)
31-37: Constructor parameter order is surprising and easy to misuseThe
Patientconstructor takes(LocalDate dateOfBirth, String address, String lastName, String firstName, String ssn), which is an unusual order compared to how you construct DTOs and use the fields elsewhere (usually firstName, lastName, address, dob).To improve readability and reduce the chance of mixing arguments in future call sites, consider either:
- Reordering the parameters to
(String firstName, String lastName, String address, LocalDate dateOfBirth, String ssn), updating the initializer accordingly, or- Replacing this multi‑arg constructor with a builder/static factory and keeping the no‑arg constructor for JPA only.
Not urgent given the limited usage, but it will make the API clearer as the model grows.
src/main/java/org/example/controller/HospitalController.java (1)
66-77: Minor cleanup: simplify null‑check and remove redundantelseThe
handleNewmethod is functionally correct, but theelseis redundant because theifbranch throws.You can simplify slightly:
- public String handleNew(@ModelAttribute NewAdmissionDTO newAdm) { - - Patient pat; - pat = patientRepository.findBySsn(newAdm.patSsn()); - if (pat == null) { - throw new IllegalArgumentException("Patient with SSN " + newAdm.patSsn() + " not found"); - } else { - admissionRepository.save(new Admission(pat, newAdm.diagnosis(), newAdm.department())); - } + public String handleNew(@ModelAttribute NewAdmissionDTO newAdm) { + Patient pat = patientRepository.findBySsn(newAdm.patSsn()); + if (pat == null) { + throw new IllegalArgumentException("Patient with SSN " + newAdm.patSsn() + " not found"); + } + admissionRepository.save(new Admission(pat, newAdm.diagnosis(), newAdm.department()));This keeps the control flow straightforward and avoids an unnecessary branch.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
.gitignore(1 hunks)pom.xml(1 hunks)src/main/java/org/example/App.java(0 hunks)src/main/java/org/example/Exercise2025Application.java(1 hunks)src/main/java/org/example/config/DataInitializer.java(1 hunks)src/main/java/org/example/config/JavaMigrationsFlywayCustomizer.java(1 hunks)src/main/java/org/example/config/SecurityConfig.java(1 hunks)src/main/java/org/example/controller/AdmissionsController.java(1 hunks)src/main/java/org/example/controller/HospitalController.java(1 hunks)src/main/java/org/example/controller/PatientsController.java(1 hunks)src/main/java/org/example/controller/WebController.java(1 hunks)src/main/java/org/example/dto/AdmissionDTO.java(1 hunks)src/main/java/org/example/dto/Handler.java(1 hunks)src/main/java/org/example/dto/NewAdmissionDTO.java(1 hunks)src/main/java/org/example/dto/PatientDTO.java(1 hunks)src/main/java/org/example/entity/Admission.java(1 hunks)src/main/java/org/example/entity/Patient.java(1 hunks)src/main/java/org/example/repository/AdmissionRepository.java(1 hunks)src/main/java/org/example/repository/PatientRepository.java(1 hunks)src/main/resources/application-dev.yml(1 hunks)src/main/resources/application.yml(1 hunks)src/main/resources/db/migration/V1__.sql(1 hunks)src/main/resources/static/style.css(1 hunks)src/main/resources/templates/admissions/admissionMenu.html(1 hunks)src/main/resources/templates/admissions/admissions.html(1 hunks)src/main/resources/templates/admissions/newAdmission.html(1 hunks)src/main/resources/templates/error.html(1 hunks)src/main/resources/templates/index.html(1 hunks)src/main/resources/templates/login.html(1 hunks)src/main/resources/templates/logout.html(1 hunks)src/main/resources/templates/patients/newPatient.html(1 hunks)src/main/resources/templates/patients/patients.html(1 hunks)src/main/resources/templates/patients/patientsMenu.html(1 hunks)src/test/java/org/example/AppIT.java(0 hunks)src/test/java/org/example/AppTest.java(0 hunks)src/test/java/org/example/ControllerTest.java(1 hunks)src/test/resources/org/example/patients.sql(1 hunks)
💤 Files with no reviewable changes (3)
- src/test/java/org/example/AppTest.java
- src/main/java/org/example/App.java
- src/test/java/org/example/AppIT.java
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-16T13:56:11.263Z
Learnt from: jennymakki
Repo: fungover/exercise2025 PR: 204
File: src/main/java/org/example/config/SecurityConfig.java:19-24
Timestamp: 2025-11-16T13:56:11.263Z
Learning: The repository fungover/exercise2025 is an exercise/learning project where hard-coded credentials and other shortcuts for ease of testing are acceptable and intentional.
Applied to files:
src/main/resources/application.ymlpom.xml
📚 Learning: 2025-11-21T16:16:54.793Z
Learnt from: LolloGro
Repo: fungover/exercise2025 PR: 211
File: src/main/resources/templates/listOfBooks.html:11-12
Timestamp: 2025-11-21T16:16:54.793Z
Learning: In the repository fungover/exercise2025, the Author DTO uses NotEmpty validation on firstName and lastName fields, and BookDto uses NotNull validation on the author field. This validation architecture ensures that books always have complete author information, making null-safety checks in Thymeleaf templates unnecessary.
Applied to files:
pom.xml
🧬 Code graph analysis (5)
src/main/java/org/example/controller/WebController.java (3)
src/main/java/org/example/controller/AdmissionsController.java (1)
Controller(12-39)src/main/java/org/example/controller/HospitalController.java (1)
Controller(15-95)src/main/java/org/example/controller/PatientsController.java (1)
Controller(12-40)
src/main/java/org/example/entity/Admission.java (1)
src/main/java/org/example/entity/Patient.java (1)
Entity(11-85)
src/main/java/org/example/controller/AdmissionsController.java (3)
src/main/java/org/example/controller/HospitalController.java (1)
Controller(15-95)src/main/java/org/example/controller/PatientsController.java (1)
Controller(12-40)src/main/java/org/example/controller/WebController.java (1)
Controller(7-25)
src/main/java/org/example/entity/Patient.java (1)
src/main/java/org/example/entity/Admission.java (1)
Entity(7-70)
src/main/java/org/example/controller/HospitalController.java (3)
src/main/java/org/example/controller/AdmissionsController.java (1)
Controller(12-39)src/main/java/org/example/controller/PatientsController.java (1)
Controller(12-40)src/main/java/org/example/controller/WebController.java (1)
Controller(7-25)
🔇 Additional comments (5)
src/main/resources/application-dev.yml (1)
1-9: Dev JPA and docker-compose settings look appropriateLimiting SQL logging and docker-compose lifecycle management to the
application-dev.ymlprofile is a good fit for local debugging; no changes needed here.pom.xml (1)
32-115: Spring Boot, Flyway, MySQL, and Testcontainers dependency setup looks coherentThe combination of Spring Boot starters, Flyway (
flyway-core+flyway-mysql), MySQL runtime driver, docker-compose support, and Testcontainers modules is consistent with a modern Boot 3.x app; just make sure the chosen Spring Boot version and plugin setup build and run cleanly in your current environment (IDE + CI).src/main/java/org/example/dto/AdmissionDTO.java (1)
1-6: AdmissionDTO record definition looks appropriateThe DTO fields and types match the controller mappings and view usage; using a
recordhere is concise and suitable.src/main/resources/templates/admissions/admissions.html (1)
11-24: The review comment incorrectly identifies a non-existent syntax errorThe
th:styleattribute on line 15 (displayed as line 11-24 in the review) DOES contain the closing}in the expression. The actual code is:th:style="${admission.dateOut == null}? 'display: block;' : 'display: none;'"This is valid Thymeleaf ternary syntax. The closing brace properly terminates the
${...}expression before the?operator. The proposed diff to add a space before?is unnecessary because there is no missing brace to fix.The optional refactor suggestion to use
th:ifinstead of inline styles remains a valid stylistic recommendation but should not be framed as a syntax error fix.Likely an incorrect or invalid review comment.
src/main/java/org/example/controller/HospitalController.java (1)
27-32: I'll verify the review comment by examining the HospitalController file to confirm the current state of the code and the validity of the concerns raised.
<function_calls>
#!/bin/bashLocate the HospitalController file
find . -type f -name "HospitalController.java"
</function_calls>
<function_calls>
#!/bin/bashRead the HospitalController file with line numbers
cat -n src/main/java/org/example/controller/HospitalController.java
</function_calls>
| .authorizeHttpRequests((authorize) -> authorize | ||
| .requestMatchers("/").permitAll() | ||
| .requestMatchers("**.css").permitAll() | ||
| .requestMatchers("/login").permitAll() | ||
| .requestMatchers("/api/**").authenticated() | ||
| .requestMatchers("/logout").authenticated() | ||
| .requestMatchers("/api/admissions/new").hasRole("ADMIN") | ||
| .anyRequest().authenticated()); |
There was a problem hiding this comment.
🧩 Analysis chain
Script executed:
#!/bin/bash
# Locate and read SecurityConfig.java
fd -t f "SecurityConfig.java" | head -5Repository: fungover/exercise2025
<shell_result>
Repository: fungover/exercise2025
Exit code: 0
stdout:
src/main/java/org/example/config/SecurityConfig.java
</shell_result>
Script executed:
#!/bin/bash
# Read SecurityConfig.java to verify the matcher ordering
cat -n src/main/java/org/example/config/SecurityConfig.javaRepository: fungover/exercise2025
<shell_result>
Repository: fungover/exercise2025
Exit code: 0
stdout:
1 package org.example.config;
2
3 import org.springframework.context.annotation.Bean;
4 import org.springframework.context.annotation.Configuration;
5 import org.springframework.security.authentication.AuthenticationManager;
6 import org.springframework.security.authentication.BadCredentialsException;
7 import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
8 import org.springframework.security.config.annotation.method.configuration.EnableMethodSecurity;
9 import org.springframework.security.config.annotation.web.builders.HttpSecurity;
10 import org.springframework.security.config.annotation.web.configurers.AbstractHttpConfigurer;
11 import org.springframework.security.core.userdetails.User;
12 import org.springframework.security.core.userdetails.UserDetails;
13 import org.springframework.security.core.userdetails.UserDetailsService;
14 import org.springframework.security.crypto.bcrypt.BCryptPasswordEncoder;
15 import org.springframework.security.crypto.password.PasswordEncoder;
16 import org.springframework.security.provisioning.InMemoryUserDetailsManager;
17 import org.springframework.security.web.SecurityFilterChain;
18
19 @Configuration
20 @EnableMethodSecurity
21 public class SecurityConfig {
22
23 @Bean
24 public SecurityFilterChain web(HttpSecurity http, AuthenticationManager authenticationManager) throws Exception {
25 http
26 .formLogin(formLogin -> formLogin.defaultSuccessUrl("/", true).loginPage("/login"))
27 .logout(logout -> {
28 logout.logoutSuccessUrl("/");
29 })
30 .authorizeHttpRequests((authorize) -> authorize
31 .requestMatchers("/").permitAll()
32 .requestMatchers("**.css").permitAll()
33 .requestMatchers("/login").permitAll()
34 .requestMatchers("/api/**").authenticated()
35 .requestMatchers("/logout").authenticated()
36 .requestMatchers("/api/admissions/new").hasRole("ADMIN")
37 .anyRequest().authenticated());
38 return http.build();
39 }
40
41 @Bean
42 public UserDetailsService userDetailsService(PasswordEncoder passwordEncoder) {
43 UserDetails admin = User.withUsername("admin")
44 .password(passwordEncoder.encode("password"))
45 .roles("ADMIN", "DEV", "USER")
46 .build();
47 return new InMemoryUserDetailsManager(admin);
48 }
49
50 @Bean
51 public AuthenticationManager authenticationManager(UserDetailsService userDetailsService, PasswordEncoder passwordEncoder) {
52 return authentication -> {
53 String username = authentication.getName();
54 String rawPassword = authentication.getCredentials().toString();
55
56 UserDetails user = userDetailsService.loadUserByUsername(username);
57
58 if (passwordEncoder.matches(rawPassword, user.getPassword())) {
59 return new UsernamePasswordAuthenticationToken(username, user.getPassword(), user.getAuthorities());
60 }
61 throw new BadCredentialsException("Bad credentials");
62 };
63 }
64
65 @Bean
66 public PasswordEncoder passwordEncoder() {
67 return new BCryptPasswordEncoder();
68 }
69
70 }
</shell_result>
Script executed:
#!/bin/bash
# Search for Spring Security documentation or comments about matcher ordering
rg -i "authorizeHttpRequests|requestMatchers" -A 5 -B 5 --type java | head -100Repository: fungover/exercise2025
<shell_result>
Repository: fungover/exercise2025
Exit code: 0
stdout:
src/main/java/org/example/config/SecurityConfig.java- http
src/main/java/org/example/config/SecurityConfig.java- .formLogin(formLogin -> formLogin.defaultSuccessUrl("/", true).loginPage("/login"))
src/main/java/org/example/config/SecurityConfig.java- .logout(logout -> {
src/main/java/org/example/config/SecurityConfig.java- logout.logoutSuccessUrl("/");
src/main/java/org/example/config/SecurityConfig.java- })
src/main/java/org/example/config/SecurityConfig.java: .authorizeHttpRequests((authorize) -> authorize
src/main/java/org/example/config/SecurityConfig.java: .requestMatchers("/").permitAll()
src/main/java/org/example/config/SecurityConfig.java: .requestMatchers("**.css").permitAll()
src/main/java/org/example/config/SecurityConfig.java: .requestMatchers("/login").permitAll()
src/main/java/org/example/config/SecurityConfig.java: .requestMatchers("/api/**").authenticated()
src/main/java/org/example/config/SecurityConfig.java: .requestMatchers("/logout").authenticated()
src/main/java/org/example/config/SecurityConfig.java: .requestMatchers("/api/admissions/new").hasRole("ADMIN")
src/main/java/org/example/config/SecurityConfig.java- .anyRequest().authenticated());
src/main/java/org/example/config/SecurityConfig.java- return http.build();
src/main/java/org/example/config/SecurityConfig.java- }
src/main/java/org/example/config/SecurityConfig.java-
src/main/java/org/example/config/SecurityConfig.java- @Bean
</shell_result>
Script executed:
#!/bin/bash
# Verify Spring Security AntPath pattern matching behavior
# Check if there are any tests or documentation about this
find . -type f -name "*.java" -o -name "*.md" -o -name "*.txt" | xargs rg -l "AntPath|matcher.*order" 2>/dev/null | head -10Repository: fungover/exercise2025
<shell_result>
Repository: fungover/exercise2025
Exit code: 0
</shell_result>
Based on my verification of the code and Spring Security's behavior:
The matcher ordering bug is confirmed—/api/admissions/new is not restricted to ADMIN users
In Spring Security's authorizeHttpRequests(), matchers are evaluated in order and the first match applies. Currently, /api/** on line 34 matches /api/admissions/new before the specific hasRole("ADMIN") rule on line 36 is reached, allowing any authenticated user to access this endpoint.
The proposed fix is correct: move the specific /api/admissions/new matcher before the generic /api/** matcher. The CSS pattern adjustment from "**.css" to "/**/*.css" is also valid—more explicit and idiomatic for AntPath patterns, though both technically function.
- .authorizeHttpRequests((authorize) -> authorize
- .requestMatchers("/").permitAll()
- .requestMatchers("**.css").permitAll()
- .requestMatchers("/login").permitAll()
- .requestMatchers("/api/**").authenticated()
- .requestMatchers("/logout").authenticated()
- .requestMatchers("/api/admissions/new").hasRole("ADMIN")
- .anyRequest().authenticated());
+ .authorizeHttpRequests((authorize) -> authorize
+ .requestMatchers("/").permitAll()
+ .requestMatchers("/**/*.css").permitAll()
+ .requestMatchers("/login").permitAll()
+ .requestMatchers("/api/admissions/new").hasRole("ADMIN")
+ .requestMatchers("/api/**").authenticated()
+ .requestMatchers("/logout").authenticated()
+ .anyRequest().authenticated());🤖 Prompt for AI Agents
In src/main/java/org/example/config/SecurityConfig.java around lines 30 to 37,
the authorization matchers are ordered so the generic "/api/**" rule precedes
the specific "/api/admissions/new" rule and thus the specific ADMIN restriction
is never applied; also the CSS pattern should be more explicit. Fix by moving
the .requestMatchers("/api/admissions/new").hasRole("ADMIN") line to appear
before .requestMatchers("/api/**").authenticated() and change the CSS matcher
from "**.css" to "/**/*.css" so the specific rules are evaluated before the
generic one and the CSS pattern uses the idiomatic AntPath form.
| public record Handler(Patient patient, NewAdmissionDTO admission) { | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Rename to a more descriptive name.
The name "Handler" is misleading and doesn't convey the purpose of this DTO. Consider a more descriptive name like PatientAdmissionRequest or NewAdmissionWithPatient that clearly indicates this record bundles a patient with admission data.
-public record Handler(Patient patient, NewAdmissionDTO admission) {
+public record PatientAdmissionRequest(Patient patient, NewAdmissionDTO admission) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public record Handler(Patient patient, NewAdmissionDTO admission) { | |
| } | |
| public record PatientAdmissionRequest(Patient patient, NewAdmissionDTO admission) { | |
| } |
🤖 Prompt for AI Agents
In src/main/java/org/example/dto/Handler.java around lines 5-6, the record name
"Handler" is non-descriptive; rename the record to a clearer name such as
PatientAdmissionRequest (or NewAdmissionWithPatient) and update the file name to
match (PatientAdmissionRequest.java), then update all references/imports/usages
across the codebase (controllers, services, tests, JSON serialization keys if
applicable) to the new type name so compilation and runtime serialization
continue to work.
| <h1>Patients Menu</h1> | ||
| <a href="/patients" th:text="@{all patients}"></a> | ||
| <a href="/patients/new" th:text="@{new patient}"></a> | ||
| <a href="/patients/find" th:text="@{find patient}"></a> | ||
| <a href="/patients/delete" th:text="@{delete patient}"></a> |
There was a problem hiding this comment.
🧩 Analysis chain
Script executed:
#!/bin/bash
# Find and examine the patientsMenu.html file
find . -name "patientsMenu.html" -type f 2>/dev/null | head -20Repository: fungover/exercise2025
<shell_result>
Repository: fungover/exercise2025
Exit code: 0
stdout:
./src/main/resources/templates/patients/patientsMenu.html
</shell_result>
Script executed:
#!/bin/bash
# Read the patientsMenu.html file
cat -n ./src/main/resources/templates/patients/patientsMenu.html
echo "---"
# Search for similar templates (admissionMenu mentioned in scratchpad)
find . -name "*admissionMenu.html" -type f 2>/dev/nullRepository: fungover/exercise2025
<shell_result>
Repository: fungover/exercise2025
Exit code: 0
stdout:
1 <!DOCTYPE html>
2 <html lang="en" xmlns:th="http://www.thymeleaf.org">
3 <head>
4 <meta charset="UTF-8">
5 <title>Patients Menu</title>
6 <link rel="stylesheet" href="/style.css">
7 </head>
8 <body>
9 <div>
10 <h1>Patients Menu</h1>
11 <a href="/patients" th:text="@{all patients}"></a>
12 <a href="/patients/new" th:text="@{new patient}"></a>
13 <a href="/patients/find" th:text="@{find patient}"></a>
14 <a href="/patients/delete" th:text="@{delete patient}"></a>
15 </div>
16 </body>
17 </html>---
./src/main/resources/templates/admissions/admissionMenu.html
</shell_result>
Script executed:
#!/bin/bash
# Read admissionMenu.html to compare patterns
cat -n ./src/main/resources/templates/admissions/admissionMenu.html
echo "---"
# Search for other th:text usage patterns in templates
rg -n "th:text" ./src/main/resources/templates/ -A1 -B1 | head -60Repository: fungover/exercise2025
<shell_result>
Repository: fungover/exercise2025
Exit code: 0
stdout:
1 <!DOCTYPE html>
2 <html lang="en" xmlns:th="http://www.thymeleaf.org">
3 <head>
4 <meta charset="UTF-8">
5 <title>Admission Menu</title>
6 <link rel="stylesheet" href="/style.css">
7 </head>
8 <body>
9 <div>
10 <h1>Admission Menu</h1>
11 <a href="/admissions" th:text="@{all admissions}" ></a>
12 <a href="/admissions/new" th:text="@{new admission}"></a>
13 <a href="/admissions/find" th:text="@{find admission}"></a>
14 <a href="/admissions/delete" th:text="@{delete admission}"></a>
15 </div>
16 </body>
17 </html>---
./src/main/resources/templates/patients/patientsMenu.html-10- <h1>Patients Menu</h1>
./src/main/resources/templates/patients/patientsMenu.html:11: <a href="/patients" th:text="@{all patients}"></a>
./src/main/resources/templates/patients/patientsMenu.html:12: <a href="/patients/new" th:text="@{new patient}"></a>
./src/main/resources/templates/patients/patientsMenu.html:13: <a href="/patients/find" th:text="@{find patient}"></a>
./src/main/resources/templates/patients/patientsMenu.html:14: <a href="/patients/delete" th:text="@{delete patient}"></a>
./src/main/resources/templates/patients/patientsMenu.html-15-</div>
--
./src/main/resources/templates/patients/patients.html-12- <li>
./src/main/resources/templates/patients/patients.html:13: <p th:text="${patient.firstName()}" style="display: inline"></p>
./src/main/resources/templates/patients/patients.html-14- <p style="display: inline"></p>
./src/main/resources/templates/patients/patients.html:15: <p th:text="${patient.lastName()}" style="display: inline"></p>
./src/main/resources/templates/patients/patients.html-16- </li>
--
./src/main/resources/templates/index.html-12- </h1>
./src/main/resources/templates/index.html:13: <a href="/patients-menu" th:text="patients"></a>
./src/main/resources/templates/index.html:14: <a href="/admissions-menu" th:text="admissions"></a>
./src/main/resources/templates/index.html-15-</div>
--
./src/main/resources/templates/admissions/admissions.html-13- <ul style="list-style-type:disc; margin-top:2em;">
./src/main/resources/templates/admissions/admissions.html:14: <li><p th:text="${'Admission ID:' + admission.admId}"></p>
./src/main/resources/templates/admissions/admissions.html-15- <form th:style="${admission.dateOut == null}? 'display: block;' : 'display: none;'"
--
./src/main/resources/templates/admissions/admissions.html-19- </li>
./src/main/resources/templates/admissions/admissions.html:20: <li><p th:text="${'Patient ID: ' + admission.patId}"></p></li>
./src/main/resources/templates/admissions/admissions.html:21: <li><p th:text="${'Date In: ' + admission.dateIn}"></p></li>
./src/main/resources/templates/admissions/admissions.html:22: <li><p th:text="${'Date Out: ' + (admission.dateOut == null? '-' : admission.dateOut)}"></p></li>
./src/main/resources/templates/admissions/admissions.html:23: <li><p th:text="${'Diagnosis: ' + admission.diagnosis}"></p></li>
./src/main/resources/templates/admissions/admissions.html:24: <li><p th:text="${'Department: ' + admission.department}"></p></li>
./src/main/resources/templates/admissions/admissions.html-25- </ul>
--
./src/main/resources/templates/admissions/admissionMenu.html-10- <h1>Admission Menu</h1>
./src/main/resources/templates/admissions/admissionMenu.html:11: <a href="/admissions" th:text="@{all admissions}" ></a>
./src/main/resources/templates/admissions/admissionMenu.html:12: <a href="/admissions/new" th:text="@{new admission}"></a>
./src/main/resources/templates/admissions/admissionMenu.html:13: <a href="/admissions/find" th:text="@{find admission}"></a>
./src/main/resources/templates/admissions/admissionMenu.html:14: <a href="/admissions/delete" th:text="@{delete admission}"></a>
./src/main/resources/templates/admissions/admissionMenu.html-15-</div>
</shell_result>
Script executed:
#!/bin/bash
# Search for any test files or error indicators related to templates
find . -name "*Test*.java" -o -name "*test*.java" | xargs grep -l "patientsMenu\|template" 2>/dev/null | head -5
echo "---"
# Check if there are any template tests or integration tests
find . -name "*.java" -type f | xargs grep -l "patientsMenu" 2>/dev/null
echo "---"
# Look for any documentation on Thymeleaf syntax in the repo
find . -name "*.md" -o -name "*.txt" | xargs grep -i "thymeleaf\|th:text" 2>/dev/null | head -10Repository: fungover/exercise2025
<shell_result>
Repository: fungover/exercise2025
Exit code: 0
stdout:
---
./src/main/java/org/example/controller/PatientsController.java
---
</shell_result>
Script executed:
#!/bin/bash
# Check PatientsController to see how patientsMenu is rendered
cat ./src/main/java/org/example/controller/PatientsController.javaRepository: fungover/exercise2025
<shell_result>
Repository: fungover/exercise2025
Exit code: 0
stdout:
package org.example.controller;
import org.example.dto.PatientDTO;
import org.example.entity.Patient;
import org.example.repository.PatientRepository;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.security.access.prepost.PreAuthorize;
import org.springframework.stereotype.Controller;
import org.springframework.ui.Model;
import org.springframework.web.bind.annotation.GetMapping;
@Controller
@PreAuthorize("hasRole('ADMIN')")
public class PatientsController {
@Autowired
PatientRepository patientRepository;
// ---- PATIENTS ----
@GetMapping("/patients-menu")
public String patientsMenu(Model model) {
return "patients/patientsMenu";
}
@GetMapping("/patients")
public String displayPatients(Model model) {
model.addAttribute("patients", patientRepository.findAll().stream()
.map(pat -> new PatientDTO(pat.getFirstName(), pat.getLastName(), pat.getAddress(), pat.getDateOfBirth()))
.toList());
return "patients/patients";
}
@GetMapping("/patients/new")
public String newPatient(Model model) {
model.addAttribute("patient", new Patient());
return "patients/newPatient";
}
}
</shell_result>
Based on my verification, the original review comment is correct and well-founded. Here's what I confirmed:
- File exists and matches: The patientsMenu.html file at lines 10-14 contains exactly the code shown
- Identical pattern in admissionMenu.html: Both menu templates have the same issue (
th:text="@{all admissions}",th:text="@{all patients}", etc.) - Actual Thymeleaf syntax violation:
@{...}is Thymeleaf's Link Expression syntax, designed for URL generationth:textexpects text content: plain strings,${expressions},#{messages}, or[[...]]- Using
@{all patients}attempts to parse "all patients" as a URL expression with spaces, which is invalid
- Template is actively used: The controller has a
/patients-menuroute that renders this template, so the error would occur at runtime
The fix suggested in the original review is correct.
Fix invalid th:text usage in patients menu links
The th:text="@{all patients}" syntax misuses Thymeleaf's URL expression (@{...}) for link text. The @{...} syntax is for URL generation, not text content, and spaces make it syntactically invalid. This will break template rendering.
Replace with plain text:
- <h1>Patients Menu</h1>
- <a href="/patients" th:text="@{all patients}"></a>
- <a href="/patients/new" th:text="@{new patient}"></a>
- <a href="/patients/find" th:text="@{find patient}"></a>
- <a href="/patients/delete" th:text="@{delete patient}"></a>
+ <h1>Patients Menu</h1>
+ <a href="/patients">All patients</a>
+ <a href="/patients/new">New patient</a>
+ <a href="/patients/find">Find patient</a>
+ <a href="/patients/delete">Delete patient</a>Alternatively, for i18n support, use th:text="#{patients.all}" etc. with message bundle keys.
Note: The same issue exists in src/main/resources/templates/admissions/admissionMenu.html.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <h1>Patients Menu</h1> | |
| <a href="/patients" th:text="@{all patients}"></a> | |
| <a href="/patients/new" th:text="@{new patient}"></a> | |
| <a href="/patients/find" th:text="@{find patient}"></a> | |
| <a href="/patients/delete" th:text="@{delete patient}"></a> | |
| <h1>Patients Menu</h1> | |
| <a href="/patients">All patients</a> | |
| <a href="/patients/new">New patient</a> | |
| <a href="/patients/find">Find patient</a> | |
| <a href="/patients/delete">Delete patient</a> |
🤖 Prompt for AI Agents
In src/main/resources/templates/patients/patientsMenu.html around lines 10-14,
the links use invalid Thymeleaf link expressions in th:text (e.g. th:text="@{all
patients}") which is a URL expression and breaks on spaces; change each th:text
to either plain literal text (e.g. th:text="'All Patients'") or to i18n message
lookups (e.g. th:text="#{patients.all}"), and apply the same fix to
src/main/resources/templates/admissions/admissionMenu.html where identical
misuse occurs.
Jorlindstrom
left a comment
Jorlindstrom
left a comment
There was a problem hiding this comment.
After reviewing the submitted code and project structure, I have identified several issues and areas for improvement that need attention before approval, along with some very positive points.
The project is well-structured packages: Entities, DTOs, controllers, repositories, and config are clearly separated.
You use modern Spring Boot features: @Profile for dev-only data initialization and method-level security with @PreAuthorize and. they are are well done.
You are mapping entities to DTOs for API responses which shows good separation of concerns.
The use of BCryptPasswordEncoder for security is correct.
Automatically populating sample data for dev profile is thoughtful and will help testing.
The project supports both API endpoints and web UI, which is a solid architectural choice.
Areas for Improvement
1. Project Startup Issues
The application fails to start due to missing docker-compose.yml.
This prevents database initialization and must be fixed.
2. Entity Layer
Patient entity uses getId() for pat_id. Consider renaming for consistency (getPatId()).
Admission automatically sets dateIn to LocalDateTime.now() and dateOut in setDateOut(). Consider allowing manual values for flexibility/testing.
@manytoone relationship in Admission defaults to EAGER fetch. Consider LAZY to improve performance.
3. Repository Layer
AdmissionRepository.getAdmissionsByPatId has invalid JPQL.
PatientRepository.findByFirstAndLastName concatenates strings, which is brittle and inefficient. Prefer using unique fields like SSN.
4. Controller Layer
Some duplicated logic between AdmissionsController and HospitalController.
Methods like writeOut silently ignore missing admissions; better error handling is recommended.
/patients endpoints are partially duplicated; consider clearer separation.
5. Security
Hardcoded in-memory admin with password "password" is insecure. Consider differentiating dev vs. prod security.
6. Data Initialization
Hardcoded sample data is acceptable for dev profile, but ensure it does not run in production.
7. DTOs
NewAdmissionDTO is defined twice; remove duplicates.
8. Testing
Only ControllerTest exists. There are no repository or service layer tests.
Conclusion:
The project demonstrates good architectural thinking and solid use of Spring Boot features. With fixes to startup issues, JPQL queries, and test coverage, this project will be robust, maintainable, and production-ready.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (9)
src/main/java/org/example/config/SecurityConfigDev.java (1)
31-31: Use a more idiomatic pattern for CSS resources.The pattern
"/**.css"is non-standard in Spring Security configurations. Consider using"/**/*.css"to match CSS files at any depth, or"/css/**"if CSS files are in a specific directory.🔎 Apply this diff to use a standard pattern:
- .requestMatchers("/", "/login", "/**.css").permitAll() + .requestMatchers("/", "/login", "/**/*.css").permitAll()src/main/java/org/example/dto/PatientAdmissionRequest.java (1)
3-5: Consider using PatientDTO instead of the entity.The DTO embeds a JPA entity (
Patient) directly. This couples the request payload to the persistence layer, which can leak internal details and complicate serialization. Consider usingPatientDTOinstead to maintain clean layer separation.pom.xml (1)
16-28: Remove or populate empty metadata sections.The license, developers, and SCM sections contain empty placeholder elements. Either populate them with actual project metadata or remove them to reduce noise.
docker-compose.yml (1)
1-10: Consider adding a volume for data persistence.The MySQL container has no volume configured. Data will be lost when the container is removed. For development convenience, consider adding a volume:
services: mysql: image: 'mysql:9.5.0' environment: - 'MYSQL_DATABASE=hospital' - 'MYSQL_PASSWORD=secret' - 'MYSQL_ROOT_PASSWORD=admin' - 'MYSQL_USER=user' ports: - '3306:3306' + volumes: + - mysql_data:/var/lib/mysql + +volumes: + mysql_data:src/main/java/org/example/config/SecurityConfigProd.java (1)
21-21: Clarify CSRF configuration intent.The empty lambda
csrf -> { }leaves CSRF enabled with defaults, which is correct for production. However, for clarity, consider usingCustomizer.withDefaults()or adding a comment explaining the intent:- .csrf(csrf -> { }) + .csrf(Customizer.withDefaults()) // CSRF enabled with defaultssrc/main/java/org/example/entity/Admission.java (2)
14-14: Remove redundant null initialization.The explicit
= nullinitialization is unnecessary since reference fields default to null in Java.🔎 Apply this diff to simplify the field declaration:
- private LocalDateTime dateOut = null; + private LocalDateTime dateOut;
19-21: Consider making the patient field private for consistency.The
patientfield has package-private visibility while other fields are explicitly private. For consistency and better encapsulation, consider making it private.🔎 Apply this diff to improve encapsulation:
@ManyToOne(fetch = FetchType.LAZY) @JoinColumn(name = "pat_id") - Patient patient; + private Patient patient;src/main/java/org/example/controller/AdmissionsController.java (1)
24-27: Prefer constructor injection over field injection.Constructor injection improves testability and makes dependencies explicit and immutable.
🔎 Apply this diff to use constructor injection:
- @Autowired - AdmissionRepository admissionRepository; - @Autowired - PatientRepository patientRepository; + private final AdmissionRepository admissionRepository; + private final PatientRepository patientRepository; + + public AdmissionsController(AdmissionRepository admissionRepository, PatientRepository patientRepository) { + this.admissionRepository = admissionRepository; + this.patientRepository = patientRepository; + }src/main/java/org/example/controller/PatientsController.java (1)
19-20: Prefer constructor injection over field injection.Constructor injection improves testability, makes dependencies explicit, and enables final fields for immutability.
🔎 Apply this diff to use constructor injection:
- @Autowired - PatientRepository patientRepository; + private final PatientRepository patientRepository; + + public PatientsController(PatientRepository patientRepository) { + this.patientRepository = patientRepository; + }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
docker-compose.yml(1 hunks)pom.xml(1 hunks)src/main/java/org/example/config/DataInitializer.java(1 hunks)src/main/java/org/example/config/SecurityConfigDev.java(1 hunks)src/main/java/org/example/config/SecurityConfigProd.java(1 hunks)src/main/java/org/example/controller/AdmissionsController.java(1 hunks)src/main/java/org/example/controller/PatientsController.java(1 hunks)src/main/java/org/example/dto/PatientAdmissionRequest.java(1 hunks)src/main/java/org/example/entity/Admission.java(1 hunks)src/main/java/org/example/entity/Patient.java(1 hunks)src/main/java/org/example/repository/AdmissionRepository.java(1 hunks)src/main/java/org/example/repository/PatientRepository.java(1 hunks)src/main/resources/application.yml(1 hunks)src/main/resources/templates/admissions/admissionMenu.html(1 hunks)src/main/resources/templates/logout.html(1 hunks)src/main/resources/templates/patients/patients.html(1 hunks)src/main/resources/templates/patients/patientsMenu.html(1 hunks)src/test/java/org/example/AdmissionRepositoryTest.java(1 hunks)src/test/java/org/example/ControllerTest.java(1 hunks)src/test/resources/org/example/patients.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- src/main/resources/templates/admissions/admissionMenu.html
- src/main/resources/templates/logout.html
- src/main/java/org/example/config/DataInitializer.java
- src/main/java/org/example/repository/PatientRepository.java
- src/main/resources/application.yml
- src/test/java/org/example/ControllerTest.java
- src/main/java/org/example/entity/Patient.java
- src/main/resources/templates/patients/patients.html
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-16T13:56:11.263Z
Learnt from: jennymakki
Repo: fungover/exercise2025 PR: 204
File: src/main/java/org/example/config/SecurityConfig.java:19-24
Timestamp: 2025-11-16T13:56:11.263Z
Learning: The repository fungover/exercise2025 is an exercise/learning project where hard-coded credentials and other shortcuts for ease of testing are acceptable and intentional.
Applied to files:
pom.xml
📚 Learning: 2025-11-21T16:16:54.793Z
Learnt from: LolloGro
Repo: fungover/exercise2025 PR: 211
File: src/main/resources/templates/listOfBooks.html:11-12
Timestamp: 2025-11-21T16:16:54.793Z
Learning: In the repository fungover/exercise2025, the Author DTO uses NotEmpty validation on firstName and lastName fields, and BookDto uses NotNull validation on the author field. This validation architecture ensures that books always have complete author information, making null-safety checks in Thymeleaf templates unnecessary.
Applied to files:
pom.xml
🧬 Code graph analysis (5)
src/main/java/org/example/config/SecurityConfigDev.java (1)
src/main/java/org/example/config/SecurityConfigProd.java (1)
Configuration(13-35)
src/main/java/org/example/controller/PatientsController.java (2)
src/main/java/org/example/controller/AdmissionsController.java (1)
Controller(20-78)src/main/java/org/example/controller/WebController.java (1)
Controller(7-25)
src/main/java/org/example/entity/Admission.java (1)
src/main/java/org/example/entity/Patient.java (1)
Entity(11-85)
src/main/java/org/example/config/SecurityConfigProd.java (1)
src/main/java/org/example/config/SecurityConfigDev.java (1)
Configuration(16-54)
src/main/java/org/example/controller/AdmissionsController.java (2)
src/main/java/org/example/controller/PatientsController.java (1)
Controller(15-59)src/main/java/org/example/controller/WebController.java (1)
Controller(7-25)
🔇 Additional comments (11)
src/main/java/org/example/config/SecurityConfigDev.java (1)
16-19: LGTM!The class annotations are correctly configured for a dev profile security setup.
src/test/resources/org/example/patients.sql (1)
1-9: LGTM!The test data is well-structured. The SSN values now correctly match the date_of_birth fields, addressing the previous review feedback.
src/main/resources/templates/patients/patientsMenu.html (1)
1-17: LGTM!The template now uses plain text for link labels, resolving the previous Thymeleaf syntax issue with
th:text="@{...}".src/test/java/org/example/AdmissionRepositoryTest.java (1)
27-66: LGTM!The test is well-structured: it sets up isolated data, exercises the repository method, and verifies both the result count and that all returned admissions belong to the expected patient.
src/main/java/org/example/repository/AdmissionRepository.java (2)
16-20: Verify JPQL field naming matches entity.Similar to the above method, verify that
admission.patient.pat_idmatches the actual entity field names. If the entity uses camelCase (e.g.,patId), the query should beadmission.patient.patId.The previous review's concern about the missing WHERE clause has been addressed.
11-14: Entity field naming violates Java conventions—use camelCase for fields, not snake_case.The
Admissionentity definesadmission_idas a field name, which violates standard Java naming conventions. Entity field names should use camelCase (e.g.,admissionId). Renameadmission_idtoadmissionIdand update the JPQL query accordingly to useadmission.admissionId. Similarly, verify that thePatiententity usespatIdinstead ofpat_idfor its field name, then update the second query to referenceadmission.patient.patId.Additionally, the custom
getAdmissionById()method can be replaced with the inheritedfindById(Long id)fromListCrudRepository, or if a direct return type is required, the custom method should unwrap theOptionalfromfindById().Likely an incorrect or invalid review comment.
docker-compose.yml (1)
3-3: MySQL 9.5.0 image is available on Docker Hub. No action required.pom.xml (2)
5-10: No action needed—Spring Boot 3.5.7 is a stable GA release.Spring Boot 3.5.7 has been released and is now available from Maven Central. 3.5.7 marks the latest stable release (GA) in the 3.x series, and the release delivers 69 bug fixes, alongside multiple enhancements to documentation and dependency management.
30-30: Java 25 is now the stable LTS release and appropriate for production use.Java 25 was released on September 16, 2025 and is the latest Long-Term Support (LTS) version. Oracle provides JDK 25 under a free use license for all users since September 2025. This is a valid and current choice for the project.
src/main/java/org/example/entity/Admission.java (1)
32-69: LGTM! Good use of selective mutability.The entity exposes setters only for fields that should be updatable post-creation (
dateOut,diagnosis,department), while keepingadmission_id,dateIn, andpatientimmutable after construction. This is a sound design choice.src/main/java/org/example/controller/PatientsController.java (1)
30-36: LGTM! Clean implementation without lazy-loading concerns.The mapping to
PatientDTOonly accesses direct Patient fields without triggering any lazy-loaded relationships, so there are no N+1 query issues here.
| @Bean | ||
| public SecurityFilterChain web(HttpSecurity http) throws Exception { | ||
| http | ||
| .csrf(AbstractHttpConfigurer::disable) |
There was a problem hiding this comment.
CSRF protection should remain enabled, even in dev.
Disabling CSRF creates security risks and can lead to issues being discovered only in production. Consider keeping CSRF enabled to ensure proper testing and avoid security vulnerabilities in development environments that may be exposed.
🔎 Apply this diff to enable CSRF protection:
- .csrf(AbstractHttpConfigurer::disable)
+ .csrf(csrf -> { })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .csrf(AbstractHttpConfigurer::disable) | |
| .csrf(csrf -> { }) |
🤖 Prompt for AI Agents
In src/main/java/org/example/config/SecurityConfigDev.java around line 24, the
configuration calls .csrf(AbstractHttpConfigurer::disable) which disables CSRF
in dev; remove that disable call (or revert to default .csrf() behavior) so CSRF
remains enabled, and if necessary add a CsrfTokenRepository or ensure your dev
controllers/pages accept/emit CSRF tokens to allow local testing without
bypassing protection.
| @Bean | ||
| public UserDetailsService userDetailsService(PasswordEncoder encoder) { | ||
| return new InMemoryUserDetailsManager( | ||
| User.withUsername("admin") | ||
| .password(encoder.encode("password")) | ||
| .roles("ADMIN", "DEV", "USER") | ||
| .build() | ||
| ); | ||
| } |
There was a problem hiding this comment.
Hardcoded credentials with weak password pose a security risk.
Even in dev environments, using "admin"/"password" credentials is dangerous if the environment becomes exposed (e.g., staging, demo, or misconfigured deployment). Consider using a stronger password or externalizing credentials via environment variables.
🔎 Apply this diff to use a stronger password:
public UserDetailsService userDetailsService(PasswordEncoder encoder) {
return new InMemoryUserDetailsManager(
User.withUsername("admin")
- .password(encoder.encode("password"))
+ .password(encoder.encode("Dev@dmin2025!"))
.roles("ADMIN", "DEV", "USER")
.build()
);
}Alternatively, externalize via environment variable:
public UserDetailsService userDetailsService(PasswordEncoder encoder) {
+ String devPassword = System.getenv().getOrDefault("DEV_ADMIN_PASSWORD", "ChangeMe123!");
return new InMemoryUserDetailsManager(
User.withUsername("admin")
- .password(encoder.encode("password"))
+ .password(encoder.encode(devPassword))
.roles("ADMIN", "DEV", "USER")
.build()
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Bean | |
| public UserDetailsService userDetailsService(PasswordEncoder encoder) { | |
| return new InMemoryUserDetailsManager( | |
| User.withUsername("admin") | |
| .password(encoder.encode("password")) | |
| .roles("ADMIN", "DEV", "USER") | |
| .build() | |
| ); | |
| } | |
| @Bean | |
| public UserDetailsService userDetailsService(PasswordEncoder encoder) { | |
| return new InMemoryUserDetailsManager( | |
| User.withUsername("admin") | |
| .password(encoder.encode("Dev@dmin2025!")) | |
| .roles("ADMIN", "DEV", "USER") | |
| .build() | |
| ); | |
| } |
| @Bean | |
| public UserDetailsService userDetailsService(PasswordEncoder encoder) { | |
| return new InMemoryUserDetailsManager( | |
| User.withUsername("admin") | |
| .password(encoder.encode("password")) | |
| .roles("ADMIN", "DEV", "USER") | |
| .build() | |
| ); | |
| } | |
| @Bean | |
| public UserDetailsService userDetailsService(PasswordEncoder encoder) { | |
| String devPassword = System.getenv().getOrDefault("DEV_ADMIN_PASSWORD", "ChangeMe123!"); | |
| return new InMemoryUserDetailsManager( | |
| User.withUsername("admin") | |
| .password(encoder.encode(devPassword)) | |
| .roles("ADMIN", "DEV", "USER") | |
| .build() | |
| ); | |
| } |
| public SecurityFilterChain prodSecurity(HttpSecurity http) throws Exception { | ||
| http | ||
| .csrf(csrf -> { }) | ||
| .httpBasic(Customizer.withDefaults()) | ||
| .authorizeHttpRequests(auth -> auth | ||
| .requestMatchers("/api/**").authenticated() | ||
| .anyRequest().denyAll() | ||
| ); | ||
|
|
||
| return http.build(); | ||
| } |
There was a problem hiding this comment.
Missing UserDetailsService in production profile.
Unlike SecurityConfigDev, this configuration lacks a UserDetailsService bean. With HTTP Basic authentication enabled, the application needs a user source to authenticate against. Without it, all authentication attempts will fail.
Consider adding a production-appropriate user store (database-backed, LDAP, etc.) or document that this is expected to be provided externally.
🤖 Prompt for AI Agents
In src/main/java/org/example/config/SecurityConfigProd.java around lines 19 to
29, the production SecurityFilterChain enables HTTP Basic but does not provide a
UserDetailsService (or other authentication user store), so authentication will
fail; add and wire a production-appropriate UserDetailsService bean (for example
a JdbcUserDetailsManager wired to your DataSource, or a custom
UserDetailsService that loads users from the production user store) and ensure
it is available to the AuthenticationManager used by this SecurityFilterChain
(inject it into the config or register it as a @Bean), or alternatively document
clearly that a UserDetailsService/LDAP provider must be supplied externally in
production. Ensure the bean is active only in the production profile or guarded
appropriately.
| @GetMapping("/admissions") | ||
| public String displayAdmissions(Model model) { | ||
| model.addAttribute("admissions", admissionRepository.findAll().stream() | ||
| .map(adm -> new AdmissionDTO(adm.getAdmission_id(), adm.getPatient().getPatId(), adm.getDateIn(), adm.getDateOut(), adm.getDiagnosis(), adm.getDepartment())) | ||
| .toList()); | ||
| return "admissions/admissions"; | ||
| } |
There was a problem hiding this comment.
Critical N+1 query problem detected.
Line 39 accesses adm.getPatient().getPatId() on lazily-fetched Patient entities within a stream over all admissions. This triggers one SELECT query per admission, resulting in N+1 database queries where N is the number of admissions.
Solution: Use a JOIN FETCH query in the repository to eagerly load the patient relationship.
🔎 Add this method to AdmissionRepository:
@Query("SELECT a FROM Admission a JOIN FETCH a.patient")
List<Admission> findAllWithPatient();Then update this controller method:
@GetMapping("/admissions")
public String displayAdmissions(Model model) {
- model.addAttribute("admissions", admissionRepository.findAll().stream()
+ model.addAttribute("admissions", admissionRepository.findAllWithPatient().stream()
.map(adm -> new AdmissionDTO(adm.getAdmission_id(), adm.getPatient().getPatId(), adm.getDateIn(), adm.getDateOut(), adm.getDiagnosis(), adm.getDepartment()))
.toList());
return "admissions/admissions";
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/org/example/controller/AdmissionsController.java around lines
36 to 42 there is an N+1 query: the stream calls adm.getPatient().getPatId() on
lazily-loaded Patient entities causing one query per admission; replace the call
to admissionRepository.findAll() with a new repository method that eagerly
fetches patients (add to AdmissionRepository a method using @Query("SELECT a
FROM Admission a JOIN FETCH a.patient") List<Admission> findAllWithPatient();
and import the necessary annotation), then update this controller to call
admissionRepository.findAllWithPatient() so Patient data is loaded in one query
before mapping to AdmissionDTO.
| @PostMapping("/api/admissions/new") | ||
| public String handleNew(@ModelAttribute NewAdmissionDTO newAdm) { | ||
|
|
||
| var pat = patientRepository.findBySsn(newAdm.patSsn()); | ||
| if (pat == null) { | ||
| throw new IllegalArgumentException("Patient with SSN " + newAdm.patSsn() + " not found"); | ||
| } else { | ||
| admissionRepository.save(new Admission(pat, newAdm.diagnosis(), newAdm.department(), LocalDateTime.now())); | ||
| } | ||
| return "redirect:/admissions"; | ||
| } |
There was a problem hiding this comment.
Improve error handling for better HTTP semantics.
Throwing IllegalArgumentException for a missing patient (line 57) results in a generic 500 error. Web APIs should return appropriate HTTP status codes like 404 for not found scenarios. Additionally, there's no validation on the incoming DTO fields.
🔎 Suggested improvements:
@PostMapping("/api/admissions/new")
public String handleNew(@ModelAttribute NewAdmissionDTO newAdm) {
-
+ // Add validation here if needed
var pat = patientRepository.findBySsn(newAdm.patSsn());
if (pat == null) {
- throw new IllegalArgumentException("Patient with SSN " + newAdm.patSsn() + " not found");
- } else {
- admissionRepository.save(new Admission(pat, newAdm.diagnosis(), newAdm.department(), LocalDateTime.now()));
+ // Consider using RedirectAttributes to pass error message
+ return "redirect:/admissions/new?error=patient_not_found";
}
+ admissionRepository.save(new Admission(pat, newAdm.diagnosis(), newAdm.department(), LocalDateTime.now()));
return "redirect:/admissions";
}Alternatively, for REST API endpoints, use ResponseEntity with proper status codes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @PostMapping("/api/admissions/new") | |
| public String handleNew(@ModelAttribute NewAdmissionDTO newAdm) { | |
| var pat = patientRepository.findBySsn(newAdm.patSsn()); | |
| if (pat == null) { | |
| throw new IllegalArgumentException("Patient with SSN " + newAdm.patSsn() + " not found"); | |
| } else { | |
| admissionRepository.save(new Admission(pat, newAdm.diagnosis(), newAdm.department(), LocalDateTime.now())); | |
| } | |
| return "redirect:/admissions"; | |
| } | |
| @PostMapping("/api/admissions/new") | |
| public String handleNew(@ModelAttribute NewAdmissionDTO newAdm) { | |
| // Add validation here if needed | |
| var pat = patientRepository.findBySsn(newAdm.patSsn()); | |
| if (pat == null) { | |
| // Consider using RedirectAttributes to pass error message | |
| return "redirect:/admissions/new?error=patient_not_found"; | |
| } | |
| admissionRepository.save(new Admission(pat, newAdm.diagnosis(), newAdm.department(), LocalDateTime.now())); | |
| return "redirect:/admissions"; | |
| } |
🤖 Prompt for AI Agents
In src/main/java/org/example/controller/AdmissionsController.java around lines
52 to 62, the controller throws IllegalArgumentException when a patient is
missing and does not validate the incoming DTO; change the behavior to return
proper HTTP semantics and validate input: accept the DTO as a @Valid parameter
and add a BindingResult to check for validation errors (return the form view or
a 400 response when invalid), and when the patient lookup returns null either
throw a ResponseStatusException(HttpStatus.NOT_FOUND, "...") or return a
ResponseEntity.notFound() to produce a 404 instead of a 500; ensure successful
saves still redirect to /admissions.
| @PostMapping("/api/admissions/{id}/writeOut") | ||
| public String writeOut(@PathVariable java.lang.Long id) { | ||
| Admission admission = admissionRepository.findById(id).orElseThrow(() -> new IllegalArgumentException("Admission not found")); | ||
| admission.setDateOut(LocalDateTime.now()); | ||
| admissionRepository.save(admission); | ||
| return "redirect:/admissions"; | ||
| } |
There was a problem hiding this comment.
Improve error handling with proper HTTP status codes.
Similar to the previous endpoint, throwing IllegalArgumentException (line 66) for a missing admission results in a 500 error instead of a more appropriate 404.
🔎 Consider this improvement:
@PostMapping("/api/admissions/{id}/writeOut")
public String writeOut(@PathVariable java.lang.Long id) {
- Admission admission = admissionRepository.findById(id).orElseThrow(() -> new IllegalArgumentException("Admission not found"));
+ Admission admission = admissionRepository.findById(id)
+ .orElseThrow(() -> new ResponseStatusException(HttpStatus.NOT_FOUND, "Admission not found"));
admission.setDateOut(LocalDateTime.now());
admissionRepository.save(admission);
return "redirect:/admissions";
}Note: This requires importing org.springframework.web.server.ResponseStatusException and org.springframework.http.HttpStatus.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @PostMapping("/api/admissions/{id}/writeOut") | |
| public String writeOut(@PathVariable java.lang.Long id) { | |
| Admission admission = admissionRepository.findById(id).orElseThrow(() -> new IllegalArgumentException("Admission not found")); | |
| admission.setDateOut(LocalDateTime.now()); | |
| admissionRepository.save(admission); | |
| return "redirect:/admissions"; | |
| } | |
| @PostMapping("/api/admissions/{id}/writeOut") | |
| public String writeOut(@PathVariable java.lang.Long id) { | |
| Admission admission = admissionRepository.findById(id) | |
| .orElseThrow(() -> new ResponseStatusException(HttpStatus.NOT_FOUND, "Admission not found")); | |
| admission.setDateOut(LocalDateTime.now()); | |
| admissionRepository.save(admission); | |
| return "redirect:/admissions"; | |
| } |
🤖 Prompt for AI Agents
In src/main/java/org/example/controller/AdmissionsController.java around lines
64 to 70, replace the IllegalArgumentException thrown when an admission is not
found with a ResponseStatusException carrying HttpStatus.NOT_FOUND so the
endpoint returns 404 instead of 500; add imports for
org.springframework.web.server.ResponseStatusException and
org.springframework.http.HttpStatus, change the orElseThrow to throw new
ResponseStatusException(HttpStatus.NOT_FOUND, "Admission not found"), and keep
the rest of the method (setting dateOut, saving, redirect) unchanged.
| @GetMapping("/api/admissions/{pat_id}") | ||
| public List<AdmissionDTO> findAllPatientsByPatId(@PathVariable java.lang.Long pat_id) { | ||
| return admissionRepository.getAdmissionsByPatId(pat_id).stream() | ||
| .map(adm -> new AdmissionDTO(adm.getAdmission_id(), adm.getPatient().getPatId(), adm.getDateIn(), adm.getDateOut(), adm.getDiagnosis(), adm.getDepartment())) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
Two critical issues: Missing @responsebody and N+1 query problem.
-
Missing @responsebody: This endpoint returns
List<AdmissionDTO>but lacks@ResponseBodyannotation, so Spring won't serialize it to JSON. It will attempt to resolve it as a view name, causing errors. -
N+1 Query: Line 75 accesses
adm.getPatient().getPatId()on lazily-fetched entities, triggering N+1 queries.
🔎 Apply these fixes:
@GetMapping("/api/admissions/{pat_id}")
+ @ResponseBody
public List<AdmissionDTO> findAllPatientsByPatId(@PathVariable java.lang.Long pat_id) {
- return admissionRepository.getAdmissionsByPatId(pat_id).stream()
+ return admissionRepository.findByPatIdWithPatient(pat_id).stream()
.map(adm -> new AdmissionDTO(adm.getAdmission_id(), adm.getPatient().getPatId(), adm.getDateIn(), adm.getDateOut(), adm.getDiagnosis(), adm.getDepartment()))
.toList();
}And add to AdmissionRepository:
@Query("SELECT a FROM Admission a JOIN FETCH a.patient WHERE a.patient.pat_id = :patId")
List<Admission> findByPatIdWithPatient(@Param("patId") Long patId);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/org/example/controller/AdmissionsController.java around lines
72 to 77, the endpoint is missing @ResponseBody (so Spring may treat the return
as a view) and it triggers an N+1 query by accessing adm.getPatient().getPatId()
on lazily-loaded patients; annotate the handler with @ResponseBody (or make the
controller a @RestController) and change the repository call to a new method
that fetches patients with the admissions using a JOIN FETCH (e.g., add a
repository method like findByPatIdWithPatient that uses "SELECT a FROM Admission
a JOIN FETCH a.patient WHERE a.patient.patId = :patId"), then call that method
here and map the results to AdmissionDTO as before so no extra queries occur.
| @PostMapping("/api/patients/new") | ||
| public String newPatient(@ModelAttribute Patient pat) { | ||
| patientRepository.save(pat); | ||
| return "redirect:/patients"; | ||
| } |
There was a problem hiding this comment.
Add validation for the patient entity before persisting.
The endpoint saves the patient without validating required fields. Although the Patient entity has @NotNull annotations, validation isn't triggered automatically without @Valid on the parameter.
🔎 Apply this diff to enable validation:
+import jakarta.validation.Valid;
+
@PostMapping("/api/patients/new")
- public String newPatient(@ModelAttribute Patient pat) {
+ public String newPatient(@Valid @ModelAttribute Patient pat, BindingResult bindingResult) {
+ if (bindingResult.hasErrors()) {
+ return "patients/newPatient";
+ }
patientRepository.save(pat);
return "redirect:/patients";
}Also import org.springframework.validation.BindingResult.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @PostMapping("/api/patients/new") | |
| public String newPatient(@ModelAttribute Patient pat) { | |
| patientRepository.save(pat); | |
| return "redirect:/patients"; | |
| } | |
| import jakarta.validation.Valid; | |
| import org.springframework.validation.BindingResult; | |
| @PostMapping("/api/patients/new") | |
| public String newPatient(@Valid @ModelAttribute Patient pat, BindingResult bindingResult) { | |
| if (bindingResult.hasErrors()) { | |
| return "patients/newPatient"; | |
| } | |
| patientRepository.save(pat); | |
| return "redirect:/patients"; | |
| } |
🤖 Prompt for AI Agents
In src/main/java/org/example/controller/PatientsController.java around lines 44
to 48, the controller saves a Patient without triggering bean validation; add
javax.validation.Valid to the @ModelAttribute parameter and add a
org.springframework.validation.BindingResult parameter immediately after it,
then check bindingResult.hasErrors() and return the patient form view if there
are validation errors; only call patientRepository.save(pat) and redirect when
there are no errors. Ensure you import
org.springframework.validation.BindingResult.
| @GetMapping("/api/patients/{id}") | ||
| public PatientDTO findPatientById(@PathVariable java.lang.Long id) { | ||
| var pat = patientRepository.findById(id); | ||
| if (pat.isEmpty()) { | ||
| throw new IllegalArgumentException("Patient with id " + id + " not found"); | ||
| } | ||
| Patient foundPat = pat.get(); | ||
| return new PatientDTO(foundPat.getFirstName(), foundPat.getLastName(), foundPat.getAddress(), foundPat.getDateOfBirth()); | ||
| } |
There was a problem hiding this comment.
Two critical issues: Missing @responsebody and poor error handling.
-
Missing @responsebody: This endpoint returns
PatientDTObut lacks@ResponseBodyannotation. Spring will attempt to resolve it as a view name instead of serializing to JSON. -
Improper error handling: Throwing
IllegalArgumentException(line 54) results in a generic 500 error instead of a proper 404 response.
🔎 Apply these fixes:
+import org.springframework.web.bind.annotation.ResponseBody;
+import org.springframework.web.server.ResponseStatusException;
+import org.springframework.http.HttpStatus;
+
@GetMapping("/api/patients/{id}")
+ @ResponseBody
public PatientDTO findPatientById(@PathVariable java.lang.Long id) {
- var pat = patientRepository.findById(id);
- if (pat.isEmpty()) {
- throw new IllegalArgumentException("Patient with id " + id + " not found");
- }
- Patient foundPat = pat.get();
+ Patient foundPat = patientRepository.findById(id)
+ .orElseThrow(() -> new ResponseStatusException(HttpStatus.NOT_FOUND, "Patient with id " + id + " not found"));
return new PatientDTO(foundPat.getFirstName(), foundPat.getLastName(), foundPat.getAddress(), foundPat.getDateOfBirth());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @GetMapping("/api/patients/{id}") | |
| public PatientDTO findPatientById(@PathVariable java.lang.Long id) { | |
| var pat = patientRepository.findById(id); | |
| if (pat.isEmpty()) { | |
| throw new IllegalArgumentException("Patient with id " + id + " not found"); | |
| } | |
| Patient foundPat = pat.get(); | |
| return new PatientDTO(foundPat.getFirstName(), foundPat.getLastName(), foundPat.getAddress(), foundPat.getDateOfBirth()); | |
| } | |
| import org.springframework.web.bind.annotation.ResponseBody; | |
| import org.springframework.web.server.ResponseStatusException; | |
| import org.springframework.http.HttpStatus; | |
| @GetMapping("/api/patients/{id}") | |
| @ResponseBody | |
| public PatientDTO findPatientById(@PathVariable java.lang.Long id) { | |
| Patient foundPat = patientRepository.findById(id) | |
| .orElseThrow(() -> new ResponseStatusException(HttpStatus.NOT_FOUND, "Patient with id " + id + " not found")); | |
| return new PatientDTO(foundPat.getFirstName(), foundPat.getLastName(), foundPat.getAddress(), foundPat.getDateOfBirth()); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/org/example/controller/PatientsController.java around lines 50
to 58, the controller method is missing @ResponseBody and throws
IllegalArgumentException which yields a 500; update the method to be a REST
response by adding the @ResponseBody annotation (or ensure the class is
@RestController) and replace the IllegalArgumentException with a
ResponseStatusException(HttpStatus.NOT_FOUND, "Patient with id " + id + " not
found") (and add the necessary import), so the endpoint returns JSON and a
proper 404 when the patient is absent.
nilsishome
left a comment
nilsishome
left a comment
There was a problem hiding this comment.
It's a really impressive project with an implemented website to boot!
The only thing I can find that hasn't been requested by CodeRabbit or JorgenLindstrom, is to maybe change PatientAdmissionRequest to PatientAdmissionRequestDTO for consistency.
You've done an excellent job nonetheless, great work! 🎉
4 participants
Summary by CodeRabbit
Release Notes
New Features
Infrastructure
✏️ Tip: You can customize this high-level summary in your review settings.