Skip to content
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.

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

feat: 1822 flex forbidden pickup type & forbidden drop off type #1936

Merged
merged 12 commits into from
Jan 15, 2025
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
package org.mobilitydata.gtfsvalidator.validator;

import static org.mobilitydata.gtfsvalidator.notice.SeverityLevel.ERROR;

import org.mobilitydata.gtfsvalidator.annotation.GtfsValidationNotice;
import org.mobilitydata.gtfsvalidator.annotation.GtfsValidator;
import org.mobilitydata.gtfsvalidator.notice.NoticeContainer;
import org.mobilitydata.gtfsvalidator.notice.ValidationNotice;
import org.mobilitydata.gtfsvalidator.table.GtfsPickupDropOff;
import org.mobilitydata.gtfsvalidator.table.GtfsStopTime;
import org.mobilitydata.gtfsvalidator.type.GtfsTime;

/**
* Validates that the `start_pickup_drop_off_window` or `end_pickup_drop_off_window` fields are not
* set when the `pickup_type` is regularly scheduled (0) or must be coordinated with the driver (3),
* and that these fields are not set when the `drop_off_type` is regularly scheduled (0).
*
* <p>Generated notices include: - {@link ForbiddenPickupTypeNotice} - {@link
* ForbiddenDropOffTypeNotice} if the `drop_off_type` is invalid.
*/
@GtfsValidator
public class PickupDropOffTypeValidator extends SingleEntityValidator<GtfsStopTime> {
@Override
public void validate(GtfsStopTime entity, NoticeContainer noticeContainer) {
if ((entity.hasStartPickupDropOffWindow() || entity.hasEndPickupDropOffWindow())
&& (entity.pickupType().equals(GtfsPickupDropOff.ALLOWED)
Copy link
Contributor

Choose a reason for hiding this comment

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

I find it confusing that the regular pickup or dropoff enum value is ALLOWED when with this new rule it's actually not allowed.
Out of scope for this PR, I guess, but can't we rename the enum value to REGULAR instead of ALLOWED. Looking at the GTFS reference, we have this for pickup_type. drop_off_type is similar:

Indicates pickup method. Valid options are:
   0 or empty - Regularly scheduled pickup.
   1 - No pickup available.
   2 - Must phone agency to arrange pickup.
   3 - Must coordinate with driver to arrange pickup.

which more in line with having a REGULAR enum vlue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @emmambd shall we open a new issue for it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused why it can't be done within the scope of this issue - how often does this enum value recur across notices?

Copy link
Contributor

@jcpitre jcpitre Jan 13, 2025

Choose a reason for hiding this comment

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

It can be done within this issue. I just don't want @qcdyx to feel obliged to do it since it has no effect on the resolution of the current issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It'll be implemented in another PR - no need to open a new issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I evaluated this request, it's not just about replacing a string because GtfsPickupDropOff is generated code, we need to identify the source template/code generation logic that produces the file. It's worthy of a new issue. #1942 @davidgamez

|| entity.pickupType().equals(GtfsPickupDropOff.ON_REQUEST_TO_DRIVER))) {
noticeContainer.addValidationNotice(
new ForbiddenPickupTypeNotice(
entity.csvRowNumber(),
entity.startPickupDropOffWindow(),
entity.endPickupDropOffWindow()));
}

if ((entity.hasStartPickupDropOffWindow() || entity.hasEndPickupDropOffWindow())
&& entity.dropOffType().equals(GtfsPickupDropOff.ALLOWED)) {
noticeContainer.addValidationNotice(
new ForbiddenDropOffTypeNotice(
entity.csvRowNumber(),
entity.startPickupDropOffWindow(),
entity.endPickupDropOffWindow()));
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

You should maybe add a shouldValidate method that allows skipping the validate call as an optimisation.
See for example:
StopTimesShapeDistTraveledPresenceValidator.java

And here is the PR where this feature was added: #1875

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@Override
public boolean shouldCallValidate(ColumnInspector header) {
return header.hasColumn(GtfsStopTime.START_PICKUP_DROP_OFF_WINDOW_FIELD_NAME)
|| header.hasColumn(GtfsStopTime.END_PICKUP_DROP_OFF_WINDOW_FIELD_NAME);
}

/**
* pickup_drop_off_window fields are forbidden when the pickup_type is regularly scheduled (0) or
* must be coordinated with the driver (3).
*/
@GtfsValidationNotice(severity = ERROR)
public static class ForbiddenPickupTypeNotice extends ValidationNotice {
/** The row of the faulty record. */
private final int csvRowNumber;

/** The start pickup drop off window of the faulty record. */
private final GtfsTime startPickupDropOffWindow;

/** The end pickup drop off window of the faulty record. */
private final GtfsTime endPickupDropOffWindow;

public ForbiddenPickupTypeNotice(
int csvRowNumber, GtfsTime startPickupDropOffWindow, GtfsTime endPickupDropOffWindow) {
this.csvRowNumber = csvRowNumber;
this.startPickupDropOffWindow = startPickupDropOffWindow;
this.endPickupDropOffWindow = endPickupDropOffWindow;
}
}

/**
* pickup_drop_off_window fields are forbidden when the drop_off_type is regularly scheduled (0).
*/
@GtfsValidationNotice(severity = ERROR)
public static class ForbiddenDropOffTypeNotice extends ValidationNotice {
/** The row of the faulty record. */
private final int csvRowNumber;

/** The start pickup drop off window of the faulty record. */
private final GtfsTime startPickupDropOffWindow;

/** The end pickup drop off window of the faulty record. */
private final GtfsTime endPickupDropOffWindow;

public ForbiddenDropOffTypeNotice(
int csvRowNumber, GtfsTime startPickupDropOffWindow, GtfsTime endPickupDropOffWindow) {
this.csvRowNumber = csvRowNumber;
this.startPickupDropOffWindow = startPickupDropOffWindow;
this.endPickupDropOffWindow = endPickupDropOffWindow;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ public void testNoticeClassFieldNames() {
"departureTime1",
"distanceKm",
"endFieldName",
"endPickupDropOffWindow",
"endValue",
"entityCount",
"entityId",
Expand Down Expand Up @@ -182,6 +183,7 @@ public void testNoticeClassFieldNames() {
"specifiedField",
"speedKph",
"startFieldName",
"startPickupDropOffWindow",
"startValue",
"stopCsvRowNumber",
"stopDesc",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
package org.mobilitydata.gtfsvalidator.validator;

import static com.google.common.truth.Truth.assertThat;

import java.util.List;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
import org.mobilitydata.gtfsvalidator.notice.NoticeContainer;
import org.mobilitydata.gtfsvalidator.notice.ValidationNotice;
import org.mobilitydata.gtfsvalidator.table.GtfsPickupDropOff;
import org.mobilitydata.gtfsvalidator.table.GtfsStopTime;
import org.mobilitydata.gtfsvalidator.type.GtfsTime;

@RunWith(JUnit4.class)
public class PickupDropOffTypeValidatorTest {
static PickupDropOffTypeValidator validator = new PickupDropOffTypeValidator();

private static List<ValidationNotice> generateNotices(GtfsStopTime stopTime) {
NoticeContainer noticeContainer = new NoticeContainer();
validator.validate(stopTime, noticeContainer);
return noticeContainer.getValidationNotices();
}

@Test
public void forbiddenDropOffTypeShouldGenerateNotice() {
GtfsStopTime stopTime =
new GtfsStopTime.Builder()
.setCsvRowNumber(1)
.setPickupType(GtfsPickupDropOff.NOT_AVAILABLE)
.setDropOffType(GtfsPickupDropOff.ALLOWED)
.setStartPickupDropOffWindow(GtfsTime.fromString("00:00:02"))
.setEndPickupDropOffWindow(GtfsTime.fromString("00:00:03"))
.build();
assertThat(generateNotices(stopTime))
.containsExactly(
new PickupDropOffTypeValidator.ForbiddenDropOffTypeNotice(
1, GtfsTime.fromString("00:00:02"), GtfsTime.fromString("00:00:03")));
}

@Test
public void allowedDropOffTypeShouldNotGenerateNotice() {
GtfsStopTime stopTime =
new GtfsStopTime.Builder()
.setCsvRowNumber(2)
.setDropOffType(GtfsPickupDropOff.NOT_AVAILABLE)
.build();
assertThat(generateNotices(stopTime)).isEmpty();
}

@Test
public void forbiddenPickupTypeShouldGenerateNotice() {
GtfsStopTime stopTime =
new GtfsStopTime.Builder()
.setCsvRowNumber(3)
.setPickupType(GtfsPickupDropOff.ALLOWED)
.setDropOffType(GtfsPickupDropOff.NOT_AVAILABLE)
.setStartPickupDropOffWindow(GtfsTime.fromString("08:00:00"))
.setEndPickupDropOffWindow(GtfsTime.fromString("09:00:00"))
.build();
assertThat(generateNotices(stopTime))
.containsExactly(
new PickupDropOffTypeValidator.ForbiddenPickupTypeNotice(
3, GtfsTime.fromString("08:00:00"), GtfsTime.fromString("09:00:00")));
}

@Test
public void allowedPickupTypeShouldNotGenerateNotice() {
GtfsStopTime stopTime =
new GtfsStopTime.Builder()
.setCsvRowNumber(4)
.setPickupType(GtfsPickupDropOff.NOT_AVAILABLE)
.build();
assertThat(generateNotices(stopTime)).isEmpty();
}
}
Loading