Skip to content

Commit

Permalink
Merge pull request #296 from conveyal/dev
Browse files Browse the repository at this point in the history
Release - Empty strings, calendar validation
landonreed authored Oct 22, 2020
2 parents 3250327 + 0ee1013 commit 715a499
Showing 21 changed files with 173 additions and 20 deletions.
Original file line number Diff line number Diff line change
@@ -44,6 +44,7 @@ public enum NewGTFSErrorType {
ROUTE_UNUSED(Priority.HIGH, "This route is defined but has no trips."),
SERVICE_NEVER_ACTIVE(Priority.MEDIUM, "A service code was defined, but is never active on any date."),
SERVICE_UNUSED(Priority.MEDIUM, "A service code was defined, but is never referenced by any trips."),
SERVICE_WITHOUT_DAYS_OF_WEEK(Priority.MEDIUM, "A service defined in calendar.txt should be active on at least one day of the week. Otherwise, it should be omitted from this file."),
SHAPE_DIST_TRAVELED_NOT_INCREASING(Priority.MEDIUM, "Shape distance traveled must increase with stop times."),
SHAPE_MISSING_COORDINATE(Priority.MEDIUM, "???"),
SHAPE_REVERSED(Priority.MEDIUM, "A shape appears to be intended for vehicles running the opposite direction on the route."),
54 changes: 42 additions & 12 deletions src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java
Original file line number Diff line number Diff line change
@@ -390,6 +390,7 @@ private void setStatementParameters(
) throws SQLException {
// JDBC SQL statements use a one-based index for setting fields/parameters
List<String> missingFieldNames = new ArrayList<>();
// One-based index for prepared statement.
int index = 1;
for (Field field : table.editorFields()) {
if (!jsonObject.has(field.name)) {
@@ -407,35 +408,42 @@ private void setStatementParameters(
LOG.debug("{}={}", field.name, value);
try {
if (value == null || value.isNull()) {
// If there is a required field missing from the JSON object, an exception will be thrown below.
if (field.isRequired() && !field.isEmptyValuePermitted()) {
// Only register the field as missing if the value is null, the field is required, and empty
// values are not permitted. For example, a null value for fare_attributes#transfers should not
// trigger a missing field exception.
missingFieldNames.add(field.name);
continue;
}
// Handle setting null value on statement
field.setNull(preparedStatement, index);
// Set value to null if empty value is OK and update JSON.
setFieldToNullAndUpdateJson(preparedStatement, jsonObject, field, index);
} else {
List<String> values = new ArrayList<>();
// For fields that are not missing, handle setting different field types.
if (value.isArray()) {
// Array field type expects comma-delimited values.
List<String> values = new ArrayList<>();
for (JsonNode node : value) {
values.add(node.asText());
}
field.setParameter(preparedStatement, index, String.join(",", values));
} else {
String text = value.asText();
// If the field is a ShortField and the string is empty, set value to null. Otherwise, set
// parameter with string.
if (field instanceof ShortField && text.isEmpty()) field.setNull(preparedStatement, index);
else field.setParameter(preparedStatement, index, text);
// If the string is empty, set value to null (for StringField, ShortField, etc.). Otherwise, set
// parameter with string value.
if (text.isEmpty()) {
// Set field to null and update JSON.
setFieldToNullAndUpdateJson(preparedStatement, jsonObject, field, index);
} else {
field.setParameter(preparedStatement, index, text);
}
}
}
} catch (StorageException e) {
LOG.warn("Could not set field {} to value {}. Attempting to parse integer seconds.", field.name, value);
if (field.name.contains("_time")) {
// FIXME: This is a hack to get arrival and departure time into the right format. Because the UI
// currently returns them as seconds since midnight rather than the Field-defined format HH:MM:SS.
// currently returns them as seconds since midnight rather than the Field-defined format HH:MM:SS.
try {
if (value == null || value.isNull()) {
if (field.isRequired()) {
@@ -461,10 +469,10 @@ private void setStatementParameters(
throw e;
}
}
// Increment index for next field.
index += 1;
}
if (missingFieldNames.size() > 0) {
// String joinedFieldNames = missingFieldNames.stream().collect(Collectors.joining(", "));
throw new SQLException(
String.format(
"The following field(s) are missing from JSON %s object: %s",
@@ -475,6 +483,23 @@ private void setStatementParameters(
}
}

/**
* Set field to null in prepared statement and update JSON object that is ultimately returned (see return value of
* {@link #update}.) in response to reflect actual database value that will be persisted. This method should be
* used in cases where the jsonObject value is missing or detected to be an empty string.
*/
private static void setFieldToNullAndUpdateJson(
PreparedStatement preparedStatement,
ObjectNode jsonObject,
Field field,
int oneBasedIndex
) throws SQLException {
// Update the jsonObject so that the JSON that gets returned correctly reflects persisted value.
jsonObject.set(field.name, null);
// Set field to null in prepared statement.
field.setNull(preparedStatement, oneBasedIndex);
}

/**
* This updates those tables that depend on the table currently being updated. For example, if updating/creating a
* pattern, this method handles deleting any pattern stops and shape points. For trips, this would handle updating
@@ -1280,21 +1305,26 @@ private void ensureReferentialIntegrity(
final boolean isCreating = id == null;
String keyField = table.getKeyFieldName();
String tableName = String.join(".", namespace, table.name);
if (jsonObject.get(keyField) == null || jsonObject.get(keyField).isNull()) {
// FIXME: generate key field automatically for certain entities (e.g., trip ID). Maybe this should be
// generated for all entities if null?
JsonNode val = jsonObject.get(keyField);
if (val == null || val.isNull() || val.asText().isEmpty()) {
// Handle different cases where the value is missing.
if ("trip_id".equals(keyField)) {
// Generate key field automatically for certain entities (e.g., trip ID).
// FIXME: Maybe this should be generated for all entities if null?
jsonObject.put(keyField, UUID.randomUUID().toString());
} else if ("agency_id".equals(keyField)) {
// agency_id is not required if there is only one row in the agency table. Otherwise, it is required.
LOG.warn("agency_id field for agency id={} is null.", id);
int rowSize = getRowCount(tableName, connection);
if (rowSize > 1 || (isCreating && rowSize > 0)) {
throw new SQLException("agency_id must not be null if more than one agency exists.");
}
} else {
// In all other cases where a key field is missing, throw an exception.
throw new SQLException(String.format("Key field %s must not be null", keyField));
}
}
// Re-get the string key value (in case trip_id was added to the JSON object above).
String keyValue = jsonObject.get(keyField).asText();
// If updating key field, check that there is no ID conflict on value (e.g., stop_id or route_id)
TIntSet uniqueIds = getIdsForCondition(tableName, keyField, keyValue, connection);
22 changes: 22 additions & 0 deletions src/main/java/com/conveyal/gtfs/validator/ServiceValidator.java
Original file line number Diff line number Diff line change
@@ -36,6 +36,7 @@
import java.util.Map;
import java.util.Set;

import static com.conveyal.gtfs.error.NewGTFSErrorType.SERVICE_WITHOUT_DAYS_OF_WEEK;
import static com.conveyal.gtfs.error.NewGTFSErrorType.TRIP_OVERLAP_IN_BLOCK;

/**
@@ -121,6 +122,11 @@ private void validateServiceInfo(ValidationResult validationResult) {

// First handle the calendar entries, which define repeating weekly schedules.
for (Calendar calendar : feed.calendars) {
// Validate that calendars apply to at least one day of the week.
if (!isCalendarUsedDuringWeek(calendar)) {
if (errorStorage != null) registerError(calendar, SERVICE_WITHOUT_DAYS_OF_WEEK);
}

try {
LocalDate endDate = calendar.end_date;
// Loop over all days in this calendar entry, recording on which ones it is active.
@@ -445,4 +451,20 @@ private class BlockInterval {
StopTime firstStop;
StopTime lastStop;
}

/**
* Checks that a {@link Calendar} entity is applicable for at least one day of the week
* (i.e. at least one of the fields for Monday-Sunday is set to '1').
* @param calendar The {@link Calendar} entity to check.
* @return true if at least one field for Monday-Sunday is set to 1, false otherwise.
*/
public static boolean isCalendarUsedDuringWeek(Calendar calendar) {
return calendar.monday == 1 ||
calendar.tuesday == 1 ||
calendar.wednesday == 1 ||
calendar.thursday == 1 ||
calendar.friday == 1 ||
calendar.saturday == 1 ||
calendar.sunday == 1;
}
}
24 changes: 24 additions & 0 deletions src/test/java/com/conveyal/gtfs/GTFSTest.java
Original file line number Diff line number Diff line change
@@ -489,6 +489,30 @@ public void canLoadFeedWithLongFieldValues () {
);
}

/**
* Tests that a GTFS feed with a service id that doesn't apply to any day of the week
* (i.e. when 'monday' through 'sunday' fields are set to zero)
* generates a validation error.
*/
@Test
public void canLoadFeedWithServiceWithoutDaysOfWeek() {
PersistenceExpectation[] expectations = PersistenceExpectation.list();
ErrorExpectation[] errorExpectations = ErrorExpectation.list(
new ErrorExpectation(NewGTFSErrorType.FEED_TRAVEL_TIMES_ROUNDED), // Not related, not worrying about this one.
new ErrorExpectation(NewGTFSErrorType.SERVICE_WITHOUT_DAYS_OF_WEEK)
);
assertThat(
"service-without-days test passes",
runIntegrationTestOnFolder(
"fake-agency-service-without-days",
nullValue(),
expectations,
errorExpectations
),
equalTo(true)
);
}

/**
* A helper method that will zip a specified folder in test/main/resources and call
* {@link #runIntegrationTestOnZipFile} on that file.
3 changes: 2 additions & 1 deletion src/test/java/com/conveyal/gtfs/dto/RouteDTO.java
Original file line number Diff line number Diff line change
@@ -21,6 +21,7 @@ public class RouteDTO {
public String route_text_color;
public Integer publicly_visible;
public Integer wheelchair_accessible;
public Integer route_sort_order;
/** This field is incorrectly set to String in order to test how empty string literals are persisted to the database. */
public String route_sort_order;
public Integer status;
}
35 changes: 30 additions & 5 deletions src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java
Original file line number Diff line number Diff line change
@@ -42,7 +42,11 @@
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.not;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertTrue;

/**
* This class contains CRUD tests for {@link JdbcTableWriter} (i.e., editing GTFS entities in the RDBMS). Set up
@@ -302,7 +306,9 @@ public void canCreateUpdateAndDeleteRoutes() throws IOException, SQLException, I
// create new object to be saved
String routeId = "500";
RouteDTO createdRoute = createSimpleTestRoute(routeId, "RTA", "500", "Hollingsworth", 3);

// Set values to empty strings/null to later verify that they are set to null in the database.
createdRoute.route_color = "";
createdRoute.route_sort_order = "";
// make sure saved data matches expected data
assertThat(createdRoute.route_id, equalTo(routeId));
// TODO: Verify with a SQL query that the database now contains the created data (we may need to use the same
@@ -312,7 +318,7 @@ public void canCreateUpdateAndDeleteRoutes() throws IOException, SQLException, I
String updatedRouteId = "600";
createdRoute.route_id = updatedRouteId;

// covert object to json and save it
// convert object to json and save it
JdbcTableWriter updateTableWriter = createTestTableWriter(routeTable);
String updateOutput = updateTableWriter.update(
createdRoute.id,
@@ -326,9 +332,17 @@ public void canCreateUpdateAndDeleteRoutes() throws IOException, SQLException, I

// make sure saved data matches expected data
assertThat(updatedRouteDTO.route_id, equalTo(updatedRouteId));
// TODO: Verify with a SQL query that the database now contains the updated data (we may need to use the same
// db connection to do this successfully?)

// Ensure route_color is null (not empty string).
LOG.info("route_color: {}", updatedRouteDTO.route_color);
assertNull(updatedRouteDTO.route_color);
// Verify that certain values are correctly set in the database.
ResultSet resultSet = getResultSetForId(updatedRouteDTO.id, routeTable);
while (resultSet.next()) {
assertResultValue(resultSet, "route_color", Matchers.nullValue());
assertResultValue(resultSet, "route_id", equalTo(createdRoute.route_id));
assertResultValue(resultSet, "route_sort_order", Matchers.nullValue());
assertResultValue(resultSet, "route_type", equalTo(createdRoute.route_type));
}
// try to delete record
JdbcTableWriter deleteTableWriter = createTestTableWriter(routeTable);
int deleteOutput = deleteTableWriter.delete(
@@ -427,9 +441,19 @@ public void shouldUpdateStopTimeShapeDistTraveledOnPatternStopUpdate() throws IO
assertThat(pattern.route_id, equalTo(routeId));
// Create trip so we can check that the stop_time values are updated after the patter update.
TripDTO tripInput = constructTimetableTrip(pattern.pattern_id, pattern.route_id, startTime, 60);
// Set trip_id to empty string to verify that it gets overwritten with auto-generated UUID.
tripInput.trip_id = "";
JdbcTableWriter createTripWriter = createTestTableWriter(Table.TRIPS);
String createdTripOutput = createTripWriter.create(mapper.writeValueAsString(tripInput), true);
TripDTO createdTrip = mapper.readValue(createdTripOutput, TripDTO.class);
// Check that trip_id is not empty.
assertNotEquals("", createdTrip.trip_id);
// Check that trip_id is a UUID.
LOG.info("New trip_id = {}", createdTrip.trip_id);
UUID uuid = UUID.fromString(createdTrip.trip_id);
assertNotNull(uuid);
// Check that trip exists.
assertThatSqlQueryYieldsRowCount(getColumnsForId(createdTrip.id, Table.TRIPS), 1);
// Check the stop_time's initial shape_dist_traveled value. TODO test that other linked fields are updated?
PreparedStatement statement = testDataSource.getConnection().prepareStatement(
String.format(
@@ -438,6 +462,7 @@ public void shouldUpdateStopTimeShapeDistTraveledOnPatternStopUpdate() throws IO
createdTrip.trip_id
)
);
LOG.info(statement.toString());
ResultSet resultSet = statement.executeQuery();
while (resultSet.next()) {
// First stop_time shape_dist_traveled should be zero.
Original file line number Diff line number Diff line change
@@ -10,14 +10,14 @@

public class MTCValidatorTest {
@Test
public void fieldlLengthShouldNotExceed() {
public void canValidateFieldLength() {
MTCValidator validator = new MTCValidator(null, null);
assertThat(validator.validateFieldLength(null, "abcdefghijklmnopqrstwxyz1234567890", 20), is(false));
assertThat(validator.validateFieldLength(null, "abcdef", 20), is(true));
}

@Test
public void fieldlLengthShouldNotExceed_usingObject() throws MalformedURLException {
public void canValidateFieldLength_usingObject() throws MalformedURLException {
MTCValidator validator = new MTCValidator(null, null);

// You can also pass objects, in that case it will use toString().
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package com.conveyal.gtfs.validator;

import com.conveyal.gtfs.model.Calendar;
import org.junit.Test;

import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;

public class ServiceValidatorTest {
@Test
public void validateCalendarDays() {
Calendar calendar = new Calendar();
assertThat(ServiceValidator.isCalendarUsedDuringWeek(calendar), is(false));

calendar.tuesday = 1;
assertThat(ServiceValidator.isCalendarUsedDuringWeek(calendar), is(true));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
agency_id,agency_name,agency_url,agency_lang,agency_phone,agency_email,agency_timezone,agency_fare_url,agency_branding_url
1,Fake Transit,http://www.example.com,,,,America/Los_Angeles,,
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
service_id,monday,tuesday,wednesday,thursday,friday,saturday,sunday,start_date,end_date
04100312-8fe1-46a5-a9f2-556f39478f57,1,1,1,1,1,1,1,20170915,20170917
service-without-days,0,0,0,0,0,0,0,20170915,20170917
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
service_id,date,exception_type
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
fare_id,price,currency_type,payment_method,transfers,transfer_duration
route_based_fare,1.23,USD,0,0,0
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
fare_id,route_id,origin_id,destination_id,contains_id
route_based_fare,1,,,
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
feed_id,feed_publisher_name,feed_publisher_url,feed_lang,feed_version
fake_transit,Conveyal,http://www.conveyal.com,en,1.0
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
trip_id,start_time,end_time,headway_secs,exact_times
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
agency_id,route_id,route_short_name,route_long_name,route_desc,route_type,route_url,route_color,route_text_color,route_branding_url
1,1,101A,Example Route,,3,,7CE6E7,FFFFFF,
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
shape_id,shape_pt_lat,shape_pt_lon,shape_pt_sequence,shape_dist_traveled
5820f377-f947-4728-ac29-ac0102cbc34e,37.0612132,-122.0074332,1,0.0000000
5820f377-f947-4728-ac29-ac0102cbc34e,37.0611720,-122.0075000,2,7.4997067
5820f377-f947-4728-ac29-ac0102cbc34e,37.0613590,-122.0076830,3,33.8739075
5820f377-f947-4728-ac29-ac0102cbc34e,37.0608780,-122.0082780,4,109.0402932
5820f377-f947-4728-ac29-ac0102cbc34e,37.0603590,-122.0088280,5,184.6078298
5820f377-f947-4728-ac29-ac0102cbc34e,37.0597610,-122.0093540,6,265.8053023
5820f377-f947-4728-ac29-ac0102cbc34e,37.0590660,-122.0099190,7,357.8617018
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
trip_id,arrival_time,departure_time,stop_id,stop_sequence,stop_headsign,pickup_type,drop_off_type,shape_dist_traveled,timepoint
a30277f8-e50a-4a85-9141-b1e0da9d429d,07:00:00,07:00:00,4u6g,1,,0,0,0.0000000,
a30277f8-e50a-4a85-9141-b1e0da9d429d,07:01:00,07:01:00,johv,2,,0,0,341.4491961,
3 changes: 3 additions & 0 deletions src/test/resources/fake-agency-service-without-days/stops.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
stop_id,stop_code,stop_name,stop_desc,stop_lat,stop_lon,zone_id,stop_url,location_type,parent_station,stop_timezone,wheelchair_boarding
4u6g,,Butler Ln,,37.0612132,-122.0074332,,,0,,,
johv,,Scotts Valley Dr & Victor Sq,,37.0590172,-122.0096058,,,0,,,
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
from_stop_id,to_stop_id,transfer_type,min_transfer_time
2 changes: 2 additions & 0 deletions src/test/resources/fake-agency-service-without-days/trips.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
route_id,trip_id,trip_headsign,trip_short_name,direction_id,block_id,shape_id,bikes_allowed,wheelchair_accessible,service_id
1,a30277f8-e50a-4a85-9141-b1e0da9d429d,,,0,,5820f377-f947-4728-ac29-ac0102cbc34e,0,0,04100312-8fe1-46a5-a9f2-556f39478f57

0 comments on commit 715a499

Please sign in to comment.