Store all alerts and add support for route_type and direction_id selectors by optionsome · Pull Request #3780 · opentripplanner/OpenTripPlanner

optionsome · 2021-12-13T09:15:01Z

Summary

Add support for route_type and direction_id selectors
Store entities with no or unhandled entity selectors with Unknown entity selector so they are kept instead of ignored
- Currently for SIRI, only do this at the end of processing an alert so the case of no entitiy selectors or known entity selectors is handled as one Unknown selector
Implement alert functionality in Legacy GraphQL API
- Add RouteType, DirectionOnRoute and Unknown types that are used as alert entitites
- Implement alerts fields and add types parameter with array of enum options
- Add alerts field to feed

Issue

closes #3779, #3486

Unit tests

Added unit tests for the alert updaters

Code style

Have you followed the suggested code style?
Yes

Documentation

Is there some documentation that should be updated?

Changelog

From title and added changelog for sandbox features

…egacy graphQL API

…and add routeType to Route

…routes alerts

optionsome

I'm a bit unsure if we should store the alerts with the original selectors and expose them through APIs that way or, for example, create as many pattern selectors as there are patterns for a direction in route's direction and expose the route_id+direction_id alerts directly through the pattern/trip types in APIs and hide the original selector structure from the API users. I think there are some use cases where users would want to display alerts that are targeted for a "super type" that covers multiple "entities" differently than when an alert is directly targeted for an individual entity.

optionsome · 2021-12-13T09:17:12Z

+                TripPattern tripPattern = getRoutingService(environment).getTripPatternForId(id);
+                return getAlertEntityOrUnknown(tripPattern, id.toString(), "trip pattern");
              }
              if (entitySelector instanceof EntitySelector.StopAndRoute) {


We could also validate that the stop is actually on the route/trip but I think it's a valid use case that you create an alert "Route x no longer stops at stop y" and want to display that on route, for example.

Decided that alerts should have the original XandY entity type even though they are not really linked together (anymore).

optionsome · 2021-12-13T09:19:36Z

+                      trip -> alerts.addAll(alertService.getTripAlerts(trip.getId(), null)));
+              break;
+            case StopsOnRoute:
+              getStops(environment).forEach(stop -> {


After I implemented this so it checks that the alerts for stops that are on route, I realised that maybe it would make sense to just fetch all StopAndRoute type alerts that have the route defined here as it might be useful to display alerts that warn that a stop is no longer on the route, for example.

Changed this so that the stop doesn't need to be on the route/trip/pattern nor do the route/trip have to go through the stop in order to return these StopAndRoute/Trip alerts.

optionsome · 2021-12-13T09:21:07Z

+                                        situationNumber
+                                );
                            }
+                            alert.addEntity(new EntitySelector.Unknown("Alert had no entities that could be handled"));


Maybe it would also be possible to add Unknown selector each time a selector was ignored because we don't handle it yet instead of adding one Unknown selector at the end like this.

Should the "add unknown entity" be inside the if-block?

Indeed... I'll fix that and update tests a bit so they will catch if suddenly new entities appear.

Fixed it now and changed tests so that they test that only the intended entities get added instead of checking that x type entities don't get added.

optionsome · 2021-12-13T09:21:41Z

+Direction on a route. In practice,
+this means the trips/patterns of a route that have the defined direction.
+"""
+type DirectionOnRoute {


Maybe it would be useful to expose the affected trips/patterns through this type.

Removed this type and instead generated Pattern selectors from the DirectionAndRoute entity selector.

optionsome · 2021-12-13T09:22:36Z

+Route type entity which covers all agencies if agency is null,
+otherwise only relevant for one agency.
+"""
+type RouteType {


Maybe it would be useful to expose the affected routes through this type.

Implemented this.

optionsome · 2021-12-13T09:25:07Z

-            } else {
-                direction = -1;
+            int directionId = MISSING_INT_FIELD_VALUE;
+            if (informed.hasDirectionId()) {


Should we also handle informed.getTrip().getDirectionId() here. I'm not quite sure what is the difference between them other than that the trip descriptor's direction was added to the spec first(?). I have a feeling that the trip descriptor's directionId should be used for fuzzy trip matching to find individual trips instead of using it just together with the route_id.

We decided that the TripDescriptor's direction_id should only be used for fuzzy matching.

optionsome · 2021-12-13T09:26:46Z

                        new FeedScopedId(feedId, routeId)
                    ));
