Add currentFuelPercent and currentRangeMeters to RentalVehichle in the GTFS GraphQL API #6272

JustCris654 · 2024-11-22T16:26:47Z

Summary

Add currentFuelPercent and currentRangeMeters fields to RentalVehicle in the graphql GTFS API

Issue

issue

I added currentRangeMeters on top of the issue because it is related

Unit tests

Write a few words on how the new code is tested.

Graphql integration test have been modified to test this fields
I tested the query with the graphiql frontend
Do all tests
pass the continuous integration service
? Yes

Documentation

I wrote the proper doc comments in the graphql schema

codecov · 2024-11-22T16:33:49Z

Codecov Report

Attention: Patch coverage is 63.26531% with 18 lines in your changes missing coverage. Please review.

Project coverage is 69.78%. Comparing base (72019c0) to head (ff8411a).
Report is 5 commits behind head on dev-2.x.

Files with missing lines	Patch %	Lines
...ental/datasources/GbfsFreeVehicleStatusMapper.java	52.17%	10 Missing and 1 partial ⚠️
...org/opentripplanner/transit/model/basic/Ratio.java	63.63%	4 Missing ⚠️
.../opentripplanner/transit/model/basic/Distance.java	66.66%	1 Missing and 1 partial ⚠️
.../apis/transmodel/model/stop/RentalVehicleType.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #6272      +/-   ##
=============================================
- Coverage      69.79%   69.78%   -0.01%     
- Complexity     17943    17958      +15     
=============================================
  Files           2046     2048       +2     
  Lines          76685    76729      +44     
  Branches        7829     7836       +7     
=============================================
+ Hits           53520    53546      +26     
- Misses         20423    20439      +16     
- Partials        2742     2744       +2

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

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

application/src/test/java/org/opentripplanner/routing/core/DistanceTest.java

application/src/main/java/org/opentripplanner/transit/model/basic/Distance.java

...tion/src/main/java/org/opentripplanner/service/vehiclerental/model/VehicleRentalVehicle.java

application/src/test/resources/org/opentripplanner/apis/gtfs/queries/rental-vehicle.graphql

optionsome · 2024-12-05T14:19:46Z

Does the library we use validate that the fuel percent is between 0 and 1 or should we do it?

JustCris654 · 2024-12-05T17:03:37Z

Does the library we use validate that the fuel percent is between 0 and 1 or should we do it?

The library validate if the value is between 0 and 1 in output (when generating the graphql response).
It returns an error like this:

{
  "errors": [
    {
      "message": "Can't serialize value (/rentalVehicles[0]/currentFuelPercent) : Value is under 0 or greater than 1.",
      "path": [
        "rentalVehicles",
        0,
        "currentFuelPercent"
      ],
      "extensions": {
        "classification": "DataFetchingException"
      }
    }
  ],
  "data": {
    "rentalVehicles": [
      {
        "name": "Ninebot A200",
        "lat": 43.772581,
        "lon": 13.132542,
        "currentFuelPercent": null
      }
    ]
  }
}

The value is not checked when received in the gbfs mapper, do we need to check it also there? I saw that in the RatioScalarTest.java has this

    var ratio = (Double) GraphQLScalars.RATIO_SCALAR.getCoercing().parseValue(HALF);

to parse a value.

optionsome · 2024-12-09T10:50:23Z

The value is not checked when received in the gbfs mapper, do we need to check it also there?

I would prefer to log some warning and ignore the value in the mapper instead of letting it be cause issues in the APIs.

optionsome · 2024-12-09T10:54:16Z

@testower do you know if there is some general policy in the GBFS java library for validating values (such as not allowing over 100% fuel percent or negative range meters)?

testower · 2024-12-09T11:01:48Z

@testower do you know if there is some general policy in the GBFS java library for validating values (such as not allowing over 100% fuel percent or negative range meters)?

The java model used in otp does not have validation annotations, so anything that can be deserialized is accepted.

optionsome · 2024-12-09T11:10:08Z

The java model used in otp does not have validation annotations, so anything that can be deserialized is accepted.

Is there an option to use validation annotations? I'm just wondering should we discuss the possibility in tomorrow's dev meeting, for example?

testower · 2024-12-09T12:56:21Z

Not with the current library. It is left out with intention. I don't think validation should be handled in the deserialization step anyway. This is runtime code / hot code path, so I think OTP should validate the data it needs to in the business layer.

testower · 2024-12-09T13:05:48Z

I should probably add some reasoning: I don't think it's desirable to reject the deserialization of a whole file update, because some particular piece of data in that update doesn't validate according to the validation annotations. The data may still be perfectly usable by OTP.

