Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refetch transit leg with a leg query of GTFS GraphQL API #6045

Merged
merged 29 commits into from
Sep 27, 2024

Conversation

vesameskanen
Copy link
Contributor

@vesameskanen vesameskanen commented Sep 5, 2024

Summary

GTFS leg object now includes a graphql node id . Transit leg can be refetched using a new leg query:

query legQuery { leg(id: "foo") { mode start { estimated { time } } } }

Such query can be used to update an itinerary and to detect if the itinerary is no longer possible because of delays or canceled trips.

Non-transit legs cannot be refetched - the response is always null.

This PR also adds missing implementation for querying leg's realtimeState via GTFS graphql API.

Documentation

In the graphql schema.

@vesameskanen vesameskanen requested a review from a team as a code owner September 5, 2024 10:42
@leonardehrenfried leonardehrenfried changed the title Add support for refetching transit leg using the node query of GTFS graphql API Refetch transit leg with node query of GTFS Graphql API Sep 5, 2024
@leonardehrenfried leonardehrenfried changed the title Refetch transit leg with node query of GTFS Graphql API Refetch transit leg with node query of GTFS GraphQL API Sep 5, 2024
@leonardehrenfried
Copy link
Member

@binh-dam-ibigroup This might be interesting for you.

@vesameskanen I almost cannot believe that it is so easy!

Copy link

codecov bot commented Sep 5, 2024

Codecov Report

Attention: Patch coverage is 48.14815% with 14 lines in your changes missing coverage. Please review.

Project coverage is 69.83%. Comparing base (c022422) to head (857b815).
Report is 71 commits behind head on dev-2.x.

Files with missing lines Patch % Lines
...pplanner/apis/gtfs/datafetchers/QueryTypeImpl.java 12.50% 7 Missing ⚠️
...ntripplanner/apis/gtfs/generated/GraphQLTypes.java 0.00% 7 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #6045      +/-   ##
=============================================
+ Coverage      69.81%   69.83%   +0.01%     
- Complexity     17418    17460      +42     
=============================================
  Files           1974     1975       +1     
  Lines          74537    74624      +87     
  Branches        7633     7638       +5     
=============================================
+ Hits           52040    52115      +75     
- Misses         19847    19857      +10     
- Partials        2650     2652       +2     

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

@optionsome
Copy link
Member

@vesameskanen I almost cannot believe that it is so easy!

This is easy because this was already done in the transmodel API. However, the transmodel API has own legs query for this because it lacks the node query/interface.

@optionsome
Copy link
Member

@vesameskanen will still add some integration tests for this.

@vesameskanen
Copy link
Contributor Author

vesameskanen commented Sep 6, 2024

GTFS integration tests currently cannot test leg refetching, because mocked itineraries lack essential data used in encoding the leg information. Integration test now tests only that legs contain the id.

@vesameskanen vesameskanen marked this pull request as draft September 11, 2024 11:15
@vesameskanen
Copy link
Contributor Author

vesameskanen commented Sep 11, 2024

I tested new legs with relay and got lots of problems. Relay does not accept nodes with same node id but different content. Either all leg types must be serialized to unique id ('NotAvailable' is not OK), or we have to revert node interface for legs and add a dedicated leg query. Any opinions?

@leonardehrenfried
Copy link
Member

leonardehrenfried commented Sep 11, 2024

Maybe I don't get it but don't all legs have the same type in GraphQL?

@optionsome
Copy link
Member

Maybe I don't get it but don't all legs have the same type in GraphQL?

@vesameskanen explained to me that the issue is that the non-transit legs all have the same id. We discussed this today and thought maybe the solution would be to generate unique ids based on actual leg data (such as departure/arrival time) even if we don't support refetching based on those ids.

I tried adding tests for refetching the legs through the node query in the graphql integration tests on Tuesday but creating/maintaining such test would be slightly difficult.

@vesameskanen vesameskanen changed the title Refetch transit leg with node query of GTFS GraphQL API Refetch transit leg with a leg query of GTFS GraphQL API Sep 26, 2024
Copy link
Member

@optionsome optionsome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might need to discuss my suggestion in a developer meeting. Idk if we should give entities global ids even if they are not refetchable with a node query. I think at least in netex, the ids are always unique and have the type in the id (if I'm not mistaken).

Comment on lines 333 to 341
public DataFetcher<Relay.ResolvedGlobalId> id() {
return environment -> {
var ref = getSource(environment).getLegReference();
if (ref == null) {
return null;
}
var id = LegReferenceSerializer.encode(ref);
return new Relay.ResolvedGlobalId("Leg", id);
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should use the global id anymore, since we have own query for this now, we can just return a string which is the output of the LegReferenceSerializer.encode(ref).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using plain string is indeed simpler. Changed.

@optionsome
Copy link
Member

I think since the leg reference has "SCHEDULED_TRANSIT_LEG_V3" in the id (example base64 decoded id ��wh��SCHEDULED_TRANSIT_LEG_V3��HSL:3002Y_20240801_To_1_1223� 2024-09-26����������HSL:1174504��HSL:2611502��), I don't think it's necessary to prefix the id with "Leg" but since it's a small effort, we can still discuss this in the dev meeting.

@optionsome
Copy link
Member

We decided to just use the LegReferenceSerializer.encode(ref) as is instead of encoding it again with the relay library. The id can be a String in the schema.

…pl.java

Co-authored-by: Joel Lappalainen <lappalj8@gmail.com>
Co-authored-by: Joel Lappalainen <lappalj8@gmail.com>
@vesameskanen vesameskanen merged commit 2cfbd7b into opentripplanner:dev-2.x Sep 27, 2024
5 checks passed
@vesameskanen vesameskanen deleted the gfts-leg-node-refetch branch September 27, 2024 07:46
@@ -189,10 +190,12 @@ public DataFetcher<Boolean> realTime() {
return environment -> getSource(environment).getRealTime();
}

// TODO
@Override
public DataFetcher<String> realtimeState() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realised that this doesn't use a proper mapper, which I think is a very useful thing.

Since I already approved, I wrote the code myself: leonardehrenfried@a763bc5

If you want you can cherry pick it into this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, too late. I will open another one myself.

@miklcct
Copy link
Contributor

miklcct commented Oct 10, 2024

My GTFS has an original_trip_id in addition to trip_id. If there is a timetable change affecting only a few days in the original calendar, a new trip_id has to be generated because it must be unique, however the original_trip_id field is the same.

In this case, can it be used to fetch the updated timetable?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants