Fix NullPointerException when itinerary.legs is null in RealtimeService.startRealtimeUpdates()

[varun-ai69]

Summary

This PR fixes a potential NullPointerException in RealtimeService.startRealtimeUpdates() when itinerary.legs is null.

Previously the method iterated directly over itinerary.legs without validating the field. If an itinerary existed but its legs list was null, the service would crash when attempting to iterate over it.

The fix adds a null check before iterating over the legs to ensure the service safely handles incomplete itinerary objects.

In addition, the tests were updated to exercise the service through the public API (onHandleIntent()) instead of relying on reflection, and a defensive null guard was added in disableListenForTripUpdates() when cancelling alarms.

Problem

In RealtimeService.startRealtimeUpdates() the code previously did:

for (Leg leg : itinerary.legs) {
    if (leg.realTime) {
        realtimeLegsOnItineraries = true;
    }
}

If itinerary.legs == null , this results in:

NullPointerException

This could occur if:
-the itinerary object is partially deserialized
-a malformed bundle is received
-external API returns an itinerary without legs

As a result, the service may crash while handling INTENT_START_CHECKS.

How to Reproduce

A test case demonstrates the issue.

@Test
public void testOnHandleIntentWithNullItineraryLegsDoesNotCrash() {
    Bundle bundle = new Bundle();

    ArrayList<Itinerary> itineraries = new ArrayList<>();
    Itinerary it = new Itinerary();
    it.legs = null;
    itineraries.add(it);

    bundle.putSerializable(OTPConstants.ITINERARIES, itineraries);
    bundle.putInt(OTPConstants.SELECTED_ITINERARY, 0);

    Intent intent = new Intent(OTPConstants.INTENT_START_CHECKS);
    intent.putExtras(bundle);

    try {
        mService.onHandleIntent(intent);
    } catch (NullPointerException e) {
        fail("onHandleIntent should not throw NPE when itinerary.legs is null: " + e.getMessage());
    }
}

Before the Fix

Running this test results in:

NullPointerException
at RealtimeService.startRealtimeUpdates()

After the Fix

The service safely handles the case and no exception is thrown, allowing the test to pass.

Fix

A defensive null check was added before iterating over itinerary.legs in
RealtimeService.startRealtimeUpdates().

Previously the code directly iterated over itinerary.legs, which could cause a
NullPointerException if the itinerary object existed but its legs field was
null.

Updated implementation:

if (itinerary.legs != null) {
    for (Leg leg : itinerary.legs) {
        if (leg.realTime) {
            realtimeLegsOnItineraries = true;
        }
    }
}

This ensures the service behaves safely even if the itinerary does not contain legs.

Additional Improvements

Added a defensive null check in disableListenForTripUpdates() before cancelling alarms to avoid potential NullPointerException if a PendingIntent cannot be created.

Tests

Added a regression test:

• testOnHandleIntentWithNullItineraryLegsDoesNotCrash

This test verifies that onHandleIntent() does not throw a NullPointerException when itinerary.legs is null.

Impact

This change:

• Prevents a potential crash in RealtimeService
• Improves robustness when handling incomplete itinerary data
• Ensures alarm cancellation is safely guarded
• Adds regression tests to prevent future regressions

No functional behavior changes occur for valid itineraries.

[aaronbrethorst]

Hey varun-ai69, thanks for taking the time to harden RealtimeService against null bundles and missing data. The core itinerary.legs null check is the right fix, and the explicit intent change for Android 14+ is a good modernization. The test for the null-legs path through the public API is well-structured. Before we can merge this, I will need you to make a couple changes:

Important

1. catch (Exception e) around new ItineraryDescription(itinerary) is too broad (RealtimeService.java:375-379) — This will silently swallow programming errors like IllegalArgumentException from TraverseMode.valueOf(), ClassCastException, or any future bugs in the constructor. The null-legs case is already handled by your guard at line 114, so by the time getSimplifiedBundle is called, itinerary.legs should not be null. Replace the try/catch with an explicit null/empty check:

   if (itinerary.legs == null || itinerary.legs.isEmpty()) {
       Log.w(TAG, "getSimplifiedBundle: itinerary has no legs");
       return null;
   }
   ItineraryDescription desc = new ItineraryDescription(itinerary);

   This handles the defensive case without swallowing unrelated exceptions.

2. Reflection-based tests are brittle (RealtimeServiceTest.java:152-187) — testGetSimplifiedBundleWithMissingItineraryDoesNotCrash and testGetSimplifiedBundleWithMissingNotificationTargetDoesNotCrash use reflection to call the private getSimplifiedBundle() method. If the method is renamed or its signature changes, these tests fail at runtime rather than compile time. The existing testOnHandleIntentWithEmptyBundleDoesNotCrash already covers the "missing itinerary" path through the public API. Please rewrite both tests to go through onHandleIntent instead. For the missing-NOTIFICATION_TARGET case, you'd need a bundle with realtime legs but no notification target:

   @Test
   public void testOnHandleIntentWithRealtimeLegsButNoNotificationTargetDoesNotCrash() {
       Bundle bundle = new Bundle();
       ArrayList<Itinerary> itineraries = new ArrayList<>();
       Itinerary it = new Itinerary();
       Leg leg = new Leg();
       leg.realTime = true;
       leg.mode = "BUS";
       it.legs = new ArrayList<>();
       it.legs.add(leg);
       itineraries.add(it);
       bundle.putSerializable(OTPConstants.ITINERARIES, itineraries);
       bundle.putInt(OTPConstants.SELECTED_ITINERARY, 0);
       // No NOTIFICATION_TARGET — exercises the null alarmIntent guard
   
       Intent intent = new Intent(OTPConstants.INTENT_START_CHECKS);
       intent.putExtras(bundle);
   
       try {
           mService.onHandleIntent(intent);
       } catch (NullPointerException e) {
           fail("Should not crash with realtime legs but missing NOTIFICATION_TARGET: "
                   + e.getMessage());
       }
   }

   This also gives you coverage of the alarmIntent == null guard in startRealtimeUpdates (lines 125-129), which currently has no test.

