chore: update bed allocation to use service module - #1166
Conversation
There was a problem hiding this comment.
Pull request overview
Migrates the bed-allocation quickstart from custom REST endpoints and mutable domain JSON models to the Timefold Service Module (ModelRest, DTO-based input/output, model convertor + validator), aligning it with the newer /v1/* API and platform-embed behavior used by other quickstarts.
Changes:
- Replace legacy REST resources/demo generator and their tests with Service Module ModelRest usage, including new integration tests for
/v1/schedules. - Introduce DTOs, model convertor, validator, and issue types to support dataset validation and solver model mapping.
- Update UI assets (index.html/app.js) and configuration (application.properties/pom.xml) to match Service Module conventions and platform embedding.
Reviewed changes
Copilot reviewed 57 out of 57 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| use-cases/bed-allocation/src/test/java/org/acme/bedallocation/support/TestRoomBuilder.java | Adds a test-only builder for Room record instances. |
| use-cases/bed-allocation/src/test/java/org/acme/bedallocation/support/TestDepartmentBuilder.java | Adds a test-only builder for Department record instances. |
| use-cases/bed-allocation/src/test/java/org/acme/bedallocation/support/TestBedBuilder.java | Adds a test-only builder for Bed record instances. |
| use-cases/bed-allocation/src/test/java/org/acme/bedallocation/support/BedPlanTestDataFactory.java | Adds shared DTO-based test data factory for solver/validator tests. |
| use-cases/bed-allocation/src/test/java/org/acme/bedallocation/solver/SolverManagerTest.java | Adds solver test using Service Module convertor + SolverManager. |
| use-cases/bed-allocation/src/test/java/org/acme/bedallocation/solver/BedPlanEnvironmentTest.java | Refactors environment test away from REST calls to use convertor + demo factory. |
| use-cases/bed-allocation/src/test/java/org/acme/bedallocation/solver/BedAllocationConstraintProviderTest.java | Refactors constraint tests to use new test builders and record-based domain. |
| use-cases/bed-allocation/src/test/java/org/acme/bedallocation/service/BedPlanValidatorTest.java | Adds unit tests for new input validation issues. |
| use-cases/bed-allocation/src/test/java/org/acme/bedallocation/rest/BedSchedulingResourceTest.java | Removes legacy REST resource test (old endpoints). |
| use-cases/bed-allocation/src/test/java/org/acme/bedallocation/rest/BedSchedulingResourceIT.java | Removes legacy native IT for old endpoints. |
| use-cases/bed-allocation/src/test/java/org/acme/bedallocation/integrationtest/BedPlanResourceIT.java | Adds new native IT for Service Module /v1/schedules flow. |
| use-cases/bed-allocation/src/test/java/org/acme/bedallocation/demo/DemoDataBuilderTest.java | Adds test coverage for the new DTO-based demo data builder. |
| use-cases/bed-allocation/src/main/resources/META-INF/resources/index.html | Updates UI guide and assets for /v1/* endpoints + platform embedding + CDN assets. |
| use-cases/bed-allocation/src/main/resources/META-INF/resources/app.js | Updates UI client logic to Service Module model endpoints and platform-run embedding. |
| use-cases/bed-allocation/src/main/resources/application.properties | Switches configuration to Service Module properties and adds model/application metadata. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/support/ObjectHelper.java | Adds list normalization helper for DTO record constructors. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/solver/BedAllocationConstraintProvider.java | Aligns constraints with new naming constants and record accessors. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/service/validation/StayIdMissingIssue.java | Adds validation issue type for missing stay IDs. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/service/validation/RoomIdMissingIssue.java | Adds validation issue type for missing room IDs. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/service/validation/NonExistingBedReferenceIssue.java | Adds validation issue type for unknown bed references. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/service/validation/DuplicateStayIdIssue.java | Adds validation issue type for duplicate stay IDs. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/service/validation/DuplicateRoomIdIssue.java | Adds validation issue type for duplicate room IDs. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/service/validation/DuplicateDepartmentIdIssue.java | Adds validation issue type for duplicate department IDs. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/service/validation/DuplicateBedIdIssue.java | Adds validation issue type for duplicate bed IDs. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/service/validation/DepartmentIdMissingIssue.java | Adds validation issue type for missing department IDs. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/service/validation/BedPlanIssue.java | Adds base issue type and OpenAPI polymorphic schema for validation issues. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/service/validation/BedIdMissingIssue.java | Adds validation issue type for missing bed IDs. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/service/BedPlanValidator.java | Implements dataset validation for IDs/duplicates/bed references. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/service/BedPlanModelConvertor.java | Implements conversion between DTO input/output and solver domain model + overrides. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/rest/exception/ScheduleSolverExceptionMapper.java | Removes legacy exception mapper tied to old REST endpoints. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/rest/exception/ScheduleSolverException.java | Removes legacy exception type tied to old REST endpoints. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/rest/exception/ErrorInfo.java | Removes legacy error payload for old REST endpoints. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/rest/DemoDataGenerator.java | Removes legacy demo data generator for old REST endpoints. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/rest/BedSchedulingResource.java | Removes legacy REST resource implementation in favor of Service Module. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/rest/BedSchedulingDemoResource.java | Removes legacy demo REST endpoint (/demo-data). |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/rest/BedPlanResource.java | Adds Service Module ModelRest interface for schedules. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/dto/StayDTO.java | Adds DTO definition for stays, with normalization and builder helpers. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/dto/RoomDTO.java | Adds DTO definition for rooms with list normalization. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/dto/DepartmentDTO.java | Adds DTO definition for departments with builder and normalization. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/dto/BedPlanOutputMetrics.java | Adds output metrics for Service Module metadata/UI. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/dto/BedPlanOutput.java | Adds model output DTO returned by Service Module. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/dto/BedPlanInputMetrics.java | Adds input metrics for Service Module metadata/UI. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/dto/BedPlanInput.java | Adds model input DTO used by Service Module. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/dto/BedPlanConfigOverrides.java | Adds config override DTO for constraint weights via Service Module. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/dto/BedDTO.java | Adds DTO definition for beds. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/domain/Stay.java | Refactors domain entity to support new constructors, record-based navigation, and equality. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/domain/Room.java | Converts Room to a record and updates equality/toString. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/domain/GenderLimitation.java | Simplifies enum to name-only values (no code mapping). |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/domain/Gender.java | Simplifies enum to name-only values (no code mapping). |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/domain/Department.java | Converts Department to a record and removes legacy mutators/logic. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/domain/BedPlanConstraintProperties.java | Centralizes constraint names for Service Module constraint references. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/domain/BedPlan.java | Updates planning solution to Service Module SolverModel + adds metrics + overrides. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/domain/Bed.java | Converts Bed to a record and updates equality/toString. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/demo/DemoDataGenerator.java | Adds Service Module demo data generator producing ModelRequest. |
| use-cases/bed-allocation/src/main/java/org/acme/bedallocation/demo/DemoDataBuilder.java | Adds seeded, DTO-based demo dataset builder replacing legacy domain generator. |
| use-cases/bed-allocation/README.md | Updates README shell snippets formatting (drops $ prompts). |
| use-cases/bed-allocation/pom.xml | Switches module to timefold-solver-service-parent and simplifies build config. |
Suppressed comments (1)
use-cases/bed-allocation/src/main/java/org/acme/bedallocation/solver/BedAllocationConstraintProvider.java:153
- preferredPatientEquipment() can throw a NullPointerException when a Stay is unassigned (bed == null), since it calls st.getRoom().equipments() in both the filter and the penalty. The other soft constraints avoid this by using null-safe accessors (e.g. getRoomCapacity()).
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
448caf7 to
33abe39
Compare
b5172ab to
6d639a9
Compare
| headers: { | ||
| 'Content-Type': 'application/json', 'Accept': 'application/json,text/plain', // plain text is required by solve() returning UUID of the solver job | ||
| 'Content-Type': 'application/json', | ||
| 'Accept': 'application/json,text/plain', | ||
| ...(PLATFORM.apiKey ? {'X-API-KEY': PLATFORM.apiKey} : {}) | ||
| } |
There was a problem hiding this comment.
That is ok for now.
| @Schema(description = "ID of the bed this stay is assigned to, or null if unassigned.") String bedId) { | ||
|
|
||
| public StayDTO withBedId(String bedId) { | ||
| return new StayDTO(id, patientName, patientGender, patientAge, patientPreferredMaximumRoomCapacity, | ||
| patientRequiredEquipments, patientPreferredEquipments, arrivalDate, departureDate, specialty, bedId); | ||
| } | ||
| } |
There was a problem hiding this comment.
Will not fix in DTO, but in the converter.
| @Schema(description = "Beds in this room.", required = true) List<BedDTO> beds) { | ||
| } |
| @Schema(description = "Rooms belonging to this department.", required = true) List<RoomDTO> rooms) { | ||
| } |
triceo
left a comment
There was a problem hiding this comment.
I am leaving some comments around things which we may want to discuss. That said, I consider this to be your call - if you decide to reject all of them outright, I will not block this PR.
(Except maybe for the AssertJ/JUnit thing. That is just confusing.)
| private static final String OXYGEN = "oxygen"; | ||
| private static final String NITROGEN = "nitrogen"; | ||
|
|
||
| private static final String CARDIOLOGY = "Cardiology"; |
There was a problem hiding this comment.
Food for thought: plain Strings carry no meaning.
new Department("Cardiology") can be treated as a problem fact, easily included in constraints etc.
Not saying it has to happen in this PR. But if we're now trying to show people good patterns, IMO this is one of the best. No plain integers, no plain strings - we are Java, we like objects, objects bring semantics.
| // Ship no constraint weight overrides in the demo input, so that any overrides coming from the | ||
| // configuration profile are applied instead of being masked. Callers that want to override | ||
| // specific weights via the input can build a BedPlanConfigOverrides and set only those. | ||
| Configuration<BedPlanConfigOverrides> configuration = new Configuration<>( |
There was a problem hiding this comment.
Do we have an opinion on var?
I see 4 mentions of the word "configuration" on a single line.
| implements | ||
| BedPlanJustification { | ||
|
|
||
| public static PreferredMaximumRoomCapacityJustification of(Stay stay) { |
There was a problem hiding this comment.
Since we already have this constructor method, why not have description as a justification argument, and put it there in this method? That would avoid 5 lines of getDescription() in each justification.
There was a problem hiding this comment.
But then we wouldn't have the individual fields anymore in the API response, which is useful to render certain things.
|
|
||
| @PlanningSolution | ||
| public class BedPlan { | ||
| public class BedPlan implements SolverModel<HardMediumSoftScore>, |
There was a problem hiding this comment.
What is our opinion on final classes?
| import org.acme.bedallocation.domain.Gender; | ||
| import org.acme.bedallocation.domain.GenderLimitation; | ||
| import org.acme.bedallocation.domain.Stay; | ||
| import org.acme.bedallocation.domain.justification.BedPlanJustification.DepartmentMaximumAgeJustification; |
There was a problem hiding this comment.
How do we feel about asterisk imports?
(Is that even doable in this case?)
| void shouldBuildData() { | ||
| BedPlanInput problem = DemoDataBuilder.builder().build(); | ||
|
|
||
| assertEquals(1, problem.departments().size()); |
There was a problem hiding this comment.
Strange mixing of AssertJ and JUnit assertions. I would prefer if we sticked to one - and since we already have AssertJ here, stick to AssertJ.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 49 changed files in this pull request and generated 1 comment.
Suppressed comments (6)
Previously missed (2) — in code that hasn't changed since the last review.
use-cases/conference-scheduling/src/main/resources/META-INF/resources/app.js:10
- PLATFORM no longer exposes apiKey (it was removed here), but setupAjax() still conditionally sends an X-API-KEY header based on PLATFORM.apiKey. This makes API key support ambiguous and relies on an undefined property. Either keep apiKey in the platform context or remove the header logic.
use-cases/bed-allocation/src/main/java/org/acme/bedallocation/domain/Stay.java:44 - The Stay constructor stores patientRequiredEquipments/patientPreferredEquipments as-is; callers (including tests/builders) can pass null, which later causes constraints like requiredPatientEquipment/preferredPatientEquipment to throw NullPointerException. Default these to empty lists (and preferably defensive-copy them).
This issue also appears on line 61 of the same file.
this.patientPreferredMaximumRoomCapacity = patientPreferredMaximumRoomCapacity;
this.patientRequiredEquipments = patientRequiredEquipments;
this.patientPreferredEquipments = patientPreferredEquipments;
use-cases/bed-allocation/src/main/java/org/acme/bedallocation/domain/Stay.java:65
- hasDepartmentSpecialty()/getSpecialtyPriority() call getDepartment() without guarding against unassigned stays (bed == null) and missing specialty, which can throw during constraint evaluation when allowsUnassigned=true. Make these methods null-safe.
public boolean hasDepartmentSpecialty() {
return getDepartment().specialtyToPriority().containsKey(specialty);
}
public int getSpecialtyPriority() {
use-cases/bed-allocation/src/main/java/org/acme/bedallocation/dto/RoomDTO.java:16
- RoomDTO.equipments is optional in the schema and can be null when deserialized from JSON. Downstream code (e.g., BedPlanModelConvertor uses Set.copyOf(roomDto.equipments())) will throw NullPointerException if it's null. Add a compact constructor to default null collections to empty (and defensive-copy them).
@Schema(description = "Medical equipment available in this room.") Set<String> equipments,
@Schema(description = "Beds in this room.", required = true) List<BedDTO> beds) {
use-cases/bed-allocation/src/main/java/org/acme/bedallocation/dto/DepartmentDTO.java:15
- DepartmentDTO.specialtyToPriority is not marked required and may deserialize as null. BedPlanModelConvertor currently does new HashMap<>(departmentDto.specialtyToPriority()), which will throw if the map is null. Add a compact constructor to default null collections to empty (and defensive-copy them).
@Schema(description = "Priority (1 is highest) of each specialty treated by this department.") Map<String, Integer> specialtyToPriority,
@Schema(description = "Rooms belonging to this department.", required = true) List<RoomDTO> rooms) {
use-cases/bed-allocation/src/main/java/org/acme/bedallocation/dto/StayDTO.java:15
- StayDTO.patientRequiredEquipments/patientPreferredEquipments are optional in the schema and can be null when deserialized from JSON. BedPlanModelConvertor currently does new ArrayList<>(dto.patientRequiredEquipments()), which will throw on null. Add a compact constructor to default these to empty lists (and defensive-copy them).
@Schema(description = "Medical equipment required by the patient.") List<String> patientRequiredEquipments,
@Schema(description = "Medical equipment preferred by the patient.") List<String> patientPreferredEquipments,
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 49 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
use-cases/bed-allocation/src/test/java/org/acme/bedallocation/support/TestHelper.java:228
- StayBuilder setters allow explicitly setting equipment lists to null, reintroducing the same null-collection problem during constraint evaluation. It’s safer to normalize null to an empty list in the builder.
| for (RoomDTO room : department.rooms()) { | ||
| if (room.id() == null || room.id().isBlank()) { | ||
| validationBuilder.addIssue(new RoomIdMissingIssue()); | ||
| } else if (!roomIds.add(room.id())) { | ||
| validationBuilder.addIssue(new DuplicateRoomIdIssue(room.id())); | ||
| // Same reasoning as above, one level down: skip this duplicate room's beds. | ||
| continue; | ||
| } | ||
| for (BedDTO bed : room.beds()) { | ||
| if (bed.id() == null || bed.id().isBlank()) { | ||
| validationBuilder.addIssue(new BedIdMissingIssue()); | ||
| } else if (!bedIds.add(bed.id())) { | ||
| validationBuilder.addIssue(new DuplicateBedIdIssue(bed.id())); | ||
| } | ||
| } | ||
| } |
| private Gender patientGender; | ||
| private int patientAge; | ||
| private Integer patientPreferredMaximumRoomCapacity; | ||
| private List<String> patientRequiredEquipments; | ||
| private List<String> patientPreferredEquipments; | ||
|
|
Description of the change
Update the bed allocation use case to the service module
Checklist
Development
Code Review