Skip to content

Commit a23e1d8

Browse files
aaronbrethorstaaronbrethorst
authored
Merge pull request #3 from OneBusAway/segfault
Fix segfault: guard nil OBA SDK responses (null JSON body)
2 parents 1c08c69 + d0e0348 commit a23e1d8

11 files changed

Lines changed: 360 additions & 290 deletions

‎validator/check_agencies.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ func (agencyUnionCheck) Name() string { return "agency-union" }
1212
func (agencyUnionCheck) Run(ctx context.Context, vc *ValidationContext) []Result {
1313
const name = "agency-union"
1414
if vc.AgenciesErr != nil || vc.Agencies == nil {
15-
return []Result{{Check: name, Status: Fail, Message: "agencies-with-coverage unavailable: " + redact(vc.AgenciesErr, vc.Config.APIKey)}}
15+
return []Result{{Check: name, Status: Fail, Message: withReason("agencies-with-coverage unavailable", vc.AgenciesErr, vc.Config.APIKey)}}
1616
}
1717

1818
apiSet := map[string]bool{}

‎validator/check_alerts.go‎

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -48,10 +48,9 @@ func (serviceAlertCheck) Run(ctx context.Context, vc *ValidationContext, src *So
4848
var out []Result
4949
for _, s := range sample {
5050
obaStop := PrefixedID(agency, s.rawStop)
51-
ad, err := vc.Client.ArrivalAndDeparture.List(ctx, obaStop, onebusaway.ArrivalAndDepartureListParams{})
52-
if err != nil {
53-
out = append(out, Result{Check: name, Source: src.Label, Status: Warn,
54-
Message: fmt.Sprintf("could not query stop %q (agency prefix may be wrong): %s", obaStop, redact(err, key))})
51+
ad, bad := queryArrivals(ctx, vc, name, src.Label, obaStop)
52+
if bad != nil {
53+
out = append(out, *bad)
5554
continue
5655
}
5756
anySituation := false
@@ -86,3 +85,22 @@ func (serviceAlertCheck) Run(ctx context.Context, vc *ValidationContext, src *So
8685
}
8786
return out
8887
}
88+
89+
// queryArrivals fetches arrivals-and-departures for a stop. It returns a non-nil
90+
// *Result — a Warn the caller should record before skipping the stop — when the
91+
// call errors or the server returns a null body, so the two cross-reference
92+
// checks (alerts, trip-updates) share identical "couldn't read this stop"
93+
// handling. A null body must never be read as "stop confirmed empty".
94+
func queryArrivals(ctx context.Context, vc *ValidationContext, check, label, obaStop string) (*onebusaway.ArrivalAndDepartureListResponse, *Result) {
95+
ad, err := vc.Client.ArrivalAndDeparture.List(ctx, obaStop, onebusaway.ArrivalAndDepartureListParams{})
96+
switch {
97+
case err != nil:
98+
return nil, &Result{Check: check, Source: label, Status: Warn,
99+
Message: fmt.Sprintf("could not query stop %q (agency prefix may be wrong): %s", obaStop, redact(err, vc.Config.APIKey))}
100+
case ad == nil:
101+
return nil, &Result{Check: check, Source: label, Status: Warn,
102+
Message: fmt.Sprintf("arrivals query for stop %q returned a null response", obaStop),
103+
Details: map[string]any{"stopId": obaStop}}
104+
}
105+
return ad, nil
106+
}

‎validator/check_alerts_test.go‎

Lines changed: 28 additions & 112 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ package validator
33
import (
44
"context"
55
"net/http"
6-
"strings"
76
"testing"
87

98
gtfs "github.com/OneBusAway/go-gtfs"
@@ -12,29 +11,19 @@ import (
1211
)
1312

1413
func TestServiceAlertFoundInSituationIDs(t *testing.T) {
15-
client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
16-
if strings.Contains(r.URL.Path, "arrivals-and-departures-for-stop") {
17-
w.Header().Set("Content-Type", "application/json")
18-
w.Write([]byte(`{"data":{"entry":{"arrivalsAndDepartures":[{"stopId":"1_ST1","tripId":"1_T1","situationIds":["1_ALERT1"]}]}}}`))
19-
return
20-
}
21-
t.Errorf("unexpected path %s", r.URL.Path)
22-
})
23-
src := &SourceContext{
24-
Label: "ds0",
25-
Config: config.DataSource{AgencyMapping: map[string]string{"KCM": "1"}},
26-
PrepErrors: map[string]error{},
27-
Static: staticForVehicle(),
28-
ServiceAlerts: &gtfs.Realtime{Alerts: []gtfs.Alert{{
29-
ID: "ALERT1",
30-
InformedEntities: []gtfs.AlertInformedEntity{{StopID: strp("ST1")}},
31-
}}},
32-
}
14+
client := arrivalsClient(t, `{"data":{"entry":{"arrivalsAndDepartures":[{"stopId":"1_ST1","tripId":"1_T1","situationIds":["1_ALERT1"]}]}}}`)
3315
vc := &ValidationContext{Config: cfgForTest("test"), Client: client}
34-
results := serviceAlertCheck{}.Run(context.Background(), vc, src)
35-
if len(results) == 0 || results[0].Status != Pass {
36-
t.Errorf("want Pass, got %+v", results)
37-
}
16+
results := serviceAlertCheck{}.Run(context.Background(), vc, alertSrcForStop())
17+
assertFirstStatus(t, results, Pass, "alert in situationIds")
18+
}
19+
20+
// A `null` arrivals response (nil SDK response, nil error) must not be mistaken
21+
// for "stop has no situations" and Fail — it is an unconfirmed query, so Warn.
22+
func TestServiceAlertNullArrivalsResponseWarns(t *testing.T) {
23+
client := arrivalsClient(t, `null`)
24+
vc := &ValidationContext{Config: cfgForTest("test"), Client: client}
25+
results := serviceAlertCheck{}.Run(context.Background(), vc, alertSrcForStop())
26+
assertFirstStatus(t, results, Warn, "null arrivals response")
3827
}
3928

4029
func TestServiceAlertNoSamplableWarns(t *testing.T) {
@@ -47,109 +36,36 @@ func TestServiceAlertNoSamplableWarns(t *testing.T) {
4736
}
4837
vc := &ValidationContext{Config: cfgForTest("test")}
4938
results := serviceAlertCheck{}.Run(context.Background(), vc, src)
50-
if results[0].Status != Warn {
51-
t.Errorf("agency-only alert not stop-referenceable: want Warn got %v", results[0].Status)
52-
}
39+
assertFirstStatus(t, results, Warn, "agency-only alert not stop-referenceable")
5340
}
5441

5542
func TestServiceAlertNoSituationsFails(t *testing.T) {
56-
client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
57-
if strings.Contains(r.URL.Path, "arrivals-and-departures-for-stop") {
58-
w.Header().Set("Content-Type", "application/json")
59-
// Arrivals present but NO situations at all, though the feed says this stop is affected.
60-
w.Write([]byte(`{"data":{"entry":{"arrivalsAndDepartures":[{"stopId":"1_ST1","tripId":"1_T1"}]}}}`))
61-
return
62-
}
63-
t.Errorf("unexpected path %s", r.URL.Path)
64-
})
65-
src := &SourceContext{
66-
Label: "ds0",
67-
Config: config.DataSource{AgencyMapping: map[string]string{"KCM": "1"}},
68-
PrepErrors: map[string]error{},
69-
Static: staticForVehicle(),
70-
ServiceAlerts: &gtfs.Realtime{Alerts: []gtfs.Alert{{
71-
ID: "ALERT1",
72-
InformedEntities: []gtfs.AlertInformedEntity{{StopID: strp("ST1")}},
73-
}}},
74-
}
43+
// Arrivals present but NO situations at all, though the feed says this stop is affected.
44+
client := arrivalsClient(t, `{"data":{"entry":{"arrivalsAndDepartures":[{"stopId":"1_ST1","tripId":"1_T1"}]}}}`)
7545
vc := &ValidationContext{Config: cfgForTest("test"), Client: client}
76-
results := serviceAlertCheck{}.Run(context.Background(), vc, src)
77-
if len(results) == 0 || results[0].Status != Fail {
78-
t.Errorf("affected stop with no situations should Fail, got %+v", results)
79-
}
46+
results := serviceAlertCheck{}.Run(context.Background(), vc, alertSrcForStop())
47+
assertFirstStatus(t, results, Fail, "affected stop with no situations")
8048
}
8149