Fit and Finish

3. Add a null guard in disableListenForTripUpdates() (RealtimeService.java:307-310) — getAlarmIntent(null) currently can't return null (the null-return path only triggers when bundle != null), but getAlarmIntent now has a null-returning contract and disableListenForTripUpdates doesn't check for it. If someone later refactors getAlarmIntent, AlarmManager.cancel(null) will throw an NPE — and this method is called from error-recovery paths throughout the class:

   public void disableListenForTripUpdates() {
       Log.d(TAG, "Disable trip updates.");
       PendingIntent alarmIntent = getAlarmIntent(null);
       if (alarmIntent != null) {
           getAlarmManager().cancel(alarmIntent);
       }
   }

4. PR title has duplicate text — The title reads "Fix NullPointerException when itinerary.legs is null in RealtimeService.startRealtimeUpdates()Fix null itinerary legs". Please clean that up.

Thanks again, and I look forward to merging this change.

[varun-ai69]

@aaronbrethorst
Thanks for the detailed review!

I’ve addressed the requested changes:

• Removed the broad catch (Exception e) and replaced it with an explicit itinerary.legs null/empty check before creating ItineraryDescription.
• Replaced the reflection-based tests with tests that exercise the service through the public API (onHandleIntent()).
• Added the test testOnHandleIntentWithRealtimeLegsButNoNotificationTargetDoesNotCrash to cover the missing NOTIFICATION_TARGET path.
• Added a null guard in disableListenForTripUpdates() before cancelling the alarm PendingIntent.
• Cleaned up the PR title as suggested.

Could you please take another look when you have a moment? Thanks!

[aaronbrethorst]

Varun, nice progress on this. All four items from round 1 have been addressed: the ItineraryDescription catch is replaced with an explicit itinerary.legs null/empty guard, the reflection-based tests are rewritten to use onHandleIntent(), the disableListenForTripUpdates() null guard is in place, and the PR title is cleaned up. The tests are well-structured — the null-legs test and the missing-NOTIFICATION_TARGET test both exercise meaningful code paths through the public API.

Two items remain before this can merge.

Critical Issues (0 found)

None.

Important Issues (2 found)

1. catch (Exception e) around copyIntoBundleSimple() is too broad

RealtimeService.java:387 — The ItineraryDescription try-catch was correctly replaced with an explicit guard (nice work), but copyIntoBundleSimple() still catches bare Exception. Looking at copyIntoBundleSimple(), the realistic failure is NullPointerException when getFrom() or getTo() returns null. A broad catch (Exception) will silently swallow unrelated errors like ClassCastException from corrupt bundle data or IllegalStateException from framework issues, making them invisible in production.

Narrow it to match the specific failure you're defending against:

try {
    new TripRequestBuilder(params).copyIntoBundleSimple(extras);
} catch (NullPointerException e) {
    Log.e(TAG, "getSimplifiedBundle: error copying trip params into bundle", e);
    return null;
}

2. Log.w at lines 127 and 326 should be Log.e

Both of these log a condition where realtime monitoring completely fails to start — that's an error, not a warning. The failure paths inside getSimplifiedBundle() already correctly use Log.e; the downstream callers should be consistent:

// Line 127
Log.e(TAG, "Not scheduling realtime updates - unable to build alarm PendingIntent");

// Line 326
Log.e(TAG, "getAlarmIntent: simplified bundle is null, returning null PendingIntent");

Suggestions (0 found)

None.

Strengths

• The explicit itinerary.legs == null || itinerary.legs.isEmpty() guard in getSimplifiedBundle() (line 377) is a much better approach than catching exceptions from the ItineraryDescription constructor — it makes the guarded condition self-documenting
• The testOnHandleIntentWithRealtimeLegsButNoNotificationTargetDoesNotCrash test carefully builds a valid transit leg with tripId and endTime so it passes through ItineraryDescription and reaches the NOTIFICATION_TARGET check
• The testOnHandleIntentWithNullItineraryLegsDoesNotCrash test includes a helpful comment explaining why DATE_TIME is omitted from the bundle
• The null guard in disableListenForTripUpdates() is forward-looking good practice

Recommended Action

Request changes. Fix the broad catch (Exception) on copyIntoBundleSimple() (item #1) and elevate Log.w to Log.e at the two monitoring-failure paths (item #2). Both are quick fixes.

[varun-ai69]

@aaronbrethorst
Thanks for the review

Thanks for the review! I have addressed all the requested changes:

-Narrowed catch (Exception) to catch (NullPointerException) in copyIntoBundleSimple()
-Elevated Log.w to Log.e at both monitoring-failure paths

Ready for re-review!