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.

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

Jump to bottom

fix: prevent exception raised when serviceId is missing from calendar file #1646

Merged
merged 8 commits into from
Feb 2, 2024
Merged

fix: prevent exception raised when serviceId is missing from calendar file #1646

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0d96b73
prevent exception raised when serviceId is missing from calendar file
davidgamez Jan 16, 2024
c658ef6
support calendar date foreign key violation
davidgamez Jan 18, 2024
29c9875
infering calendar start and end date from calendar_date when calendar…
davidgamez Jan 18, 2024
a57009f
calendar expired only get trigger from calendar_date when no calendar…
davidgamez Jan 25, 2024
3605f4d
fix period calculation ignoring deleted services
davidgamez Jan 26, 2024
a412159
ignoring service id with no periods
davidgamez Jan 26, 2024
c0b4b95
add test coverage to expire_calendar notice
davidgamez Jan 29, 2024
294931c
Merge branch 'master' into fix/calendar-expired-missing-serviceid
davidgamez Feb 2, 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
31 changes: 17 additions & 14 deletions main/src/main/java/org/mobilitydata/gtfsvalidator/util/CalendarUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,7 @@
import com.google.common.base.Preconditions;
import com.google.common.collect.Multimaps;
import java.time.LocalDate;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.SortedSet;
import java.util.*;
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import org.mobilitydata.gtfsvalidator.table.GtfsCalendar;
Expand All @@ -36,8 +29,8 @@ private CalendarUtil() {}
public static ServicePeriod createServicePeriod(
@Nullable GtfsCalendar calendar, @Nonnull List<GtfsCalendarDate> calendarDates) {
// Store service period from calendar.txt, if provided.
LocalDate serviceStart;
LocalDate serviceEnd;
LocalDate serviceStart = null;
LocalDate serviceEnd = null;
byte weeklyPattern;
if (calendar != null) {
serviceStart = calendar.startDate().getLocalDate();
Expand All @@ -56,21 +49,31 @@ public static ServicePeriod createServicePeriod(
calendar.saturdayValue(),
calendar.sundayValue());
} else {
serviceStart = LocalDate.EPOCH;
serviceEnd = LocalDate.EPOCH;
weeklyPattern = 0;
}

// Store exception days from calendar_dates.txt, if any.
Set<LocalDate> addedDays = new HashSet<>();
Set<LocalDate> removedDays = new HashSet<>();
for (GtfsCalendarDate calendarDate : calendarDates) {
(calendarDate.exceptionType() == GtfsCalendarDateExceptionType.SERVICE_ADDED
if (calendar == null) {
LocalDate date = calendarDate.date().getLocalDate();
if (GtfsCalendarDateExceptionType.SERVICE_ADDED.equals(calendarDate.exceptionType())
&& (serviceStart == null || serviceStart.isAfter(date))) {
serviceStart = date;
}
if (GtfsCalendarDateExceptionType.SERVICE_ADDED.equals(calendarDate.exceptionType())
&& (serviceEnd == null || serviceEnd.isBefore(date))) {
serviceEnd = date;
}
}
(GtfsCalendarDateExceptionType.SERVICE_ADDED.equals(calendarDate.exceptionType())
? addedDays
: removedDays)
.add(calendarDate.date().getLocalDate());
}

serviceStart = serviceStart == null ? LocalDate.EPOCH : serviceStart;
serviceEnd = serviceEnd == null ? serviceStart : serviceEnd;
return new ServicePeriod(serviceStart, serviceEnd, weeklyPattern, addedDays, removedDays);
}

Expand Down
33 changes: 28 additions & 5 deletions main/src/main/java/org/mobilitydata/gtfsvalidator/validator/ExpiredCalendarValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,7 @@
import static org.mobilitydata.gtfsvalidator.notice.SeverityLevel.WARNING;

import java.time.LocalDate;
import java.util.Map;
import java.util.SortedSet;
import java.util.*;
import javax.inject.Inject;
import org.mobilitydata.gtfsvalidator.annotation.GtfsValidationNotice;
import org.mobilitydata.gtfsvalidator.annotation.GtfsValidationNotice.UrlRef;
Expand Down Expand Up @@ -54,12 +53,36 @@ public void validate(NoticeContainer noticeContainer) {
final Map<String, SortedSet<LocalDate>> servicePeriodMap =
CalendarUtil.servicePeriodToServiceDatesMap(
CalendarUtil.buildServicePeriodMap(calendarTable, calendarDateTable));
boolean isCalendarTableEmpty = calendarTable.getEntities().isEmpty();
List<ExpiredCalendarNotice> expiredCalendarDatesNotices = new ArrayList<>();
boolean allCalendarAreExpired = true;
for (var serviceId : servicePeriodMap.keySet()) {
SortedSet<LocalDate> serviceDates = servicePeriodMap.get(serviceId);
if (!serviceDates.isEmpty() && serviceDates.last().isBefore(dateForValidation.getDate())) {
int csvRowNumber = calendarTable.byServiceId(serviceId).get().csvRowNumber();
noticeContainer.addValidationNotice(new ExpiredCalendarNotice(csvRowNumber, serviceId));
if (serviceDates.isEmpty()) {
continue;
}
if (serviceDates.last().isBefore(dateForValidation.getDate())) {
if (calendarTable.byServiceId(serviceId).isPresent()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpicking here. In the case you know the calendarTable is empty (isCalendarTableEmpty == true), why bother doing a lookup on it?

Suggested change
if (calendarTable.byServiceId(serviceId).isPresent()) {
if (!isCalendarTableEmpty && calendarTable.byServiceId(serviceId).isPresent()) {

Copy link
Member Author

Choose a reason for hiding this comment

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

That's true. However, reducing the logic in the if condition helps readability. In either case, empty or not, the byService(serviceId) method will behave the same.

noticeContainer.addValidationNotice(
new ExpiredCalendarNotice(
calendarTable.byServiceId(serviceId).get().csvRowNumber(), serviceId));
} else if (isCalendarTableEmpty && allCalendarAreExpired) {
// Taking the first of the calendar dates for the service id.
// This is the case of foreign key violation or calendar.txt not provided.
// In this case all serviceId need to be expired to report the notices.
Optional<GtfsCalendarDate> firstCalendarDate =
calendarDateTable.byServiceId(serviceId).stream()
.min(Comparator.comparingInt(GtfsCalendarDate::csvRowNumber));
expiredCalendarDatesNotices.add(
new ExpiredCalendarNotice(
firstCalendarDate.map(GtfsCalendarDate::csvRowNumber).orElse(0), serviceId));
}
} else {
allCalendarAreExpired = false;
}
}
if (isCalendarTableEmpty && allCalendarAreExpired) {
noticeContainer.addValidationNotices(expiredCalendarDatesNotices);
}
}

Expand Down
4 changes: 2 additions & 2 deletions main/src/test/java/org/mobilitydata/gtfsvalidator/util/CalendarUtilTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,8 +165,8 @@ public void buildServicePeriodMap() {
ImmutableSet.of()),
"s3",
new ServicePeriod(
LocalDate.EPOCH,
LocalDate.EPOCH,
LocalDate.of(2021, 3, 8),
LocalDate.of(2021, 3, 8),
(byte) 0,
ImmutableSet.of(LocalDate.of(2021, 3, 8)),
ImmutableSet.of(LocalDate.of(2021, 3, 9))));
Expand Down
116 changes: 116 additions & 0 deletions .../src/test/java/org/mobilitydata/gtfsvalidator/validator/ExpiredCalendarValidatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,4 +215,120 @@ public void calendarWithNoDaysShouldNotGenerateNotice() {
.validate(container);
assertThat(container.getValidationNotices()).isEmpty();
}

@Test
public void calendarDateWithForeignKeyViolationShouldNotGenerateNotice() {
NoticeContainer container = new NoticeContainer();

List<GtfsCalendar> calendars =
ImmutableList.of(
new GtfsCalendar.Builder()
.setCsvRowNumber(2)
.setServiceId("SERVICE_ID")
.setStartDate(GtfsDate.fromLocalDate(TEST_NOW.minusDays(7)))
.setEndDate(GtfsDate.fromLocalDate(TEST_NOW))
.build());

GtfsCalendarTableContainer calendarTable =
GtfsCalendarTableContainer.forEntities(calendars, container);
var calendarDateTable =
GtfsCalendarDateTableContainer.forEntities(
ImmutableList.of(
new GtfsCalendarDate.Builder()
.setCsvRowNumber(3)
.setServiceId("NOT_SERVICE_ID")
.setDate(GtfsDate.fromLocalDate(TEST_NOW.minusDays(2)))
.setExceptionType(GtfsCalendarDateExceptionType.SERVICE_REMOVED)
.build(),
new GtfsCalendarDate.Builder()
.setCsvRowNumber(2)
.setServiceId("NOT_SERVICE_ID")
.setDate(GtfsDate.fromLocalDate(TEST_NOW.minusDays(3)))
.setExceptionType(GtfsCalendarDateExceptionType.SERVICE_ADDED)
.build(),
new GtfsCalendarDate.Builder()
.setCsvRowNumber(4)
.setServiceId("NOT_SERVICE_ID")
.setDate(GtfsDate.fromLocalDate(TEST_NOW.minusDays(1)))
.setExceptionType(GtfsCalendarDateExceptionType.SERVICE_ADDED)
.build()),
container);
new ExpiredCalendarValidator(new DateForValidation(TEST_NOW), calendarTable, calendarDateTable)
.validate(container);
assertThat(container.getValidationNotices()).isEmpty();
}

@Test
public void calendarDateWithAtLeastCalendarDateNotExpiredShouldNotGenerateNotice() {
NoticeContainer container = new NoticeContainer();

List<GtfsCalendar> calendars = ImmutableList.of();

GtfsCalendarTableContainer calendarTable =
GtfsCalendarTableContainer.forEntities(calendars, container);
var calendarDateTable =
GtfsCalendarDateTableContainer.forEntities(
ImmutableList.of(
new GtfsCalendarDate.Builder()
.setCsvRowNumber(3)
.setServiceId("SERVICE_ID_1")
.setDate(GtfsDate.fromLocalDate(TEST_NOW.minusDays(2)))
.setExceptionType(GtfsCalendarDateExceptionType.SERVICE_ADDED)
.build(),
new GtfsCalendarDate.Builder()
.setCsvRowNumber(2)
.setServiceId("SERVICE_ID_2")
.setDate(GtfsDate.fromLocalDate(TEST_NOW.minusDays(3)))
.setExceptionType(GtfsCalendarDateExceptionType.SERVICE_ADDED)
.build(),
new GtfsCalendarDate.Builder()
.setCsvRowNumber(4)
.setServiceId("SERVICE_ID_3")
.setDate(GtfsDate.fromLocalDate(TEST_NOW.plusDays(1)))
.setExceptionType(GtfsCalendarDateExceptionType.SERVICE_ADDED)
.build()),
container);
new ExpiredCalendarValidator(new DateForValidation(TEST_NOW), calendarTable, calendarDateTable)
.validate(container);
assertThat(container.getValidationNotices()).isEmpty();
}

@Test
public void calendarDateWithAllCalendarDatesExpiredShouldGenerateNotice() {
NoticeContainer container = new NoticeContainer();

List<GtfsCalendar> calendars = ImmutableList.of();

GtfsCalendarTableContainer calendarTable =
GtfsCalendarTableContainer.forEntities(calendars, container);
var calendarDateTable =
GtfsCalendarDateTableContainer.forEntities(
ImmutableList.of(
new GtfsCalendarDate.Builder()
.setCsvRowNumber(3)
.setServiceId("SERVICE_ID_3")
.setDate(GtfsDate.fromLocalDate(TEST_NOW.minusDays(2)))
.setExceptionType(GtfsCalendarDateExceptionType.SERVICE_ADDED)
.build(),
new GtfsCalendarDate.Builder()
.setCsvRowNumber(2)
.setServiceId("SERVICE_ID_2")
.setDate(GtfsDate.fromLocalDate(TEST_NOW.minusDays(3)))
.setExceptionType(GtfsCalendarDateExceptionType.SERVICE_ADDED)
.build(),
new GtfsCalendarDate.Builder()
.setCsvRowNumber(1)
.setServiceId("SERVICE_ID_1")
.setDate(GtfsDate.fromLocalDate(TEST_NOW.minusDays(1)))
.setExceptionType(GtfsCalendarDateExceptionType.SERVICE_ADDED)
.build()),
container);
new ExpiredCalendarValidator(new DateForValidation(TEST_NOW), calendarTable, calendarDateTable)
.validate(container);
assertThat(container.getValidationNotices())
.containsExactly(
new ExpiredCalendarValidator.ExpiredCalendarNotice(3, "SERVICE_ID_3"),
new ExpiredCalendarValidator.ExpiredCalendarNotice(2, "SERVICE_ID_2"),
new ExpiredCalendarValidator.ExpiredCalendarNotice(1, "SERVICE_ID_1"));
}
}
Loading