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

feat: 1534 service window in summary report #1837

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from
Draft

feat: 1534 service window in summary report #1837

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
38cfec4
service window first commit
qcdyx Sep 5, 2024
5c8c4d8
added logic when both calendars.txt and calendar_dates.txt are used
qcdyx Sep 9, 2024
1e269a2
renamed method
qcdyx Sep 9, 2024
e1a3a26
added null check for earliestStartDate and lateEndDate
qcdyx Sep 9, 2024
9bf2721
added edge case logic
qcdyx Sep 9, 2024
594d5d5
added testLoadServiceDateRange and modified loadServiceDateRange logic
qcdyx Sep 10, 2024
a20686a
formatted code
qcdyx Sep 10, 2024
2e5cfa7
Merge branch 'master' into 1534-service-window-in-summary-report
qcdyx Sep 10, 2024
f962997
add null check before loadFeedInfo; use a field noticeContainer in Fe…
qcdyx Sep 12, 2024
6547447
revert back changes
qcdyx Sep 12, 2024
55489e7
called loadServiceDateRange separately from loadFeedInfo
qcdyx Sep 12, 2024
50287f2
checked in feedInfo before calling loadFeedInfo
qcdyx Sep 12, 2024
5de42bb
resolved PR comments
qcdyx Sep 16, 2024
5e1bdbf
Merge branch 'master' into 1534-service-window-in-summary-report
qcdyx Sep 16, 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
133 changes: 131 additions & 2 deletions main/src/main/java/org/mobilitydata/gtfsvalidator/report/model/FeedMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,12 @@
import com.google.common.collect.ImmutableSortedSet;
import com.vladsch.flexmark.util.misc.Pair;
import java.time.LocalDate;
import java.time.format.DateTimeFormatter;
import java.util.*;
import java.util.function.Function;
import org.mobilitydata.gtfsvalidator.table.*;
import org.mobilitydata.gtfsvalidator.util.CalendarUtil;
import org.mobilitydata.gtfsvalidator.util.ServicePeriod;

