Add option to include stations in `nearest` search #5390

nurmAV · 2023-10-03T13:30:40Z

Summary

Adds an option to include both stops and stations when searching for stops. If both are included, only the parent station of a stop is returned if one exists.

Unit tests

The code was tested manually

codecov · 2023-10-03T13:46:59Z

Codecov Report

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

Comparison is base (3163631) 67.36% compared to head (fd1973d) 67.45%.
Report is 2 commits behind head on dev-2.x.

Files	Patch %	Lines
...ntripplanner/apis/gtfs/generated/GraphQLTypes.java	40.00%	3 Missing ⚠️
...outing/graphfinder/PlaceFinderTraverseVisitor.java	89.65%	1 Missing and 2 partials ⚠️
...pplanner/apis/gtfs/datafetchers/QueryTypeImpl.java	71.42%	1 Missing and 1 partial ⚠️
...va/org/opentripplanner/apis/gtfs/GraphQLUtils.java	0.00%	1 Missing ⚠️
.../gtfs/datafetchers/PlaceInterfaceTypeResolver.java	0.00%	0 Missing and 1 partial ⚠️
...anner/apis/transmodel/TransmodelGraphQLSchema.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #5390      +/-   ##
=============================================
+ Coverage      67.36%   67.45%   +0.08%     
- Complexity     16167    16193      +26     
=============================================
  Files           1858     1858              
  Lines          71093    71126      +33     
  Branches        7403     7412       +9     
=============================================
+ Hits           47893    47976      +83     
+ Misses         20743    20678      -65     
- Partials        2457     2472      +15

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

optionsome · 2023-10-03T13:42:58Z

src/main/java/org/opentripplanner/apis/gtfs/datafetchers/PlaceInterfaceTypeResolver.java

+    if (o instanceof Station) {
+      return schema.getObjectType("Stop");
+    }


I think the the if statement above this can be expanded to check for Station also and this block can be removed.

optionsome · 2023-10-03T13:43:41Z

src/main/java/org/opentripplanner/apis/gtfs/generated/GraphQLDataFetchers.java

Run the optimize imports thing on intellij to get rid of extra imports here.

optionsome · 2023-10-03T13:44:01Z

src/main/java/org/opentripplanner/apis/gtfs/generated/GraphQLTypes.java

optionsome · 2023-10-03T13:52:52Z

src/main/java/org/opentripplanner/routing/graphfinder/PlaceFinderTraverseVisitor.java

-      placesFound.add(new PlaceAtDistance(stop, distance));
-      seenStops.add(stop.getId());
+      if (
+        includeStopsAndStations &&


This code is now inside the includeStops block. I think we should separate these so includeStops doesn't need to be and probably shouldn't be true when includeStopsAndStations is true (at least on the GraphQL argument level). I'm not quite sure what should happen if someone has defined both filters as true, should we include the stations in addition to their stops or should we just throw some exception that this combination of filters is not allowed.

optionsome · 2023-10-03T14:06:28Z

src/main/java/org/opentripplanner/routing/graphfinder/PlaceFinderTraverseVisitor.java

+        includeStopsAndStations &&
+        stop.getParentStation() != null &&
+        !seenStops.contains(stop.getParentStation().getId()) &&
+        !seenStops.contains(stop.getId())


Isn't this check unnecessary because if the stop has a parent station, we add the parent stations id to the seenStops, not the stop's.

src/main/java/org/opentripplanner/routing/graphfinder/PlaceFinderTraverseVisitor.java

optionsome · 2023-10-03T14:16:18Z

src/main/java/org/opentripplanner/routing/graphfinder/PlaceFinderTraverseVisitor.java

+      } else {
+        // Do not accumulate the same station many times
+        if (
+          stop.getParentStation() != null && seenStops.contains(stop.getParentStation().getId())
+        ) return;
+        seenStops.add(stop.getId());
+        placesFound.add(new PlaceAtDistance(stop, distance));
+      }


I took a while for me to understand this logic. I think it would be simpler if we first check if a stop has a parent station or not and then inside those blocks do other checks etc.

optionsome · 2023-10-03T14:18:14Z

src/main/resources/org/opentripplanner/apis/gtfs/schema.graphqls