...java/org/opentripplanner/updater/vehicle_rental/datasources/GbfsFreeVehicleStatusMapper.java

leonardehrenfried · 2024-12-17T12:40:03Z

You need to resolve merge conflicts.

JustCris654 · 2024-12-17T14:27:48Z

...java/org/opentripplanner/updater/vehicle_rental/datasources/GbfsFreeVehicleStatusMapper.java

+      // if the propulsion type has an engine current_range_meters is required
+      if (
+        vehicle.getVehicleTypeId() != null &&
+        vehicleTypes.get(vehicle.getVehicleTypeId()) != null &&


I added this check, can you double check if it's right?
I'm not sure but vehicle_type_id is REQUIRED if the vehicle_types.json file is defined, that file is REQUIRED for systems with free_bike_status.json and if this file is not included then all vehicles are non motorized bicycles.
Therefore if the vehicleTypeId is not present in the vehicleTypes map I can assume that the file vehicle_types.json is not present and all vehicles are not motorized, so the propulsion type is human and range is not needed.

I think this is correct, but this is in an area where a lot of feeds get things wrong so this validation might cause issues but I'm not sure if I'm against this or not.

I spoke to @hbruch about this and he said that there are a number of feeds that don't include it where they should but he is in favour of enforcing the spec anyway. If we are not strict with data producers, they will never learn.

...ication/src/main/java/org/opentripplanner/service/vehiclerental/model/RentalVehicleFuel.java

application/src/main/java/org/opentripplanner/transit/model/basic/Ratio.java

application/src/main/java/org/opentripplanner/transit/model/basic/Distance.java

application/src/main/java/org/opentripplanner/transit/model/basic/Ratio.java

optionsome · 2024-12-31T15:10:52Z

application/src/main/java/org/opentripplanner/transit/model/basic/Ratio.java

+
+  @Override
+  public String toString() {
+    return this.ratio.toString();


Should the toString output the number as a percent (like 24.5%), I'm not sure. With cost for example we have a custom toString format.

We discussed this today in the dev meeting and decided that the to string should just return the double as string, not a percent.

Ok, so it's fine as it is

optionsome · 2024-12-31T15:12:29Z

...java/org/opentripplanner/updater/vehicle_rental/datasources/GbfsFreeVehicleStatusMapper.java

+          vehicle.getCurrentFuelPercent(),
+          e.getMessage()
+        );
+      } catch (NullPointerException e) {}


Do we need to catch npe, when would it happen?

current_fuel_percent is an optional field so getCurrentFuelPercent could return null

optionsome · 2024-12-31T15:20:39Z

...java/org/opentripplanner/updater/vehicle_rental/datasources/GbfsFreeVehicleStatusMapper.java