+                } else if (directionId != MISSING_INT_FIELD_VALUE) {
+                    alertText.addEntity(new EntitySelector.DirectionAndRoute(


I decided to add this new DirectionAndRoute type instead of using the Pattern selector. The pattern selector is not being used for anything, can it be removed or should this be changed to use it?

Removed TripPattern entity selector as it is not being used for anything and couldn't think of a way it could be used in the near future either.

…ates

…ate alerts for patterns

…together

…ates

leonardehrenfried

I would really like to see the filter logic being made a lot more readable by reducing the multi-line, multi-indented filter functions into smaller bits.

Also, I think the filter classes can share a significant part of the code. I see lots of repetition.

t2gran · 2022-01-06T19:21:36Z

We discussed this PR today, at the developer meeting. I agree with @leonardehrenfried that both GraphQL APIs is kind of ugly, but I hope we can get rid of them, not too long time into the future. They are also both considered "Sandbox" features so I am unsure if they are the right place to spent time cleaning up.

@optionsome wrote:

I'm a bit unsure if we should store the alerts with the original selectors and expose them through APIs that way or, for example, create as many pattern selectors as there are patterns for a direction in route's direction and expose the route_id+direction_id alerts directly through the pattern/trip types in APIs and hide the original selector structure from the API users. I think there are some use cases where users would want to display alerts that are targeted for a "super type" that covers multiple "entities" differently than when an alert is directly targeted for an individual entity.

I can only give a general advice here, but this what I think: OTP is a travel planner and we keep some relevant information (like the Index API). To solve the problem (OTP features) we sometimes transform input models(pattern selectors) into simpler or easier to work with data structures - if these are not sufficient to recreate the input, then we kindly ask the consumer requesting the information to look at the input instead. @optionsome you know much better than me, what is right in this case. You have my support if you want to reduce the "information value" if that lead to a simpler API for most clients or the maintenance of OTP become easier.

optionsome · 2022-01-07T14:53:56Z

Indeed there is a lot of room for improvement in the GraphQL API code, both to make it more readable and also to optimize the performance. I was aware while writing this code that at least some parts of it need to be rewritten soon anyway as @t2gran said, therefore I didn't spend too much time on polishing the code. We should consider @leonardehrenfried 's comments when implementing the new API but I also feel like that maybe we shouldn't spend too much time on improving the legacy GraphQL API at this point.

I refactored the siri test changes as it was easy to do while keeping it still simple to read hopefully 2eb024a.

optionsome · 2022-01-07T15:07:47Z

Does anyone have a clue why the code suddenly doesn't build in github actions? I only touched some tests a bit after last successful build and locally it compiles for me at least?

…ates

t2gran

I do not have much to contribute here, but as @leonardehrenfried alreaddy pointed out there are some repetition between the two APIs - and it seem that the APIs also do quite a bit of generic business logic - if so that logic belongs in the core of OTP, not in the API mapping.

A good guidance is that logic witch is use-case specific should remain in the API mappers while reusable logic should be pushed to domain model. We can do this cleanup as part of this PR or later, when doing the new API.

We will clean up the APIs when working on the new API

optionsome added 17 commits November 30, 2021 16:49

Include alerts with no or unhandled entities

9b8bb16

Add unknown entity to legacy graphQL api

ac94831

Refactor Unknown selector to include description

3fb6009

Update javadoc

372ddc3

Include route_type entities from GTFS RT and add initial support to l…

8aacb2b

…egacy graphQL API

Rename models to avoid conflict, make RouteType entities feed scoped …

b1bc518

…and add routeType to Route

Add option to get route type and other relevant alerts directly from …

b8456ef

…routes alerts

Handle direction and route combo from gtfs rt alerts

106767a

Couple of minor fixes to route alerts impl

ca33f14

Implement use of DIRECTION_ON_ROUTE arg for route alerts

e71d2a8

Refactor to use switch instead of if else mess

43f05da

Fix copypaste mistake

51e71e9

Add missing breaks

12e6c44

Implement, add and remove alerts for types

40d1b3c

If an entity is not found, return Unknown entity

94434fc

Rename direction -> directionId

524ad21

Add impl for DirectionOnRoute

305a9aa

optionsome requested a review from a team as a code owner December 13, 2021 09:15

optionsome commented Dec 13, 2021

View reviewed changes

Add tests for route_type entities

19eeebd

t2gran added this to the 2.1 milestone Dec 13, 2021

optionsome added 7 commits December 13, 2021 16:55

Don't always add Unknown entity to siri alerts and update tests

daa1333

Merge remote-tracking branch 'upstream/dev-2.x' into alert-entity-upd…

000b09b

…ates

Remove unnecessary TODO

87734f6

Remove TripPattern entity selector and use DirectionAndRoute to gener…

b1d8f98

…ate alerts for patterns

Return StopAndRoute/Trip type alerts regardless if the pair is still …

05ee1a8

…together

Add routes to RouteType

44f83a0

Update sandbox changelogs

0a1e2a1

t2gran added !New Feature A functional feature targeting the end user. Regression labels Dec 16, 2021

t2gran requested a review from leonardehrenfried January 4, 2022 10:00

Merge remote-tracking branch 'upstream/dev-2.x' into alert-entity-upd…

596e422

…ates

leonardehrenfried previously requested changes Jan 5, 2022

View reviewed changes

Clean code

2eb024a

t2gran self-requested a review January 11, 2022 10:32

t2gran added the +Sandbox This will be implemented as a Sandbox feature label Jan 13, 2022

optionsome added 2 commits January 18, 2022 12:55

Merge remote-tracking branch 'upstream/dev-2.x' into alert-entity-upd…

a047221

…ates

Update route type function calls

e9a8ed4

t2gran approved these changes Jan 24, 2022

View reviewed changes

hannesj approved these changes Jan 25, 2022

View reviewed changes

optionsome merged commit bc6cc6d into opentripplanner:dev-2.x Jan 25, 2022

optionsome deleted the alert-entity-updates branch January 25, 2022 10:13

t2gran pushed a commit that referenced this pull request Jan 25, 2022

Add changelog entry for #3780 [ci skip]

3031654

t2gran removed the Regression label Sep 24, 2025

Uh oh!

Conversation

optionsome commented Dec 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Issue

Unit tests

Code style

Documentation

Changelog

Uh oh!

optionsome left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

t2gran commented Jan 6, 2022

Uh oh!

optionsome commented Jan 7, 2022

Uh oh!

optionsome commented Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

t2gran left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

optionsome commented Dec 13, 2021 •

edited

Loading

optionsome commented Jan 7, 2022 •

edited

Loading