public class FeedMetadata {
/*
Expand All @@ -19,6 +22,7 @@ public class FeedMetadata {
public static final String FEED_INFO_FEED_LANGUAGE = "Feed Language";
public static final String FEED_INFO_FEED_START_DATE = "Feed Start Date";
public static final String FEED_INFO_FEED_END_DATE = "Feed End Date";
public static final String FEED_INFO_SERVICE_WINDOW = "Service Date Range";

/*
* Use these strings as keys in the counts map. Also used to specify the info that will appear in
Expand Down Expand Up @@ -81,11 +85,24 @@ public static FeedMetadata from(GtfsFeedContainer feedContainer, ImmutableSet<St
if (feedContainer.getTableForFilename(GtfsFeedInfo.FILENAME).isPresent()) {
qcdyx marked this conversation as resolved.
Show resolved Hide resolved
feedMetadata.loadFeedInfo(
qcdyx marked this conversation as resolved.
Show resolved Hide resolved
(GtfsTableContainer<GtfsFeedInfo>)
feedContainer.getTableForFilename(GtfsFeedInfo.FILENAME).get());
feedContainer.getTableForFilename(GtfsFeedInfo.FILENAME).get(),
(GtfsTableContainer<GtfsTrip>) feedContainer.getTableForFilename(GtfsTrip.FILENAME).get(),
(GtfsTableContainer<GtfsCalendar>)
feedContainer.getTableForFilename(GtfsCalendar.FILENAME).get(),
(GtfsTableContainer<GtfsCalendarDate>)
feedContainer.getTableForFilename(GtfsCalendarDate.FILENAME).get());
}

feedMetadata.loadAgencyData(
(GtfsTableContainer<GtfsAgency>)
feedContainer.getTableForFilename(GtfsAgency.FILENAME).get());

feedMetadata.loadServiceDateRange(
(GtfsTableContainer<GtfsTrip>) feedContainer.getTableForFilename(GtfsTrip.FILENAME).get(),
(GtfsTableContainer<GtfsCalendar>)
feedContainer.getTableForFilename(GtfsCalendar.FILENAME).get(),
(GtfsTableContainer<GtfsCalendarDate>)
feedContainer.getTableForFilename(GtfsCalendarDate.FILENAME).get());
feedMetadata.loadSpecFeatures(feedContainer);
return feedMetadata;
}
Expand Down Expand Up @@ -358,7 +375,11 @@ private void loadAgencyData(GtfsTableContainer<GtfsAgency> agencyTable) {
}
}

private void loadFeedInfo(GtfsTableContainer<GtfsFeedInfo> feedTable) {
private void loadFeedInfo(
GtfsTableContainer<GtfsFeedInfo> feedTable,
GtfsTableContainer<GtfsTrip> tripContainer,
GtfsTableContainer<GtfsCalendar> calendarTable,
GtfsTableContainer<GtfsCalendarDate> calendarDateTable) {
var info = feedTable.getEntities().isEmpty() ? null : feedTable.getEntities().get(0);

feedInfo.put(FEED_INFO_PUBLISHER_NAME, info == null ? "N/A" : info.feedPublisherName());
Expand All @@ -379,6 +400,114 @@ private void loadFeedInfo(GtfsTableContainer<GtfsFeedInfo> feedTable) {
}
}

/**
* Loads the service date range by determining the earliest start date and the latest end date for
* all services referenced with a trip\_id in `trips.txt`. It handles three cases: 1. When only
* `calendars.txt` is used. 2. When only `calendar\_dates.txt` is used. 3. When both
* `calendars.txt` and `calendar\_dates.txt` are used.
*
* @param tripContainer the container for `trips.txt` data
* @param calendarTable the container for `calendars.txt` data
* @param calendarDateTable the container for `calendar\_dates.txt` data
*/
public void loadServiceDateRange(
GtfsTableContainer<GtfsTrip> tripContainer,
GtfsTableContainer<GtfsCalendar> calendarTable,
GtfsTableContainer<GtfsCalendarDate> calendarDateTable) {
List<GtfsTrip> trips = tripContainer.getEntities();
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure tripContainer cannot be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://gtfs.org/documentation/schedule/reference/#tripstxt, trips.txt is required, so I assumed it's non null, isn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, if trips.txt is absent it will give a error notice, but do we have a guarantee that the code here will not be reached in that case? If it is, we'll get an NPE which in general is not good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, it could be empty. I'll add the logic.


LocalDate earliestStartDate = null;
LocalDate latestEndDate = null;
if ((calendarDateTable == null) && (calendarTable != null)) {
// When only calendars.txt is used
List<GtfsCalendar> calendars = calendarTable.getEntities();
for (GtfsTrip trip : trips) {
String serviceId = trip.serviceId();
for (GtfsCalendar calendar : calendars) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't this loop be replaced by something like this:
GtfsCalendar calendar = ((GtfsCalendarTableContainer) calendarTable).byServiceId(serviceId);

if (calendar.serviceId().equals(serviceId)) {
LocalDate startDate = calendar.startDate().getLocalDate();
LocalDate endDate = calendar.endDate().getLocalDate();

if (startDate != null || endDate != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these be null or will they be some default (e.g. 1970-01-01) if the date is not in the file?

Copy link
Contributor Author

@qcdyx qcdyx Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

you are right, they can be unix time 1970-01-01, in that case, we should just display N/A

if (earliestStartDate == null || startDate.isBefore(earliestStartDate)) {
earliestStartDate = startDate;
}
if (latestEndDate == null || endDate.isAfter(latestEndDate)) {
latestEndDate = endDate;
}
}
}
}
}
} else if ((calendarDateTable != null) && (calendarTable == null)) {
// When only calendar_dates.txt is used
List<GtfsCalendarDate> calendarDates = calendarDateTable.getEntities();
for (GtfsTrip trip : trips) {
String serviceId = trip.serviceId();
for (GtfsCalendarDate calendarDate : calendarDates) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't this loop be replaced by something like this:
GtfsCalendarDate calendarDate = ((GtfsCalendarDateTableContainer) calendarDateTable).byServiceId(serviceId);

if (calendarDate.serviceId().equals(serviceId)) {
LocalDate date = calendarDate.date().getLocalDate();
Copy link
Contributor

@jcpitre jcpitre Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe you should check if the exception_type is addition before using it in the calculation, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When only calendar_dates.txt is used, can we assume that the exception_type is always 1? @emmambd

if (date != null) {
if (earliestStartDate == null || date.isBefore(earliestStartDate)) {
earliestStartDate = date;
}
if (latestEndDate == null || date.isAfter(latestEndDate)) {
latestEndDate = date;
}
}
}
}
}
} else if ((calendarTable != null) && (calendarDateTable != null)) {
// When both calendars.txt and calendar_dates.txt are used
Map<String, ServicePeriod> servicePeriods =
CalendarUtil.buildServicePeriodMap(
(GtfsCalendarTableContainer) calendarTable,
(GtfsCalendarDateTableContainer) calendarDateTable);
List<LocalDate> removedDates = new ArrayList<>();
for (GtfsTrip trip : trips) {
String serviceId = trip.serviceId();
ServicePeriod servicePeriod = servicePeriods.get(serviceId);
LocalDate startDate = servicePeriod.getServiceStart();
LocalDate endDate = servicePeriod.getServiceEnd();

if (startDate != null && endDate != null) {
if (earliestStartDate == null || startDate.isBefore(earliestStartDate)) {
earliestStartDate = startDate;
}
if (latestEndDate == null || endDate.isAfter(latestEndDate)) {
latestEndDate = endDate;
}
}
removedDates.addAll(servicePeriod.getRemovedDays());
}

for (LocalDate date : removedDates) {
if (date.isEqual(earliestStartDate)) {
earliestStartDate = date.plusDays(1);
}
if (date.isEqual(latestEndDate)) {
latestEndDate = date.minusDays(1);
}
}
}
DateTimeFormatter formatter = DateTimeFormatter.ofPattern("MMMM d, yyyy");
if ((earliestStartDate == null) && (latestEndDate == null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this generate an NPE if earliestStartDate is null and latestEndDate is not null?

|| earliestStartDate.isAfter(latestEndDate)) {
feedInfo.put(FEED_INFO_SERVICE_WINDOW, "N/A");
} else if (earliestStartDate == null && latestEndDate != null) {
feedInfo.put(FEED_INFO_SERVICE_WINDOW, latestEndDate.format(formatter));
} else if (latestEndDate == null && earliestStartDate != null) {
feedInfo.put(FEED_INFO_SERVICE_WINDOW, earliestStartDate.format(formatter));
} else {
StringBuilder serviceWindow = new StringBuilder();
serviceWindow.append(earliestStartDate);
serviceWindow.append(" to ");
serviceWindow.append(latestEndDate);
feedInfo.put(FEED_INFO_SERVICE_WINDOW, serviceWindow.toString());
}
}

