Skip to content

Add vehicleRentalsByBbox query to GTFS GraphQL API#6186

Merged
optionsome merged 20 commits into
opentripplanner:dev-2.xfrom
JustCris654:add_vehicleRentalStationsByBbox
Nov 21, 2024
Merged

Add vehicleRentalsByBbox query to GTFS GraphQL API#6186
optionsome merged 20 commits into
opentripplanner:dev-2.xfrom
JustCris654:add_vehicleRentalStationsByBbox

Conversation

@JustCris654

@JustCris654 JustCris654 commented Oct 22, 2024

Copy link
Copy Markdown
Contributor

Summary

Add vehicleRentalStationsByBbox query to gtfs graphql schema

Unit tests

Documentation

I added the proper comments in the schema.graphqls file

@codecov

codecov Bot commented Oct 22, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 57.50000% with 17 lines in your changes missing coverage. Please review.

Project coverage is 69.72%. Comparing base (559fac5) to head (f28273d).
Report is 398 commits behind head on dev-2.x.

Files with missing lines Patch % Lines
...ntripplanner/apis/gtfs/generated/GraphQLTypes.java 0.00% 15 Missing ⚠️
...pis/gtfs/datafetchers/RentalPlaceTypeResolver.java 75.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #6186      +/-   ##
=============================================
- Coverage      69.90%   69.72%   -0.18%     
+ Complexity     17722    17714       -8     
=============================================
  Files           1998     2010      +12     
  Lines          75443    75898     +455     
  Branches        7718     7766      +48     
=============================================
+ Hits           52739    52921     +182     
- Misses         20025    20265     +240     
- Partials        2679     2712      +33     

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


🚨 Try these New Features:

@JustCris654
JustCris654 marked this pull request as ready for review October 22, 2024 13:54
@JustCris654
JustCris654 requested a review from a team as a code owner October 22, 2024 13:54
@t2gran t2gran added this to the 2.7 (next release) milestone Oct 22, 2024
Comment thread application/src/main/resources/org/opentripplanner/apis/gtfs/schema.graphqls Outdated

@optionsome optionsome left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Comment thread application/src/main/resources/org/opentripplanner/apis/gtfs/schema.graphqls Outdated
@optionsome

Copy link
Copy Markdown
Member

What is your use case for this btw? Are you only interested in the stations, not floating vehicles? Would it make more sense to use a vector map layer to achieve something similar to this?

@JustCris654

Copy link
Copy Markdown
Contributor Author

If possible I prefer to keep it a graphql query because we have a backend that calls OTP and add some custom data to the returned object.
You're right about the floating vehicles, I need also them, what's the best way to add them? I should add another query for them because the objects are different for stations and floating vehicles, right?

@optionsome

Copy link
Copy Markdown
Member

You're right about the floating vehicles, I need also them, what's the best way to add them? I should add another query for them because the objects are different for stations and floating vehicles, right?

There are two options. Either separate queries or one query that returns a new interface/union type which we could call VehicleRentalPlace. Perhaps using an interface could make sense here since these two types have some fields in common. Then you can call the query something like vehicleRentalPlacesByBbox return [VehicleRentalPlace!]! (for lists, I think we should always aim to make the list and its values non-null, return empty list if nothing is found). I think we call the interface VehicleRentalPlace within OTP code but we can also discuss if that is the best name in this context and do we need to prefix or postfix it with the word "Interface".

@JustCris654

JustCris654 commented Oct 29, 2024

Copy link
Copy Markdown
Contributor Author

I think the easiest way to do this is to have two different queries for stations and free floating but I also like the idea of having one single query that does both.
I thought of something like this but I'm not sure of the implementation because I surely can't use the VehicleRentalPlace interface to represent every field, for example availableVehicles: RentalVehicleEntityCounts can't be added in VehicleRentalPlace in my opinion because logically is a property of a station. We could add only the number of available vehicles but we would lose their types so it's still not the best solution.
Do you have any opinion about this?

