Fix arrivals-and-departures returning bare "null" on unresolvable route refs#462
Merged
Merged
Conversation
…te refs A corrupt stop->route-collection index entry can resolve to a null RouteBean (getRouteForId returns null). That null propagated into StopBean.getRoutes() and NPEd in BeanFactoryV2.getStop(), which runs outside the action's try/catch, so the exception unwound out of show() with _response unset and the ModelDriven serializer emitted a literal "null" body with HTTP 200 -- blanking nearly the entire SDMTS Blue Line via the nearby-stops reference build. - StopBeanServiceImpl: drop unresolvable route beans (route-collection and static-route paths) with a WARN instead of leaking nulls - BeanFactoryV2.getStop: skip null RouteBean, mirroring the existing staticRoutes guard - StopWithArrivalsAndDeparturesBeanServiceImpl: skip null nearby StopBean - ArrivalsAndDeparturesForStopAction: wrap bean conversion in try/catch so a failure returns a proper error envelope instead of a bare "null" - Tests for the route-collection and static-route guards, and for the serializer null-route guard Bundle data root cause tracked in #461.
|
|
📝 WalkthroughWalkthroughAdds null guards across ChangesNull Guards for Route and Stop Resolution
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
onebusaway-transit-data-federation/src/main/java/org/onebusaway/transit_data_federation/impl/beans/StopWithArrivalsAndDeparturesBeanServiceImpl.java (1)
149-166: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMissing null guard for nearby stop resolution in the multi-stop path.
Line 150 calls
_stopBeanService.getStopForId(id, serviceInterval)and assigns the result tostop, but does not check for null before dereferencing it at lines 151–166. Based on the null check you added at lines 79–82 in the single-stop method above,getStopForIdcan return null when a stop does not resolve. If that happens here, the code will throw a NullPointerException when accessingstop.getLat()(line 153),stop.getRoutes()(line 159), or other methods.Add the same defensive null check here as you did in the single-stop path.
🛡️ Proposed fix to guard against unresolvable nearby stops
for (AgencyAndId id : allNearbyStopIds) { StopBean stop = _stopBeanService.getStopForId(id, serviceInterval); + if (stop == null) { + _log.warn("nearby stop {} did not resolve to a stop; skipping", id); + continue; + } if (center != null) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@onebusaway-transit-data-federation/src/main/java/org/onebusaway/transit_data_federation/impl/beans/StopWithArrivalsAndDeparturesBeanServiceImpl.java` around lines 149 - 166, The for loop iterating over allNearbyStopIds calls _stopBeanService.getStopForId(id, serviceInterval) but does not check if the returned stop is null before dereferencing it in subsequent operations like stop.getLat() and stop.getRoutes(). Add a null check immediately after the getStopForId call within the loop, similar to the defensive null check you added in the single-stop path above, and skip processing that iteration if stop is null. This will prevent NullPointerException when getStopForId returns null for an unresolvable stop.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@onebusaway-transit-data-federation/src/main/java/org/onebusaway/transit_data_federation/impl/beans/StopWithArrivalsAndDeparturesBeanServiceImpl.java`:
- Around line 149-166: The for loop iterating over allNearbyStopIds calls
_stopBeanService.getStopForId(id, serviceInterval) but does not check if the
returned stop is null before dereferencing it in subsequent operations like
stop.getLat() and stop.getRoutes(). Add a null check immediately after the
getStopForId call within the loop, similar to the defensive null check you added
in the single-stop path above, and skip processing that iteration if stop is
null. This will prevent NullPointerException when getStopForId returns null for
an unresolvable stop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f386d7c-807c-4341-a624-13347c3ceef5
📒 Files selected for processing (6)
onebusaway-api-core/src/main/java/org/onebusaway/api/model/transit/BeanFactoryV2.javaonebusaway-api-core/src/test/java/org/onebusaway/api/model/transit/BeanFactoryV2Test.javaonebusaway-api-webapp/src/main/java/org/onebusaway/api/actions/api/where/ArrivalsAndDeparturesForStopAction.javaonebusaway-transit-data-federation/src/main/java/org/onebusaway/transit_data_federation/impl/beans/StopBeanServiceImpl.javaonebusaway-transit-data-federation/src/main/java/org/onebusaway/transit_data_federation/impl/beans/StopWithArrivalsAndDeparturesBeanServiceImpl.javaonebusaway-transit-data-federation/src/test/java/org/onebusaway/transit_data_federation/impl/beans/StopBeanServiceImplTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
RouteBean(_routeBeanService.getRouteForId()returns null per its contract). That null propagated intoStopBean.getRoutes()and NPE'd inBeanFactoryV2.getStop()— which runs outside the action's try/catch — so the exception unwound out ofshow()with_responseunset, and theModelDrivenserializer emitted a literalnullbody with HTTP 200.MTS_510) stops: the single-stop arrivals path builds references for up to 100 nearby stops, so one orphan-route stop poisoned every stop near it. Reproduces even with a zero-width time window; not realtime-related; bundle calendar covers today.null.Changes
StopBeanServiceImpl— skip unresolvable route beans on both the route-collection and static-route paths, with a WARN (instead of leaking a null into the stop's routes).BeanFactoryV2.getStop— skip nullRouteBeanin the routes loop, mirroring the existingstaticRoutesguard immediately below it.StopWithArrivalsAndDeparturesBeanServiceImpl— skip null nearbyStopBean(with a WARN).ArrivalsAndDeparturesForStopAction.show()— wrap the V1/V2 bean conversion in a try/catch →setExceptionResponse(), so any future conversion failure returns a real error envelope rather than a barenull.StopBeanServiceImplTest), and serializer null-route guard (BeanFactoryV2Test).The underlying bundle corruption (why a stop carries an unresolvable service-date route collection) is tracked separately in #461; this PR is the defensive/serialization fix so one bad record can't blank the API.
Test plan
StopBeanServiceImplTest— red→green for the route-collection orphan; new static-route orphan test; full beans package (24 tests) greenBeanFactoryV2Test— red (NPE, the bare-nullcause) → green after the guardonebusaway-api-webapptest-compilepasses (action change compiles)Review notes (non-blocking)
BeanFactoryV2.getStopguard is intentionally un-logged, matching the adjacent pre-existing silentstaticRoutesguard and avoiding WARN spam on a hot path; the service-layer guard (which does log) is the primary trace.@Autowiredwith no setters, so a focused test would need a reflection-injection pattern new to this repo. Judged not worth it for a trivial log +setExceptionResponse()block.Summary by CodeRabbit
Bug Fixes
Tests