private boolean hasAtLeastOneRecordInFile(
GtfsFeedContainer feedContainer, String featureFilename) {
var table = feedContainer.getTableForFilename(featureFilename);
Expand Down
84 changes: 83 additions & 1 deletion main/src/test/java/org/mobilitydata/gtfsvalidator/report/model/FeedMetadataTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
package org.mobilitydata.gtfsvalidator.report.model;

import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.assertEquals;

import com.google.common.collect.ImmutableList;
import java.io.BufferedWriter;
import java.io.File;
import java.io.FileWriter;
import java.io.IOException;
import java.time.LocalDate;
import java.util.List;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
Expand All @@ -17,6 +19,7 @@
import org.mobilitydata.gtfsvalidator.input.GtfsInput;
import org.mobilitydata.gtfsvalidator.notice.NoticeContainer;
import org.mobilitydata.gtfsvalidator.table.*;
import org.mobilitydata.gtfsvalidator.type.GtfsDate;
import org.mobilitydata.gtfsvalidator.validator.*;

public class FeedMetadataTest {
Expand All @@ -30,6 +33,12 @@ public class FeedMetadataTest {
.build();
ValidatorLoader validatorLoader;
File rootDir;
NoticeContainer noticeContainer = new NoticeContainer();

private GtfsTableContainer<GtfsTrip> tripContainer;
private GtfsTableContainer<GtfsCalendar> calendarTable;
private GtfsTableContainer<GtfsCalendarDate> calendarDateTable;
private FeedMetadata feedMetadata = new FeedMetadata();

private void createDataFile(String filename, String content) throws IOException {
File dataFile = tmpDir.newFile("data/" + filename);
Expand All @@ -50,12 +59,85 @@ public void setup() throws IOException, ValidatorLoaderException {
ValidatorLoader.createForClasses(ClassGraphDiscovery.discoverValidatorsInDefaultPackage());
}

public static GtfsTrip createTrip(int csvRowNumber, String serviceId) {
return new GtfsTrip.Builder().setCsvRowNumber(csvRowNumber).setServiceId(serviceId).build();
}

public static GtfsCalendar createCalendar(
int csvRowNumber, String serviceId, GtfsDate startDate, GtfsDate endDate) {
return new GtfsCalendar.Builder()
.setCsvRowNumber(csvRowNumber)
.setServiceId(serviceId)
.setStartDate(startDate)
.setEndDate(endDate)
.build();
}

public static GtfsCalendarDate createCalendarDate(
int csvRowNumber,
String serviceId,
GtfsDate date,
GtfsCalendarDateExceptionType exceptionType) {
return new GtfsCalendarDate.Builder()
.setCsvRowNumber(csvRowNumber)
.setServiceId(serviceId)
.setDate(date)
.setExceptionType(exceptionType)
.build();
}

@Test
public void testLoadServiceDateRange() {
GtfsTrip trip1 = createTrip(1, "JUN24-MVS-SUB-Weekday-01");
GtfsTrip trip2 = createTrip(2, "JUN24-MVS-SUB-Weekday-02");
// when(tripContainer.getEntities()).thenReturn(List.of(trip1, trip2));
tripContainer = GtfsTripTableContainer.forEntities(List.of(trip1, trip2), noticeContainer);
GtfsCalendar calendar1 =
createCalendar(
1,
"JUN24-MVS-SUB-Weekday-01",
GtfsDate.fromLocalDate(LocalDate.of(2024, 1, 1)),
GtfsDate.fromLocalDate(LocalDate.of(2024, 12, 20)));
GtfsCalendar calendar2 =
createCalendar(
2,
"JUN24-MVS-SUB-Weekday-02",
GtfsDate.fromLocalDate(LocalDate.of(2024, 6, 1)),
GtfsDate.fromLocalDate(LocalDate.of(2024, 12, 31)));
// when(calendarTable.getEntities()).thenReturn(List.of(calendar1, calendar2));
calendarTable =
GtfsCalendarTableContainer.forEntities(List.of(calendar1, calendar2), noticeContainer);
GtfsCalendarDate calendarDate1 =
createCalendarDate(
1,
"JUN24-MVS-SUB-Weekday-01",
GtfsDate.fromLocalDate(LocalDate.of(2024, 1, 1)),
GtfsCalendarDateExceptionType.SERVICE_REMOVED);
GtfsCalendarDate calendarDate2 =
createCalendarDate(
2,
"JUN24-MVS-SUB-Weekday-02",
GtfsDate.fromLocalDate(LocalDate.of(2024, 6, 1)),
GtfsCalendarDateExceptionType.SERVICE_ADDED);
// when(calendarDateTable.getEntities()).thenReturn(List.of(calendarDate1, calendarDate2));
calendarDateTable =
GtfsCalendarDateTableContainer.forEntities(
List.of(calendarDate1, calendarDate2), noticeContainer);

// Call the method
feedMetadata.loadServiceDateRange(tripContainer, calendarTable, calendarDateTable);

// Verify the result
String expectedServiceWindow = "2024-01-02 to 2024-12-31";
assertEquals(
expectedServiceWindow, feedMetadata.feedInfo.get(FeedMetadata.FEED_INFO_SERVICE_WINDOW));
}

private void validateSpecFeature(
String specFeature,
Boolean expectedValue,
ImmutableList<Class<? extends GtfsTableDescriptor<?>>> tableDescriptors)
throws IOException, InterruptedException {
NoticeContainer noticeContainer = new NoticeContainer();
feedLoaderMock = new GtfsFeedLoader(tableDescriptors);
try (GtfsInput gtfsInput = GtfsInput.createFromPath(rootDir.toPath(), noticeContainer)) {
GtfsFeedContainer feedContainer =
Expand Down
Loading