Vehicle rental updates by optionsome · Pull Request #3632 · opentripplanner/OpenTripPlanner

optionsome · 2021-10-05T07:48:22Z

Summary

Updates Smoove bike rental data source
Properly implements all bike rental station fields in the legacy GraphQL API
Adds allowPickup, allowPickupNow, allowDropoffNow and operative to the legacy GraphQL API
Updates file generation for the legacy GraphQL API

Issue

closes #3631

Unit tests

Updated tests

Code style

Have you followed the suggested code style?

Yes

Documentation

Updated sandbox documentation

Changelog

TODO

…QL API

…acy API

t2gran · 2021-10-05T08:57:15Z

I have looked at the core changes and those looks ok, so feel free to merge this when it is ready. We will update the Transmodel API after this get merged.

leonardehrenfried · 2021-10-05T20:00:15Z

This looks good. I have a questions: does upgrading the codegen libs solve the problem of manually having to fix the enum code?

hannesj · 2021-10-05T20:09:01Z

This looks good. I have a questions: does upgrading the codegen libs solve the problem of manually having to fix the enum code?

Yes, that was fixed in a recent version of the plugin

optionsome · 2021-10-07T07:23:48Z

We discussed this on tuesday's meeting and decided that it's better if we split the floating vehicles into own query type in the GraphQL APIs. I'll do that for the legacy GraphQL API in the scope of this pr.

optionsome · 2021-10-18T13:41:56Z

This looks good. I have a questions: does upgrading the codegen libs solve the problem of manually having to fix the enum code?

Noticed today that there are still some issues with enum type arguments that to my knowledge require editing the LegacyGraphQLTypes.java file manually. For example for the routes query arguments, the following code gets now generated:

if (args.get("transportModes") != null) {
    this.transportModes = (Iterable<LegacyGraphQLMode>) args.get("transportModes");
}

which needs to be edited to

if (args.get("transportModes") != null) {
    this.transportModes = ((List<String>) args.get("transportModes")).stream()
        .map(LegacyGraphQLMode::valueOf)
        .collect(Collectors.toList());
}

… runtime

…egacy graphql API

optionsome

There is still only possible to filter by ids using "bikeRentalStations" in the nearest query. This could be changed but I'm not sure how often this functionality is even used. Also it would be nice if it was possible to filter nearest query so you would only get vehicles or stations.

optionsome · 2021-10-19T07:40:22Z

