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 6 commits
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
7 changes: 5 additions & 2 deletions ...xt-test/java/org/opentripplanner/ext/smoovebikerental/SmooveBikeRentalDataSourceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import java.util.List;
import org.junit.jupiter.api.Test;
import org.opentripplanner.service.vehiclerental.model.VehicleRentalPlace;
import org.opentripplanner.updater.vehicle_rental.datasources.params.AllowedRentalType;
import org.opentripplanner.updater.spi.HttpHeaders;

class SmooveBikeRentalDataSourceTest {
Expand All @@ -18,7 +19,8 @@ void makeStation() {
"file:src/ext-test/resources/smoovebikerental/smoove.json",
null,
true,
HttpHeaders.empty()
HttpHeaders.empty(),
AllowedRentalType.ALL
)
);
assertTrue(source.update());
Expand Down Expand Up @@ -84,7 +86,8 @@ void makeStationWithoutOverloading() {
"file:src/ext-test/resources/smoovebikerental/smoove.json",
null,
false,
HttpHeaders.empty()
HttpHeaders.empty(),
AllowedRentalType.ALL
)
);
assertTrue(source.update());
Expand Down
9 changes: 8 additions & 1 deletion ...t/java/org/opentripplanner/ext/smoovebikerental/SmooveBikeRentalDataSourceParameters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package org.opentripplanner.ext.smoovebikerental;