@@ -986,6 +986,10 @@ enum FilterPlaceType {

    """Parking lots that contain spaces for cars"""
    CAR_PARK
+
+
+    """Stops or stations"""


I think we should add more documentation here what this actually does and also it's relation to the STOP filter (and vice versa on the STOP filter).

I would vote to rename this to STATION and the following semantics:

STATION: only return stations, not stops. if a single stop is within radius then its station is also.

STOP: only return stops, not stations

STATION,STOP: return all stations but only those stops that don't have a parent station. if a single stop is within radius then its parent station is also.

Ok I think agree with you, lets do it like that. We need to document this behaviour well here in the schema.

Yes, good documentation is important because it's going to be confusing no matter what the semantics will be.

optionsome · 2023-10-03T14:21:39Z

src/main/java/org/opentripplanner/routing/graphfinder/PlaceFinderTraverseVisitor.java

@@ -80,6 +81,8 @@ public PlaceFinderTraverseVisitor(
    includeVehicleRentals = shouldInclude(filterByPlaceTypes, PlaceType.VEHICLE_RENT);
    includeCarParking = shouldInclude(filterByPlaceTypes, PlaceType.CAR_PARK);
    includeBikeParking = shouldInclude(filterByPlaceTypes, PlaceType.BIKE_PARK);
+    includeStopsAndStations = shouldInclude(filterByPlaceTypes, PlaceType.STOP_OR_STATION);


I think now this is true by default if no filters are defined. However, I think we maybe shouldn't have this true by default as the client software might have existing handling for when Stop type objects are returned which might break as now this sometimes would return stations which also use Stop type but have some crucial fields as null. Also again, now by default we have both STOP and STOP_OR_STATION on by default and it's a bit unclear what it means.

optionsome · 2023-10-03T14:22:48Z

src/main/java/org/opentripplanner/routing/graphfinder/PlaceFinderTraverseVisitor.java

+        // Do not accumulate the same station many times
+        if (
+          stop.getParentStation() != null && seenStops.contains(stop.getParentStation().getId())
+        ) return;


Try to always wrap things inside an if in a {} block and preferable also split that block to a new line as it's easy to miss something happening on the same line where there is an if statement.

optionsome · 2023-10-03T14:27:02Z

I wonder if we already have some existing tests for this part of the code and how difficult it would be to write a test for the GraphQL API. @leonardehrenfried probably has a better idea.

leonardehrenfried · 2023-10-03T17:04:18Z

I wonder if we already have some existing tests for this part of the code and how difficult it would be to write a test for the GraphQL API. @leonardehrenfried probably has a better idea.

There are tests for other visitors which you can use as a template:

MaxCountSkipEdgeStrategyTest
StopFinderTraverseVisitorTest

You will likely have to add methods to TestStateBuilder.

leonardehrenfried · 2023-10-10T17:40:24Z

@miles-grant-ibigroup @daniel-heppner-ibigroup @binh-dam-ibigroup You should probably know that this change is coming.

miles-grant-ibigroup · 2023-10-10T17:55:18Z

This can be disabled right? Or like not included in the query

leonardehrenfried · 2023-10-10T18:24:29Z

If you actively select what kind of items you want, nothing will change.

I don't think we haven't quite decided if the stations will be included by default or not.

daniel-heppner-ibigroup · 2023-10-10T22:19:19Z

If the default changes to include stations but we aren't looking for the station type on the nearest response, I think that means we could be missing stops with a parent station, correct? Our options would be to either support stations or set the default to only include stops, it seems. It isn't a big deal to update that, especially since the nearby view isn't merged yet on our end.

leonardehrenfried · 2023-10-11T07:56:45Z

If the default changes to include stations but we aren't looking for the station type on the nearest response, I think that means we could be missing stops with a parent station, correct?

Yes, it will start returning Station from which you have to fetch the child stops.

The safest option for you would be to explicitly select what you want and then nothing will change.

optionsome · 2023-10-11T14:58:28Z

I don't think we should return stations by default, it's not the most completely obvious selection and it has some backwards compatibility issues as stations and stops use the same type but don't contain the same information.

…TraverseVisitor

src/main/resources/org/opentripplanner/apis/gtfs/schema.graphqls

leonardehrenfried · 2023-10-12T10:14:39Z

src/main/java/org/opentripplanner/routing/graphfinder/PlaceFinderTraverseVisitor.java

@@ -80,6 +81,8 @@ public PlaceFinderTraverseVisitor(
    includeVehicleRentals = shouldInclude(filterByPlaceTypes, PlaceType.VEHICLE_RENT);
    includeCarParking = shouldInclude(filterByPlaceTypes, PlaceType.CAR_PARK);
    includeBikeParking = shouldInclude(filterByPlaceTypes, PlaceType.BIKE_PARK);
+    includeStations = shouldInclude(filterByPlaceTypes, PlaceType.STATION);


We need special handling for this as we don't want to return stations by default.

This fact needs to also documented in the query docs.

Is there some better way to do so than to simply add a check for whether filterByPlaceTypes is empty? Where exactly are the query docs that should be changed?

No, that's fine.

The docs to mentioned this are here:

OpenTripPlanner/src/main/resources/org/opentripplanner/apis/gtfs/schema.graphqls

Lines 2503 to 2506 in 7b48c4d

"""

Only return places that are one of these types, e.g. `STOP` or `VEHICLE_RENT`

"""

filterByPlaceTypes: [FilterPlaceType]

Please also add a test for this slightly strange behaviour.

Please make sure that stations are only included if you explicitly enable them.

Co-authored-by: Leonard Ehrenfried <mail@leonard.io>

leonardehrenfried · 2023-10-12T11:20:04Z

Can you please also write a test for the GraphQL aspect?

You will have to write a query like this: https://github.com/opentripplanner/OpenTripPlanner/blob/621b802d93a2eb595b645b77be21a36da7314842/src/test/resources/org/opentripplanner/apis/gtfs/queries/patterns.graphql#L1-L0 but for the nearest query.

You then need to run GraphQLIntegrationTest.

In order to get useful results for the query, you might have to write a test implementation of that interface:

OpenTripPlanner/src/main/java/org/opentripplanner/routing/graphfinder/GraphFinder.java

Line 18 in 581eae5

public interface GraphFinder {

optionsome

The stop ids filtering doesn't seem to work. Also, while testing this, I realized that the filtering is broken(?) also upstream. In OTP1 the nearest search only returned the entities that were included in the filters. In OTP2, the nearest search returns only the entities of a type that had some id filter if they were included in it but also additionally all entities of different types if there were no filters for those. Based on the documentation and how it was OTP1, I think we should only return the elements that were included in id filters if any id filters are specified for any type. I think that behaviour should be fixed within the scope of this pr to match the OTP1 behaviour.

optionsome · 2023-11-28T15:48:04Z

src/main/java/org/opentripplanner/routing/graphfinder/PlaceFinderTraverseVisitor.java

+        !seenStops.contains(stop.getParentStation().getId()) &&
+        stopIsIncludedByStationFilter(stop)


Do we need these two checks as they are already checked above?

optionsome · 2023-11-28T15:49:02Z

src/main/java/org/opentripplanner/routing/graphfinder/PlaceFinderTraverseVisitor.java

-      placesFound.add(new PlaceAtDistance(stop, distance));
+      seenStops.add(stop.getParentStation().getId());
+      placesFound.add(new PlaceAtDistance(stop.getParentStation(), distance));
+    } else if (includeStops && !seenStops.contains(stop.getId())) {


Same with this seenStops check.

src/main/resources/org/opentripplanner/apis/gtfs/schema.graphqls

src/test/java/org/opentripplanner/routing/graphfinder/PlaceFinderTraverseVisitorTest.java

optionsome · 2023-11-28T15:56:38Z

src/ext-test/resources/gtfsgraphqlapi/queries/nearest.graphql

This file is in the old resource path before the api was moved into the core code. Is this file used in tests anymore? I saw there was another test file for this query that was slightly different.

You could potentially have this is a separate test or merge this and the other test so that both DEPARTURE_ROW and STOP are returned.

optionsome · 2023-11-28T16:01:44Z

src/test/resources/org/opentripplanner/apis/gtfs/queries/nearest.graphql

The whitespace usage in this file is slightly inconsistent, there is whitespace before the last } for example.

Co-authored-by: Joel Lappalainen <lappalj8@gmail.com>

optionsome · 2023-12-08T13:30:56Z

src/main/java/org/opentripplanner/routing/graphfinder/PlaceFinderTraverseVisitor.java

+      (includeStations && !stop.isPartOfStation() && !stopIsIncludedByStopFilter(stop)) ||
+      (!includeStations && !stopIsIncludedByStopFilter(stop)) ||
+      (stop.isPartOfStation() && !stopIsIncludedByStationFilter(stop)) ||


This part is a bit hard to follow but I don't know what to do about it.

I would put each part in between || into a separate method which has a name and possibly a bit of Javadoc.

This doesn't make the logic less complicated but helps someone else understand what each piece does.

optionsome · 2023-12-08T13:37:05Z

src/main/java/org/opentripplanner/routing/graphfinder/PlaceFinderTraverseVisitor.java

+    } else if (includeStations && stop.getParentStation() != null) {
+      seenStops.add(stop.getParentStation().getId());
+      placesFound.add(new PlaceAtDistance(stop.getParentStation(), distance));
+    } else if (includeStops) {
      seenStops.add(stop.getId());
+      placesFound.add(new PlaceAtDistance(stop, distance));
    }


Isn't it enough to have just these two checks and remove the first if? If I'm not missing anything, I think this also covers the case where both includeStops and includeStations is true.

src/main/java/org/opentripplanner/routing/graphfinder/PlaceFinderTraverseVisitor.java

Co-authored-by: Leonard Ehrenfried <mail@leonard.io>

src/main/java/org/opentripplanner/apis/gtfs/datafetchers/QueryTypeImpl.java

leonardehrenfried

We are getting close - just the refactoring of the complex if statement.

Co-authored-by: Leonard Ehrenfried <mail@leonard.io>

optionsome · 2023-12-14T12:47:48Z

While testing that the transmodel API still works, I noticed there is a multiModalMode argument in the transmodel API's nearest query. What does that do @t2gran . Is it similar to what we are trying to do in the scope of this pr? The search also outputs StopPlace, is that in practice the same thing as a station?

optionsome · 2023-12-14T12:48:20Z

Also, should we also mark the transmodel APIs filterByIds as deprecated?

optionsome · 2023-12-14T21:54:08Z

I tested this a bit more and checked the transmodel API code. Transmodel API returns both the quay (stop in gtfs terminology) that belongs to a StopPlace (sort of the same thing as station in gtfs) and the StopPlace itself. This differs from the behaviour we designed for the GTFS API in this pr.

The code that makes the transmodel API return the StopPlaces resides in the transmodel API code and it modifies the results returned by the method that is used by the GTFS API also.

It might make sense to remove the transmodel API specific code and reuse the logic in the shared core code but I guess we need to keep the transmodel API behaviour as is and it would complicate the core code even more. I don't really want to put the burden of refactoring this to @nurmAV within the scope of this pr.

Add option to include stations in stop search

c2d17db

nurmAV requested a review from a team as a code owner October 3, 2023 13:30

Fix formatting

b3ce4df

nurmAV mentioned this pull request Oct 3, 2023

fix: show more nearby stops that are not part of station HSLdevcom/digitransit-ui#4870

Merged

optionsome requested changes Oct 3, 2023

View reviewed changes

leonardehrenfried changed the title ~~Add option to include stations in stop search~~ Add option to include stations in nearby search Oct 10, 2023

leonardehrenfried changed the title ~~Add option to include stations in nearby search~~ Add option to include stations in nearby search Oct 10, 2023

t2gran added this to the 2.5 (next release) milestone Oct 10, 2023

leonardehrenfried changed the title ~~Add option to include stations in nearby search~~ Add option to include stations in nearest search Oct 10, 2023

viljaminurminen-cgi added 5 commits October 12, 2023 11:12

Add place type for station and modify handleStop logic in PlaceFinder…

327067d

…TraverseVisitor

Remove unused code

a2a3bb4

Remove further unused code

94ea8c3

Add tests for PlaceFinderTraverseVisitor

e860a68

Optimize imports

699bb47

leonardehrenfried reviewed Oct 12, 2023

View reviewed changes

src/main/resources/org/opentripplanner/apis/gtfs/schema.graphqls Outdated Show resolved Hide resolved

leonardehrenfried reviewed Oct 12, 2023

View reviewed changes

src/main/resources/org/opentripplanner/apis/gtfs/schema.graphqls Outdated Show resolved Hide resolved

leonardehrenfried reviewed Oct 12, 2023

View reviewed changes

Apply suggestions from code review

895085f

Co-authored-by: Leonard Ehrenfried <mail@leonard.io>

viljaminurminen-cgi added 2 commits October 17, 2023 09:39

Add a nearest query to GraphQLIntegrationTest

d367d1a

Merge branch 'otp-dev-2.x' into DT-5799

1dc5cf8

viljaminurminen-cgi added 2 commits November 28, 2023 16:52

Fix how stops are created in tests

b2f04a0

Fix nearest query and response in integration test

a5fa1f4

optionsome requested changes Nov 28, 2023

View reviewed changes

nurmAV and others added 5 commits November 30, 2023 08:43

Apply feedback from review

e5892a2

Co-authored-by: Joel Lappalainen <lappalj8@gmail.com>

Remove redundant seenStops checks

b373fc8

Fix stop filtering when only searcing for stops

3f51bb6

Deprecate filterByIds in nearest search

b4dff93

Remove needless test in old API location

a08b8e4

optionsome reviewed Dec 8, 2023

View reviewed changes

viljaminurminen-cgi added 2 commits December 12, 2023 09:10

Remove unnecessary logic

bd20cba

Clean up code

7092574

leonardehrenfried reviewed Dec 12, 2023

View reviewed changes

src/main/java/org/opentripplanner/routing/graphfinder/PlaceFinderTraverseVisitor.java Outdated Show resolved Hide resolved

leonardehrenfried self-requested a review December 12, 2023 09:25

Apply feedback from review

8627c7e

Co-authored-by: Leonard Ehrenfried <mail@leonard.io>

leonardehrenfried reviewed Dec 12, 2023

View reviewed changes

src/main/java/org/opentripplanner/apis/gtfs/datafetchers/QueryTypeImpl.java Outdated Show resolved Hide resolved

leonardehrenfried requested changes Dec 12, 2023

View reviewed changes

viljaminurminen-cgi and others added 3 commits December 12, 2023 15:28

Improve readability of complex if statement

1bc6785

Apply feedback from review

57d064c

Co-authored-by: Leonard Ehrenfried <mail@leonard.io>

Fix formatting

fb202d8

leonardehrenfried previously approved these changes Dec 12, 2023

View reviewed changes

Merge branch 'otp-dev-2.x' into DT-5799

fd1973d

nurmAV dismissed leonardehrenfried’s stale review via fd1973d December 19, 2023 10:10

leonardehrenfried approved these changes Dec 19, 2023

View reviewed changes

optionsome approved these changes Dec 19, 2023

View reviewed changes

optionsome merged commit 5ed8b70 into opentripplanner:dev-2.x Dec 19, 2023
5 checks passed

optionsome deleted the DT-5799 branch December 19, 2023 11:51

t2gran pushed a commit that referenced this pull request Dec 19, 2023

Add changelog entry for #5390 [ci skip]

0f353fd

	"""
	Only return places that are one of these types, e.g. `STOP` or `VEHICLE_RENT`
	"""
	filterByPlaceTypes: [FilterPlaceType]

		!seenStops.contains(stop.getParentStation().getId()) &&
		stopIsIncludedByStationFilter(stop)

Add option to include stations in nearest search #5390

Add option to include stations in nearest search #5390

Conversation

nurmAV commented Oct 3, 2023

Summary

Unit tests

codecov bot commented Oct 3, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonardehrenfried Oct 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

optionsome commented Oct 3, 2023

leonardehrenfried commented Oct 3, 2023 • edited

leonardehrenfried commented Oct 10, 2023

miles-grant-ibigroup commented Oct 10, 2023

leonardehrenfried commented Oct 10, 2023

daniel-heppner-ibigroup commented Oct 10, 2023

leonardehrenfried commented Oct 11, 2023

optionsome commented Oct 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonardehrenfried commented Oct 12, 2023

optionsome left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonardehrenfried left a comment

Choose a reason for hiding this comment

optionsome commented Dec 14, 2023

optionsome commented Dec 14, 2023

optionsome commented Dec 14, 2023

Add option to include stations in `nearest` search #5390

Add option to include stations in `nearest` search #5390

codecov bot commented Oct 3, 2023 •

edited

leonardehrenfried Oct 5, 2023 •

edited

leonardehrenfried commented Oct 3, 2023 •

edited

optionsome left a comment •

edited