+
+      if (args.getLegacyGraphQLIds() != null) {
+        ArrayListMultimap<String, VehicleRentalStation> vehicleRentalStations =
+                vehicleRentalStationService.getVehicleRentalStations()


Should there be a helper method for getting only vehicles/stations in VehicleRentalStationService and/or should we store the vehicles and the stations in separate Maps there?

I think a helper method would suffice

Ok that's done now.

optionsome · 2021-10-19T07:43:18Z

+    id: ID!
+
+    """ID of the vehicle in the format of network:id"""
+    vehicleId: String


Should this id have a more generic term that could be shared between RentalVehicle and VehicleRentalStation?

hannesj

Looks good for the common code, minor comments for the sandbox.

hannesj · 2021-10-19T08:14:45Z

+
+      if (args.getLegacyGraphQLIds() != null) {
+        ArrayListMultimap<String, VehicleRentalStation> vehicleRentalStations =
+                vehicleRentalStationService.getVehicleRentalStations()


I think a helper method would suffice

hannesj · 2021-10-19T08:18:26Z

+      SelectionSet set = environment.getField().getFields().get(0).getSelectionSet();
+      boolean queryHasBikeRentalStationFragment = set != null && set.getSelections()
+              .stream()
+              .filter(selection -> selection instanceof InlineFragment)


What about FragmentDefinition?

Seems like we can only see the name of a FragmentDefinition from TypeResolutionEnvironment. So currently, we can't add support for deciding which object type to return based on those types of fragments so we will always return VehicleRentalStation. This breaks backwards compatibility a bit but we only use inline fragments for the nearest query in our UI so it's ok to have this problem for a while. I created an issue in graphql-java project about adding more information about fragments to the environment so this can be fixed later.

optionsome · 2021-10-21T08:49:31Z

On Tuesday's discussion, we decided to evaluate if https://github.com/kobylynskyi/graphql-java-codegen could be used to replace the code generation based on the GraphQL schema but I won't do it in the scope of this pull request. I think this pull request is now ready for final review.

hannesj · 2021-10-21T09:21:08Z

    VehicleRentalStationService service = graph.getVehicleRentalStationService();
    if (service == null) {return List.of();}
-    return service.getVehicleRentalStations()
+    return service.getVehicleRentalPlaces()


We should probably split this layer in two, so that they can be showed on different zoom levels. This can be done later

t2gran · 2021-10-21T13:37:34Z

    public ZonedDateTime lastReported;

    // OTP internal data
+    public boolean allowOverloading = false;


Add some JavaDoc to this.

There is some javadoc in the VehicleRentalPlace for it. All fields here seem to be missing javadoc. There's currently no mention anywhere that this information is not in the GBFS spec but I'm not sure what would be the approriate place for it.

You could just link to the VehicleRentalPlace in the class JavaDoc then. I try to avoid redundant documentation:

/** See {link VehicleRentalPlace} ... */

Should that be done for this field and/or whole class and/or all fields/methods?

I added a couple of comments that link the implementing classes to the interface class.

…pdates

optionsome and others added 9 commits October 4, 2021 16:35

Implement allowOverloading as a config parameter for Smoove updater

7f58d4f

Implement bike rental station state

5213923

Add capacity to smoove stations and implement capacity in legacy Grap…

cdb4cfc

…QL API

Add tests

e1d40fe

Fix formatting

acc4d3c

Rerun legacy graphql codegen

22b19b0

Update test snapshot

78ec465

Fix isAllowPickup method for vehicles

0b6a11f

Add allowPickup, allowPickupNow, allowDropoffNow and operative to leg…

b886bf1

…acy API

optionsome requested a review from a team as a code owner October 5, 2021 07:48

Update sandbox Changelogs

34376ac

t2gran added this to the 2.1 milestone Oct 5, 2021

t2gran added the +Sandbox This will be implemented as a Sandbox feature label Oct 5, 2021

leonardehrenfried previously approved these changes Oct 5, 2021

View reviewed changes

Fix capacity type for getter

999d8dc

optionsome added 3 commits October 18, 2021 23:21

Use older version of @graphql-codegen/java to spot enum issues before…

3106df3

… runtime

Do initial steps towards separating rental vehicles and stations in l…

eecfe22

…egacy graphql API

Update changelog

8496f17

optionsome dismissed leonardehrenfried’s stale review via 8496f17 October 18, 2021 20:27

Use VehicleRentalStation for node and place resolvers by default

f2361d0

optionsome commented Oct 19, 2021

View reviewed changes

hannesj previously approved these changes Oct 19, 2021

View reviewed changes

Refactor and add station/vehicle getters to to service

c86f9dd

optionsome dismissed hannesj’s stale review via c86f9dd October 21, 2021 08:30

hannesj reviewed Oct 21, 2021

View reviewed changes

Use vehicle rental stations instead of places in transmodel API

cb8507f

hannesj previously approved these changes Oct 21, 2021

View reviewed changes

t2gran reviewed Oct 21, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/dev-2.x' into vehicle-rental-u…

9f15b05

…pdates

optionsome dismissed hannesj’s stale review via 9f15b05 October 22, 2021 11:56

Add couple of javadoc links

69829e6

t2gran approved these changes Nov 2, 2021

View reviewed changes

hannesj approved these changes Nov 2, 2021

View reviewed changes

optionsome merged commit f72f1fb into opentripplanner:dev-2.x Nov 2, 2021

optionsome deleted the vehicle-rental-updates branch November 2, 2021 20:43

t2gran pushed a commit that referenced this pull request Nov 2, 2021

Add changelog entry for #3632 [ci skip]

87123eb

Conversation

optionsome commented Oct 5, 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!

t2gran commented Oct 5, 2021

Uh oh!

leonardehrenfried commented Oct 5, 2021

Uh oh!

hannesj commented Oct 5, 2021

Uh oh!

optionsome commented Oct 7, 2021

Uh oh!

optionsome commented Oct 18, 2021

Uh oh!

optionsome left a comment

Choose a reason for hiding this comment

Uh oh!

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

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!

hannesj 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!

optionsome commented Oct 21, 2021

Uh oh!

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

optionsome commented Oct 5, 2021 •

edited

Loading

optionsome Oct 19, 2021 •

edited

Loading