import javax.annotation.Nullable;
import org.opentripplanner.updater.vehicle_rental.datasources.params.AllowedRentalType;
import org.opentripplanner.updater.spi.HttpHeaders;
import org.opentripplanner.updater.vehicle_rental.VehicleRentalSourceType;
import org.opentripplanner.updater.vehicle_rental.datasources.params.VehicleRentalDataSourceParameters;
Expand All @@ -12,7 +13,8 @@ public record SmooveBikeRentalDataSourceParameters(
String url,
String network,
boolean overloadingAllowed,
HttpHeaders httpHeaders
HttpHeaders httpHeaders,
AllowedRentalType allowedRentalType
)
implements VehicleRentalDataSourceParameters {
/**
Expand All @@ -29,4 +31,9 @@ public String getNetwork(String defaultValue) {
public VehicleRentalSourceType sourceType() {
return VehicleRentalSourceType.SMOOVE;
}

@Override
public boolean allowRentalType(AllowedRentalType rentalType) {
return allowedRentalType == null || allowedRentalType == AllowedRentalType.ALL || rentalType == allowedRentalType;
}
}
5 changes: 4 additions & 1 deletion ...entripplanner/ext/vehiclerentalservicedirectory/VehicleRentalServiceDirectoryFetcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import org.opentripplanner.updater.spi.GraphUpdater;
import org.opentripplanner.updater.vehicle_rental.VehicleRentalUpdater;
import org.opentripplanner.updater.vehicle_rental.datasources.VehicleRentalDataSourceFactory;
import org.opentripplanner.updater.vehicle_rental.datasources.params.AllowedRentalType;
import org.opentripplanner.updater.vehicle_rental.datasources.params.GbfsVehicleRentalDataSourceParameters;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -110,7 +111,9 @@ private static List<GbfsVehicleRentalDataSourceParameters> buildListOfNetworksFr
networkName,
networkParams.geofencingZones(),
// overloadingAllowed - not part of GBFS, not supported here
false
false,
// allowedRentalType not supported
AllowedRentalType.ALL
)
);
} else {
Expand Down
18 changes: 16 additions & 2 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,15 +1,18 @@
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;
import static org.opentripplanner.standalone.config.framework.json.OtpVersion.V2_3;
import static org.opentripplanner.standalone.config.framework.json.OtpVersion.V2_7;

import org.opentripplanner.ext.smoovebikerental.SmooveBikeRentalDataSourceParameters;
import org.opentripplanner.standalone.config.framework.json.NodeAdapter;
import org.opentripplanner.standalone.config.routerconfig.updaters.HttpHeadersConfig;
import org.opentripplanner.updater.spi.HttpHeaders;
import org.opentripplanner.updater.vehicle_rental.VehicleRentalSourceType;
import org.opentripplanner.updater.vehicle_rental.datasources.params.AllowedRentalType;
import org.opentripplanner.updater.vehicle_rental.datasources.params.GbfsVehicleRentalDataSourceParameters;
import org.opentripplanner.updater.vehicle_rental.datasources.params.VehicleRentalDataSourceParameters;

Expand Down Expand Up @@ -43,13 +46,15 @@ public VehicleRentalDataSourceParameters create() {
headers(),
network(),
geofencingZones(),
overloadingAllowed()
overloadingAllowed(),
allowedRentalType()
);
case SMOOVE -> new SmooveBikeRentalDataSourceParameters(
url(),
network(),
overloadingAllowed(),
headers()
headers(),
allowedRentalType()
);
};
}
Expand Down Expand Up @@ -121,4 +126,13 @@ private boolean geofencingZones() {
)
.asBoolean(false);
}

private AllowedRentalType allowedRentalType() {
return c
.of("allowedRentalType")
.since(V2_7)
.summary(AllowedRentalType.ALL.typeDescription())
.description(docEnumValueList(AllowedRentalType.values()))
.asEnum(AllowedRentalType.ALL);
}
}
63 changes: 33 additions & 30 deletions ...a/org/opentripplanner/updater/vehicle_rental/datasources/GbfsVehicleRentalDataSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.opentripplanner.service.vehiclerental.model.RentalVehicleType;
import org.opentripplanner.service.vehiclerental.model.VehicleRentalPlace;
import org.opentripplanner.service.vehiclerental.model.VehicleRentalSystem;
import org.opentripplanner.updater.vehicle_rental.datasources.params.AllowedRentalType;
import org.opentripplanner.updater.vehicle_rental.datasources.params.GbfsVehicleRentalDataSourceParameters;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -76,42 +77,44 @@ public List<VehicleRentalPlace> getUpdates() {

List<VehicleRentalPlace> stations = new LinkedList<>();

// Both station information and status are required for all systems using stations
GBFSStationInformation stationInformation = loader.getFeed(GBFSStationInformation.class);
GBFSStationStatus stationStatus = loader.getFeed(GBFSStationStatus.class);
if (stationInformation != null && stationStatus != null) {
// Index all the station status entries on their station ID.
Map<String, GBFSStation> statusLookup = stationStatus
.getData()
.getStations()
.stream()
.collect(Collectors.toMap(GBFSStation::getStationId, Function.identity()));
GbfsStationStatusMapper stationStatusMapper = new GbfsStationStatusMapper(
statusLookup,
vehicleTypes
);
GbfsStationInformationMapper stationInformationMapper = new GbfsStationInformationMapper(
system,
vehicleTypes,
params.allowKeepingRentedVehicleAtDestination(),
params.overloadingAllowed()
);

// Iterate over all known stations, and if we have any status information add it to those station objects.
stations.addAll(
stationInformation
if (params.allowRentalType(AllowedRentalType.STATIONS)) {
// Both station information and status are required for all systems using stations
GBFSStationInformation stationInformation = loader.getFeed(GBFSStationInformation.class);
GBFSStationStatus stationStatus = loader.getFeed(GBFSStationStatus.class);
if (stationInformation != null && stationStatus != null) {
// Index all the station status entries on their station ID.
Map<String, GBFSStation> statusLookup = stationStatus
.getData()
.getStations()
.stream()
.map(stationInformationMapper::mapStationInformation)
.filter(Objects::nonNull)
.peek(stationStatusMapper::fillStationStatus)
.toList()
);
.collect(Collectors.toMap(GBFSStation::getStationId, Function.identity()));
GbfsStationStatusMapper stationStatusMapper = new GbfsStationStatusMapper(
statusLookup,
vehicleTypes
);
GbfsStationInformationMapper stationInformationMapper = new GbfsStationInformationMapper(
system,
vehicleTypes,
params.allowKeepingRentedVehicleAtDestination(),
params.overloadingAllowed()
);

// Iterate over all known stations, and if we have any status information add it to those station objects.
stations.addAll(
stationInformation
.getData()
.getStations()
.stream()
.map(stationInformationMapper::mapStationInformation)
.filter(Objects::nonNull)
.peek(stationStatusMapper::fillStationStatus)
.toList()
);
}
}

// Append the floating bike stations.
if (OTPFeature.FloatingBike.isOn()) {
if (OTPFeature.FloatingBike.isOn() && params.allowRentalType(AllowedRentalType.VEHICLES)) {
GBFSFreeBikeStatus freeBikeStatus = loader.getFeed(GBFSFreeBikeStatus.class);
if (freeBikeStatus != null) {
GbfsFreeVehicleStatusMapper freeVehicleStatusMapper = new GbfsFreeVehicleStatusMapper(
Expand Down
32 changes: 32 additions & 0 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
@@ -0,0 +1,32 @@
package org.opentripplanner.updater.vehicle_rental.datasources.params;

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 (
"This is temporary and will be removed in a future version of OTP. 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;
}
}
16 changes: 15 additions & 1 deletion ...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 @@ -10,11 +11,24 @@ public record GbfsVehicleRentalDataSourceParameters(
HttpHeaders httpHeaders,
String network,
boolean geofencingZones,
boolean overloadingAllowed
boolean overloadingAllowed,
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;
}

@Override
public boolean allowRentalType(AllowedRentalType rentalType) {
return (
allowedRentalType == null ||
allowedRentalType == AllowedRentalType.ALL ||
allowedRentalType == rentalType
);
}
}
2 changes: 2 additions & 0 deletions ...pplanner/updater/vehicle_rental/datasources/params/VehicleRentalDataSourceParameters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,6 @@ public interface VehicleRentalDataSourceParameters {
VehicleRentalSourceType sourceType();

HttpHeaders httpHeaders();

boolean allowRentalType(AllowedRentalType rentalType);
}
6 changes: 6 additions & 0 deletions ...on/src/test/java/org/opentripplanner/updater/vehicle_rental/VehicleRentalUpdaterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import org.opentripplanner.updater.GraphWriterRunnable;
import org.opentripplanner.updater.spi.HttpHeaders;
import org.opentripplanner.updater.vehicle_rental.datasources.VehicleRentalDatasource;
import org.opentripplanner.updater.vehicle_rental.datasources.params.AllowedRentalType;
import org.opentripplanner.updater.vehicle_rental.datasources.params.VehicleRentalDataSourceParameters;

class VehicleRentalUpdaterTest {
Expand Down Expand Up @@ -103,5 +104,10 @@ public VehicleRentalSourceType sourceType() {
public HttpHeaders httpHeaders() {
return HttpHeaders.empty();
}

@Override
public boolean allowRentalType(AllowedRentalType rentalType) {
return true;
}
}
}
10 changes: 7 additions & 3 deletions ...g/opentripplanner/updater/vehicle_rental/datasources/GbfsVehicleRentalDataSourceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import org.opentripplanner.service.vehiclerental.model.RentalVehicleType;
import org.opentripplanner.service.vehiclerental.model.VehicleRentalPlace;
import org.opentripplanner.updater.spi.HttpHeaders;
import org.opentripplanner.updater.vehicle_rental.datasources.params.AllowedRentalType;
import org.opentripplanner.updater.vehicle_rental.datasources.params.GbfsVehicleRentalDataSourceParameters;

/**
Expand All @@ -32,7 +33,8 @@ void makeStationFromV22() {
HttpHeaders.empty(),
null,
false,
false
false,
AllowedRentalType.ALL
),
new OtpHttpClientFactory()
);
Expand Down Expand Up @@ -123,7 +125,8 @@ void geofencing() {
HttpHeaders.empty(),
null,
true,
false
false,
AllowedRentalType.ALL
),
new OtpHttpClientFactory()
);
Expand Down Expand Up @@ -165,7 +168,8 @@ void makeStationFromV10() {
HttpHeaders.empty(),
network,
false,
true
true,
AllowedRentalType.ALL
),
new OtpHttpClientFactory()
);
Expand Down
Loading
Loading