8250
func TestServiceAlertSituationsButNoMatchWarns(t *testing.T) {
83-
client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
84-
if strings.Contains(r.URL.Path, "arrivals-and-departures-for-stop") {
85-
w.Header().Set("Content-Type", "application/json")
86-
// Situations exist but none match the feed alert id.
87-
w.Write([]byte(`{"data":{"entry":{"arrivalsAndDepartures":[{"stopId":"1_ST1","tripId":"1_T1","situationIds":["1_DIFFERENT"]}]}}}`))
88-
return
89-
}
90-
t.Errorf("unexpected path %s", r.URL.Path)
91-
})
92-
src := &SourceContext{
93-
Label: "ds0",
94-
Config: config.DataSource{AgencyMapping: map[string]string{"KCM": "1"}},
95-
PrepErrors: map[string]error{},
96-
Static: staticForVehicle(),
97-
ServiceAlerts: &gtfs.Realtime{Alerts: []gtfs.Alert{{
98-
ID: "ALERT1",
99-
InformedEntities: []gtfs.AlertInformedEntity{{StopID: strp("ST1")}},
100-
}}},
101-
}
51+
// Situations exist but none match the feed alert id.
52+
client := arrivalsClient(t, `{"data":{"entry":{"arrivalsAndDepartures":[{"stopId":"1_ST1","tripId":"1_T1","situationIds":["1_DIFFERENT"]}]}}}`)
10253
vc := &ValidationContext{Config: cfgForTest("test"), Client: client}
103-
results := serviceAlertCheck{}.Run(context.Background(), vc, src)
104-
if len(results) == 0 || results[0].Status != Warn {
105-
t.Errorf("situations present but no match should Warn, got %+v", results)
106-
}
54+
results := serviceAlertCheck{}.Run(context.Background(), vc, alertSrcForStop())
55+
assertFirstStatus(t, results, Warn, "situations present but no match")
10756
}
10857

