Add a configurable limit for the search window

[vpaturet]

Summary

This PR ensures that the search window cannot exceed a configurable duration, with a default set to 24 hours.
Due to the way timetable data are collected before a Raptor trip search, using a search window larger than 24 hours may lead to inconsistent search results.
Limiting the search window prevents also potential performance issues.

The API returns the following response with HTTP code 200:

{
  "errors": [
    {
      "message": "Exception while fetching data (/trip) : The search window cannot exceed 24 hours",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
      "path": [
        "trip"
      ],
      "extensions": {
        "classification": "DataFetchingException"
      }
    }
  ],
  "data": null
}

Issue

No

Unit tests

Added unit tests

Documentation

Updated documentation

[codecov]

Codecov Report

Attention: 65 lines in your changes are missing coverage. Please review.

Comparison is base (63ab0de) 65.92% compared to head (79ba498) 66.60%.
Report is 594 commits behind head on dev-2.x.

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #5293      +/-   ##
=============================================
+ Coverage      65.92%   66.60%   +0.68%     
- Complexity     14820    15297     +477     
=============================================
  Files           1773     1790      +17     
  Lines          68789    69406     +617     
  Branches        7289     7310      +21     
=============================================
+ Hits           45346    46225     +879     
+ Misses         20960    20704     -256     
+ Partials        2483     2477       -6     

Files	Coverage Δ
...nner/ext/ridehailing/service/uber/UberService.java	75.64% <100.00%> (ø)
...t/smoovebikerental/SmooveBikeRentalDataSource.java	100.00% <100.00%> (ø)
...ehicleparking/hslpark/HslFacilitiesDownloader.java	54.34% <100.00%> (ø)
.../ext/vehicleparking/hslpark/HslHubsDownloader.java	54.34% <100.00%> (ø)
.../org/opentripplanner/framework/retry/OtpRetry.java	100.00% <100.00%> (+9.37%)	⬆️
...ntripplanner/routing/api/request/RouteRequest.java	79.24% <100.00%> (+2.90%)	⬆️
...pentripplanner/standalone/config/RouterConfig.java	84.61% <100.00%> (+0.40%)	⬆️
...lone/config/routerconfig/TransitRoutingConfig.java	96.72% <100.00%> (+0.19%)	⬆️
...tripplanner/updater/spi/GenericJsonDataSource.java	83.33% <100.00%> (ø)
...ner/updater/trip/GtfsRealtimeTripUpdateSource.java	61.53% <100.00%> (ø)
... and 15 more

... and 403 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[leonardehrenfried]

The description says that there are unit tests but I can't see any.

[vpaturet]

No unit tests, I use the check mark to state that the existing ones are passing.
Do we have a test infrastructure to test the HTTP responses?

[vpaturet]

No, my bad, I mixed up with the other JSON error mapping PR. This one needs a unit test.

[leonardehrenfried]

It's not super hard to write tests for the JAXB resource classes, you can simply instantiate them. I did it here:

OpenTripPlanner/src/test/java/org/opentripplanner/api/common/PlannerResourceTest.java

Line 21 in f614405

class PlannerResourceTest {

It's not terribly elegant but it works, but you're right: in this case you need something simpler.

[vpaturet]

Added validation for zero or negative search window.
For zero or negative search window, the response used to be:

{
  "data": {
    "trip": {
      "routingErrors": [
        {
          "code": "noTransitConnectionInSearchWindow",
          "description": "No trip found. There may be no transit service within the maximum specified distance or at the specified time, or your start or end point might not be safely accessible."
        }
      ],
      "tripPatterns": []
    }
  }
}

It is now:

{
  "errors": [
    {
      "message": "Exception while fetching data (/trip) : The search window must be a strictly positive duration",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
      "path": [
        "trip"
      ],
      "extensions": {
        "classification": "DataFetchingException"
      }
    }
  ],
  "data": null
}

[vpaturet]

That was a bit too quick, it fails for reverse search :-)

[vpaturet]

@t2gran why do we set the duration to zero there?

OpenTripPlanner/src/test/java/org/opentripplanner/transit/speed_test/SpeedTestRequest.java

Line 83 in 9601170

request.setSearchWindow(Duration.ZERO);

[vpaturet]

A zero-length search window is valid, only negative search windows should be rejected --> validation adjusted accordingly.

[t2gran]

FYI: searchWindow=0s is used to run plain Raptor without minute iterations (RangeRaptor). This is used to produce heuristics.

[vpaturet]

What would be an appropriate name for a router config parameter specifying the maximum search window that can be set through the API?
There is already transit.dynamicSearchWindow.maxWindow
Possibly transit.maxStaticSearchWindow?

[vpaturet]

Added support for configurable max search window with default to 24 hours.
@t2gran I used a different approach than the one you suggested to disentangle the circular dependency between routingDefaults and transit configurations, this looks simpler to me.

[t2gran]

I find it easier to understand if null is used for UNSET, and NOT Duration.ZERO. Add @nullable to the field and setter.

[vpaturet]

Done

[t2gran]

The default value should not be defined here - not the responsibility of this class. Can you remind me that we fix this after this is merged? This follows an existing pattern and I would like to fix all of them - and that does not belong in this PR.

[leonardehrenfried]

The code here looks good but what happens it if you set transit.dynamicSearchWindow.maxWindow=36h and the computed dynamic window is higher than this max value?

[vpaturet]

Tested locally with maxSearchWindow=65m and dynamicSearchWindow.maxWindow=5h: when omitting the searchWindow parameter for a given query, a dynamic search window of 70m was used: thus the parameter maxSearchWindow was ignored.
In other words: the two limits can be set independently from one another.

Is this what we want?

[leonardehrenfried]

I think I'm fine with this behaviour if we mention it in the docs.