diff --git a/internal/routing/optimizer.go b/internal/routing/optimizer.go index 6eb8242..afac7cb 100644 --- a/internal/routing/optimizer.go +++ b/internal/routing/optimizer.go @@ -13,7 +13,6 @@ import ( "encoding/json" "fmt" "net/http" - "strconv" "time" "doormile/constants" @@ -28,7 +27,12 @@ import ( var BaseURL string const ( - optimizePath = "/api/v1/optimization/createdeliveries" + // Doormile's own endpoint on the Route Optimization API. It speaks + // Doormile's vocabulary (bookingid, pickuplatitude) rather than the + // provider one jupiter uses (deliveryid, pickuplat), validates its body, + // and returns properly typed numbers. The provider endpoint is left alone: + // jupiter is live on it. + optimizePath = "/api/v1/optimization/doormile/sequence" // Road-network sequencing is not instant — a real call for a handful of // stops took several seconds against Valhalla — but it must not hold a @@ -52,28 +56,42 @@ type stop struct { DeliveryLng float64 } -// The optimizer's field names are load-bearing and easy to get wrong: -// pickuplat/deliverylat, NOT pickuplatitude/deliverylatitude. Sending the wrong -// names does not error — it returns HTTP 200 with every coordinate defaulted to -// "0.0", no reordering, and all distances zero. Verified against the live -// service on 2026-08-11. Coordinates go as strings, which is what it expects. +// The Doormile endpoint takes real coordinates rather than the provider +// endpoint's strings, and rejects a body it cannot read instead of returning +// HTTP 200 with everything silently zeroed — which is what the provider +// endpoint does when the field names are wrong, and why this one exists. type optimizeRequestItem struct { - Deliveryid int `json:"deliveryid"` - Orderid string `json:"orderid"` - Pickuplat string `json:"pickuplat"` - Pickuplong string `json:"pickuplong"` - Deliverylat string `json:"deliverylat"` - Deliverylong string `json:"deliverylong"` + Bookingid int `json:"bookingid"` + Bookingno string `json:"bookingno,omitempty"` + Bookingassignmentid int `json:"bookingassignmentid,omitempty"` + Pickuplatitude float64 `json:"pickuplatitude"` + Pickuplongitude float64 `json:"pickuplongitude"` + Deliverylatitude float64 `json:"deliverylatitude"` + Deliverylongitude float64 `json:"deliverylongitude"` +} + +type optimizeRequest struct { + Tenantid int `json:"tenantid,omitempty"` + Mileruserid int `json:"mileruserid,omitempty"` + Bookings []optimizeRequestItem `json:"bookings"` +} + +type optimizeResponseStop struct { + Bookingid int `json:"bookingid"` + Bookingassignmentid int `json:"bookingassignmentid"` + Step int `json:"step"` + Previouskms float64 `json:"previouskms"` + Cumulativekms float64 `json:"cumulativekms"` + Etaminutes int `json:"etaminutes"` + Cumulativeeta int `json:"cumulativeeta"` } -// Numeric fields come back inconsistently typed — previouskms as a number, -// actualkms and eta as strings — so everything numeric is decoded loosely and -// coerced rather than bound to a concrete type. type optimizeResponse struct { - Code int `json:"code"` - Status bool `json:"status"` - Message string `json:"message"` - Details []map[string]interface{} `json:"details"` + Success bool `json:"success"` + Stopcount int `json:"stopcount"` + Totalkms float64 `json:"totalkms"` + Totaleta int `json:"totaleta"` + Stops []optimizeResponseStop `json:"stops"` } // Result is one sequenced stop, keyed back to the assignment it came from. @@ -189,18 +207,17 @@ func optimize(stops []stop) ([]Result, error) { items := make([]optimizeRequestItem, 0, len(stops)) for _, s := range stops { items = append(items, optimizeRequestItem{ - // deliveryid carries our assignment id out and back — it is the only - // field the optimizer echoes that we can key on. - Deliveryid: s.AssignmentID, - Orderid: s.BookingNo, - Pickuplat: coord(s.PickupLat), - Pickuplong: coord(s.PickupLng), - Deliverylat: coord(s.DeliveryLat), - Deliverylong: coord(s.DeliveryLng), + Bookingid: s.BookingID, + Bookingno: s.BookingNo, + Bookingassignmentid: s.AssignmentID, + Pickuplatitude: s.PickupLat, + Pickuplongitude: s.PickupLng, + Deliverylatitude: s.DeliveryLat, + Deliverylongitude: s.DeliveryLng, }) } - body, err := json.Marshal(items) + body, err := json.Marshal(optimizeRequest{Bookings: items}) if err != nil { return nil, fmt.Errorf("marshal stops: %w", err) } @@ -225,8 +242,8 @@ func optimize(stops []stop) ([]Result, error) { if err := json.NewDecoder(resp.Body).Decode(&out); err != nil { return nil, fmt.Errorf("decode optimizer response: %w", err) } - if !out.Status || len(out.Details) == 0 { - return nil, fmt.Errorf("optimizer reported failure: %s", out.Message) + if !out.Success || len(out.Stops) == 0 { + return nil, fmt.Errorf("optimizer returned no sequence") } known := make(map[int]struct{}, len(stops)) @@ -234,26 +251,25 @@ func optimize(stops []stop) ([]Result, error) { known[s.AssignmentID] = struct{}{} } - results := make([]Result, 0, len(out.Details)) - for _, d := range out.Details { - id := asInt(d["deliveryid"]) - if _, ok := known[id]; !ok { + results := make([]Result, 0, len(out.Stops)) + for _, d := range out.Stops { + if _, ok := known[d.Bookingassignmentid]; !ok { // Never write to an assignment we did not send. Without this an // echoed or stale id could reorder some other rider's work. - utils.Warn("routing: optimizer returned unknown deliveryid", "deliveryid", id) + utils.Warn("routing: optimizer returned unknown assignment", + "bookingassignmentid", d.Bookingassignmentid, "bookingid", d.Bookingid) continue } - step := asInt(d["step"]) - if step <= 0 { + if d.Step <= 0 { continue } results = append(results, Result{ - AssignmentID: id, - Step: step, - PreviousKM: asFloat(d["previouskms"]), - CumulativeKM: asFloat(d["cumulativekms"]), - ETAMinutes: asInt(d["eta"]), - CumulativeETA: asInt(d["cumulative_eta"]), + AssignmentID: d.Bookingassignmentid, + Step: d.Step, + PreviousKM: d.Previouskms, + CumulativeKM: d.Cumulativekms, + ETAMinutes: d.Etaminutes, + CumulativeETA: d.Cumulativeeta, }) } @@ -262,39 +278,3 @@ func optimize(stops []stop) ([]Result, error) { } return results, nil } - -// coord formats a coordinate the way the optimizer expects: a string, with -// enough precision to distinguish neighbouring addresses. -func coord(f float64) string { - return strconv.FormatFloat(f, 'f', 6, 64) -} - -func asFloat(v interface{}) float64 { - switch t := v.(type) { - case float64: - return t - case string: - f, err := strconv.ParseFloat(t, 64) - if err != nil { - return 0 - } - return f - } - return 0 -} - -func asInt(v interface{}) int { - switch t := v.(type) { - case float64: - return int(t) - case string: - // Some numeric fields arrive as strings, and a few of those are decimal - // ("20.0"), so parse as float and truncate rather than Atoi. - f, err := strconv.ParseFloat(t, 64) - if err != nil { - return 0 - } - return int(f) - } - return 0 -} diff --git a/internal/routing/optimizer_test.go b/internal/routing/optimizer_test.go index 9dfd2a2..b274174 100644 --- a/internal/routing/optimizer_test.go +++ b/internal/routing/optimizer_test.go @@ -1,68 +1,172 @@ package routing -import "testing" +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "testing" +) -// The optimizer returns the same logical field as a number in one place and a -// string in another — previouskms came back as 4, actualkms as "5.09", eta as -// "20". Binding those to concrete types would have silently zeroed half the -// response, so the coercion is worth pinning. -func TestAsFloat(t *testing.T) { +// stubOptimizer stands in for the Route Optimization API. It captures the +// request body so the outbound contract can be asserted, and returns whatever +// the test tells it to. +func stubOptimizer(t *testing.T, captured *optimizeRequest, respond func(optimizeRequest) optimizeResponse) *httptest.Server { + t.Helper() + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != optimizePath { + t.Errorf("posted to %s, want %s", r.URL.Path, optimizePath) + } + var req optimizeRequest + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + t.Errorf("stub could not decode request: %v", err) + } + if captured != nil { + *captured = req + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(respond(req)) + })) +} + +func testStops() []stop { + return []stop{ + {AssignmentID: 1, BookingID: 101, BookingNo: "A", PickupLat: 11.0045, PickupLng: 76.9612, DeliveryLat: 11.0510, DeliveryLng: 76.9300}, + {AssignmentID: 2, BookingID: 102, BookingNo: "B", PickupLat: 11.0045, PickupLng: 76.9612, DeliveryLat: 11.0168, DeliveryLng: 76.9558}, + {AssignmentID: 3, BookingID: 103, BookingNo: "C", PickupLat: 11.0045, PickupLng: 76.9612, DeliveryLat: 10.9938, DeliveryLng: 76.9954}, + } +} + +// The whole point of the Doormile endpoint is that it takes Doormile's field +// names. Sending the provider ones (pickuplat/deliverylat) against the provider +// endpoint returns HTTP 200 with everything silently zeroed, so this contract is +// worth pinning rather than trusting. +func TestOptimizeSendsDoormileFieldNames(t *testing.T) { + var got optimizeRequest + srv := stubOptimizer(t, &got, func(req optimizeRequest) optimizeResponse { + return optimizeResponse{Success: true, Stops: []optimizeResponseStop{ + {Bookingassignmentid: 1, Bookingid: 101, Step: 1}, + }} + }) + defer srv.Close() + BaseURL = srv.URL + defer func() { BaseURL = "" }() + + if _, err := optimize(testStops()); err != nil { + t.Fatalf("optimize: %v", err) + } + + if len(got.Bookings) != 3 { + t.Fatalf("sent %d bookings, want 3", len(got.Bookings)) + } + first := got.Bookings[0] + if first.Bookingid != 101 || first.Bookingassignmentid != 1 { + t.Errorf("identity fields wrong: %+v", first) + } + // Coordinates must survive as real numbers, not be rounded or stringified + // into a different place. + if first.Pickuplatitude != 11.0045 || first.Deliverylongitude != 76.93 { + t.Errorf("coordinates mangled: %+v", first) + } +} + +func TestOptimizeMapsResultsBackToAssignments(t *testing.T) { + srv := stubOptimizer(t, nil, func(req optimizeRequest) optimizeResponse { + // Reverse the order, as a real resequencing would. + return optimizeResponse{Success: true, Stops: []optimizeResponseStop{ + {Bookingassignmentid: 3, Bookingid: 103, Step: 1, Previouskms: 4, Cumulativekms: 4, Etaminutes: 14, Cumulativeeta: 14}, + {Bookingassignmentid: 2, Bookingid: 102, Step: 2, Previouskms: 5, Cumulativekms: 9, Etaminutes: 8, Cumulativeeta: 22}, + {Bookingassignmentid: 1, Bookingid: 101, Step: 3, Previouskms: 5, Cumulativekms: 14, Etaminutes: 8, Cumulativeeta: 30}, + }} + }) + defer srv.Close() + BaseURL = srv.URL + defer func() { BaseURL = "" }() + + results, err := optimize(testStops()) + if err != nil { + t.Fatalf("optimize: %v", err) + } + if len(results) != 3 { + t.Fatalf("got %d results, want 3", len(results)) + } + if results[0].AssignmentID != 3 || results[0].Step != 1 || results[0].CumulativeETA != 14 { + t.Errorf("first result wrong: %+v", results[0]) + } + if results[2].AssignmentID != 1 || results[2].CumulativeKM != 14 { + t.Errorf("last result wrong: %+v", results[2]) + } +} + +// These steps get written straight onto assignment rows, so a step for an +// assignment we never sent must never make it through — it would reorder some +// other rider's work. +func TestOptimizeDiscardsUnknownAssignments(t *testing.T) { + srv := stubOptimizer(t, nil, func(req optimizeRequest) optimizeResponse { + return optimizeResponse{Success: true, Stops: []optimizeResponseStop{ + {Bookingassignmentid: 999, Bookingid: 999, Step: 1}, + {Bookingassignmentid: 2, Bookingid: 102, Step: 2, Cumulativekms: 9}, + }} + }) + defer srv.Close() + BaseURL = srv.URL + defer func() { BaseURL = "" }() + + results, err := optimize(testStops()) + if err != nil { + t.Fatalf("optimize: %v", err) + } + if len(results) != 1 || results[0].AssignmentID != 2 { + t.Fatalf("unknown assignment leaked through: %+v", results) + } +} + +// Step 0 means "not sequenced". Persisting it would read as a position. +func TestOptimizeDropsZeroSteps(t *testing.T) { + srv := stubOptimizer(t, nil, func(req optimizeRequest) optimizeResponse { + return optimizeResponse{Success: true, Stops: []optimizeResponseStop{ + {Bookingassignmentid: 1, Bookingid: 101, Step: 0}, + {Bookingassignmentid: 2, Bookingid: 102, Step: 1}, + }} + }) + defer srv.Close() + BaseURL = srv.URL + defer func() { BaseURL = "" }() + + results, err := optimize(testStops()) + if err != nil { + t.Fatalf("optimize: %v", err) + } + if len(results) != 1 || results[0].AssignmentID != 2 { + t.Fatalf("step 0 was kept: %+v", results) + } +} + +func TestOptimizeErrorsAreSurfacedNotSilent(t *testing.T) { cases := []struct { - name string - in interface{} - want float64 + name string + handler http.HandlerFunc }{ - {"number", float64(4), 4}, - {"decimal string", "5.09", 5.09}, - {"integer string", "14", 14}, - {"zero string the API sends for missing coords", "0.0", 0}, - {"nil", nil, 0}, - {"unparseable", "n/a", 0}, - {"wrong type", true, 0}, + {"http 500", func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(500) }}, + {"success false", func(w http.ResponseWriter, r *http.Request) { + _ = json.NewEncoder(w).Encode(optimizeResponse{Success: false}) + }}, + {"empty stops", func(w http.ResponseWriter, r *http.Request) { + _ = json.NewEncoder(w).Encode(optimizeResponse{Success: true, Stops: nil}) + }}, + {"garbage body", func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte("not json")) + }}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - if got := asFloat(tc.in); got != tc.want { - t.Fatalf("asFloat(%#v) = %v, want %v", tc.in, got, tc.want) + srv := httptest.NewServer(tc.handler) + defer srv.Close() + BaseURL = srv.URL + defer func() { BaseURL = "" }() + + if _, err := optimize(testStops()); err == nil { + t.Fatal("expected an error, got nil — a failed optimise must not look like success") } }) } } - -func TestAsInt(t *testing.T) { - cases := []struct { - name string - in interface{} - want int - }{ - {"number", float64(3), 3}, - {"integer string", "20", 20}, - // Atoi would fail on this and yield 0, which as a step number would - // silently drop the stop from the sequence. - {"decimal string", "20.0", 20}, - {"truncates rather than rounds", "20.9", 20}, - {"nil", nil, 0}, - {"unparseable", "", 0}, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - if got := asInt(tc.in); got != tc.want { - t.Fatalf("asInt(%#v) = %v, want %v", tc.in, got, tc.want) - } - }) - } -} - -// Coordinates must go out as strings with real precision. Formatting with too -// few decimals would collapse neighbouring delivery addresses onto the same -// point and make the ordering meaningless. -func TestCoordKeepsPrecision(t *testing.T) { - if got := coord(11.0045); got != "11.004500" { - t.Fatalf("coord(11.0045) = %q", got) - } - // ~11m apart in Coimbatore; these must not format identically. - a, b := coord(11.004500), coord(11.004600) - if a == b { - t.Fatalf("distinct coordinates formatted identically: %q", a) - } -}