[Detail Bug] Directions routes can reference wrong/out-of-range steps when null steps are normalized away

[detail-app]

Detail Bug Report

https://app.detail.dev/org_befd6425-a158-4e24-9d4d-1e5c08769515/bugs/bug_ded8730f-1de7-4806-bef0-585c16cb196a

Summary

• Context: DirectionsResponse is a core domain record that aggregates directions data, including routes and steps, where routes reference steps by their index in the steps list.
• Bug: The DirectionsResponse constructor uses a generic normalizeList method to filter out null elements from the steps list, which causes indices to shift and breaks the stepIndexes references in DirectionsRoute.
• Actual vs. expected: Null elements in the steps list are removed, causing subsequent steps to shift to lower indices and making existing stepIndexes in DirectionsRoute either point to the wrong step or throw an IndexOutOfBoundsException; these null elements should be preserved as empty placeholders to maintain index stability.
• Impact: Application crashes with IndexOutOfBoundsException or silent data corruption (routes showing wrong steps) when processing a directions response containing null steps.

Code with Bug

public DirectionsResponse {
    origin = normalizeOptional(origin);
    destination = normalizeOptional(destination);
    routes = normalizeList(routes);
    steps = normalizeList(steps); // <-- BUG 🔴 filtering nulls shifts indices; routes reference steps by index
    stepPaths = normalizeStepPaths(stepPaths);
}

private static <T> List<T> normalizeList(List<T> rawList) {
    if (rawList == null) {
        return List.of();
    }
    return rawList.stream()
        .filter(Objects::nonNull) // <-- BUG 🔴 destructive for index-sensitive lists like steps
        .toList();
}

Explanation

DirectionsRoute.stepIndexes assumes the steps list is stable by position. Filtering null elements in steps compacts the list, shifting all subsequent indices. As a result, existing stepIndexes can become out of range (triggering IndexOutOfBoundsException) or point to the wrong step (silent corruption).

Codebase Inconsistency

normalizeStepPaths in the same file preserves list size by mapping null entries to empty lists (to maintain alignment), but steps does not, which also breaks alignment between steps and stepPaths.

private static List<List<Location>> normalizeStepPaths(List<List<Location>> rawList) {
    if (rawList == null) {
        return List.of();
    }
    return rawList.stream()
        .map(DirectionsResponse::normalizeStepPath) // preserves size even when entries are null
        .toList();
}

Failing Test

@Test
void testStepIndexBreakage() {
    DirectionsStep step0 = new DirectionsStep(Optional.of(0), Optional.empty(), Optional.empty(), Optional.empty(), Optional.empty());
    DirectionsStep step2 = new DirectionsStep(Optional.of(2), Optional.empty(), Optional.empty(), Optional.empty(), Optional.empty());

    List<DirectionsStep> stepsWithNull = new ArrayList<>();
    stepsWithNull.add(step0);
    stepsWithNull.add(null); // This null will be filtered out
    stepsWithNull.add(step2);

    DirectionsRoute route = new DirectionsRoute(
        Optional.of("Route 1"),
        Optional.empty(),
        Optional.empty(),
        Optional.empty(),
        List.of(0, 2), // Reference to index 2 (step2)
        Optional.empty()
    );

    DirectionsResponse response = new DirectionsResponse(
        Optional.empty(),
        Optional.empty(),
        List.of(route),
        stepsWithNull,
        List.of(List.of(), List.of(), List.of())
    );

    // After normalization, steps size is 2 (null was removed)
    assertEquals(2, response.steps().size());

    List<Integer> indexes = response.routes().get(0).stepIndexes();
    assertEquals(2, (int)indexes.get(1));

    // This throws IndexOutOfBoundsException because index 2 is now out of bounds
    assertThrows(IndexOutOfBoundsException.class, () -> response.steps().get(indexes.get(1)));
}

Recommended Fix

Implement a specific normalization method for steps that replaces null elements with an empty DirectionsStep placeholder, ensuring the list size and indices remain stable.

public DirectionsResponse {
    origin = normalizeOptional(origin);
    destination = normalizeOptional(destination);
    routes = normalizeList(routes);
    steps = normalizeSteps(steps); // <-- FIX 🟢
    stepPaths = normalizeStepPaths(stepPaths);
}

private static List<DirectionsStep> normalizeSteps(List<DirectionsStep> rawList) {
    if (rawList == null) {
        return List.of();
    }
    return rawList.stream()
        .map(s -> s == null ? new DirectionsStep(Optional.empty(), Optional.empty(), Optional.empty(), Optional.empty(), Optional.empty()) : s) // <-- FIX 🟢 preserve indices
        .toList();
}

History

This bug was introduced in commit e58b274. The original change aimed to prevent NullPointerExceptions when handling sparse API responses by filtering null elements during list normalization. However, this filtering logic is destructive for index-sensitive lists like steps in DirectionsResponse, as it shifts element positions and breaks correlations with other fields.