10958
func TestServiceAlertFoundInGlobalReferences(t *testing.T) {
110-
client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
111-
if strings.Contains(r.URL.Path, "arrivals-and-departures-for-stop") {
112-
w.Header().Set("Content-Type", "application/json")
113-
// situationIds empty on the arrival, but the alert IS in references.situations
114-
w.Write([]byte(`{"data":{"entry":{"arrivalsAndDepartures":[{"stopId":"1_ST1","tripId":"1_T1"}]},"references":{"situations":[{"id":"1_ALERT1"}]}}}`))
115-
return
116-
}
117-
t.Errorf("unexpected path %s", r.URL.Path)
118-
})
119-
src := &SourceContext{
120-
Label: "ds0",
121-
Config: config.DataSource{AgencyMapping: map[string]string{"KCM": "1"}},
122-
PrepErrors: map[string]error{},
123-
Static: staticForVehicle(),
124-
ServiceAlerts: &gtfs.Realtime{Alerts: []gtfs.Alert{{
125-
ID: "ALERT1",
126-
InformedEntities: []gtfs.AlertInformedEntity{{StopID: strp("ST1")}},
127-
}}},
128-
}
59+
// situationIds empty on the arrival, but the alert IS in references.situations.
60+
client := arrivalsClient(t, `{"data":{"entry":{"arrivalsAndDepartures":[{"stopId":"1_ST1","tripId":"1_T1"}]},"references":{"situations":[{"id":"1_ALERT1"}]}}}`)
12961
vc := &ValidationContext{Config: cfgForTest("test"), Client: client}
130-
results := serviceAlertCheck{}.Run(context.Background(), vc, src)
131-
if len(results) == 0 || results[0].Status != Pass {
132-
t.Errorf("alert in global references.situations should Pass, got %+v", results)
133-
}
62+
results := serviceAlertCheck{}.Run(context.Background(), vc, alertSrcForStop())
63+
assertFirstStatus(t, results, Pass, "alert in global references.situations")
13464
}
13565

13666
func TestServiceAlert404StopWarns(t *testing.T) {
137-
client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
138-
w.WriteHeader(http.StatusNotFound)
139-
})
140-
src := &SourceContext{
141-
Label: "ds0",
142-
Config: config.DataSource{AgencyMapping: map[string]string{"KCM": "1"}},
143-
PrepErrors: map[string]error{},
144-
Static: staticForVehicle(),
145-
ServiceAlerts: &gtfs.Realtime{Alerts: []gtfs.Alert{{
146-
ID: "ALERT1",
147-
InformedEntities: []gtfs.AlertInformedEntity{{StopID: strp("ST1")}},
148-
}}},
149-
}
67+
client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNotFound) })
15068
vc := &ValidationContext{Config: cfgForTest("test"), Client: client}
151-
results := serviceAlertCheck{}.Run(context.Background(), vc, src)
152-
if len(results) == 0 || results[0].Status != Warn {
153-
t.Errorf("404 on stop should Warn (not Fail), got %+v", results)
154-
}
69+
results := serviceAlertCheck{}.Run(context.Background(), vc, alertSrcForStop())
70+
assertFirstStatus(t, results, Warn, "404 on stop should Warn not Fail")
15571
}

