Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
One or more issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
Updates the vehicle-routing guide to use the service module, maps enrichment, asynchronous REST APIs, and incremental shadow variables.
Changes:
- Reworks the domain model and constraints for medium-score unassigned visits.
- Adds DTOs, model conversion, REST configuration, and service testing guidance.
- Updates the quickstart source link and service-module Maven setup.
| File | Description |
|---|---|
| docs/src/modules/ROOT/pages/quickstart/quarkus-vehicle-routing/vehicle-routing-solution.adoc | Updated as part of this pull request. |
| docs/src/modules/ROOT/pages/quickstart/quarkus-vehicle-routing/vehicle-routing-model.adoc | Updated as part of this pull request. |
| docs/src/modules/ROOT/pages/quickstart/quarkus-vehicle-routing/vehicle-routing-constraints.adoc | Updated as part of this pull request. |
| docs/src/modules/ROOT/pages/quickstart/quarkus-vehicle-routing/vehicle-routing-api.adoc | Updated as part of this pull request. |
| docs/src/modules/ROOT/pages/quickstart/quarkus-vehicle-routing/quarkus-vehicle-routing-quickstart.adoc | Updated as part of this pull request. |
| docs/src/modules/ROOT/pages/_attributes.adoc | Updated as part of this pull request. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Change handling of duplicate visit IDs in visitMap collection. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Clarified the description of the 'enable-fallback' option to specify that it applies when the remote service is unavailable. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
8005ece to
785b3c0
Compare
|
@rsynek, update this so it fixes all merge issues. |
There was a problem hiding this comment.
🔵 Needs a closer look
A moderate active finding remains because the match weight optimizes assigned service duration rather than the documented goal of maximizing the number of assigned visits.
No open issues 🎉
2 resolved since last review
Review effort: Lite
| public double getNearbyDistance(Visit origin, LocationAware destination) { | ||
| return origin.getLocation().getTravelTimeTo(destination.getLocation()).seconds(); |
| return 0; | ||
| } | ||
| return cumulativeDrivingTime + lastVisit.getLocation().getDrivingTimeTo(homeLocation); | ||
| return cumulativeDrivingTime + lastVisit.getLocation().getTravelTimeTo(homeLocation).seconds(); |
| @@ -149,22 +111,26 @@ Specifically, these classes are the input of the problem: | |||
| * The `vehicles` field with all vehicles | |||
| ** This is a list of planning entities, because they change during solving. | |||
| } | ||
|
|
||
| private static Location toLocation(LocationInputDTO dto) { | ||
| return new Location(dto.latitude(), dto.longitude()); |
rsynek
left a comment
There was a problem hiding this comment.
Nicely written! Please see my question and comments inline; some parts may need a bit of polishing.
|
|
||
| @QuarkusTest | ||
| class VehicleRoutingConstraintProviderTest { | ||
| class VehicleRoutePlanConstraintProviderTest { |
There was a problem hiding this comment.
The test content looks very different from what we have in the qs/use-cases/vehicle-routing. It's it a bit confusing for the reader?
| .body(vehicleRoutePlan) | ||
| .expect().contentType(ContentType.TEXT) | ||
| .when().post("/route-plans") | ||
| .body(mapper.writeValueAsString(Map.of("modelInput", input))) |
There was a problem hiding this comment.
No need to go for random object serialized to JSON. We have the ModelRequest in the solver-service that acts as the top-level DTO and contains the modelInput field.
| === Generate the initial OpenAPI specification | ||
|
|
||
| This test verifies that after solving that it found a feasible solution (no hard constraints broken). | ||
| Before running a full build with `mvn install`, generate the initial OpenAPI specification file. |
There was a problem hiding this comment.
This has been already improved and if the file is missing, the maven build generates it. Just checked that with the QS. Feel free to try for yourself.
| return 0; | ||
| } | ||
|
|
||
| Visit lastVisit = visits.get(visits.size() - 1); |
There was a problem hiding this comment.
Strictly speaking, this is a dangerous pattern to teach our users. If the related constraint starts from a vehicle shift and does not join the last visit, there is a risk of a score corruption, since Bavet won't propagate the last visit's change to this constraint.
Not introduced to this PR, so not in scope to fix it here. Also, we have it in the QS for a while already.
| } | ||
|
|
||
| protected Constraint vehicleCapacity(ConstraintFactory factory) { | ||
| public Constraint vehicleCapacity(ConstraintFactory factory) { |
There was a problem hiding this comment.
Nitpick: the way the constraint is written allows temporary inconsistencies:
vehicle capacity = 10, visit A demands 12, the next visit B demands -4, which leads to no penalty.
| constructor() | ||
| // Every solve builds its own one-off matrix rather than reusing a named, pre-built one. | ||
| @Override | ||
| public Optional<String> getLocationSetName() { |
There was a problem hiding this comment.
Can be removed, as the interface already comes with a default impl.
| import org.acme.vehiclerouting.domain.Location; | ||
| * `getLocations()` returns every location the matrix needs to cover: | ||
| every vehicle's home location plus every visit's location. | ||
| * `getLocationSetName()` returns empty, so each solve builds its own one-off matrix |
There was a problem hiding this comment.
See my above comment; perhaps not worth even mentioning the method here.
| ---- | ||
| -- | ||
| ==== No newline at end of file | ||
| Running locally, you use the local computation. |
There was a problem hiding this comment.
Should we at least mention that locally we use the Haversine with a fixed speed factor?
| ---- | ||
| package org.acme.vehiclerouting.dto.input; | ||
|
|
||
| public record LocationInputDTO(Double latitude, Double longitude) { |
There was a problem hiding this comment.
Any particular reason to duplicate the original Location? It's JSON-serializable and used across our models.



Updated the vehicle routing use case guide to use the new service module. We don't describe everything in detail yet, but at least it mentions the maps service and the updated structure (to better lean into incremental) which I'm also fixing in TimefoldAI/timefold-quickstarts#1218.