Skip to content

Filter import of rental data by pickup type #6240

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Nov 21, 2024
Merged

Filter import of rental data by pickup type #6240

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3a1f3de
filter data from an updater
sharhio Nov 7, 2024
55e7913
Updater config documentation
sharhio Nov 7, 2024
985817a
refactoring, specify docs
sharhio Nov 11, 2024
e516350
use all instead off null
sharhio Nov 13, 2024
993bed1
documented, never null constructor
sharhio Nov 13, 2024
49a0f78
clarified the temporariness
sharhio Nov 15, 2024
42c318f
rename and refactor
sharhio Nov 20, 2024
39750a2
Merge remote-tracking branch 'otp/dev-2.x' into filter-updater-data
sharhio Nov 20, 2024
f148008
Update application/src/main/java/org/opentripplanner/updater/vehicle_…
sharhio Nov 20, 2024
7735de7
updaterConfig doc updated
sharhio Nov 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 3 additions & 12 deletions ...ipplanner/standalone/config/routerconfig/updaters/sources/VehicleRentalSourceFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.opentripplanner.standalone.config.routerconfig.updaters.sources;

import static org.opentripplanner.standalone.config.framework.json.EnumMapper.docEnumValueList;
import static org.opentripplanner.standalone.config.framework.json.OtpVersion.V1_5;
import static org.opentripplanner.standalone.config.framework.json.OtpVersion.V2_1;
import static org.opentripplanner.standalone.config.framework.json.OtpVersion.V2_2;
Expand Down Expand Up @@ -130,18 +131,8 @@ private AllowedRentalType allowedRentalType() {
return c
.of("allowedRentalType")
.since(V2_7)
.summary("Temporary parameter. The type of rental data to include.")
.description(
"""
This parameter is temporary and will be removed in a future version of OTP.

The type of rental data to include. This can be one of the following:

- `ALL`: Include all data types.
- `STATIONS`: Include station data only.
- `VEHICLES`: Include floating vehicle data only.
"""
)
.summary(AllowedRentalType.ALL.typeDescription())
.description(docEnumValueList(AllowedRentalType.values()))
.asEnum(AllowedRentalType.ALL);
}
}
33 changes: 29 additions & 4 deletions ...java/org/opentripplanner/updater/vehicle_rental/datasources/params/AllowedRentalType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,32 @@
package org.opentripplanner.updater.vehicle_rental.datasources.params;

public enum AllowedRentalType {
STATIONS,
VEHICLES,
ALL,
import org.opentripplanner.framework.doc.DocumentedEnum;

/**
* This is temporary and will be removed in a future version of OTP.
*
* Enum to specify the type of rental data that is allowed to be read from the data source.
*/
public enum AllowedRentalType implements DocumentedEnum<AllowedRentalType> {
Copy link
Member

Choose a reason for hiding this comment

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

@optionsome / @leonardehrenfried What is the right name for this?

I would not include "Allowed" in the name, it restrict the type, so it can not be reused.

Copy link
Member

Choose a reason for hiding this comment

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

It's the "type" (?) of the rental: either free-floating or station-based.

Copy link
Member

Choose a reason for hiding this comment

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

We discussed that we want to use a Set of opt-in rental types. I've used the same pattern when filtering what to import from NeTEx:

OpenTripPlanner/application/src/main/java/org/opentripplanner/netex/loader/parser/NetexDocumentParser.java

Lines 82 to 84 in f4bfedf

} else if (!ignoredFeatures.contains(FARE_FRAME) && value instanceof FareFrame) {
parse((FareFrame) value, new FareFrameParser());
} else if (value instanceof CompositeFrame) {

In our case we probably want to use opt-in rather than opt-out as it's easier to reason about.

Copy link
Member

@t2gran t2gran Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This class should be renamed to RentalPickupType and have {STATION, FREE_FLOTING} as values, not ALL.

@leonardehrenfried do you agree? I changed the VEHICLES to FREE_FLOTING. Also use singular, not plural in enums.

In places where there is a set of these you should use Set<RentalPickupType>. I would define a constant in the enum class for ALL like this:

public static final Set<RentalPickupType> ALL = Collections.unmodifiableSet(EnumSet.allOf(RentalPickupType.class));

The unmodifiableSet(...) is needed to avoid modification and you should use it instead of Set.copyOf() (witch convert the internal set to a HashSet).

Copy link
Member

Choose a reason for hiding this comment

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

I agree.

STATIONS("Only station data is allowed."),
VEHICLES("Only vehicle data is allowed."),
ALL("All types of rental data are allowed.");

private final String description;

AllowedRentalType(String description) {
this.description = description.stripIndent().trim();
}

@Override
public String typeDescription() {
return (
"Temporary parameter. Use this to specify the type of rental data that is allowed to be read from the data source."
);
}

@Override
public String enumValueDescription() {
return description;
}
}
4 changes: 4 additions & 0 deletions ...nner/updater/vehicle_rental/datasources/params/GbfsVehicleRentalDataSourceParameters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.opentripplanner.updater.vehicle_rental.datasources.params;

import java.util.Objects;
import org.opentripplanner.updater.spi.HttpHeaders;
import org.opentripplanner.updater.vehicle_rental.VehicleRentalSourceType;

Expand All @@ -14,6 +15,9 @@ public record GbfsVehicleRentalDataSourceParameters(
AllowedRentalType allowedRentalType
Copy link
Member

Choose a reason for hiding this comment

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

Please write a constructor that makes sure this is never null. Like this:

OpenTripPlanner/application/src/main/java/org/opentripplanner/routing/alertpatch/AlertUrl.java

Lines 7 to 9 in f492446

public AlertUrl {
Objects.requireNonNull(uri);
}

)
implements VehicleRentalDataSourceParameters {
public GbfsVehicleRentalDataSourceParameters {
Objects.requireNonNull(allowedRentalType);
}
@Override
public VehicleRentalSourceType sourceType() {
return VehicleRentalSourceType.GBFS;
Expand Down
38 changes: 17 additions & 21 deletions doc/user/UpdaterConfig.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,19 +313,19 @@ GBFS form factors:
<!-- vehicle-rental BEGIN -->
<!-- NOTE! This section is auto-generated. Do not change, change doc in code instead. -->

| Config Parameter | Type | Summary | Req./Opt. | Default Value | Since |
|---------------------------------------------------------------------------------------|:---------------:|---------------------------------------------------------------------------------|:----------:|---------------|:-----:|
| type = "vehicle-rental" | `enum` | The type of the updater. | *Required* | | 1.5 |
| [allowKeepingRentedVehicleAtDestination](#u_1_allowKeepingRentedVehicleAtDestination) | `boolean` | If a vehicle should be allowed to be kept at the end of a station-based rental. | *Optional* | `false` | 2.1 |
| [allowedRentalType](#u_1_allowedRentalType) | `enum` | Temporary parameter. The type of rental data to include. | *Optional* | `"all"` | 2.7 |
| frequency | `duration` | How often the data should be updated. | *Optional* | `"PT1M"` | 1.5 |
| [geofencingZones](#u_1_geofencingZones) | `boolean` | Compute rental restrictions based on GBFS 2.2 geofencing zones. | *Optional* | `false` | 2.3 |
| language | `string` | TODO | *Optional* | | 2.1 |
| [network](#u_1_network) | `string` | The name of the network to override the one derived from the source data. | *Optional* | | 1.5 |
| overloadingAllowed | `boolean` | Allow leaving vehicles at a station even though there are no free slots. | *Optional* | `false` | 2.2 |
| [sourceType](#u_1_sourceType) | `enum` | What source of vehicle rental updater to use. | *Required* | | 1.5 |
| url | `string` | The URL to download the data from. | *Required* | | 1.5 |
| [headers](#u_1_headers) | `map of string` | HTTP headers to add to the request. Any header key, value can be inserted. | *Optional* | | 1.5 |
| Config Parameter | Type | Summary | Req./Opt. | Default Value | Since |
|---------------------------------------------------------------------------------------|:---------------:|-------------------------------------------------------------------------------------------------------------------|:----------:|---------------|:-----:|
| type = "vehicle-rental" | `enum` | The type of the updater. | *Required* | | 1.5 |
| [allowKeepingRentedVehicleAtDestination](#u_1_allowKeepingRentedVehicleAtDestination) | `boolean` | If a vehicle should be allowed to be kept at the end of a station-based rental. | *Optional* | `false` | 2.1 |
| [allowedRentalType](#u_1_allowedRentalType) | `enum` | Temporary parameter. Use this to specify the type of rental data that is allowed to be read from the data source. | *Optional* | `"all"` | 2.7 |
| frequency | `duration` | How often the data should be updated. | *Optional* | `"PT1M"` | 1.5 |
| [geofencingZones](#u_1_geofencingZones) | `boolean` | Compute rental restrictions based on GBFS 2.2 geofencing zones. | *Optional* | `false` | 2.3 |
| language | `string` | TODO | *Optional* | | 2.1 |
| [network](#u_1_network) | `string` | The name of the network to override the one derived from the source data. | *Optional* | | 1.5 |
| overloadingAllowed | `boolean` | Allow leaving vehicles at a station even though there are no free slots. | *Optional* | `false` | 2.2 |
| [sourceType](#u_1_sourceType) | `enum` | What source of vehicle rental updater to use. | *Required* | | 1.5 |
| url | `string` | The URL to download the data from. | *Required* | | 1.5 |
| [headers](#u_1_headers) | `map of string` | HTTP headers to add to the request. Any header key, value can be inserted. | *Optional* | | 1.5 |


##### Parameter details
Expand Down Expand Up @@ -353,15 +353,11 @@ For this to be possible three things need to be configured:
**Path:** /updaters/[1]
**Enum values:** `stations` | `vehicles` | `all`

Temporary parameter. The type of rental data to include.
Temporary parameter. Use this to specify the type of rental data that is allowed to be read from the data source.

This parameter is temporary and will be removed in a future version of OTP.
Copy link
Member

Choose a reason for hiding this comment

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

This should still be included in some way, probably replace the "Temporary parameter" in the sentence above this with this content..


The type of rental data to include. This can be one of the following:

- `ALL`: Include all data types.
- `STATIONS`: Include station data only.
- `VEHICLES`: Include floating vehicle data only.
- `stations` Only station data is allowed.
- `vehicles` Only vehicle data is allowed.
- `all` All types of rental data are allowed.


<h4 id="u_1_geofencingZones">geofencingZones</h4>
Expand Down
Loading