‎validator/check_endpoints.go‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,8 @@ func (endpointsCheck) Run(ctx context.Context, vc *ValidationContext) []Result {
3131

3232
// 1. current-time
3333
ct, err := vc.Client.CurrentTime.Get(ctx)
34-
if err != nil {
35-
add("current-time", Fail, "current-time failed: "+redact(err, key), nil)
34+
if err != nil || ct == nil {
35+
add("current-time", Fail, withReason("current-time failed", err, key), nil)
3636
skipRest("current-time failed")
3737
return out
3838
}
@@ -48,7 +48,7 @@ func (endpointsCheck) Run(ctx context.Context, vc *ValidationContext) []Result {
4848

4949
// 2. agencies-with-coverage (pre-fetched into the context)
5050
if vc.Agencies == nil || vc.AgenciesErr != nil {
51-
add("agencies-with-coverage", Fail, "agencies-with-coverage failed: "+redact(vc.AgenciesErr, key), nil)
51+
add("agencies-with-coverage", Fail, withReason("agencies-with-coverage failed", vc.AgenciesErr, key), nil)
5252
pop()
5353
skipRest("agencies-with-coverage failed")
5454
return out
@@ -65,8 +65,8 @@ func (endpointsCheck) Run(ctx context.Context, vc *ValidationContext) []Result {
6565

6666
// 3. routes-for-agency
6767
routes, err := vc.Client.RoutesForAgency.List(ctx, agencyID)
68-
if err != nil || len(routes.Data.List) == 0 {
69-
add("routes-for-agency", Fail, "routes-for-agency empty/failed: "+redact(err, key), map[string]any{"agencyId": agencyID})
68+
if err != nil || routes == nil || len(routes.Data.List) == 0 {
69+
add("routes-for-agency", Fail, withReason("routes-for-agency empty/failed", err, key), map[string]any{"agencyId": agencyID})
7070
pop()
7171
skipRest("routes-for-agency failed")
7272
return out
@@ -77,8 +77,8 @@ func (endpointsCheck) Run(ctx context.Context, vc *ValidationContext) []Result {
7777

7878
// 4. stops-for-route
7979
sfr, err := vc.Client.StopsForRoute.List(ctx, routeID, onebusaway.StopsForRouteListParams{})
80-
if err != nil || len(sfr.Data.Entry.StopIDs) == 0 {
81-
add("stops-for-route", Fail, "stops-for-route empty/failed: "+redact(err, key), map[string]any{"routeId": routeID})
80+
if err != nil || sfr == nil || len(sfr.Data.Entry.StopIDs) == 0 {
81+
add("stops-for-route", Fail, withReason("stops-for-route empty/failed", err, key), map[string]any{"routeId": routeID})
8282
pop()
8383
skipRest("stops-for-route failed")
8484
return out
@@ -89,8 +89,8 @@ func (endpointsCheck) Run(ctx context.Context, vc *ValidationContext) []Result {
8989

9090
// 5. stop
9191
st, err := vc.Client.Stop.Get(ctx, stopID)
92-
if err != nil || st.Data.Entry.ID != stopID {
93-
add("stop", Fail, "stop lookup failed/mismatch: "+redact(err, key), map[string]any{"stopId": stopID})
92+
if err != nil || st == nil || st.Data.Entry.ID != stopID {
93+
add("stop", Fail, withReason("stop lookup failed/mismatch", err, key), map[string]any{"stopId": stopID})
9494
pop()
9595
skipRest("stop failed")
9696
return out
@@ -104,8 +104,8 @@ func (endpointsCheck) Run(ctx context.Context, vc *ValidationContext) []Result {
104104
Lat: onebusaway.Float(lat),
105105
Lon: onebusaway.Float(lon),
106106
})
107-
if err != nil || loc.Data.OutOfRange || len(loc.Data.List) == 0 {
108-
add("stops-for-location", Fail, "stops-for-location empty/out-of-range/failed: "+redact(err, key), nil)
107+
if err != nil || loc == nil || loc.Data.OutOfRange || len(loc.Data.List) == 0 {
108+
add("stops-for-location", Fail, withReason("stops-for-location empty/out-of-range/failed", err, key), nil)
109109
pop()
110110
skipRest("stops-for-location failed")
111111
return out
@@ -115,8 +115,8 @@ func (endpointsCheck) Run(ctx context.Context, vc *ValidationContext) []Result {
115115

116116
// 7. arrivals-and-departures-for-stop
117117
ad, err := vc.Client.ArrivalAndDeparture.List(ctx, stopID, onebusaway.ArrivalAndDepartureListParams{})
118-
if err != nil {
119-
add("arrivals-and-departures-for-stop", Fail, "arrivals failed: "+redact(err, key), map[string]any{"stopId": stopID})
118+
if err != nil || ad == nil {
119+
add("arrivals-and-departures-for-stop", Fail, withReason("arrivals failed", err, key), map[string]any{"stopId": stopID})
120120
return out
121121
}
122122
n := len(ad.Data.Entry.ArrivalsAndDepartures)

0 commit comments

Comments
 (0)