+      // if the propulsion type has an engine current_range_meters is required
+      if (
+        vehicle.getVehicleTypeId() != null &&
+        vehicleTypes.get(vehicle.getVehicleTypeId()) != null &&


I think this is correct, but this is in an area where a lot of feeds get things wrong so this validation might cause issues but I'm not sure if I'm against this or not.

optionsome · 2024-12-31T15:21:08Z

...java/org/opentripplanner/updater/vehicle_rental/datasources/GbfsFreeVehicleStatusMapper.java

+        RentalVehicleType.PropulsionType.HUMAN &&
+        rangeMeters == null
+      ) {
+        return null;


We should log a warning here since the whole vehicle is ignored.

I added the check to be compliant to the GBFS standard and I think it's better this way in the future but I see that it could be an issue if feeds get this wrong. I can remove the check if you say it's for the best, I don't think it would break anything.
Maybe I can add just a warning and return the object?

application/src/main/java/org/opentripplanner/transit/model/basic/Ratio.java

leonardehrenfried · 2025-01-09T09:16:59Z

application/src/test/java/org/opentripplanner/routing/core/RatioTest.java

+import org.junit.jupiter.api.Test;
+import org.opentripplanner.transit.model.basic.Ratio;
+
+public class RatioTest {


t2gran

I looked the Ratio implementation and how to provide a factory method witch handles validation errors. As mentioned in the dev-meeting today, my first attempt was to use a Result object - this was rather ugly and did not increase the readability or lowered the complexity(maintenance cost). So, after a bit of refactoring I went for a solution witch uses an error-handler and return an Optional - this was easy to implement and looks good on the caller side.

Please merge in these changes:

https://github.com/opentripplanner/OpenTripPlanner/tree/pr-6272

or cherry-pick commits:

The the two commits in the branch above fixes ALL my suggestions below, but I wanted to leave the comments so you can understand why I changed the code.

t2gran · 2025-01-09T19:45:21Z

application/src/main/java/org/opentripplanner/transit/model/basic/Ratio.java

+
+  @Override
+  public int hashCode() {
+    return Objects.hash(this.ratio);


You should use the Double not Objects hash function here.

t2gran · 2025-01-09T19:46:38Z

application/src/main/java/org/opentripplanner/transit/model/basic/Ratio.java

+
+  @Override
+  public boolean equals(Object other) {
+    if (other instanceof Ratio ratio) {


Do not use 'instanceof' in equals methods, follw the same practise as other value-objects.

t2gran · 2025-01-09T19:46:56Z

application/src/main/java/org/opentripplanner/transit/model/basic/Ratio.java

+ */
+public class Ratio {
+
+  private final Double ratio;


Suggested change

private final Double ratio;

private final double ratio;

t2gran · 2025-01-09T19:47:19Z

application/src/main/java/org/opentripplanner/transit/model/basic/Ratio.java

+    return this.ratio.toString();
+  }
+
+  public Double asDouble() {


Suggested change

public Double asDouble() {

public double asDouble() {

t2gran · 2025-01-09T20:27:22Z

@testower do you know if there is some general policy in the GBFS java library for validating values (such as not allowing over 100% fuel percent or negative range meters)?

The java model used in otp does not have validation annotations, so anything that can be deserialized is accepted.

Just to be clear - We do not want "insane" values in OTP. The DTOs used to import can accept it, but not the core OTP model. In general there is two categories of data:

pass-through data, witch we can be a bit loose on the validation and let the consumer take action (OTP API clients).
data used by the business logic in OTP - where we do validate more strict to avoid complex logic in OTP. This category also include fields we currently does not use, but we are likely to use in a planning request in the future. This apply to fuelRatio.

JustCris654 changed the title ~~Rental vehicle new gbfs fields~~ Add currentFuelPercent and currentRangeMeters to RentalVehichle in the GTFS GraphQL API Nov 22, 2024

JustCris654 marked this pull request as ready for review November 22, 2024 16:40

JustCris654 requested a review from a team as a code owner November 22, 2024 16:40

leonardehrenfried reviewed Nov 25, 2024

View reviewed changes

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

JustCris654 marked this pull request as draft November 26, 2024 06:40

JustCris654 force-pushed the rentalVehicle_new_gbfs_fields branch from 2eef86c to 00a1e4b Compare November 27, 2024 11:54

JustCris654 marked this pull request as ready for review December 2, 2024 11:13

habrahamsson-skanetrafiken requested a review from optionsome December 3, 2024 10:44

optionsome reviewed Dec 3, 2024

View reviewed changes

leonardehrenfried reviewed Dec 4, 2024

View reviewed changes

...tion/src/main/java/org/opentripplanner/service/vehiclerental/model/VehicleRentalVehicle.java Outdated Show resolved Hide resolved

leonardehrenfried reviewed Dec 4, 2024

View reviewed changes

...tion/src/main/java/org/opentripplanner/service/vehiclerental/model/VehicleRentalVehicle.java Outdated Show resolved Hide resolved

leonardehrenfried reviewed Dec 4, 2024

View reviewed changes

...tion/src/main/java/org/opentripplanner/service/vehiclerental/model/VehicleRentalVehicle.java Outdated Show resolved Hide resolved

leonardehrenfried reviewed Dec 4, 2024

View reviewed changes

application/src/test/resources/org/opentripplanner/apis/gtfs/queries/rental-vehicle.graphql Outdated Show resolved Hide resolved

optionsome reviewed Dec 9, 2024

View reviewed changes

...java/org/opentripplanner/updater/vehicle_rental/datasources/GbfsFreeVehicleStatusMapper.java Outdated Show resolved Hide resolved

JustCris654 force-pushed the rentalVehicle_new_gbfs_fields branch 2 times, most recently from b52ebca to 284133a Compare December 17, 2024 13:48

JustCris654 commented Dec 17, 2024

View reviewed changes

optionsome requested changes Dec 31, 2024

View reviewed changes

JustCris654 added 2 commits January 7, 2025 15:52

add currentFuelPercent to VehicleRentalVehicle and gtfs graphql API

7fb3810

add currentRangeMeters to VehicleRentalVehicle and gtfs graphql API

b61c9f3

JustCris654 added 20 commits January 7, 2025 15:52

use Distance class for currentRangeMeters

4686f7c

add greater and less than methods for Distance

e93c828

Distance class tests

49c4682

represent currentRangeMeters with integer type

54292e2

format RentalVehicleImpl file

851c8b5

proper naming for static variables

d26799f

use Ratio scalar for currentFuelPercent

e0bd36a

rename currentRangeMeters to currentRange

e9a572d

move conversion of Distance to-from meters to the api and gbfs mapping

498d547

remove unused code Distance class

eda75c8

group range and percent in fuel type

3e7bf5b

log warn if fuelPercent is invalid

7d15f00

check currentRangeMeters validity in free rental vehicle

9b9b585

Ratio class

c90bdd4

Ratio class for fuel percent validation

346449b

Ratio class and test format

6edf98b

fix check when range is required

0b1920d

format GbfsFreeVehicleStatusMapper

54b0bc0

add range to scooter in GbfsFreeVehicleStatusMapperTest

f3e0a71

general fixes Ratio.java and Distance.java

26d8d7d

JustCris654 force-pushed the rentalVehicle_new_gbfs_fields branch from fc01a87 to 26d8d7d Compare January 7, 2025 14:52

RentalvehicleFuel properties docs comments

4788055

leonardehrenfried reviewed Jan 9, 2025

View reviewed changes

application/src/main/java/org/opentripplanner/transit/model/basic/Ratio.java Outdated Show resolved Hide resolved

leonardehrenfried reviewed Jan 9, 2025

View reviewed changes

javadoc comment for Ratio class

ff8411a

leonardehrenfried previously approved these changes Jan 9, 2025

View reviewed changes

check getCurrentFuelPercent null value and drop NPE

6778169

JustCris654 dismissed leonardehrenfried’s stale review via 6778169 January 9, 2025 15:14

t2gran requested changes Jan 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add currentFuelPercent and currentRangeMeters to RentalVehichle in the GTFS GraphQL API #6272

Add currentFuelPercent and currentRangeMeters to RentalVehichle in the GTFS GraphQL API #6272

JustCris654 commented Nov 22, 2024 •

edited

Loading

codecov bot commented Nov 22, 2024 •

edited

Loading

optionsome commented Dec 5, 2024

JustCris654 commented Dec 5, 2024

optionsome commented Dec 9, 2024

optionsome commented Dec 9, 2024

testower commented Dec 9, 2024

optionsome commented Dec 9, 2024

testower commented Dec 9, 2024 •

edited

Loading

testower commented Dec 9, 2024

leonardehrenfried commented Dec 17, 2024

JustCris654 Dec 17, 2024

optionsome Dec 31, 2024

leonardehrenfried Jan 10, 2025

optionsome Dec 31, 2024 •

edited

Loading

optionsome Jan 7, 2025

JustCris654 Jan 7, 2025

optionsome Dec 31, 2024

JustCris654 Jan 7, 2025

optionsome Dec 31, 2024

optionsome Dec 31, 2024

JustCris654 Jan 7, 2025

leonardehrenfried Jan 9, 2025

JustCris654 Jan 9, 2025

t2gran left a comment •

edited

Loading

t2gran Jan 9, 2025

t2gran Jan 9, 2025

t2gran Jan 9, 2025

t2gran Jan 9, 2025

t2gran commented Jan 9, 2025 •

edited

Loading

Add currentFuelPercent and currentRangeMeters to RentalVehichle in the GTFS GraphQL API #6272

Are you sure you want to change the base?

Add currentFuelPercent and currentRangeMeters to RentalVehichle in the GTFS GraphQL API #6272

Conversation

JustCris654 commented Nov 22, 2024 • edited Loading

Summary

Issue

Unit tests

Documentation

codecov bot commented Nov 22, 2024 • edited Loading

Codecov Report

optionsome commented Dec 5, 2024

JustCris654 commented Dec 5, 2024

optionsome commented Dec 9, 2024

optionsome commented Dec 9, 2024

testower commented Dec 9, 2024

optionsome commented Dec 9, 2024

testower commented Dec 9, 2024 • edited Loading

testower commented Dec 9, 2024

leonardehrenfried commented Dec 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

optionsome Dec 31, 2024 • edited Loading

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

Choose a reason for hiding this comment

t2gran left a comment • edited Loading

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

t2gran commented Jan 9, 2025 • edited Loading

JustCris654 commented Nov 22, 2024 •

edited

Loading

codecov bot commented Nov 22, 2024 •

edited

Loading

testower commented Dec 9, 2024 •

edited

Loading

optionsome Dec 31, 2024 •

edited

Loading

t2gran left a comment •

edited

Loading

t2gran commented Jan 9, 2025 •

edited

Loading