type VehicleRentalPlace implements Node PlaceInterface {
  """
  If true, vehicles can be returned to this station if the station has spaces available
  or allows overloading.
  """
  allowDropoff: Boolean
  "If true, vehicles can be currently returned to this station."
  allowDropoffNow: Boolean
  "If true, vehicles can be returned even if spacesAvailable is zero or vehicles > capacity."
  allowOverloading: Boolean
  "If true, vehicles can be picked up from this station if the station has vehicles available."
  allowPickup: Boolean
  "If true, vehicles can be currently picked up from this station."
  allowPickupNow: Boolean
  "Nominal capacity (number of racks) of the rental station. One if free floating vehicle."
  capacity: Int
  "Number of vehicles currently available on the rental station, grouped by vehicle type."
  availableVehicles: RentalVehicleEntityCounts
  "Global object ID provided by Relay. This value can be used to refetch this object using **node** query."
  id: ID!
  "Latitude of the vehicle (WGS 84)"
  lat: Float
  "Longitude of the vehicle (WGS 84)"
  lon: Float
  "Name of the vehicle"
  name: String!
  "If true, station is on and in service."
  operative: Boolean
  "The vehicle rental network information. This is referred as system in the GBFS terminology."
  rentalNetwork: VehicleRentalNetwork!
  "Platform-specific URLs to begin the vehicle."
  rentalUris: VehicleRentalUris
  "ID of the vehicle in the format of network:id"
  stationId: String
  "Vehicle current fuel percentage. Highest battery percent among vehicles if it is a station."
  currentFuelPercent: Float
}

I added also the currentFuelPercent property, it is related to an issue I opened a week ago. Of course I will not implement it here.

@optionsome

optionsome commented Oct 29, 2024

Copy link
Copy Markdown
Member

I thought of something like this but I'm not sure of the implementation because I surely can't use the VehicleRentalPlace interface to represent every field, for example availableVehicles: RentalVehicleEntityCounts can't be added in VehicleRentalPlace in my opinion because logically is a property of a station. We could add only the number of available vehicles but we would lose their types so it's still not the best solution. Do you have any opinion about this?

If you create a new interface, you don't have to specify all the fields from both types there, instead you can only specify the common types. So something like

interface VehicleRental {
  "Latitude of the vehicle rental object (WGS 84)"
  lat: Float
  "Longitude of the vehicle rental object (WGS 84)"
  lon: Float
  "If true, a vehicle can be picked up."
  allowPickupNow: Boolean
  "If true, rental object is not disabled."
  operative: Boolean
  "The vehicle rental network information. This is referred as system in the GBFS terminology."
  rentalNetwork: VehicleRentalNetwork!
}

Then you can change the existing types to implement this interface (which they sort of do already)

type VehicleRentalStation implements Node & PlaceInterface & VehicleRental

type RentalVehicle implements Node & PlaceInterface & VehicleRental

Then you can from vehicleRentalByBbox [VehicleRental!]!.

In practice, it means you can either use named or inlined fragments to use these types in query (this example is with inlined fragments)

{
  vehicleRentalByBbox(<args>) {
    lat
    lon
    ... on VehicleRentalStation {
      stationId
    }
    ... on RentalVehicle {
      vehicleId
    }
  }
}

@JustCris654 JustCris654 changed the title add vehicleRentalStationsByBbox query to gtfs graqphql schema add vehicleRentalsByBbox query to gtfs graqphql schema Oct 30, 2024
@JustCris654

Copy link
Copy Markdown
Contributor Author

Hi, thanks for the input. I implemented the VehicleRental interface with the relative query.
Also added the graphql integration test and removed the vehicleRentalStationsByBbox query.

Comment thread application/src/main/resources/org/opentripplanner/apis/gtfs/schema.graphqls Outdated
@leonardehrenfried leonardehrenfried changed the title add vehicleRentalsByBbox query to gtfs graqphql schema Add vehicleRentalsByBbox query to GTFS GraphQL API Nov 4, 2024
@t2gran t2gran added the !Improvement A functional improvement or micro feature label Nov 5, 2024
Comment thread application/src/main/resources/org/opentripplanner/apis/gtfs/schema.graphqls Outdated
Comment thread application/src/main/resources/org/opentripplanner/apis/gtfs/schema.graphqls Outdated
@JustCris654
JustCris654 marked this pull request as draft November 18, 2024 11:58
@JustCris654
JustCris654 marked this pull request as ready for review November 20, 2024 13:12
Comment thread application/src/main/resources/org/opentripplanner/apis/gtfs/schema.graphqls Outdated
Co-authored-by: Joel Lappalainen <lappalj8@gmail.com>
@optionsome
optionsome merged commit 35725f3 into opentripplanner:dev-2.x Nov 21, 2024
t2gran pushed a commit that referenced this pull request Nov 21, 2024
@JustCris654
JustCris654 deleted the add_vehicleRentalStationsByBbox branch November 22, 2024 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

!Improvement A functional improvement or micro feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants