From b85a922bebc6b86eaee8d79f345967fe59b4d5b7 Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Wed, 1 Apr 2020 13:57:51 -0400 Subject: [PATCH 01/17] refactor(Feed): allow additional validators --- src/main/java/com/conveyal/gtfs/GTFS.java | 5 +++-- .../java/com/conveyal/gtfs/loader/Feed.java | 19 +++++++++++-------- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/GTFS.java b/src/main/java/com/conveyal/gtfs/GTFS.java index 769154445..0c5af6778 100644 --- a/src/main/java/com/conveyal/gtfs/GTFS.java +++ b/src/main/java/com/conveyal/gtfs/GTFS.java @@ -7,6 +7,7 @@ import com.conveyal.gtfs.loader.JdbcGtfsSnapshotter; import com.conveyal.gtfs.loader.SnapshotResult; import com.conveyal.gtfs.util.InvalidNamespaceException; +import com.conveyal.gtfs.validator.FeedValidator; import com.conveyal.gtfs.validator.ValidationResult; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.io.Files; @@ -94,9 +95,9 @@ public static SnapshotResult makeSnapshot (String feedId, DataSource dataSource) /** * Once a feed has been loaded into the database, examine its contents looking for various problems and errors. */ - public static ValidationResult validate (String feedId, DataSource dataSource) { + public static ValidationResult validate (String feedId, DataSource dataSource, FeedValidator... additionalValidators) { Feed feed = new Feed(dataSource, feedId); - ValidationResult result = feed.validate(); + ValidationResult result = feed.validate(additionalValidators); return result; } diff --git a/src/main/java/com/conveyal/gtfs/loader/Feed.java b/src/main/java/com/conveyal/gtfs/loader/Feed.java index 475f1e707..271b7fd10 100644 --- a/src/main/java/com/conveyal/gtfs/loader/Feed.java +++ b/src/main/java/com/conveyal/gtfs/loader/Feed.java @@ -74,7 +74,7 @@ public Feed (DataSource dataSource, String tablePrefix) { * TODO allow validation within feed loading process, so the same connection can be used, and we're certain loaded data is 100% visible. * That would also avoid having to reconnect the error storage to the DB. */ - public ValidationResult validate () { + public ValidationResult validate (FeedValidator... additionalValidators) { long validationStartTime = System.currentTimeMillis(); // Create an empty validation result that will have its fields populated by certain validators. ValidationResult validationResult = new ValidationResult(); @@ -89,13 +89,16 @@ public ValidationResult validate () { int errorCountBeforeValidation = errorStorage.getErrorCount(); List feedValidators = Arrays.asList( - new MisplacedStopValidator(this, errorStorage, validationResult), - new DuplicateStopsValidator(this, errorStorage), - new FaresValidator(this, errorStorage), - new FrequencyValidator(this, errorStorage), - new TimeZoneValidator(this, errorStorage), - new NewTripTimesValidator(this, errorStorage), - new NamesValidator(this, errorStorage)); + new MisplacedStopValidator(this, errorStorage, validationResult), + new DuplicateStopsValidator(this, errorStorage), + new FaresValidator(this, errorStorage), + new FrequencyValidator(this, errorStorage), + new TimeZoneValidator(this, errorStorage), + new NewTripTimesValidator(this, errorStorage), + new NamesValidator(this, errorStorage) + ); + // Add any additional validators specified in args. + feedValidators.addAll(Arrays.asList(additionalValidators)); for (FeedValidator feedValidator : feedValidators) { String validatorName = feedValidator.getClass().getSimpleName(); From 8b72831c08ef741a1cec8fc61ae7d160ae8ee939 Mon Sep 17 00:00:00 2001 From: binh-dam-ibigroup <56846598+binh-dam-ibigroup@users.noreply.github.com> Date: Wed, 1 Apr 2020 19:37:21 -0400 Subject: [PATCH 02/17] test(MTC): Set up test files for MTC field length validation. Some modifications regarding passing additional FeedValidator classes are included. BREAKING CHANGE: Change in the type of optional arguments for GTFS.validate and Feed.validate --- src/main/java/com/conveyal/gtfs/GTFS.java | 5 +- .../conveyal/gtfs/error/FieldLengthError.java | 17 + .../conveyal/gtfs/error/NewGTFSErrorType.java | 1 + .../java/com/conveyal/gtfs/loader/Feed.java | 21 +- .../gtfs/validator/FieldLengthValidator.java | 18 + .../conveyal/gtfs/validator/MTCValidator.java | 16 + .../java/com/conveyal/gtfs/GTFSTestMTC.java | 661 ++++++++++++++++++ .../validator/FieldLengthValidatorTest.java | 32 + .../fake-agency-mtc-long-fields/agency.txt | 5 + .../fake-agency-mtc-long-fields/calendar.txt | 2 + .../calendar_dates.txt | 2 + .../fare_attributes.txt | 2 + .../fare_rules.txt | 2 + .../fake-agency-mtc-long-fields/feed_info.txt | 2 + .../frequencies.txt | 2 + .../fake-agency-mtc-long-fields/routes.txt | 2 + .../fake-agency-mtc-long-fields/shapes.txt | 8 + .../stop_times.txt | 5 + .../fake-agency-mtc-long-fields/stops.txt | 6 + .../fake-agency-mtc-long-fields/transfers.txt | 1 + .../fake-agency-mtc-long-fields/trips.txt | 4 + 21 files changed, 808 insertions(+), 6 deletions(-) create mode 100644 src/main/java/com/conveyal/gtfs/error/FieldLengthError.java create mode 100644 src/main/java/com/conveyal/gtfs/validator/FieldLengthValidator.java create mode 100644 src/main/java/com/conveyal/gtfs/validator/MTCValidator.java create mode 100644 src/test/java/com/conveyal/gtfs/GTFSTestMTC.java create mode 100644 src/test/java/com/conveyal/gtfs/validator/FieldLengthValidatorTest.java create mode 100755 src/test/resources/fake-agency-mtc-long-fields/agency.txt create mode 100755 src/test/resources/fake-agency-mtc-long-fields/calendar.txt create mode 100755 src/test/resources/fake-agency-mtc-long-fields/calendar_dates.txt create mode 100755 src/test/resources/fake-agency-mtc-long-fields/fare_attributes.txt create mode 100755 src/test/resources/fake-agency-mtc-long-fields/fare_rules.txt create mode 100644 src/test/resources/fake-agency-mtc-long-fields/feed_info.txt create mode 100755 src/test/resources/fake-agency-mtc-long-fields/frequencies.txt create mode 100755 src/test/resources/fake-agency-mtc-long-fields/routes.txt create mode 100755 src/test/resources/fake-agency-mtc-long-fields/shapes.txt create mode 100755 src/test/resources/fake-agency-mtc-long-fields/stop_times.txt create mode 100755 src/test/resources/fake-agency-mtc-long-fields/stops.txt create mode 100755 src/test/resources/fake-agency-mtc-long-fields/transfers.txt create mode 100755 src/test/resources/fake-agency-mtc-long-fields/trips.txt diff --git a/src/main/java/com/conveyal/gtfs/GTFS.java b/src/main/java/com/conveyal/gtfs/GTFS.java index 0c5af6778..25dc6d40e 100644 --- a/src/main/java/com/conveyal/gtfs/GTFS.java +++ b/src/main/java/com/conveyal/gtfs/GTFS.java @@ -30,6 +30,7 @@ import java.sql.ResultSet; import java.sql.SQLException; import java.sql.Statement; +import java.util.Arrays; import static com.conveyal.gtfs.util.Util.ensureValidNamespace; @@ -95,9 +96,9 @@ public static SnapshotResult makeSnapshot (String feedId, DataSource dataSource) /** * Once a feed has been loaded into the database, examine its contents looking for various problems and errors. */ - public static ValidationResult validate (String feedId, DataSource dataSource, FeedValidator... additionalValidators) { + public static ValidationResult validate (String feedId, DataSource dataSource, Class... additionalValidators) { Feed feed = new Feed(dataSource, feedId); - ValidationResult result = feed.validate(additionalValidators); + ValidationResult result = feed.validate(Arrays.asList(additionalValidators)); return result; } diff --git a/src/main/java/com/conveyal/gtfs/error/FieldLengthError.java b/src/main/java/com/conveyal/gtfs/error/FieldLengthError.java new file mode 100644 index 000000000..b9fece918 --- /dev/null +++ b/src/main/java/com/conveyal/gtfs/error/FieldLengthError.java @@ -0,0 +1,17 @@ +package com.conveyal.gtfs.error; + +import java.io.Serializable; + +/** Indicates that the length of a field is invalid. */ +public class FieldLengthError extends GTFSError implements Serializable { + public static final long serialVersionUID = 1L; + + public FieldLengthError(String file, long line, String field) { + super(file, line, field); + } + + @Override public String getMessage() { + return String.format("Field length is invalid."); + } + +} diff --git a/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java b/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java index 88911627d..ccbee56d9 100644 --- a/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java +++ b/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java @@ -11,6 +11,7 @@ public enum NewGTFSErrorType { URL_FORMAT(Priority.MEDIUM, "URL format should be ://?#"), LANGUAGE_FORMAT(Priority.LOW, "Language should be specified with a valid BCP47 tag."), ILLEGAL_FIELD_VALUE(Priority.MEDIUM, "Fields may not contain tabs, carriage returns or new lines."), + FIELD_VALUE_TOO_LONG(Priority.MEDIUM, "Field values have too many characters."), INTEGER_FORMAT(Priority.MEDIUM, "Incorrect integer format."), FARE_TRANSFER_MISMATCH(Priority.MEDIUM, "A fare that does not permit transfers has a non-zero transfer duration."), FREQUENCY_PERIOD_OVERLAP(Priority.MEDIUM, "A frequency for a trip overlaps with another frequency defined for the same trip."), diff --git a/src/main/java/com/conveyal/gtfs/loader/Feed.java b/src/main/java/com/conveyal/gtfs/loader/Feed.java index 271b7fd10..65dde2518 100644 --- a/src/main/java/com/conveyal/gtfs/loader/Feed.java +++ b/src/main/java/com/conveyal/gtfs/loader/Feed.java @@ -7,15 +7,19 @@ import com.conveyal.gtfs.storage.StorageException; import com.conveyal.gtfs.util.InvalidNamespaceException; import com.conveyal.gtfs.validator.*; +import com.google.common.collect.Lists; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import javax.sql.DataSource; +import java.lang.reflect.Constructor; +import java.lang.reflect.InvocationTargetException; import java.sql.Connection; import java.sql.SQLException; import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.stream.Collectors; import static com.conveyal.gtfs.error.NewGTFSErrorType.VALIDATOR_FAILED; @@ -74,7 +78,7 @@ public Feed (DataSource dataSource, String tablePrefix) { * TODO allow validation within feed loading process, so the same connection can be used, and we're certain loaded data is 100% visible. * That would also avoid having to reconnect the error storage to the DB. */ - public ValidationResult validate (FeedValidator... additionalValidators) { + public ValidationResult validate (List> additionalValidatorClasses) { long validationStartTime = System.currentTimeMillis(); // Create an empty validation result that will have its fields populated by certain validators. ValidationResult validationResult = new ValidationResult(); @@ -88,7 +92,7 @@ public ValidationResult validate (FeedValidator... additionalValidators) { } int errorCountBeforeValidation = errorStorage.getErrorCount(); - List feedValidators = Arrays.asList( + ArrayList feedValidators = Lists.newArrayList( new MisplacedStopValidator(this, errorStorage, validationResult), new DuplicateStopsValidator(this, errorStorage), new FaresValidator(this, errorStorage), @@ -97,8 +101,17 @@ public ValidationResult validate (FeedValidator... additionalValidators) { new NewTripTimesValidator(this, errorStorage), new NamesValidator(this, errorStorage) ); - // Add any additional validators specified in args. - feedValidators.addAll(Arrays.asList(additionalValidators)); + + // Instantiate any additional validators and + // add them to the list of validators. + for (Class validatorClass : additionalValidatorClasses) { + try { + Constructor constructor = validatorClass.getConstructor(Feed.class, SQLErrorStorage.class); + feedValidators.add(constructor.newInstance(this, errorStorage)); + } catch (NoSuchMethodException | InstantiationException | IllegalAccessException | InvocationTargetException e) { + e.printStackTrace(); + } + } for (FeedValidator feedValidator : feedValidators) { String validatorName = feedValidator.getClass().getSimpleName(); diff --git a/src/main/java/com/conveyal/gtfs/validator/FieldLengthValidator.java b/src/main/java/com/conveyal/gtfs/validator/FieldLengthValidator.java new file mode 100644 index 000000000..e98cefba4 --- /dev/null +++ b/src/main/java/com/conveyal/gtfs/validator/FieldLengthValidator.java @@ -0,0 +1,18 @@ +package com.conveyal.gtfs.validator; + +import com.conveyal.gtfs.error.SQLErrorStorage; +import com.conveyal.gtfs.loader.Feed; + +/** + * Unlike FeedValidators that are run against the entire feed, these validators are run against the stop_times for + * a specific trip. This is an optimization that allows us to fetch and group those stop_times only once. + */ +public class FieldLengthValidator extends Validator { + public FieldLengthValidator(Feed feed, SQLErrorStorage errorStorage) { + super(feed, errorStorage); + } + + public boolean validateFieldLength(String value, int length) { + return value != null && value.length() > length; + } +} diff --git a/src/main/java/com/conveyal/gtfs/validator/MTCValidator.java b/src/main/java/com/conveyal/gtfs/validator/MTCValidator.java new file mode 100644 index 000000000..24265e320 --- /dev/null +++ b/src/main/java/com/conveyal/gtfs/validator/MTCValidator.java @@ -0,0 +1,16 @@ +package com.conveyal.gtfs.validator; + +import com.conveyal.gtfs.error.SQLErrorStorage; +import com.conveyal.gtfs.loader.Feed; + +public class MTCValidator extends FeedValidator { + + public MTCValidator(Feed feed, SQLErrorStorage errorStorage) { + super(feed, errorStorage); + } + + @Override + public void validate() { + + } +} diff --git a/src/test/java/com/conveyal/gtfs/GTFSTestMTC.java b/src/test/java/com/conveyal/gtfs/GTFSTestMTC.java new file mode 100644 index 000000000..2b2b3cbe1 --- /dev/null +++ b/src/test/java/com/conveyal/gtfs/GTFSTestMTC.java @@ -0,0 +1,661 @@ +package com.conveyal.gtfs; + + +import com.conveyal.gtfs.error.NewGTFSErrorType; +import com.conveyal.gtfs.error.SQLErrorStorage; +import com.conveyal.gtfs.loader.Feed; +import com.conveyal.gtfs.loader.FeedLoadResult; +import com.conveyal.gtfs.loader.SnapshotResult; +import com.conveyal.gtfs.storage.ErrorExpectation; +import com.conveyal.gtfs.storage.ExpectedFieldType; +import com.conveyal.gtfs.storage.PersistenceExpectation; +import com.conveyal.gtfs.storage.RecordExpectation; +import com.conveyal.gtfs.util.InvalidNamespaceException; +import com.conveyal.gtfs.validator.FeedValidator; +import com.conveyal.gtfs.validator.MTCValidator; +import com.conveyal.gtfs.validator.ValidationResult; +import com.csvreader.CsvReader; +import com.google.common.collect.ArrayListMultimap; +import com.google.common.collect.Multimap; +import com.google.common.io.Files; +import org.apache.commons.io.FileUtils; +import org.apache.commons.io.input.BOMInputStream; +import org.hamcrest.Matcher; +import org.hamcrest.comparator.ComparatorMatcherBuilder; +import org.junit.Before; +import org.junit.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import javax.sql.DataSource; +import java.io.*; +import java.lang.reflect.Constructor; +import java.nio.charset.Charset; +import java.sql.Connection; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.util.Arrays; +import java.util.Collection; +import java.util.Iterator; +import java.util.zip.ZipEntry; +import java.util.zip.ZipFile; + +import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.core.IsNull.nullValue; +import static org.junit.Assert.assertThat; + +/** + * A test suite for the {@link GTFS} Class. + */ +public class GTFSTestMTC { + private final ByteArrayOutputStream outContent = new ByteArrayOutputStream(); + private static final String JDBC_URL = "jdbc:postgresql://localhost"; + private static final Logger LOG = LoggerFactory.getLogger(GTFSTestMTC.class); + + // setup a stream to capture the output from the program + @Before + public void setUpStreams() { + System.setOut(new PrintStream(outContent)); + } + + /** + * Tests that a GTFS feed with long field values generates corresponding validation errors. + */ + @Test + public void canLoadFeedWithLongFieldValues () { + PersistenceExpectation[] expectations = PersistenceExpectation.list(); + ErrorExpectation[] errorExpectations = ErrorExpectation.list( + new ErrorExpectation(NewGTFSErrorType.MISSING_FIELD), + new ErrorExpectation(NewGTFSErrorType.FIELD_VALUE_TOO_LONG), + new ErrorExpectation(NewGTFSErrorType.ROUTE_LONG_NAME_CONTAINS_SHORT_NAME), + new ErrorExpectation(NewGTFSErrorType.FEED_TRAVEL_TIMES_ROUNDED), + new ErrorExpectation(NewGTFSErrorType.STOP_UNUSED, equalTo("1234567")), + new ErrorExpectation(NewGTFSErrorType.DATE_NO_SERVICE) + ); + assertThat( + "Long-field-value test passes", + runIntegrationTestOnFolder("fake-agency-mtc-long-fields", 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. + * TODO: Refactor-consolidate with the same method in GTFSTest.java. + */ + private boolean runIntegrationTestOnFolder( + String folderName, + Matcher fatalExceptionExpectation, + PersistenceExpectation[] persistenceExpectations, + ErrorExpectation[] errorExpectations + ) { + LOG.info("Running integration test on folder {}", folderName); + // zip up test folder into temp zip file + String zipFileName = null; + try { + zipFileName = TestUtils.zipFolderFiles(folderName, true); + } catch (IOException e) { + e.printStackTrace(); + return false; + } + return runIntegrationTestOnZipFile(zipFileName, fatalExceptionExpectation, persistenceExpectations, errorExpectations); + } + + /** + * A helper method that will run GTFS#main with a certain zip file. + * This tests whether a GTFS zip file can be loaded without any errors. The full list of steps includes: + * 1. GTFS#load + * 2. GTFS#validate + * 3. exportGtfs/check exported GTFS integrity + * 4. makeSnapshot + * 5. Delete feed/namespace + * TODO: Refactor-consolidate with the same method in GTFSTest.java. + */ + private boolean runIntegrationTestOnZipFile( + String zipFileName, + Matcher fatalExceptionExpectation, + PersistenceExpectation[] persistenceExpectations, + ErrorExpectation[] errorExpectations + ) { + String testDBName = TestUtils.generateNewDB(); + String dbConnectionUrl = String.join("/", JDBC_URL, testDBName); + DataSource dataSource = GTFS.createDataSource( + dbConnectionUrl, + null, + null + ); + + String namespace; + + // Verify that loading the feed completes and data is stored properly + try (Connection connection = dataSource.getConnection()) { + // load and validate feed + LOG.info("load and validate GTFS file {}", zipFileName); + FeedLoadResult loadResult = GTFS.load(zipFileName, dataSource); + ValidationResult validationResult = GTFS.validate(loadResult.uniqueIdentifier, dataSource, MTCValidator.class); + + assertThat(validationResult.fatalException, is(fatalExceptionExpectation)); + namespace = loadResult.uniqueIdentifier; + assertThatImportedGtfsMeetsExpectations(connection, namespace, persistenceExpectations, errorExpectations); + } catch (SQLException e) { + TestUtils.dropDB(testDBName); + e.printStackTrace(); + return false; + } catch (AssertionError e) { + TestUtils.dropDB(testDBName); + throw e; + } + + // Verify that exporting the feed (in non-editor mode) completes and data is outputted properly + try { + LOG.info("export GTFS from created namespace"); + File tempFile = exportGtfs(namespace, dataSource, false); + assertThatExportedGtfsMeetsExpectations(tempFile, persistenceExpectations, false); + } catch (IOException e) { + TestUtils.dropDB(testDBName); + e.printStackTrace(); + return false; + } catch (AssertionError e) { + TestUtils.dropDB(testDBName); + throw e; + } + + // Verify that making a snapshot from an existing feed database, then exporting that snapshot to a GTFS zip file + // works as expected + try { + LOG.info("copy GTFS from created namespace"); + SnapshotResult copyResult = GTFS.makeSnapshot(namespace, dataSource); + assertThatSnapshotIsErrorFree(copyResult); + LOG.info("export GTFS from copied namespace"); + File tempFile = exportGtfs(copyResult.uniqueIdentifier, dataSource, true); + assertThatExportedGtfsMeetsExpectations(tempFile, persistenceExpectations, true); + } catch (IOException e) { + e.printStackTrace(); + TestUtils.dropDB(testDBName); + return false; + } catch (AssertionError e) { + TestUtils.dropDB(testDBName); + throw e; + } + + // Verify that deleting a feed works as expected. + try (Connection connection = dataSource.getConnection()) { + LOG.info("Deleting GTFS feed from database."); + GTFS.delete(namespace, dataSource); + + String sql = String.format("select * from feeds where namespace = '%s'", namespace); + LOG.info(sql); + ResultSet resultSet = connection.prepareStatement(sql).executeQuery(); + while (resultSet.next()) { + // Assert that the feed registry shows feed as deleted. + assertThat(resultSet.getBoolean("deleted"), is(true)); + } + // Ensure that schema no longer exists for namespace (note: this is Postgres specific). + String schemaSql = String.format( + "SELECT * FROM information_schema.schemata where schema_name = '%s'", + namespace + ); + LOG.info(schemaSql); + ResultSet schemaResultSet = connection.prepareStatement(schemaSql).executeQuery(); + int schemaCount = 0; + while (schemaResultSet.next()) schemaCount++; + // There should be no schema records matching the deleted namespace. + assertThat(schemaCount, is(0)); + } catch (SQLException | InvalidNamespaceException e) { + e.printStackTrace(); + TestUtils.dropDB(testDBName); + return false; + } catch (AssertionError e) { + TestUtils.dropDB(testDBName); + throw e; + } + + // This should be run following all of the above tests (any new tests should go above these lines). + TestUtils.dropDB(testDBName); + return true; + } + + private void assertThatLoadIsErrorFree(FeedLoadResult loadResult) { + assertThat(loadResult.fatalException, is(nullValue())); + assertThat(loadResult.agency.fatalException, is(nullValue())); + assertThat(loadResult.calendar.fatalException, is(nullValue())); + assertThat(loadResult.calendarDates.fatalException, is(nullValue())); + assertThat(loadResult.fareAttributes.fatalException, is(nullValue())); + assertThat(loadResult.fareRules.fatalException, is(nullValue())); + assertThat(loadResult.feedInfo.fatalException, is(nullValue())); + assertThat(loadResult.frequencies.fatalException, is(nullValue())); + assertThat(loadResult.routes.fatalException, is(nullValue())); + assertThat(loadResult.shapes.fatalException, is(nullValue())); + assertThat(loadResult.stops.fatalException, is(nullValue())); + assertThat(loadResult.stopTimes.fatalException, is(nullValue())); + assertThat(loadResult.transfers.fatalException, is(nullValue())); + assertThat(loadResult.trips.fatalException, is(nullValue())); + } + + private void assertThatSnapshotIsErrorFree(SnapshotResult snapshotResult) { + assertThatLoadIsErrorFree(snapshotResult); + assertThat(snapshotResult.scheduleExceptions.fatalException, is(nullValue())); + } + + /** + * Helper function to export a GTFS from the database to a temporary zip file. + */ + private File exportGtfs(String namespace, DataSource dataSource, boolean fromEditor) throws IOException { + File tempFile = File.createTempFile("snapshot", ".zip"); + GTFS.export(namespace, tempFile.getAbsolutePath(), dataSource, fromEditor); + return tempFile; + } + + private class ValuePair { + private final Object expected; + private final Object found; + private ValuePair (Object expected, Object found) { + this.expected = expected; + this.found = found; + } + } + + /** + * Run through the list of persistence expectations to make sure that the feed was imported properly into the + * database. + */ + private void assertThatImportedGtfsMeetsExpectations( + Connection connection, + String namespace, + PersistenceExpectation[] persistenceExpectations, + ErrorExpectation[] errorExpectations + ) throws SQLException { + // Store field mismatches here (to provide assertion statements with more details). + Multimap fieldsWithMismatches = ArrayListMultimap.create(); + // Check that no validators failed during validation. + assertThat( + "One or more validators failed during GTFS import.", + countValidationErrorsOfType(connection, namespace, NewGTFSErrorType.VALIDATOR_FAILED), + equalTo(0) + ); + // run through testing expectations + LOG.info("testing expectations of record storage in the database"); + for (PersistenceExpectation persistenceExpectation : persistenceExpectations) { + // select all entries from a table + String sql = String.format( + "select * from %s.%s", + namespace, + persistenceExpectation.tableName + ); + LOG.info(sql); + ResultSet rs = connection.prepareStatement(sql).executeQuery(); + boolean foundRecord = false; + int numRecordsSearched = 0; + while (rs.next()) { + numRecordsSearched++; + LOG.info("record {} in ResultSet", numRecordsSearched); + boolean allFieldsMatch = true; + for (RecordExpectation recordExpectation: persistenceExpectation.recordExpectations) { + switch (recordExpectation.expectedFieldType) { + case DOUBLE: + double doubleVal = rs.getDouble(recordExpectation.fieldName); + LOG.info("{}: {}", recordExpectation.fieldName, doubleVal); + if (doubleVal != recordExpectation.doubleExpectation) { + allFieldsMatch = false; + } + break; + case INT: + int intVal = rs.getInt(recordExpectation.fieldName); + LOG.info("{}: {}", recordExpectation.fieldName, intVal); + if (intVal != recordExpectation.intExpectation) { + fieldsWithMismatches.put( + recordExpectation.fieldName, + new ValuePair(recordExpectation.stringExpectation, intVal) + ); + allFieldsMatch = false; + } + break; + case STRING: + String strVal = rs.getString(recordExpectation.fieldName); + LOG.info("{}: {}", recordExpectation.fieldName, strVal); + if (strVal == null && recordExpectation.stringExpectation == null) { + break; + } else if (strVal == null || !strVal.equals(recordExpectation.stringExpectation)) { + fieldsWithMismatches.put( + recordExpectation.fieldName, + new ValuePair(recordExpectation.stringExpectation, strVal) + ); + LOG.error("Expected {}, found {}", recordExpectation.stringExpectation, strVal); + allFieldsMatch = false; + } + break; + + } + if (!allFieldsMatch) { + break; + } + } + // all fields match expectations! We have found the record. + if (allFieldsMatch) { + LOG.info("Database record satisfies expectations."); + foundRecord = true; + break; + } else { + LOG.error("Persistence mismatch on record {}", numRecordsSearched); + } + } + assertThatPersistenceExpectationRecordWasFound(numRecordsSearched, foundRecord, fieldsWithMismatches); + } + // Expect zero errors if errorExpectations is null. + if (errorExpectations == null) errorExpectations = new ErrorExpectation[]{}; + // Check that error expectations match errors stored in database. + LOG.info("Checking {} error expectations", errorExpectations.length); + // select all entries from error table + String sql = String.format("select * from %s.errors", namespace); + LOG.info(sql); + ResultSet rs = connection.prepareStatement(sql).executeQuery(); + int errorCount = 0; + Iterator errorExpectationIterator = Arrays.stream(errorExpectations).iterator(); + while (rs.next()) { + errorCount++; + String errorType = rs.getString("error_type"); + String entityType = rs.getString("entity_type"); + String entityId = rs.getString("entity_id"); + String badValue = rs.getString("bad_value"); + LOG.info("Found error {}: {} {} {} {}", errorCount, errorType, entityId, entityType, badValue); + // Skip error expectation if not exists. But continue iteration to count all errors. + if (!errorExpectationIterator.hasNext()) continue; + ErrorExpectation errorExpectation = errorExpectationIterator.next(); + LOG.info("Expecting error {}: {}", errorCount, errorExpectation.errorTypeMatcher); + // Error expectation must contain error type matcher. The others are optional. + assertThat(errorType, errorExpectation.errorTypeMatcher); + if (errorExpectation.entityTypeMatcher != null) assertThat(entityType, errorExpectation.entityTypeMatcher); + if (errorExpectation.entityIdMatcher != null) assertThat(entityId, errorExpectation.entityIdMatcher); + if (errorExpectation.badValueMatcher != null) assertThat(badValue, errorExpectation.badValueMatcher); + } + assertThat( + "Error count is equal to number of error expectations.", + errorCount, + equalTo(errorExpectations.length)); + } + + private static int countValidationErrorsOfType( + Connection connection, + String namespace, + NewGTFSErrorType errorType + ) throws SQLException { + String errorCheckSql = String.format( + "select * from %s.errors where error_type = '%s'", + namespace, + errorType); + LOG.info(errorCheckSql); + ResultSet errorResults = connection.prepareStatement(errorCheckSql).executeQuery(); + int errorCount = 0; + while (errorResults.next()) { + errorCount++; + } + return errorCount; + } + + /** + * Helper to assert that the GTFS that was exported to a zip file matches all data expectations defined in the + * persistence expectations. + */ + private void assertThatExportedGtfsMeetsExpectations( + File tempFile, + PersistenceExpectation[] persistenceExpectations, + boolean fromEditor + ) throws IOException { + LOG.info("testing expectations of csv outputs in an exported gtfs"); + + ZipFile gtfsZipfile = new ZipFile(tempFile.getAbsolutePath()); + + // iterate through all expectations + for (PersistenceExpectation persistenceExpectation : persistenceExpectations) { + // No need to check that errors were exported because it is an internal table only. + if ("errors".equals(persistenceExpectation.tableName)) continue; + final String tableFileName = persistenceExpectation.tableName + ".txt"; + LOG.info(String.format("reading table: %s", tableFileName)); + + ZipEntry entry = gtfsZipfile.getEntry(tableFileName); + + // ensure file exists in zip + if (entry == null) { + throw new AssertionError( + String.format("expected table %s not found in outputted zip file", tableFileName) + ); + } + + // prepare to read the file + InputStream zipInputStream = gtfsZipfile.getInputStream(entry); + // Skip any byte order mark that may be present. Files must be UTF-8, + // but the GTFS spec says that "files that include the UTF byte order mark are acceptable". + InputStream bomInputStream = new BOMInputStream(zipInputStream); + CsvReader csvReader = new CsvReader(bomInputStream, ',', Charset.forName("UTF8")); + csvReader.readHeaders(); + + boolean foundRecord = false; + int numRecordsSearched = 0; + + // read each record + while (csvReader.readRecord() && !foundRecord) { + numRecordsSearched++; + LOG.info(String.format("record %d in csv file", numRecordsSearched)); + boolean allFieldsMatch = true; + + // iterate through all rows in record to determine if it's the one we're looking for + for (RecordExpectation recordExpectation: persistenceExpectation.recordExpectations) { + String val = csvReader.get(recordExpectation.fieldName); + String expectation = recordExpectation.getStringifiedExpectation(fromEditor); + LOG.info(String.format( + "%s: %s (Expectation: %s)", + recordExpectation.fieldName, + val, + expectation + )); + if (val.isEmpty() && expectation == null) { + // First check that the csv value is an empty string and that the expectation is null. Null + // exported from the database to a csv should round trip into an empty string, so this meets the + // expectation. + break; + } else if (!val.equals(expectation)) { + // sometimes there are slight differences in decimal precision in various fields + // check if the decimal delta is acceptable + if (equalsWithNumericDelta(val, recordExpectation)) continue; + allFieldsMatch = false; + break; + } + } + // all fields match expectations! We have found the record. + if (allFieldsMatch) { + LOG.info("CSV record satisfies expectations."); + foundRecord = true; + } + } + assertThatPersistenceExpectationRecordWasFound(numRecordsSearched, foundRecord, null); + } + } + + /** + * Check whether a potentially numeric value is equal given potentially small decimal deltas + */ + private boolean equalsWithNumericDelta(String val, RecordExpectation recordExpectation) { + if (recordExpectation.expectedFieldType != ExpectedFieldType.DOUBLE) return false; + double d; + try { + d = Double.parseDouble(val); + } + catch(NumberFormatException nfe) + { + return false; + } + return Math.abs(d - recordExpectation.doubleExpectation) < recordExpectation.acceptedDelta; + } + + /** + * Helper method to make sure a persistence expectation was actually found after searching through records + */ + private void assertThatPersistenceExpectationRecordWasFound( + int numRecordsSearched, + boolean foundRecord, + Multimap mismatches + ) { + assertThat( + "No records found in the ResultSet/CSV file", + numRecordsSearched, + ComparatorMatcherBuilder.usingNaturalOrdering().greaterThan(0) + ); + if (mismatches != null) { + for (String field : mismatches.keySet()) { + Collection valuePairs = mismatches.get(field); + for (ValuePair valuePair : valuePairs) { + assertThat( + String.format("The value expected for %s was not found", field), + valuePair.found, + equalTo(valuePair.expected) + ); + } + } + } else { + assertThat( + "The record as defined in the PersistenceExpectation was not found.", + foundRecord, + equalTo(true) + ); + } + } + + /** + * Persistence expectations for use with the GTFS contained within the "fake-agency" resources folder. + */ + private PersistenceExpectation[] fakeAgencyPersistenceExpectations = new PersistenceExpectation[]{ + new PersistenceExpectation( + "agency", + new RecordExpectation[]{ + new RecordExpectation("agency_id", "1"), + new RecordExpectation("agency_name", "Fake Transit"), + new RecordExpectation("agency_timezone", "America/Los_Angeles") + } + ), + new PersistenceExpectation( + "calendar", + new RecordExpectation[]{ + new RecordExpectation( + "service_id", "04100312-8fe1-46a5-a9f2-556f39478f57" + ), + new RecordExpectation("monday", 1), + new RecordExpectation("tuesday", 1), + new RecordExpectation("wednesday", 1), + new RecordExpectation("thursday", 1), + new RecordExpectation("friday", 1), + new RecordExpectation("saturday", 1), + new RecordExpectation("sunday", 1), + new RecordExpectation("start_date", "20170915"), + new RecordExpectation("end_date", "20170917") + } + ), + new PersistenceExpectation( + "calendar_dates", + new RecordExpectation[]{ + new RecordExpectation( + "service_id", "04100312-8fe1-46a5-a9f2-556f39478f57" + ), + new RecordExpectation("date", 20170916), + new RecordExpectation("exception_type", 2) + } + ), + new PersistenceExpectation( + "fare_attributes", + new RecordExpectation[]{ + new RecordExpectation("fare_id", "route_based_fare"), + new RecordExpectation("price", 1.23, 0), + new RecordExpectation("currency_type", "USD") + } + ), + new PersistenceExpectation( + "fare_rules", + new RecordExpectation[]{ + new RecordExpectation("fare_id", "route_based_fare"), + new RecordExpectation("route_id", "1") + } + ), + new PersistenceExpectation( + "feed_info", + new RecordExpectation[]{ + new RecordExpectation("feed_id", "fake_transit"), + new RecordExpectation("feed_publisher_name", "Conveyal"), + new RecordExpectation( + "feed_publisher_url", "http://www.conveyal.com" + ), + new RecordExpectation("feed_lang", "en"), + new RecordExpectation("feed_version", "1.0") + } + ), + new PersistenceExpectation( + "frequencies", + new RecordExpectation[]{ + new RecordExpectation("trip_id", "frequency-trip"), + new RecordExpectation("start_time", 28800, "08:00:00"), + new RecordExpectation("end_time", 32400, "09:00:00"), + new RecordExpectation("headway_secs", 1800), + new RecordExpectation("exact_times", 0) + } + ), + new PersistenceExpectation( + "routes", + new RecordExpectation[]{ + new RecordExpectation("agency_id", "1"), + new RecordExpectation("route_id", "1"), + new RecordExpectation("route_short_name", "1"), + new RecordExpectation("route_long_name", "Route 1"), + new RecordExpectation("route_type", 3), + new RecordExpectation("route_color", "7CE6E7") + } + ), + new PersistenceExpectation( + "shapes", + new RecordExpectation[]{ + new RecordExpectation( + "shape_id", "5820f377-f947-4728-ac29-ac0102cbc34e" + ), + new RecordExpectation("shape_pt_lat", 37.061172, 0.00001), + new RecordExpectation("shape_pt_lon", -122.007500, 0.00001), + new RecordExpectation("shape_pt_sequence", 2), + new RecordExpectation("shape_dist_traveled", 7.4997067, 0.01) + } + ), + new PersistenceExpectation( + "stop_times", + new RecordExpectation[]{ + new RecordExpectation( + "trip_id", "a30277f8-e50a-4a85-9141-b1e0da9d429d" + ), + new RecordExpectation("arrival_time", 25200, "07:00:00"), + new RecordExpectation("departure_time", 25200, "07:00:00"), + new RecordExpectation("stop_id", "4u6g"), + // the string expectation for stop_sequence is different because of how stop_times are + // converted to 0-based indexes in Table.normalizeAndCloneStopTimes + new RecordExpectation("stop_sequence", 1, "1", "0"), + new RecordExpectation("pickup_type", 0), + new RecordExpectation("drop_off_type", 0), + new RecordExpectation("shape_dist_traveled", 0.0, 0.01) + } + ), + new PersistenceExpectation( + "trips", + new RecordExpectation[]{ + new RecordExpectation( + "trip_id", "a30277f8-e50a-4a85-9141-b1e0da9d429d" + ), + new RecordExpectation( + "service_id", "04100312-8fe1-46a5-a9f2-556f39478f57" + ), + new RecordExpectation("route_id", "1"), + new RecordExpectation("direction_id", 0), + new RecordExpectation( + "shape_id", "5820f377-f947-4728-ac29-ac0102cbc34e" + ), + new RecordExpectation("bikes_allowed", 0), + new RecordExpectation("wheelchair_accessible", 0) + } + ) + }; +} diff --git a/src/test/java/com/conveyal/gtfs/validator/FieldLengthValidatorTest.java b/src/test/java/com/conveyal/gtfs/validator/FieldLengthValidatorTest.java new file mode 100644 index 000000000..27a47cc95 --- /dev/null +++ b/src/test/java/com/conveyal/gtfs/validator/FieldLengthValidatorTest.java @@ -0,0 +1,32 @@ +package com.conveyal.gtfs.validator; + +import com.conveyal.gtfs.GTFS; +import com.conveyal.gtfs.GTFSFeed; +import com.conveyal.gtfs.TestUtils; +import com.conveyal.gtfs.error.SQLErrorStorage; +import org.junit.BeforeClass; +import org.junit.Test; + +import java.io.IOException; + +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.MatcherAssert.assertThat; + +public class FieldLengthValidatorTest { + @Test + public void validateFieldlLength() { +/* + String[] args = {"-u", "blah"}; + GTFS.main(args); + + SQLErrorStorage errorStorage = new SQLErrorStorage(); + FieldLengthValidator validator = new FieldLengthValidator(null, null); + final String longFieldValue = "abcdefghijklmnopqrstwxyz1234567890"; + final String shortFieldValue = "abcdef"; + final int length = 20; + + assertThat(validator.validateFieldLength(longFieldValue, length), is(true)); + assertThat(validator.validateFieldLength(shortFieldValue, length), is(false)); + */ + } +} diff --git a/src/test/resources/fake-agency-mtc-long-fields/agency.txt b/src/test/resources/fake-agency-mtc-long-fields/agency.txt new file mode 100755 index 000000000..1d3884acc --- /dev/null +++ b/src/test/resources/fake-agency-mtc-long-fields/agency.txt @@ -0,0 +1,5 @@ +agency_id,agency_name,agency_url,agency_lang,agency_phone,agency_email,agency_timezone,agency_fare_url,agency_branding_url +1,Fake Transit,,,,,America/Los_Angeles,, +2,Agency name with more than fifty 50 characters that does not meet MTC guidelines,,,,,America/Los_Angeles,, +Agency_id_with_more_than_50_characters_that_does_not_meet_MTC_guidelines,Fake Transit,,,,,America/Los_Angeles,, +4,Fake Transit,http://www.agency.com/long/url/____________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________,,,,America/Los_Angeles,, diff --git a/src/test/resources/fake-agency-mtc-long-fields/calendar.txt b/src/test/resources/fake-agency-mtc-long-fields/calendar.txt new file mode 100755 index 000000000..4c9b7fd13 --- /dev/null +++ b/src/test/resources/fake-agency-mtc-long-fields/calendar.txt @@ -0,0 +1,2 @@ +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 diff --git a/src/test/resources/fake-agency-mtc-long-fields/calendar_dates.txt b/src/test/resources/fake-agency-mtc-long-fields/calendar_dates.txt new file mode 100755 index 000000000..403ee2bbe --- /dev/null +++ b/src/test/resources/fake-agency-mtc-long-fields/calendar_dates.txt @@ -0,0 +1,2 @@ +service_id,date,exception_type +04100312-8fe1-46a5-a9f2-556f39478f57,20170916,2 \ No newline at end of file diff --git a/src/test/resources/fake-agency-mtc-long-fields/fare_attributes.txt b/src/test/resources/fake-agency-mtc-long-fields/fare_attributes.txt new file mode 100755 index 000000000..3173d016d --- /dev/null +++ b/src/test/resources/fake-agency-mtc-long-fields/fare_attributes.txt @@ -0,0 +1,2 @@ +fare_id,price,currency_type,payment_method,transfers,transfer_duration +route_based_fare,1.23,USD,0,0,0 \ No newline at end of file diff --git a/src/test/resources/fake-agency-mtc-long-fields/fare_rules.txt b/src/test/resources/fake-agency-mtc-long-fields/fare_rules.txt new file mode 100755 index 000000000..05d2aadf8 --- /dev/null +++ b/src/test/resources/fake-agency-mtc-long-fields/fare_rules.txt @@ -0,0 +1,2 @@ +fare_id,route_id,origin_id,destination_id,contains_id +route_based_fare,1,,, \ No newline at end of file diff --git a/src/test/resources/fake-agency-mtc-long-fields/feed_info.txt b/src/test/resources/fake-agency-mtc-long-fields/feed_info.txt new file mode 100644 index 000000000..ceac60810 --- /dev/null +++ b/src/test/resources/fake-agency-mtc-long-fields/feed_info.txt @@ -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 \ No newline at end of file diff --git a/src/test/resources/fake-agency-mtc-long-fields/frequencies.txt b/src/test/resources/fake-agency-mtc-long-fields/frequencies.txt new file mode 100755 index 000000000..9baceff3a --- /dev/null +++ b/src/test/resources/fake-agency-mtc-long-fields/frequencies.txt @@ -0,0 +1,2 @@ +trip_id,start_time,end_time,headway_secs,exact_times +frequency-trip,08:00:00,09:00:00,1800,0 \ No newline at end of file diff --git a/src/test/resources/fake-agency-mtc-long-fields/routes.txt b/src/test/resources/fake-agency-mtc-long-fields/routes.txt new file mode 100755 index 000000000..35ea7aa67 --- /dev/null +++ b/src/test/resources/fake-agency-mtc-long-fields/routes.txt @@ -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,1,Route 1,,3,,7CE6E7,FFFFFF, diff --git a/src/test/resources/fake-agency-mtc-long-fields/shapes.txt b/src/test/resources/fake-agency-mtc-long-fields/shapes.txt new file mode 100755 index 000000000..3f2e3fd13 --- /dev/null +++ b/src/test/resources/fake-agency-mtc-long-fields/shapes.txt @@ -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 diff --git a/src/test/resources/fake-agency-mtc-long-fields/stop_times.txt b/src/test/resources/fake-agency-mtc-long-fields/stop_times.txt new file mode 100755 index 000000000..22534abf2 --- /dev/null +++ b/src/test/resources/fake-agency-mtc-long-fields/stop_times.txt @@ -0,0 +1,5 @@ +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, +frequency-trip,08:00:00,08:00:00,4u6g,1,,0,0,0.0000000, +frequency-trip,08:29:00,08:29:00,1234,2,,0,0,341.4491961, diff --git a/src/test/resources/fake-agency-mtc-long-fields/stops.txt b/src/test/resources/fake-agency-mtc-long-fields/stops.txt new file mode 100755 index 000000000..50d746d2b --- /dev/null +++ b/src/test/resources/fake-agency-mtc-long-fields/stops.txt @@ -0,0 +1,6 @@ +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,,, +123,,Parent Station,,37.0666,-122.0777,,,1,,, +1234,,Long Stop Name of more than 100 characters that exceeds MTC guidelines regarding the length of this field,,37.06662,-122.07772,,,0,123,, +1234567,,Unused stop,,37.06668,-122.07781,,,0,123,, \ No newline at end of file diff --git a/src/test/resources/fake-agency-mtc-long-fields/transfers.txt b/src/test/resources/fake-agency-mtc-long-fields/transfers.txt new file mode 100755 index 000000000..357103c47 --- /dev/null +++ b/src/test/resources/fake-agency-mtc-long-fields/transfers.txt @@ -0,0 +1 @@ +from_stop_id,to_stop_id,transfer_type,min_transfer_time diff --git a/src/test/resources/fake-agency-mtc-long-fields/trips.txt b/src/test/resources/fake-agency-mtc-long-fields/trips.txt new file mode 100755 index 000000000..d751e202c --- /dev/null +++ b/src/test/resources/fake-agency-mtc-long-fields/trips.txt @@ -0,0 +1,4 @@ +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 +1,frequency-trip,This is a trip headsign of more than 120 characters that exceeds the MTC guidelines on the number of characters,,0,,5820f377-f947-4728-ac29-ac0102cbc34e,0,0,04100312-8fe1-46a5-a9f2-556f39478f57 +1,trip-with-long-shortname,trip headsign,This is a trip short name of more than 50 characters beyond MTC guidelines,0,,5820f377-f947-4728-ac29-ac0102cbc34e,0,0,04100312-8fe1-46a5-a9f2-556f39478f57 \ No newline at end of file From eeb6b3240e914db8c9c9fbe0580a40d6354a5d1d Mon Sep 17 00:00:00 2001 From: binh-dam-ibigroup <56846598+binh-dam-ibigroup@users.noreply.github.com> Date: Thu, 2 Apr 2020 11:48:44 -0400 Subject: [PATCH 03/17] feat(MTCValidator): Implement MTC validator and complete test. --- src/main/java/com/conveyal/gtfs/GTFS.java | 14 +- .../gtfs/validator/FieldLengthValidator.java | 18 - .../conveyal/gtfs/validator/MTCValidator.java | 33 + src/test/java/com/conveyal/gtfs/GTFSTest.java | 34 +- .../java/com/conveyal/gtfs/GTFSTestMTC.java | 661 ------------------ .../validator/FieldLengthValidatorTest.java | 32 - .../gtfs/validator/MTCValidatorTest.java | 28 + .../fake-agency-mtc-long-fields/agency.txt | 6 +- .../calendar_dates.txt | 1 - .../fake-agency-mtc-long-fields/routes.txt | 2 +- .../stop_times.txt | 2 + .../fake-agency-mtc-long-fields/stops.txt | 1 - .../fake-agency-mtc-long-fields/trips.txt | 2 +- 13 files changed, 106 insertions(+), 728 deletions(-) delete mode 100644 src/main/java/com/conveyal/gtfs/validator/FieldLengthValidator.java delete mode 100644 src/test/java/com/conveyal/gtfs/GTFSTestMTC.java delete mode 100644 src/test/java/com/conveyal/gtfs/validator/FieldLengthValidatorTest.java create mode 100644 src/test/java/com/conveyal/gtfs/validator/MTCValidatorTest.java diff --git a/src/main/java/com/conveyal/gtfs/GTFS.java b/src/main/java/com/conveyal/gtfs/GTFS.java index 25dc6d40e..78ad63f00 100644 --- a/src/main/java/com/conveyal/gtfs/GTFS.java +++ b/src/main/java/com/conveyal/gtfs/GTFS.java @@ -16,10 +16,10 @@ import org.apache.commons.dbcp2.DriverManagerConnectionFactory; import org.apache.commons.dbcp2.PoolableConnectionFactory; import org.apache.commons.dbcp2.PoolingDataSource; -import org.apache.commons.dbutils.DbUtils; import org.apache.commons.pool2.impl.GenericObjectPool; import org.slf4j.Logger; import org.slf4j.LoggerFactory; + import javax.sql.DataSource; import java.io.File; import java.io.IOException; @@ -29,8 +29,8 @@ import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.SQLException; -import java.sql.Statement; -import java.util.Arrays; +import java.util.ArrayList; +import java.util.List; import static com.conveyal.gtfs.util.Util.ensureValidNamespace; @@ -96,9 +96,13 @@ public static SnapshotResult makeSnapshot (String feedId, DataSource dataSource) /** * Once a feed has been loaded into the database, examine its contents looking for various problems and errors. */ - public static ValidationResult validate (String feedId, DataSource dataSource, Class... additionalValidators) { + public static ValidationResult validate (String feedId, DataSource dataSource) { + return validate(feedId, dataSource, new ArrayList<>()); + } + + public static ValidationResult validate (String feedId, DataSource dataSource, List> additionalValidatorClasses) { Feed feed = new Feed(dataSource, feedId); - ValidationResult result = feed.validate(Arrays.asList(additionalValidators)); + ValidationResult result = feed.validate(additionalValidatorClasses); return result; } diff --git a/src/main/java/com/conveyal/gtfs/validator/FieldLengthValidator.java b/src/main/java/com/conveyal/gtfs/validator/FieldLengthValidator.java deleted file mode 100644 index e98cefba4..000000000 --- a/src/main/java/com/conveyal/gtfs/validator/FieldLengthValidator.java +++ /dev/null @@ -1,18 +0,0 @@ -package com.conveyal.gtfs.validator; - -import com.conveyal.gtfs.error.SQLErrorStorage; -import com.conveyal.gtfs.loader.Feed; - -/** - * Unlike FeedValidators that are run against the entire feed, these validators are run against the stop_times for - * a specific trip. This is an optimization that allows us to fetch and group those stop_times only once. - */ -public class FieldLengthValidator extends Validator { - public FieldLengthValidator(Feed feed, SQLErrorStorage errorStorage) { - super(feed, errorStorage); - } - - public boolean validateFieldLength(String value, int length) { - return value != null && value.length() > length; - } -} diff --git a/src/main/java/com/conveyal/gtfs/validator/MTCValidator.java b/src/main/java/com/conveyal/gtfs/validator/MTCValidator.java index 24265e320..ab1815348 100644 --- a/src/main/java/com/conveyal/gtfs/validator/MTCValidator.java +++ b/src/main/java/com/conveyal/gtfs/validator/MTCValidator.java @@ -2,7 +2,16 @@ import com.conveyal.gtfs.error.SQLErrorStorage; import com.conveyal.gtfs.loader.Feed; +import com.conveyal.gtfs.model.*; +import static com.conveyal.gtfs.error.NewGTFSErrorType.FIELD_VALUE_TOO_LONG; + +/** + * MTCValidator validates a GTFS feed according to the following + * additional guidelines by 511 MTC: + * - Field values should not exceed some number of characters. + * Those requirements can be found by searching for the word 'character'. + */ public class MTCValidator extends FeedValidator { public MTCValidator(Feed feed, SQLErrorStorage errorStorage) { @@ -11,6 +20,30 @@ public MTCValidator(Feed feed, SQLErrorStorage errorStorage) { @Override public void validate() { + for (Agency agency : feed.agencies) { + fieldLengthShouldNotExceed(agency, agency.agency_id, 50); + fieldLengthShouldNotExceed(agency, agency.agency_name, 50); + fieldLengthShouldNotExceed(agency, agency.agency_url, 500); + } + + for (Stop stop : feed.stops) { + fieldLengthShouldNotExceed(stop, stop.stop_name, 100); + } + + for (Trip trip : feed.trips) { + fieldLengthShouldNotExceed(trip, trip.trip_headsign, 120); + fieldLengthShouldNotExceed(trip, trip.trip_short_name, 50); + } + + // TODO: Handle calendar_attributes.txt? + } + boolean fieldLengthShouldNotExceed(Entity entity, Object objValue, int maxLength) { + String value = objValue != null ? objValue.toString() : ""; + if (value.length() > maxLength) { + if (errorStorage != null) registerError(entity, FIELD_VALUE_TOO_LONG, value); + return false; + } + return true; } } diff --git a/src/test/java/com/conveyal/gtfs/GTFSTest.java b/src/test/java/com/conveyal/gtfs/GTFSTest.java index 25be50ac8..095e19885 100644 --- a/src/test/java/com/conveyal/gtfs/GTFSTest.java +++ b/src/test/java/com/conveyal/gtfs/GTFSTest.java @@ -9,12 +9,13 @@ import com.conveyal.gtfs.storage.PersistenceExpectation; import com.conveyal.gtfs.storage.RecordExpectation; import com.conveyal.gtfs.util.InvalidNamespaceException; +import com.conveyal.gtfs.validator.FeedValidator; +import com.conveyal.gtfs.validator.MTCValidator; import com.conveyal.gtfs.validator.ValidationResult; import com.csvreader.CsvReader; import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.Multimap; import com.google.common.io.Files; -import org.apache.commons.dbutils.DbUtils; import org.apache.commons.io.FileUtils; import org.apache.commons.io.input.BOMInputStream; import org.hamcrest.Matcher; @@ -32,14 +33,11 @@ import java.io.PrintStream; import java.nio.charset.Charset; import java.sql.Connection; -import java.sql.DriverManager; -import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.SQLException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; -import java.util.Date; import java.util.Iterator; import java.util.List; import java.util.zip.ZipEntry; @@ -59,6 +57,8 @@ public class GTFSTest { private static final String JDBC_URL = "jdbc:postgresql://localhost"; private static final Logger LOG = LoggerFactory.getLogger(GTFSTest.class); + private List> extraValidatorClasses = new ArrayList<>(); + // setup a stream to capture the output from the program @Before public void setUpStreams() { @@ -306,6 +306,30 @@ public void canLoadAndExportSimpleAgencyWithOnlyCalendarDates() { ); } + /** + * Tests that a GTFS feed with long field values generates corresponding + * validation errors per MTC guidelines. + */ + @Test + public void canLoadFeedWithLongFieldValues () { + extraValidatorClasses.add(MTCValidator.class); + + PersistenceExpectation[] expectations = PersistenceExpectation.list(); + ErrorExpectation[] errorExpectations = ErrorExpectation.list( + new ErrorExpectation(NewGTFSErrorType.FIELD_VALUE_TOO_LONG), + new ErrorExpectation(NewGTFSErrorType.FIELD_VALUE_TOO_LONG), + new ErrorExpectation(NewGTFSErrorType.FIELD_VALUE_TOO_LONG), + new ErrorExpectation(NewGTFSErrorType.FIELD_VALUE_TOO_LONG), + new ErrorExpectation(NewGTFSErrorType.FIELD_VALUE_TOO_LONG), + new ErrorExpectation(NewGTFSErrorType.FIELD_VALUE_TOO_LONG), + new ErrorExpectation(NewGTFSErrorType.FEED_TRAVEL_TIMES_ROUNDED) // Not related, not worrying about this one. + ); + assertThat( + "Long-field-value test passes", + runIntegrationTestOnFolder("fake-agency-mtc-long-fields", nullValue(), expectations, errorExpectations), + equalTo(true) + ); + } /** * A helper method that will zip a specified folder in test/main/resources and call @@ -359,7 +383,7 @@ private boolean runIntegrationTestOnZipFile( // load and validate feed LOG.info("load and validate GTFS file {}", zipFileName); FeedLoadResult loadResult = GTFS.load(zipFileName, dataSource); - ValidationResult validationResult = GTFS.validate(loadResult.uniqueIdentifier, dataSource); + ValidationResult validationResult = GTFS.validate(loadResult.uniqueIdentifier, dataSource, extraValidatorClasses); assertThat(validationResult.fatalException, is(fatalExceptionExpectation)); namespace = loadResult.uniqueIdentifier; diff --git a/src/test/java/com/conveyal/gtfs/GTFSTestMTC.java b/src/test/java/com/conveyal/gtfs/GTFSTestMTC.java deleted file mode 100644 index 2b2b3cbe1..000000000 --- a/src/test/java/com/conveyal/gtfs/GTFSTestMTC.java +++ /dev/null @@ -1,661 +0,0 @@ -package com.conveyal.gtfs; - - -import com.conveyal.gtfs.error.NewGTFSErrorType; -import com.conveyal.gtfs.error.SQLErrorStorage; -import com.conveyal.gtfs.loader.Feed; -import com.conveyal.gtfs.loader.FeedLoadResult; -import com.conveyal.gtfs.loader.SnapshotResult; -import com.conveyal.gtfs.storage.ErrorExpectation; -import com.conveyal.gtfs.storage.ExpectedFieldType; -import com.conveyal.gtfs.storage.PersistenceExpectation; -import com.conveyal.gtfs.storage.RecordExpectation; -import com.conveyal.gtfs.util.InvalidNamespaceException; -import com.conveyal.gtfs.validator.FeedValidator; -import com.conveyal.gtfs.validator.MTCValidator; -import com.conveyal.gtfs.validator.ValidationResult; -import com.csvreader.CsvReader; -import com.google.common.collect.ArrayListMultimap; -import com.google.common.collect.Multimap; -import com.google.common.io.Files; -import org.apache.commons.io.FileUtils; -import org.apache.commons.io.input.BOMInputStream; -import org.hamcrest.Matcher; -import org.hamcrest.comparator.ComparatorMatcherBuilder; -import org.junit.Before; -import org.junit.Test; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -import javax.sql.DataSource; -import java.io.*; -import java.lang.reflect.Constructor; -import java.nio.charset.Charset; -import java.sql.Connection; -import java.sql.ResultSet; -import java.sql.SQLException; -import java.util.Arrays; -import java.util.Collection; -import java.util.Iterator; -import java.util.zip.ZipEntry; -import java.util.zip.ZipFile; - -import static org.hamcrest.CoreMatchers.containsString; -import static org.hamcrest.CoreMatchers.is; -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.core.IsNull.nullValue; -import static org.junit.Assert.assertThat; - -/** - * A test suite for the {@link GTFS} Class. - */ -public class GTFSTestMTC { - private final ByteArrayOutputStream outContent = new ByteArrayOutputStream(); - private static final String JDBC_URL = "jdbc:postgresql://localhost"; - private static final Logger LOG = LoggerFactory.getLogger(GTFSTestMTC.class); - - // setup a stream to capture the output from the program - @Before - public void setUpStreams() { - System.setOut(new PrintStream(outContent)); - } - - /** - * Tests that a GTFS feed with long field values generates corresponding validation errors. - */ - @Test - public void canLoadFeedWithLongFieldValues () { - PersistenceExpectation[] expectations = PersistenceExpectation.list(); - ErrorExpectation[] errorExpectations = ErrorExpectation.list( - new ErrorExpectation(NewGTFSErrorType.MISSING_FIELD), - new ErrorExpectation(NewGTFSErrorType.FIELD_VALUE_TOO_LONG), - new ErrorExpectation(NewGTFSErrorType.ROUTE_LONG_NAME_CONTAINS_SHORT_NAME), - new ErrorExpectation(NewGTFSErrorType.FEED_TRAVEL_TIMES_ROUNDED), - new ErrorExpectation(NewGTFSErrorType.STOP_UNUSED, equalTo("1234567")), - new ErrorExpectation(NewGTFSErrorType.DATE_NO_SERVICE) - ); - assertThat( - "Long-field-value test passes", - runIntegrationTestOnFolder("fake-agency-mtc-long-fields", 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. - * TODO: Refactor-consolidate with the same method in GTFSTest.java. - */ - private boolean runIntegrationTestOnFolder( - String folderName, - Matcher fatalExceptionExpectation, - PersistenceExpectation[] persistenceExpectations, - ErrorExpectation[] errorExpectations - ) { - LOG.info("Running integration test on folder {}", folderName); - // zip up test folder into temp zip file - String zipFileName = null; - try { - zipFileName = TestUtils.zipFolderFiles(folderName, true); - } catch (IOException e) { - e.printStackTrace(); - return false; - } - return runIntegrationTestOnZipFile(zipFileName, fatalExceptionExpectation, persistenceExpectations, errorExpectations); - } - - /** - * A helper method that will run GTFS#main with a certain zip file. - * This tests whether a GTFS zip file can be loaded without any errors. The full list of steps includes: - * 1. GTFS#load - * 2. GTFS#validate - * 3. exportGtfs/check exported GTFS integrity - * 4. makeSnapshot - * 5. Delete feed/namespace - * TODO: Refactor-consolidate with the same method in GTFSTest.java. - */ - private boolean runIntegrationTestOnZipFile( - String zipFileName, - Matcher fatalExceptionExpectation, - PersistenceExpectation[] persistenceExpectations, - ErrorExpectation[] errorExpectations - ) { - String testDBName = TestUtils.generateNewDB(); - String dbConnectionUrl = String.join("/", JDBC_URL, testDBName); - DataSource dataSource = GTFS.createDataSource( - dbConnectionUrl, - null, - null - ); - - String namespace; - - // Verify that loading the feed completes and data is stored properly - try (Connection connection = dataSource.getConnection()) { - // load and validate feed - LOG.info("load and validate GTFS file {}", zipFileName); - FeedLoadResult loadResult = GTFS.load(zipFileName, dataSource); - ValidationResult validationResult = GTFS.validate(loadResult.uniqueIdentifier, dataSource, MTCValidator.class); - - assertThat(validationResult.fatalException, is(fatalExceptionExpectation)); - namespace = loadResult.uniqueIdentifier; - assertThatImportedGtfsMeetsExpectations(connection, namespace, persistenceExpectations, errorExpectations); - } catch (SQLException e) { - TestUtils.dropDB(testDBName); - e.printStackTrace(); - return false; - } catch (AssertionError e) { - TestUtils.dropDB(testDBName); - throw e; - } - - // Verify that exporting the feed (in non-editor mode) completes and data is outputted properly - try { - LOG.info("export GTFS from created namespace"); - File tempFile = exportGtfs(namespace, dataSource, false); - assertThatExportedGtfsMeetsExpectations(tempFile, persistenceExpectations, false); - } catch (IOException e) { - TestUtils.dropDB(testDBName); - e.printStackTrace(); - return false; - } catch (AssertionError e) { - TestUtils.dropDB(testDBName); - throw e; - } - - // Verify that making a snapshot from an existing feed database, then exporting that snapshot to a GTFS zip file - // works as expected - try { - LOG.info("copy GTFS from created namespace"); - SnapshotResult copyResult = GTFS.makeSnapshot(namespace, dataSource); - assertThatSnapshotIsErrorFree(copyResult); - LOG.info("export GTFS from copied namespace"); - File tempFile = exportGtfs(copyResult.uniqueIdentifier, dataSource, true); - assertThatExportedGtfsMeetsExpectations(tempFile, persistenceExpectations, true); - } catch (IOException e) { - e.printStackTrace(); - TestUtils.dropDB(testDBName); - return false; - } catch (AssertionError e) { - TestUtils.dropDB(testDBName); - throw e; - } - - // Verify that deleting a feed works as expected. - try (Connection connection = dataSource.getConnection()) { - LOG.info("Deleting GTFS feed from database."); - GTFS.delete(namespace, dataSource); - - String sql = String.format("select * from feeds where namespace = '%s'", namespace); - LOG.info(sql); - ResultSet resultSet = connection.prepareStatement(sql).executeQuery(); - while (resultSet.next()) { - // Assert that the feed registry shows feed as deleted. - assertThat(resultSet.getBoolean("deleted"), is(true)); - } - // Ensure that schema no longer exists for namespace (note: this is Postgres specific). - String schemaSql = String.format( - "SELECT * FROM information_schema.schemata where schema_name = '%s'", - namespace - ); - LOG.info(schemaSql); - ResultSet schemaResultSet = connection.prepareStatement(schemaSql).executeQuery(); - int schemaCount = 0; - while (schemaResultSet.next()) schemaCount++; - // There should be no schema records matching the deleted namespace. - assertThat(schemaCount, is(0)); - } catch (SQLException | InvalidNamespaceException e) { - e.printStackTrace(); - TestUtils.dropDB(testDBName); - return false; - } catch (AssertionError e) { - TestUtils.dropDB(testDBName); - throw e; - } - - // This should be run following all of the above tests (any new tests should go above these lines). - TestUtils.dropDB(testDBName); - return true; - } - - private void assertThatLoadIsErrorFree(FeedLoadResult loadResult) { - assertThat(loadResult.fatalException, is(nullValue())); - assertThat(loadResult.agency.fatalException, is(nullValue())); - assertThat(loadResult.calendar.fatalException, is(nullValue())); - assertThat(loadResult.calendarDates.fatalException, is(nullValue())); - assertThat(loadResult.fareAttributes.fatalException, is(nullValue())); - assertThat(loadResult.fareRules.fatalException, is(nullValue())); - assertThat(loadResult.feedInfo.fatalException, is(nullValue())); - assertThat(loadResult.frequencies.fatalException, is(nullValue())); - assertThat(loadResult.routes.fatalException, is(nullValue())); - assertThat(loadResult.shapes.fatalException, is(nullValue())); - assertThat(loadResult.stops.fatalException, is(nullValue())); - assertThat(loadResult.stopTimes.fatalException, is(nullValue())); - assertThat(loadResult.transfers.fatalException, is(nullValue())); - assertThat(loadResult.trips.fatalException, is(nullValue())); - } - - private void assertThatSnapshotIsErrorFree(SnapshotResult snapshotResult) { - assertThatLoadIsErrorFree(snapshotResult); - assertThat(snapshotResult.scheduleExceptions.fatalException, is(nullValue())); - } - - /** - * Helper function to export a GTFS from the database to a temporary zip file. - */ - private File exportGtfs(String namespace, DataSource dataSource, boolean fromEditor) throws IOException { - File tempFile = File.createTempFile("snapshot", ".zip"); - GTFS.export(namespace, tempFile.getAbsolutePath(), dataSource, fromEditor); - return tempFile; - } - - private class ValuePair { - private final Object expected; - private final Object found; - private ValuePair (Object expected, Object found) { - this.expected = expected; - this.found = found; - } - } - - /** - * Run through the list of persistence expectations to make sure that the feed was imported properly into the - * database. - */ - private void assertThatImportedGtfsMeetsExpectations( - Connection connection, - String namespace, - PersistenceExpectation[] persistenceExpectations, - ErrorExpectation[] errorExpectations - ) throws SQLException { - // Store field mismatches here (to provide assertion statements with more details). - Multimap fieldsWithMismatches = ArrayListMultimap.create(); - // Check that no validators failed during validation. - assertThat( - "One or more validators failed during GTFS import.", - countValidationErrorsOfType(connection, namespace, NewGTFSErrorType.VALIDATOR_FAILED), - equalTo(0) - ); - // run through testing expectations - LOG.info("testing expectations of record storage in the database"); - for (PersistenceExpectation persistenceExpectation : persistenceExpectations) { - // select all entries from a table - String sql = String.format( - "select * from %s.%s", - namespace, - persistenceExpectation.tableName - ); - LOG.info(sql); - ResultSet rs = connection.prepareStatement(sql).executeQuery(); - boolean foundRecord = false; - int numRecordsSearched = 0; - while (rs.next()) { - numRecordsSearched++; - LOG.info("record {} in ResultSet", numRecordsSearched); - boolean allFieldsMatch = true; - for (RecordExpectation recordExpectation: persistenceExpectation.recordExpectations) { - switch (recordExpectation.expectedFieldType) { - case DOUBLE: - double doubleVal = rs.getDouble(recordExpectation.fieldName); - LOG.info("{}: {}", recordExpectation.fieldName, doubleVal); - if (doubleVal != recordExpectation.doubleExpectation) { - allFieldsMatch = false; - } - break; - case INT: - int intVal = rs.getInt(recordExpectation.fieldName); - LOG.info("{}: {}", recordExpectation.fieldName, intVal); - if (intVal != recordExpectation.intExpectation) { - fieldsWithMismatches.put( - recordExpectation.fieldName, - new ValuePair(recordExpectation.stringExpectation, intVal) - ); - allFieldsMatch = false; - } - break; - case STRING: - String strVal = rs.getString(recordExpectation.fieldName); - LOG.info("{}: {}", recordExpectation.fieldName, strVal); - if (strVal == null && recordExpectation.stringExpectation == null) { - break; - } else if (strVal == null || !strVal.equals(recordExpectation.stringExpectation)) { - fieldsWithMismatches.put( - recordExpectation.fieldName, - new ValuePair(recordExpectation.stringExpectation, strVal) - ); - LOG.error("Expected {}, found {}", recordExpectation.stringExpectation, strVal); - allFieldsMatch = false; - } - break; - - } - if (!allFieldsMatch) { - break; - } - } - // all fields match expectations! We have found the record. - if (allFieldsMatch) { - LOG.info("Database record satisfies expectations."); - foundRecord = true; - break; - } else { - LOG.error("Persistence mismatch on record {}", numRecordsSearched); - } - } - assertThatPersistenceExpectationRecordWasFound(numRecordsSearched, foundRecord, fieldsWithMismatches); - } - // Expect zero errors if errorExpectations is null. - if (errorExpectations == null) errorExpectations = new ErrorExpectation[]{}; - // Check that error expectations match errors stored in database. - LOG.info("Checking {} error expectations", errorExpectations.length); - // select all entries from error table - String sql = String.format("select * from %s.errors", namespace); - LOG.info(sql); - ResultSet rs = connection.prepareStatement(sql).executeQuery(); - int errorCount = 0; - Iterator errorExpectationIterator = Arrays.stream(errorExpectations).iterator(); - while (rs.next()) { - errorCount++; - String errorType = rs.getString("error_type"); - String entityType = rs.getString("entity_type"); - String entityId = rs.getString("entity_id"); - String badValue = rs.getString("bad_value"); - LOG.info("Found error {}: {} {} {} {}", errorCount, errorType, entityId, entityType, badValue); - // Skip error expectation if not exists. But continue iteration to count all errors. - if (!errorExpectationIterator.hasNext()) continue; - ErrorExpectation errorExpectation = errorExpectationIterator.next(); - LOG.info("Expecting error {}: {}", errorCount, errorExpectation.errorTypeMatcher); - // Error expectation must contain error type matcher. The others are optional. - assertThat(errorType, errorExpectation.errorTypeMatcher); - if (errorExpectation.entityTypeMatcher != null) assertThat(entityType, errorExpectation.entityTypeMatcher); - if (errorExpectation.entityIdMatcher != null) assertThat(entityId, errorExpectation.entityIdMatcher); - if (errorExpectation.badValueMatcher != null) assertThat(badValue, errorExpectation.badValueMatcher); - } - assertThat( - "Error count is equal to number of error expectations.", - errorCount, - equalTo(errorExpectations.length)); - } - - private static int countValidationErrorsOfType( - Connection connection, - String namespace, - NewGTFSErrorType errorType - ) throws SQLException { - String errorCheckSql = String.format( - "select * from %s.errors where error_type = '%s'", - namespace, - errorType); - LOG.info(errorCheckSql); - ResultSet errorResults = connection.prepareStatement(errorCheckSql).executeQuery(); - int errorCount = 0; - while (errorResults.next()) { - errorCount++; - } - return errorCount; - } - - /** - * Helper to assert that the GTFS that was exported to a zip file matches all data expectations defined in the - * persistence expectations. - */ - private void assertThatExportedGtfsMeetsExpectations( - File tempFile, - PersistenceExpectation[] persistenceExpectations, - boolean fromEditor - ) throws IOException { - LOG.info("testing expectations of csv outputs in an exported gtfs"); - - ZipFile gtfsZipfile = new ZipFile(tempFile.getAbsolutePath()); - - // iterate through all expectations - for (PersistenceExpectation persistenceExpectation : persistenceExpectations) { - // No need to check that errors were exported because it is an internal table only. - if ("errors".equals(persistenceExpectation.tableName)) continue; - final String tableFileName = persistenceExpectation.tableName + ".txt"; - LOG.info(String.format("reading table: %s", tableFileName)); - - ZipEntry entry = gtfsZipfile.getEntry(tableFileName); - - // ensure file exists in zip - if (entry == null) { - throw new AssertionError( - String.format("expected table %s not found in outputted zip file", tableFileName) - ); - } - - // prepare to read the file - InputStream zipInputStream = gtfsZipfile.getInputStream(entry); - // Skip any byte order mark that may be present. Files must be UTF-8, - // but the GTFS spec says that "files that include the UTF byte order mark are acceptable". - InputStream bomInputStream = new BOMInputStream(zipInputStream); - CsvReader csvReader = new CsvReader(bomInputStream, ',', Charset.forName("UTF8")); - csvReader.readHeaders(); - - boolean foundRecord = false; - int numRecordsSearched = 0; - - // read each record - while (csvReader.readRecord() && !foundRecord) { - numRecordsSearched++; - LOG.info(String.format("record %d in csv file", numRecordsSearched)); - boolean allFieldsMatch = true; - - // iterate through all rows in record to determine if it's the one we're looking for - for (RecordExpectation recordExpectation: persistenceExpectation.recordExpectations) { - String val = csvReader.get(recordExpectation.fieldName); - String expectation = recordExpectation.getStringifiedExpectation(fromEditor); - LOG.info(String.format( - "%s: %s (Expectation: %s)", - recordExpectation.fieldName, - val, - expectation - )); - if (val.isEmpty() && expectation == null) { - // First check that the csv value is an empty string and that the expectation is null. Null - // exported from the database to a csv should round trip into an empty string, so this meets the - // expectation. - break; - } else if (!val.equals(expectation)) { - // sometimes there are slight differences in decimal precision in various fields - // check if the decimal delta is acceptable - if (equalsWithNumericDelta(val, recordExpectation)) continue; - allFieldsMatch = false; - break; - } - } - // all fields match expectations! We have found the record. - if (allFieldsMatch) { - LOG.info("CSV record satisfies expectations."); - foundRecord = true; - } - } - assertThatPersistenceExpectationRecordWasFound(numRecordsSearched, foundRecord, null); - } - } - - /** - * Check whether a potentially numeric value is equal given potentially small decimal deltas - */ - private boolean equalsWithNumericDelta(String val, RecordExpectation recordExpectation) { - if (recordExpectation.expectedFieldType != ExpectedFieldType.DOUBLE) return false; - double d; - try { - d = Double.parseDouble(val); - } - catch(NumberFormatException nfe) - { - return false; - } - return Math.abs(d - recordExpectation.doubleExpectation) < recordExpectation.acceptedDelta; - } - - /** - * Helper method to make sure a persistence expectation was actually found after searching through records - */ - private void assertThatPersistenceExpectationRecordWasFound( - int numRecordsSearched, - boolean foundRecord, - Multimap mismatches - ) { - assertThat( - "No records found in the ResultSet/CSV file", - numRecordsSearched, - ComparatorMatcherBuilder.usingNaturalOrdering().greaterThan(0) - ); - if (mismatches != null) { - for (String field : mismatches.keySet()) { - Collection valuePairs = mismatches.get(field); - for (ValuePair valuePair : valuePairs) { - assertThat( - String.format("The value expected for %s was not found", field), - valuePair.found, - equalTo(valuePair.expected) - ); - } - } - } else { - assertThat( - "The record as defined in the PersistenceExpectation was not found.", - foundRecord, - equalTo(true) - ); - } - } - - /** - * Persistence expectations for use with the GTFS contained within the "fake-agency" resources folder. - */ - private PersistenceExpectation[] fakeAgencyPersistenceExpectations = new PersistenceExpectation[]{ - new PersistenceExpectation( - "agency", - new RecordExpectation[]{ - new RecordExpectation("agency_id", "1"), - new RecordExpectation("agency_name", "Fake Transit"), - new RecordExpectation("agency_timezone", "America/Los_Angeles") - } - ), - new PersistenceExpectation( - "calendar", - new RecordExpectation[]{ - new RecordExpectation( - "service_id", "04100312-8fe1-46a5-a9f2-556f39478f57" - ), - new RecordExpectation("monday", 1), - new RecordExpectation("tuesday", 1), - new RecordExpectation("wednesday", 1), - new RecordExpectation("thursday", 1), - new RecordExpectation("friday", 1), - new RecordExpectation("saturday", 1), - new RecordExpectation("sunday", 1), - new RecordExpectation("start_date", "20170915"), - new RecordExpectation("end_date", "20170917") - } - ), - new PersistenceExpectation( - "calendar_dates", - new RecordExpectation[]{ - new RecordExpectation( - "service_id", "04100312-8fe1-46a5-a9f2-556f39478f57" - ), - new RecordExpectation("date", 20170916), - new RecordExpectation("exception_type", 2) - } - ), - new PersistenceExpectation( - "fare_attributes", - new RecordExpectation[]{ - new RecordExpectation("fare_id", "route_based_fare"), - new RecordExpectation("price", 1.23, 0), - new RecordExpectation("currency_type", "USD") - } - ), - new PersistenceExpectation( - "fare_rules", - new RecordExpectation[]{ - new RecordExpectation("fare_id", "route_based_fare"), - new RecordExpectation("route_id", "1") - } - ), - new PersistenceExpectation( - "feed_info", - new RecordExpectation[]{ - new RecordExpectation("feed_id", "fake_transit"), - new RecordExpectation("feed_publisher_name", "Conveyal"), - new RecordExpectation( - "feed_publisher_url", "http://www.conveyal.com" - ), - new RecordExpectation("feed_lang", "en"), - new RecordExpectation("feed_version", "1.0") - } - ), - new PersistenceExpectation( - "frequencies", - new RecordExpectation[]{ - new RecordExpectation("trip_id", "frequency-trip"), - new RecordExpectation("start_time", 28800, "08:00:00"), - new RecordExpectation("end_time", 32400, "09:00:00"), - new RecordExpectation("headway_secs", 1800), - new RecordExpectation("exact_times", 0) - } - ), - new PersistenceExpectation( - "routes", - new RecordExpectation[]{ - new RecordExpectation("agency_id", "1"), - new RecordExpectation("route_id", "1"), - new RecordExpectation("route_short_name", "1"), - new RecordExpectation("route_long_name", "Route 1"), - new RecordExpectation("route_type", 3), - new RecordExpectation("route_color", "7CE6E7") - } - ), - new PersistenceExpectation( - "shapes", - new RecordExpectation[]{ - new RecordExpectation( - "shape_id", "5820f377-f947-4728-ac29-ac0102cbc34e" - ), - new RecordExpectation("shape_pt_lat", 37.061172, 0.00001), - new RecordExpectation("shape_pt_lon", -122.007500, 0.00001), - new RecordExpectation("shape_pt_sequence", 2), - new RecordExpectation("shape_dist_traveled", 7.4997067, 0.01) - } - ), - new PersistenceExpectation( - "stop_times", - new RecordExpectation[]{ - new RecordExpectation( - "trip_id", "a30277f8-e50a-4a85-9141-b1e0da9d429d" - ), - new RecordExpectation("arrival_time", 25200, "07:00:00"), - new RecordExpectation("departure_time", 25200, "07:00:00"), - new RecordExpectation("stop_id", "4u6g"), - // the string expectation for stop_sequence is different because of how stop_times are - // converted to 0-based indexes in Table.normalizeAndCloneStopTimes - new RecordExpectation("stop_sequence", 1, "1", "0"), - new RecordExpectation("pickup_type", 0), - new RecordExpectation("drop_off_type", 0), - new RecordExpectation("shape_dist_traveled", 0.0, 0.01) - } - ), - new PersistenceExpectation( - "trips", - new RecordExpectation[]{ - new RecordExpectation( - "trip_id", "a30277f8-e50a-4a85-9141-b1e0da9d429d" - ), - new RecordExpectation( - "service_id", "04100312-8fe1-46a5-a9f2-556f39478f57" - ), - new RecordExpectation("route_id", "1"), - new RecordExpectation("direction_id", 0), - new RecordExpectation( - "shape_id", "5820f377-f947-4728-ac29-ac0102cbc34e" - ), - new RecordExpectation("bikes_allowed", 0), - new RecordExpectation("wheelchair_accessible", 0) - } - ) - }; -} diff --git a/src/test/java/com/conveyal/gtfs/validator/FieldLengthValidatorTest.java b/src/test/java/com/conveyal/gtfs/validator/FieldLengthValidatorTest.java deleted file mode 100644 index 27a47cc95..000000000 --- a/src/test/java/com/conveyal/gtfs/validator/FieldLengthValidatorTest.java +++ /dev/null @@ -1,32 +0,0 @@ -package com.conveyal.gtfs.validator; - -import com.conveyal.gtfs.GTFS; -import com.conveyal.gtfs.GTFSFeed; -import com.conveyal.gtfs.TestUtils; -import com.conveyal.gtfs.error.SQLErrorStorage; -import org.junit.BeforeClass; -import org.junit.Test; - -import java.io.IOException; - -import static org.hamcrest.CoreMatchers.is; -import static org.hamcrest.MatcherAssert.assertThat; - -public class FieldLengthValidatorTest { - @Test - public void validateFieldlLength() { -/* - String[] args = {"-u", "blah"}; - GTFS.main(args); - - SQLErrorStorage errorStorage = new SQLErrorStorage(); - FieldLengthValidator validator = new FieldLengthValidator(null, null); - final String longFieldValue = "abcdefghijklmnopqrstwxyz1234567890"; - final String shortFieldValue = "abcdef"; - final int length = 20; - - assertThat(validator.validateFieldLength(longFieldValue, length), is(true)); - assertThat(validator.validateFieldLength(shortFieldValue, length), is(false)); - */ - } -} diff --git a/src/test/java/com/conveyal/gtfs/validator/MTCValidatorTest.java b/src/test/java/com/conveyal/gtfs/validator/MTCValidatorTest.java new file mode 100644 index 000000000..ff2f18cde --- /dev/null +++ b/src/test/java/com/conveyal/gtfs/validator/MTCValidatorTest.java @@ -0,0 +1,28 @@ +package com.conveyal.gtfs.validator; + +import org.junit.Test; + +import java.net.MalformedURLException; +import java.net.URL; + +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.MatcherAssert.assertThat; + +public class MTCValidatorTest { + @Test + public void fieldlLengthShouldNotExceed() { + MTCValidator validator = new MTCValidator(null, null); + assertThat(validator.fieldLengthShouldNotExceed(null, "abcdefghijklmnopqrstwxyz1234567890", 20), is(false)); + assertThat(validator.fieldLengthShouldNotExceed(null, "abcdef", 20), is(true)); + } + + @Test + public void fieldlLengthShouldNotExceed_usingObject() throws MalformedURLException { + MTCValidator validator = new MTCValidator(null, null); + + // You can also pass objects, in that case it will use toString(). + URL url = new URL("http://www.gtfs.org"); + assertThat(validator.fieldLengthShouldNotExceed(null, url, 10), is(false)); + assertThat(validator.fieldLengthShouldNotExceed(null, url, 30), is(true)); + } +} diff --git a/src/test/resources/fake-agency-mtc-long-fields/agency.txt b/src/test/resources/fake-agency-mtc-long-fields/agency.txt index 1d3884acc..ecd839d0b 100755 --- a/src/test/resources/fake-agency-mtc-long-fields/agency.txt +++ b/src/test/resources/fake-agency-mtc-long-fields/agency.txt @@ -1,5 +1,5 @@ agency_id,agency_name,agency_url,agency_lang,agency_phone,agency_email,agency_timezone,agency_fare_url,agency_branding_url -1,Fake Transit,,,,,America/Los_Angeles,, -2,Agency name with more than fifty 50 characters that does not meet MTC guidelines,,,,,America/Los_Angeles,, -Agency_id_with_more_than_50_characters_that_does_not_meet_MTC_guidelines,Fake Transit,,,,,America/Los_Angeles,, +1,Fake Transit,http://www.example.com,,,,America/Los_Angeles,, +2,Agency name with more than fifty 50 characters that does not meet MTC guidelines,http://www.example.com,,,,America/Los_Angeles,, +Agency_id_with_more_than_50_characters_that_does_not_meet_MTC_guidelines,Fake Transit,http://www.example.com,,,,America/Los_Angeles,, 4,Fake Transit,http://www.agency.com/long/url/____________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________,,,,America/Los_Angeles,, diff --git a/src/test/resources/fake-agency-mtc-long-fields/calendar_dates.txt b/src/test/resources/fake-agency-mtc-long-fields/calendar_dates.txt index 403ee2bbe..74c1ef632 100755 --- a/src/test/resources/fake-agency-mtc-long-fields/calendar_dates.txt +++ b/src/test/resources/fake-agency-mtc-long-fields/calendar_dates.txt @@ -1,2 +1 @@ service_id,date,exception_type -04100312-8fe1-46a5-a9f2-556f39478f57,20170916,2 \ No newline at end of file diff --git a/src/test/resources/fake-agency-mtc-long-fields/routes.txt b/src/test/resources/fake-agency-mtc-long-fields/routes.txt index 35ea7aa67..10f39224c 100755 --- a/src/test/resources/fake-agency-mtc-long-fields/routes.txt +++ b/src/test/resources/fake-agency-mtc-long-fields/routes.txt @@ -1,2 +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,1,Route 1,,3,,7CE6E7,FFFFFF, +1,1,101A,Example Route,,3,,7CE6E7,FFFFFF, diff --git a/src/test/resources/fake-agency-mtc-long-fields/stop_times.txt b/src/test/resources/fake-agency-mtc-long-fields/stop_times.txt index 22534abf2..33ab446ac 100755 --- a/src/test/resources/fake-agency-mtc-long-fields/stop_times.txt +++ b/src/test/resources/fake-agency-mtc-long-fields/stop_times.txt @@ -3,3 +3,5 @@ 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, frequency-trip,08:00:00,08:00:00,4u6g,1,,0,0,0.0000000, frequency-trip,08:29:00,08:29:00,1234,2,,0,0,341.4491961, +trip-with-long-shortname,08:00:00,08:00:00,4u6g,1,,0,0,0.0000000, +trip-with-long-shortname,08:29:00,08:29:00,1234,2,,0,0,341.4491961, diff --git a/src/test/resources/fake-agency-mtc-long-fields/stops.txt b/src/test/resources/fake-agency-mtc-long-fields/stops.txt index 50d746d2b..9ac372a00 100755 --- a/src/test/resources/fake-agency-mtc-long-fields/stops.txt +++ b/src/test/resources/fake-agency-mtc-long-fields/stops.txt @@ -3,4 +3,3 @@ stop_id,stop_code,stop_name,stop_desc,stop_lat,stop_lon,zone_id,stop_url,locatio johv,,Scotts Valley Dr & Victor Sq,,37.0590172,-122.0096058,,,0,,, 123,,Parent Station,,37.0666,-122.0777,,,1,,, 1234,,Long Stop Name of more than 100 characters that exceeds MTC guidelines regarding the length of this field,,37.06662,-122.07772,,,0,123,, -1234567,,Unused stop,,37.06668,-122.07781,,,0,123,, \ No newline at end of file diff --git a/src/test/resources/fake-agency-mtc-long-fields/trips.txt b/src/test/resources/fake-agency-mtc-long-fields/trips.txt index d751e202c..9b6feaa36 100755 --- a/src/test/resources/fake-agency-mtc-long-fields/trips.txt +++ b/src/test/resources/fake-agency-mtc-long-fields/trips.txt @@ -1,4 +1,4 @@ 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 -1,frequency-trip,This is a trip headsign of more than 120 characters that exceeds the MTC guidelines on the number of characters,,0,,5820f377-f947-4728-ac29-ac0102cbc34e,0,0,04100312-8fe1-46a5-a9f2-556f39478f57 +1,frequency-trip,This is a trip headsign of more than 120 characters that exceeds the MTC guidelines on the number of characters for the trip headsign,trip-short-name,0,,5820f377-f947-4728-ac29-ac0102cbc34e,0,0,04100312-8fe1-46a5-a9f2-556f39478f57 1,trip-with-long-shortname,trip headsign,This is a trip short name of more than 50 characters beyond MTC guidelines,0,,5820f377-f947-4728-ac29-ac0102cbc34e,0,0,04100312-8fe1-46a5-a9f2-556f39478f57 \ No newline at end of file From 3f4251597538df3c8078f4527ee8f40e738e868a Mon Sep 17 00:00:00 2001 From: binh-dam-ibigroup <56846598+binh-dam-ibigroup@users.noreply.github.com> Date: Thu, 2 Apr 2020 12:19:32 -0400 Subject: [PATCH 04/17] docs: Refine comments and error messages. --- .../conveyal/gtfs/error/NewGTFSErrorType.java | 2 +- .../conveyal/gtfs/validator/MTCValidator.java | 19 +++++++++++++------ 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java b/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java index ccbee56d9..2fd516d4d 100644 --- a/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java +++ b/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java @@ -11,7 +11,7 @@ public enum NewGTFSErrorType { URL_FORMAT(Priority.MEDIUM, "URL format should be ://?#"), LANGUAGE_FORMAT(Priority.LOW, "Language should be specified with a valid BCP47 tag."), ILLEGAL_FIELD_VALUE(Priority.MEDIUM, "Fields may not contain tabs, carriage returns or new lines."), - FIELD_VALUE_TOO_LONG(Priority.MEDIUM, "Field values have too many characters."), + FIELD_VALUE_TOO_LONG(Priority.MEDIUM, "Field value has too many characters."), INTEGER_FORMAT(Priority.MEDIUM, "Incorrect integer format."), FARE_TRANSFER_MISMATCH(Priority.MEDIUM, "A fare that does not permit transfers has a non-zero transfer duration."), FREQUENCY_PERIOD_OVERLAP(Priority.MEDIUM, "A frequency for a trip overlaps with another frequency defined for the same trip."), diff --git a/src/main/java/com/conveyal/gtfs/validator/MTCValidator.java b/src/main/java/com/conveyal/gtfs/validator/MTCValidator.java index ab1815348..710ed671d 100644 --- a/src/main/java/com/conveyal/gtfs/validator/MTCValidator.java +++ b/src/main/java/com/conveyal/gtfs/validator/MTCValidator.java @@ -7,10 +7,9 @@ import static com.conveyal.gtfs.error.NewGTFSErrorType.FIELD_VALUE_TOO_LONG; /** - * MTCValidator validates a GTFS feed according to the following - * additional guidelines by 511 MTC: - * - Field values should not exceed some number of characters. - * Those requirements can be found by searching for the word 'character'. + * MTCValidator checks in a GTFS feed that the length of certain field values + * do not exceed the 511 MTC guidelines. (TODO: add guidelines URL.) + * To refer to specific limits, search the guidelines for the word 'character'. */ public class MTCValidator extends FeedValidator { @@ -38,10 +37,18 @@ public void validate() { // TODO: Handle calendar_attributes.txt? } - boolean fieldLengthShouldNotExceed(Entity entity, Object objValue, int maxLength) { + /** + * Checks that the length of a string (or Object.toString()) does not exceed a length. + * Reports an error if the length is exceeded. + * @param entity The containing GTFS entity (for error reporting purposes). + * @param objValue The value to check. + * @param maxLength The length to check, should be positive or zero. + * @return true if the length of objValue.toString() is maxLength or less or if objValue is null; false otherwise. + */ + public boolean fieldLengthShouldNotExceed(Entity entity, Object objValue, int maxLength) { String value = objValue != null ? objValue.toString() : ""; if (value.length() > maxLength) { - if (errorStorage != null) registerError(entity, FIELD_VALUE_TOO_LONG, value); + if (errorStorage != null) registerError(entity, FIELD_VALUE_TOO_LONG, "[over " + maxLength + " characters] " + value); return false; } return true; From 4058c9b80bac3e3f474f6a802df191ef9c89c817 Mon Sep 17 00:00:00 2001 From: binh-dam-ibigroup <56846598+binh-dam-ibigroup@users.noreply.github.com> Date: Thu, 2 Apr 2020 13:30:30 -0400 Subject: [PATCH 05/17] refactor(FieldLengthError): Remove unused class. --- .../conveyal/gtfs/error/FieldLengthError.java | 17 ----------------- 1 file changed, 17 deletions(-) delete mode 100644 src/main/java/com/conveyal/gtfs/error/FieldLengthError.java diff --git a/src/main/java/com/conveyal/gtfs/error/FieldLengthError.java b/src/main/java/com/conveyal/gtfs/error/FieldLengthError.java deleted file mode 100644 index b9fece918..000000000 --- a/src/main/java/com/conveyal/gtfs/error/FieldLengthError.java +++ /dev/null @@ -1,17 +0,0 @@ -package com.conveyal.gtfs.error; - -import java.io.Serializable; - -/** Indicates that the length of a field is invalid. */ -public class FieldLengthError extends GTFSError implements Serializable { - public static final long serialVersionUID = 1L; - - public FieldLengthError(String file, long line, String field) { - super(file, line, field); - } - - @Override public String getMessage() { - return String.format("Field length is invalid."); - } - -} From d423983ccfc3931bab2a8e1e625d603377e23442 Mon Sep 17 00:00:00 2001 From: binh-dam-ibigroup <56846598+binh-dam-ibigroup@users.noreply.github.com> Date: Thu, 2 Apr 2020 15:51:29 -0400 Subject: [PATCH 06/17] refactor: Replace calling reflection on FeedValidator constructors with callback approach. --- src/main/java/com/conveyal/gtfs/GTFS.java | 15 ++-- .../java/com/conveyal/gtfs/loader/Feed.java | 29 ++++---- .../validator/CustomValidatorRequest.java | 20 ++++++ src/test/java/com/conveyal/gtfs/GTFSTest.java | 71 +++++++++++++++---- 4 files changed, 98 insertions(+), 37 deletions(-) create mode 100644 src/main/java/com/conveyal/gtfs/validator/CustomValidatorRequest.java diff --git a/src/main/java/com/conveyal/gtfs/GTFS.java b/src/main/java/com/conveyal/gtfs/GTFS.java index 78ad63f00..a1d28f92b 100644 --- a/src/main/java/com/conveyal/gtfs/GTFS.java +++ b/src/main/java/com/conveyal/gtfs/GTFS.java @@ -7,7 +7,7 @@ import com.conveyal.gtfs.loader.JdbcGtfsSnapshotter; import com.conveyal.gtfs.loader.SnapshotResult; import com.conveyal.gtfs.util.InvalidNamespaceException; -import com.conveyal.gtfs.validator.FeedValidator; +import com.conveyal.gtfs.validator.CustomValidatorRequest; import com.conveyal.gtfs.validator.ValidationResult; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.io.Files; @@ -29,8 +29,6 @@ import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.SQLException; -import java.util.ArrayList; -import java.util.List; import static com.conveyal.gtfs.util.Util.ensureValidNamespace; @@ -94,15 +92,18 @@ public static SnapshotResult makeSnapshot (String feedId, DataSource dataSource) } /** - * Once a feed has been loaded into the database, examine its contents looking for various problems and errors. + * Shorthand for calling validate() without custom validators. */ public static ValidationResult validate (String feedId, DataSource dataSource) { - return validate(feedId, dataSource, new ArrayList<>()); + return validate(feedId, dataSource, null); } - public static ValidationResult validate (String feedId, DataSource dataSource, List> additionalValidatorClasses) { + /** + * Once a feed has been loaded into the database, examine its contents looking for various problems and errors. + */ + public static ValidationResult validate (String feedId, DataSource dataSource, CustomValidatorRequest customValidatorReq) { Feed feed = new Feed(dataSource, feedId); - ValidationResult result = feed.validate(additionalValidatorClasses); + ValidationResult result = feed.validate(customValidatorReq); return result; } diff --git a/src/main/java/com/conveyal/gtfs/loader/Feed.java b/src/main/java/com/conveyal/gtfs/loader/Feed.java index 65dde2518..2699f2025 100644 --- a/src/main/java/com/conveyal/gtfs/loader/Feed.java +++ b/src/main/java/com/conveyal/gtfs/loader/Feed.java @@ -12,14 +12,10 @@ import org.slf4j.LoggerFactory; import javax.sql.DataSource; -import java.lang.reflect.Constructor; -import java.lang.reflect.InvocationTargetException; import java.sql.Connection; import java.sql.SQLException; import java.util.ArrayList; -import java.util.Arrays; import java.util.List; -import java.util.stream.Collectors; import static com.conveyal.gtfs.error.NewGTFSErrorType.VALIDATOR_FAILED; @@ -73,12 +69,19 @@ public Feed (DataSource dataSource, String tablePrefix) { stopTimes = new JDBCTableReader(Table.STOP_TIMES, dataSource, tablePrefix, EntityPopulator.STOP_TIME); } + /** + * Shorthand for calling validate() without custom validators. + */ + public ValidationResult validate () { + return validate(null); + } + /** * TODO check whether validation has already occurred, overwrite results. * TODO allow validation within feed loading process, so the same connection can be used, and we're certain loaded data is 100% visible. * That would also avoid having to reconnect the error storage to the DB. */ - public ValidationResult validate (List> additionalValidatorClasses) { + public ValidationResult validate (CustomValidatorRequest customValidatorReq) { long validationStartTime = System.currentTimeMillis(); // Create an empty validation result that will have its fields populated by certain validators. ValidationResult validationResult = new ValidationResult(); @@ -92,7 +95,7 @@ public ValidationResult validate (List> additiona } int errorCountBeforeValidation = errorStorage.getErrorCount(); - ArrayList feedValidators = Lists.newArrayList( + List feedValidators = Lists.newArrayList( new MisplacedStopValidator(this, errorStorage, validationResult), new DuplicateStopsValidator(this, errorStorage), new FaresValidator(this, errorStorage), @@ -102,16 +105,10 @@ public ValidationResult validate (List> additiona new NamesValidator(this, errorStorage) ); - // Instantiate any additional validators and - // add them to the list of validators. - for (Class validatorClass : additionalValidatorClasses) { - try { - Constructor constructor = validatorClass.getConstructor(Feed.class, SQLErrorStorage.class); - feedValidators.add(constructor.newInstance(this, errorStorage)); - } catch (NoSuchMethodException | InstantiationException | IllegalAccessException | InvocationTargetException e) { - e.printStackTrace(); - } - } + // Add additional validators, if any provided. + List customValidators = null; + if (customValidatorReq != null) customValidators = customValidatorReq.getCustomValidators(this, errorStorage); + if (customValidators != null) feedValidators.addAll(customValidators); for (FeedValidator feedValidator : feedValidators) { String validatorName = feedValidator.getClass().getSimpleName(); diff --git a/src/main/java/com/conveyal/gtfs/validator/CustomValidatorRequest.java b/src/main/java/com/conveyal/gtfs/validator/CustomValidatorRequest.java new file mode 100644 index 000000000..4237a8cec --- /dev/null +++ b/src/main/java/com/conveyal/gtfs/validator/CustomValidatorRequest.java @@ -0,0 +1,20 @@ +package com.conveyal.gtfs.validator; + +import com.conveyal.gtfs.error.SQLErrorStorage; +import com.conveyal.gtfs.loader.Feed; + +import java.util.List; + +/** + * An interface with a callback to instantiate additional custom GTFS Feed validators + * using a Feed and SQLErrorStorage objects. + */ +public interface CustomValidatorRequest { + /** + * The callback that instantiates and returns instances of custom FeedValidator objects + * constructed using the provided feed and error storage objects. + * @param feed The feed being validated. + * @param errorStorage The object that handles error storage. + */ + public List getCustomValidators(Feed feed, SQLErrorStorage errorStorage); +} diff --git a/src/test/java/com/conveyal/gtfs/GTFSTest.java b/src/test/java/com/conveyal/gtfs/GTFSTest.java index 095e19885..86f3f0fdb 100644 --- a/src/test/java/com/conveyal/gtfs/GTFSTest.java +++ b/src/test/java/com/conveyal/gtfs/GTFSTest.java @@ -9,11 +9,13 @@ import com.conveyal.gtfs.storage.PersistenceExpectation; import com.conveyal.gtfs.storage.RecordExpectation; import com.conveyal.gtfs.util.InvalidNamespaceException; +import com.conveyal.gtfs.validator.CustomValidatorRequest; import com.conveyal.gtfs.validator.FeedValidator; import com.conveyal.gtfs.validator.MTCValidator; import com.conveyal.gtfs.validator.ValidationResult; import com.csvreader.CsvReader; import com.google.common.collect.ArrayListMultimap; +import com.google.common.collect.Lists; import com.google.common.collect.Multimap; import com.google.common.io.Files; import org.apache.commons.io.FileUtils; @@ -316,18 +318,39 @@ public void canLoadFeedWithLongFieldValues () { PersistenceExpectation[] expectations = PersistenceExpectation.list(); ErrorExpectation[] errorExpectations = ErrorExpectation.list( - new ErrorExpectation(NewGTFSErrorType.FIELD_VALUE_TOO_LONG), - new ErrorExpectation(NewGTFSErrorType.FIELD_VALUE_TOO_LONG), - new ErrorExpectation(NewGTFSErrorType.FIELD_VALUE_TOO_LONG), - new ErrorExpectation(NewGTFSErrorType.FIELD_VALUE_TOO_LONG), - new ErrorExpectation(NewGTFSErrorType.FIELD_VALUE_TOO_LONG), - new ErrorExpectation(NewGTFSErrorType.FIELD_VALUE_TOO_LONG), - new ErrorExpectation(NewGTFSErrorType.FEED_TRAVEL_TIMES_ROUNDED) // Not related, not worrying about this one. + new ErrorExpectation(NewGTFSErrorType.FIELD_VALUE_TOO_LONG), + new ErrorExpectation(NewGTFSErrorType.FIELD_VALUE_TOO_LONG), + new ErrorExpectation(NewGTFSErrorType.FIELD_VALUE_TOO_LONG), + new ErrorExpectation(NewGTFSErrorType.FIELD_VALUE_TOO_LONG), + new ErrorExpectation(NewGTFSErrorType.FIELD_VALUE_TOO_LONG), + new ErrorExpectation(NewGTFSErrorType.FIELD_VALUE_TOO_LONG), + new ErrorExpectation(NewGTFSErrorType.FEED_TRAVEL_TIMES_ROUNDED) // Not related, not worrying about this one. ); assertThat( - "Long-field-value test passes", - runIntegrationTestOnFolder("fake-agency-mtc-long-fields", nullValue(), expectations, errorExpectations), - equalTo(true) + "Long-field-value test passes", + runIntegrationTestOnFolder("fake-agency-mtc-long-fields", nullValue(), expectations, errorExpectations, + (feed, errorSource) -> { + return Lists.newArrayList(new MTCValidator(feed, errorSource)); + }), + equalTo(true) + ); + } + + /** + * Shorthand for next method. + */ + private boolean runIntegrationTestOnFolder( + String folderName, + Matcher fatalExceptionExpectation, + PersistenceExpectation[] persistenceExpectations, + ErrorExpectation[] errorExpectations + ) { + return runIntegrationTestOnFolder( + folderName, + fatalExceptionExpectation, + persistenceExpectations, + errorExpectations, + null ); } @@ -339,7 +362,8 @@ private boolean runIntegrationTestOnFolder( String folderName, Matcher fatalExceptionExpectation, PersistenceExpectation[] persistenceExpectations, - ErrorExpectation[] errorExpectations + ErrorExpectation[] errorExpectations, + CustomValidatorRequest customValidatorReq ) { LOG.info("Running integration test on folder {}", folderName); // zip up test folder into temp zip file @@ -350,7 +374,25 @@ private boolean runIntegrationTestOnFolder( e.printStackTrace(); return false; } - return runIntegrationTestOnZipFile(zipFileName, fatalExceptionExpectation, persistenceExpectations, errorExpectations); + return runIntegrationTestOnZipFile(zipFileName, fatalExceptionExpectation, persistenceExpectations, errorExpectations, customValidatorReq); + } + + /** + * Shorthand for next method. + */ + private boolean runIntegrationTestOnZipFile( + String zipFileName, + Matcher fatalExceptionExpectation, + PersistenceExpectation[] persistenceExpectations, + ErrorExpectation[] errorExpectations + ) { + return runIntegrationTestOnZipFile( + zipFileName, + fatalExceptionExpectation, + persistenceExpectations, + errorExpectations, + null + ); } /** @@ -366,7 +408,8 @@ private boolean runIntegrationTestOnZipFile( String zipFileName, Matcher fatalExceptionExpectation, PersistenceExpectation[] persistenceExpectations, - ErrorExpectation[] errorExpectations + ErrorExpectation[] errorExpectations, + CustomValidatorRequest customValidatorReq ) { String testDBName = TestUtils.generateNewDB(); String dbConnectionUrl = String.join("/", JDBC_URL, testDBName); @@ -383,7 +426,7 @@ private boolean runIntegrationTestOnZipFile( // load and validate feed LOG.info("load and validate GTFS file {}", zipFileName); FeedLoadResult loadResult = GTFS.load(zipFileName, dataSource); - ValidationResult validationResult = GTFS.validate(loadResult.uniqueIdentifier, dataSource, extraValidatorClasses); + ValidationResult validationResult = GTFS.validate(loadResult.uniqueIdentifier, dataSource, customValidatorReq); assertThat(validationResult.fatalException, is(fatalExceptionExpectation)); namespace = loadResult.uniqueIdentifier; From 9b94a0ecc099d7b21997645954a43c9da1b1072f Mon Sep 17 00:00:00 2001 From: binh-dam-ibigroup <56846598+binh-dam-ibigroup@users.noreply.github.com> Date: Thu, 2 Apr 2020 17:14:35 -0400 Subject: [PATCH 07/17] refactor(Feed): Replace callback interface to create MTCValidator with Java's BiFunction equivalent. --- src/main/java/com/conveyal/gtfs/GTFS.java | 10 ++++++---- .../java/com/conveyal/gtfs/loader/Feed.java | 5 +++-- .../validator/CustomValidatorRequest.java | 20 ------------------- src/test/java/com/conveyal/gtfs/GTFSTest.java | 16 +++++++-------- 4 files changed, 16 insertions(+), 35 deletions(-) delete mode 100644 src/main/java/com/conveyal/gtfs/validator/CustomValidatorRequest.java diff --git a/src/main/java/com/conveyal/gtfs/GTFS.java b/src/main/java/com/conveyal/gtfs/GTFS.java index a1d28f92b..52ba26165 100644 --- a/src/main/java/com/conveyal/gtfs/GTFS.java +++ b/src/main/java/com/conveyal/gtfs/GTFS.java @@ -1,5 +1,6 @@ package com.conveyal.gtfs; +import com.conveyal.gtfs.error.SQLErrorStorage; import com.conveyal.gtfs.loader.Feed; import com.conveyal.gtfs.loader.FeedLoadResult; import com.conveyal.gtfs.loader.JdbcGtfsExporter; @@ -7,7 +8,7 @@ import com.conveyal.gtfs.loader.JdbcGtfsSnapshotter; import com.conveyal.gtfs.loader.SnapshotResult; import com.conveyal.gtfs.util.InvalidNamespaceException; -import com.conveyal.gtfs.validator.CustomValidatorRequest; +import com.conveyal.gtfs.validator.FeedValidator; import com.conveyal.gtfs.validator.ValidationResult; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.io.Files; @@ -29,6 +30,8 @@ import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.SQLException; +import java.util.List; +import java.util.function.BiFunction; import static com.conveyal.gtfs.util.Util.ensureValidNamespace; @@ -101,10 +104,9 @@ public static ValidationResult validate (String feedId, DataSource dataSource) { /** * Once a feed has been loaded into the database, examine its contents looking for various problems and errors. */ - public static ValidationResult validate (String feedId, DataSource dataSource, CustomValidatorRequest customValidatorReq) { + public static ValidationResult validate (String feedId, DataSource dataSource, BiFunction> customValidatorRequest) { Feed feed = new Feed(dataSource, feedId); - ValidationResult result = feed.validate(customValidatorReq); - return result; + return feed.validate(customValidatorRequest); } /** diff --git a/src/main/java/com/conveyal/gtfs/loader/Feed.java b/src/main/java/com/conveyal/gtfs/loader/Feed.java index 2699f2025..180095a44 100644 --- a/src/main/java/com/conveyal/gtfs/loader/Feed.java +++ b/src/main/java/com/conveyal/gtfs/loader/Feed.java @@ -16,6 +16,7 @@ import java.sql.SQLException; import java.util.ArrayList; import java.util.List; +import java.util.function.BiFunction; import static com.conveyal.gtfs.error.NewGTFSErrorType.VALIDATOR_FAILED; @@ -81,7 +82,7 @@ public ValidationResult validate () { * TODO allow validation within feed loading process, so the same connection can be used, and we're certain loaded data is 100% visible. * That would also avoid having to reconnect the error storage to the DB. */ - public ValidationResult validate (CustomValidatorRequest customValidatorReq) { + public ValidationResult validate (BiFunction> customValidatorRequest) { long validationStartTime = System.currentTimeMillis(); // Create an empty validation result that will have its fields populated by certain validators. ValidationResult validationResult = new ValidationResult(); @@ -107,7 +108,7 @@ public ValidationResult validate (CustomValidatorRequest customValidatorReq) { // Add additional validators, if any provided. List customValidators = null; - if (customValidatorReq != null) customValidators = customValidatorReq.getCustomValidators(this, errorStorage); + if (customValidatorRequest != null) customValidators = customValidatorRequest.apply(this, errorStorage); if (customValidators != null) feedValidators.addAll(customValidators); for (FeedValidator feedValidator : feedValidators) { diff --git a/src/main/java/com/conveyal/gtfs/validator/CustomValidatorRequest.java b/src/main/java/com/conveyal/gtfs/validator/CustomValidatorRequest.java deleted file mode 100644 index 4237a8cec..000000000 --- a/src/main/java/com/conveyal/gtfs/validator/CustomValidatorRequest.java +++ /dev/null @@ -1,20 +0,0 @@ -package com.conveyal.gtfs.validator; - -import com.conveyal.gtfs.error.SQLErrorStorage; -import com.conveyal.gtfs.loader.Feed; - -import java.util.List; - -/** - * An interface with a callback to instantiate additional custom GTFS Feed validators - * using a Feed and SQLErrorStorage objects. - */ -public interface CustomValidatorRequest { - /** - * The callback that instantiates and returns instances of custom FeedValidator objects - * constructed using the provided feed and error storage objects. - * @param feed The feed being validated. - * @param errorStorage The object that handles error storage. - */ - public List getCustomValidators(Feed feed, SQLErrorStorage errorStorage); -} diff --git a/src/test/java/com/conveyal/gtfs/GTFSTest.java b/src/test/java/com/conveyal/gtfs/GTFSTest.java index 86f3f0fdb..bd361042e 100644 --- a/src/test/java/com/conveyal/gtfs/GTFSTest.java +++ b/src/test/java/com/conveyal/gtfs/GTFSTest.java @@ -2,6 +2,8 @@ import com.conveyal.gtfs.error.NewGTFSErrorType; +import com.conveyal.gtfs.error.SQLErrorStorage; +import com.conveyal.gtfs.loader.Feed; import com.conveyal.gtfs.loader.FeedLoadResult; import com.conveyal.gtfs.loader.SnapshotResult; import com.conveyal.gtfs.storage.ErrorExpectation; @@ -9,7 +11,6 @@ import com.conveyal.gtfs.storage.PersistenceExpectation; import com.conveyal.gtfs.storage.RecordExpectation; import com.conveyal.gtfs.util.InvalidNamespaceException; -import com.conveyal.gtfs.validator.CustomValidatorRequest; import com.conveyal.gtfs.validator.FeedValidator; import com.conveyal.gtfs.validator.MTCValidator; import com.conveyal.gtfs.validator.ValidationResult; @@ -42,6 +43,7 @@ import java.util.Collection; import java.util.Iterator; import java.util.List; +import java.util.function.BiFunction; import java.util.zip.ZipEntry; import java.util.zip.ZipFile; @@ -59,8 +61,6 @@ public class GTFSTest { private static final String JDBC_URL = "jdbc:postgresql://localhost"; private static final Logger LOG = LoggerFactory.getLogger(GTFSTest.class); - private List> extraValidatorClasses = new ArrayList<>(); - // setup a stream to capture the output from the program @Before public void setUpStreams() { @@ -314,8 +314,6 @@ public void canLoadAndExportSimpleAgencyWithOnlyCalendarDates() { */ @Test public void canLoadFeedWithLongFieldValues () { - extraValidatorClasses.add(MTCValidator.class); - PersistenceExpectation[] expectations = PersistenceExpectation.list(); ErrorExpectation[] errorExpectations = ErrorExpectation.list( new ErrorExpectation(NewGTFSErrorType.FIELD_VALUE_TOO_LONG), @@ -363,7 +361,7 @@ private boolean runIntegrationTestOnFolder( Matcher fatalExceptionExpectation, PersistenceExpectation[] persistenceExpectations, ErrorExpectation[] errorExpectations, - CustomValidatorRequest customValidatorReq + BiFunction> customValidatorRequest ) { LOG.info("Running integration test on folder {}", folderName); // zip up test folder into temp zip file @@ -374,7 +372,7 @@ private boolean runIntegrationTestOnFolder( e.printStackTrace(); return false; } - return runIntegrationTestOnZipFile(zipFileName, fatalExceptionExpectation, persistenceExpectations, errorExpectations, customValidatorReq); + return runIntegrationTestOnZipFile(zipFileName, fatalExceptionExpectation, persistenceExpectations, errorExpectations, customValidatorRequest); } /** @@ -409,7 +407,7 @@ private boolean runIntegrationTestOnZipFile( Matcher fatalExceptionExpectation, PersistenceExpectation[] persistenceExpectations, ErrorExpectation[] errorExpectations, - CustomValidatorRequest customValidatorReq + BiFunction> customValidatorRequest ) { String testDBName = TestUtils.generateNewDB(); String dbConnectionUrl = String.join("/", JDBC_URL, testDBName); @@ -426,7 +424,7 @@ private boolean runIntegrationTestOnZipFile( // load and validate feed LOG.info("load and validate GTFS file {}", zipFileName); FeedLoadResult loadResult = GTFS.load(zipFileName, dataSource); - ValidationResult validationResult = GTFS.validate(loadResult.uniqueIdentifier, dataSource, customValidatorReq); + ValidationResult validationResult = GTFS.validate(loadResult.uniqueIdentifier, dataSource, customValidatorRequest); assertThat(validationResult.fatalException, is(fatalExceptionExpectation)); namespace = loadResult.uniqueIdentifier; From 13696e55d840976d73c5faf3a919d722f54786ca Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Thu, 2 Apr 2020 17:39:44 -0400 Subject: [PATCH 08/17] refactor(validate): remove list from custom validator interface --- src/main/java/com/conveyal/gtfs/GTFS.java | 11 ++------ .../java/com/conveyal/gtfs/loader/Feed.java | 17 ++++--------- .../validator/CustomValidatorRequest.java | 8 +++--- src/test/java/com/conveyal/gtfs/GTFSTest.java | 25 +++++++++++++------ 4 files changed, 27 insertions(+), 34 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/GTFS.java b/src/main/java/com/conveyal/gtfs/GTFS.java index a1d28f92b..f6d2cf756 100644 --- a/src/main/java/com/conveyal/gtfs/GTFS.java +++ b/src/main/java/com/conveyal/gtfs/GTFS.java @@ -91,19 +91,12 @@ public static SnapshotResult makeSnapshot (String feedId, DataSource dataSource) return result; } - /** - * Shorthand for calling validate() without custom validators. - */ - public static ValidationResult validate (String feedId, DataSource dataSource) { - return validate(feedId, dataSource, null); - } - /** * Once a feed has been loaded into the database, examine its contents looking for various problems and errors. */ - public static ValidationResult validate (String feedId, DataSource dataSource, CustomValidatorRequest customValidatorReq) { + public static ValidationResult validate (String feedId, DataSource dataSource, CustomValidatorRequest... customValidatorRequests) { Feed feed = new Feed(dataSource, feedId); - ValidationResult result = feed.validate(customValidatorReq); + ValidationResult result = feed.validate(customValidatorRequests); return result; } diff --git a/src/main/java/com/conveyal/gtfs/loader/Feed.java b/src/main/java/com/conveyal/gtfs/loader/Feed.java index 2699f2025..3d194ba45 100644 --- a/src/main/java/com/conveyal/gtfs/loader/Feed.java +++ b/src/main/java/com/conveyal/gtfs/loader/Feed.java @@ -69,19 +69,12 @@ public Feed (DataSource dataSource, String tablePrefix) { stopTimes = new JDBCTableReader(Table.STOP_TIMES, dataSource, tablePrefix, EntityPopulator.STOP_TIME); } - /** - * Shorthand for calling validate() without custom validators. - */ - public ValidationResult validate () { - return validate(null); - } - /** * TODO check whether validation has already occurred, overwrite results. * TODO allow validation within feed loading process, so the same connection can be used, and we're certain loaded data is 100% visible. * That would also avoid having to reconnect the error storage to the DB. */ - public ValidationResult validate (CustomValidatorRequest customValidatorReq) { + public ValidationResult validate (CustomValidatorRequest... customValidatorRequests) { long validationStartTime = System.currentTimeMillis(); // Create an empty validation result that will have its fields populated by certain validators. ValidationResult validationResult = new ValidationResult(); @@ -105,10 +98,10 @@ public ValidationResult validate (CustomValidatorRequest customValidatorReq) { new NamesValidator(this, errorStorage) ); - // Add additional validators, if any provided. - List customValidators = null; - if (customValidatorReq != null) customValidators = customValidatorReq.getCustomValidators(this, errorStorage); - if (customValidators != null) feedValidators.addAll(customValidators); + for (CustomValidatorRequest request : customValidatorRequests) { + FeedValidator validator = request.apply(this, errorStorage); + feedValidators.add(validator); + } for (FeedValidator feedValidator : feedValidators) { String validatorName = feedValidator.getClass().getSimpleName(); diff --git a/src/main/java/com/conveyal/gtfs/validator/CustomValidatorRequest.java b/src/main/java/com/conveyal/gtfs/validator/CustomValidatorRequest.java index 4237a8cec..bec593778 100644 --- a/src/main/java/com/conveyal/gtfs/validator/CustomValidatorRequest.java +++ b/src/main/java/com/conveyal/gtfs/validator/CustomValidatorRequest.java @@ -3,11 +3,9 @@ import com.conveyal.gtfs.error.SQLErrorStorage; import com.conveyal.gtfs.loader.Feed; -import java.util.List; - /** - * An interface with a callback to instantiate additional custom GTFS Feed validators - * using a Feed and SQLErrorStorage objects. + * An interface with a callback to instantiate a custom GTFS Feed validator + * using the Feed and SQLErrorStorage objects. */ public interface CustomValidatorRequest { /** @@ -16,5 +14,5 @@ public interface CustomValidatorRequest { * @param feed The feed being validated. * @param errorStorage The object that handles error storage. */ - public List getCustomValidators(Feed feed, SQLErrorStorage errorStorage); + FeedValidator apply(Feed feed, SQLErrorStorage errorStorage); } diff --git a/src/test/java/com/conveyal/gtfs/GTFSTest.java b/src/test/java/com/conveyal/gtfs/GTFSTest.java index 86f3f0fdb..efec7e32c 100644 --- a/src/test/java/com/conveyal/gtfs/GTFSTest.java +++ b/src/test/java/com/conveyal/gtfs/GTFSTest.java @@ -328,10 +328,13 @@ public void canLoadFeedWithLongFieldValues () { ); assertThat( "Long-field-value test passes", - runIntegrationTestOnFolder("fake-agency-mtc-long-fields", nullValue(), expectations, errorExpectations, - (feed, errorSource) -> { - return Lists.newArrayList(new MTCValidator(feed, errorSource)); - }), + runIntegrationTestOnFolder( + "fake-agency-mtc-long-fields", + nullValue(), + expectations, + errorExpectations, + MTCValidator::new + ), equalTo(true) ); } @@ -363,7 +366,7 @@ private boolean runIntegrationTestOnFolder( Matcher fatalExceptionExpectation, PersistenceExpectation[] persistenceExpectations, ErrorExpectation[] errorExpectations, - CustomValidatorRequest customValidatorReq + CustomValidatorRequest customValidatorRequest ) { LOG.info("Running integration test on folder {}", folderName); // zip up test folder into temp zip file @@ -374,7 +377,13 @@ private boolean runIntegrationTestOnFolder( e.printStackTrace(); return false; } - return runIntegrationTestOnZipFile(zipFileName, fatalExceptionExpectation, persistenceExpectations, errorExpectations, customValidatorReq); + return runIntegrationTestOnZipFile( + zipFileName, + fatalExceptionExpectation, + persistenceExpectations, + errorExpectations, + customValidatorRequest + ); } /** @@ -409,7 +418,7 @@ private boolean runIntegrationTestOnZipFile( Matcher fatalExceptionExpectation, PersistenceExpectation[] persistenceExpectations, ErrorExpectation[] errorExpectations, - CustomValidatorRequest customValidatorReq + CustomValidatorRequest customValidatorRequest ) { String testDBName = TestUtils.generateNewDB(); String dbConnectionUrl = String.join("/", JDBC_URL, testDBName); @@ -426,7 +435,7 @@ private boolean runIntegrationTestOnZipFile( // load and validate feed LOG.info("load and validate GTFS file {}", zipFileName); FeedLoadResult loadResult = GTFS.load(zipFileName, dataSource); - ValidationResult validationResult = GTFS.validate(loadResult.uniqueIdentifier, dataSource, customValidatorReq); + ValidationResult validationResult = GTFS.validate(loadResult.uniqueIdentifier, dataSource, customValidatorRequest); assertThat(validationResult.fatalException, is(fatalExceptionExpectation)); namespace = loadResult.uniqueIdentifier; From df848402d0155d2b6523fb1b76efd196842c0070 Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Fri, 3 Apr 2020 10:20:50 -0400 Subject: [PATCH 09/17] refactor(validator): rename validator creator --- src/main/java/com/conveyal/gtfs/GTFS.java | 6 ++-- .../java/com/conveyal/gtfs/loader/Feed.java | 33 +++++++++++-------- ...atorRequest.java => ValidatorCreator.java} | 5 +-- src/test/java/com/conveyal/gtfs/GTFSTest.java | 11 +++---- 4 files changed, 30 insertions(+), 25 deletions(-) rename src/main/java/com/conveyal/gtfs/validator/{CustomValidatorRequest.java => ValidatorCreator.java} (80%) diff --git a/src/main/java/com/conveyal/gtfs/GTFS.java b/src/main/java/com/conveyal/gtfs/GTFS.java index f6d2cf756..bdb9eaa9b 100644 --- a/src/main/java/com/conveyal/gtfs/GTFS.java +++ b/src/main/java/com/conveyal/gtfs/GTFS.java @@ -7,7 +7,7 @@ import com.conveyal.gtfs.loader.JdbcGtfsSnapshotter; import com.conveyal.gtfs.loader.SnapshotResult; import com.conveyal.gtfs.util.InvalidNamespaceException; -import com.conveyal.gtfs.validator.CustomValidatorRequest; +import com.conveyal.gtfs.validator.ValidatorCreator; import com.conveyal.gtfs.validator.ValidationResult; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.io.Files; @@ -94,9 +94,9 @@ public static SnapshotResult makeSnapshot (String feedId, DataSource dataSource) /** * Once a feed has been loaded into the database, examine its contents looking for various problems and errors. */ - public static ValidationResult validate (String feedId, DataSource dataSource, CustomValidatorRequest... customValidatorRequests) { + public static ValidationResult validate (String feedId, DataSource dataSource, ValidatorCreator... customValidators) { Feed feed = new Feed(dataSource, feedId); - ValidationResult result = feed.validate(customValidatorRequests); + ValidationResult result = feed.validate(customValidators); return result; } diff --git a/src/main/java/com/conveyal/gtfs/loader/Feed.java b/src/main/java/com/conveyal/gtfs/loader/Feed.java index 3d194ba45..1bad2a1cf 100644 --- a/src/main/java/com/conveyal/gtfs/loader/Feed.java +++ b/src/main/java/com/conveyal/gtfs/loader/Feed.java @@ -15,6 +15,7 @@ import java.sql.Connection; import java.sql.SQLException; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import static com.conveyal.gtfs.error.NewGTFSErrorType.VALIDATOR_FAILED; @@ -74,32 +75,36 @@ public Feed (DataSource dataSource, String tablePrefix) { * TODO allow validation within feed loading process, so the same connection can be used, and we're certain loaded data is 100% visible. * That would also avoid having to reconnect the error storage to the DB. */ - public ValidationResult validate (CustomValidatorRequest... customValidatorRequests) { + public ValidationResult validate (ValidatorCreator... additionalValidators) { long validationStartTime = System.currentTimeMillis(); // Create an empty validation result that will have its fields populated by certain validators. ValidationResult validationResult = new ValidationResult(); // Error tables should already be present from the initial load. // Reconnect to the existing error tables. - SQLErrorStorage errorStorage = null; + SQLErrorStorage errorStorage; try { errorStorage = new SQLErrorStorage(dataSource.getConnection(), tablePrefix, false); } catch (SQLException | InvalidNamespaceException ex) { throw new StorageException(ex); } int errorCountBeforeValidation = errorStorage.getErrorCount(); - - List feedValidators = Lists.newArrayList( - new MisplacedStopValidator(this, errorStorage, validationResult), - new DuplicateStopsValidator(this, errorStorage), - new FaresValidator(this, errorStorage), - new FrequencyValidator(this, errorStorage), - new TimeZoneValidator(this, errorStorage), - new NewTripTimesValidator(this, errorStorage), - new NamesValidator(this, errorStorage) + List feedValidators = new ArrayList<>(); + // Add misplaced stop validator (takes non-standard args for constructor). + feedValidators.add(new MisplacedStopValidator(this, errorStorage, validationResult)); + // For other validators, add to list of validators to create. + List validatorsToCreate = Lists.newArrayList( + DuplicateStopsValidator::new, + FaresValidator::new, + FrequencyValidator::new, + TimeZoneValidator::new, + NewTripTimesValidator::new, + NamesValidator::new ); - - for (CustomValidatorRequest request : customValidatorRequests) { - FeedValidator validator = request.apply(this, errorStorage); + // Add any additional validators specified in this method's args. + validatorsToCreate.addAll(Arrays.asList(additionalValidators)); + // Instantiate validators and add to list of feed validators to run. + for (ValidatorCreator creator : validatorsToCreate) { + FeedValidator validator = creator.createValidator(this, errorStorage); feedValidators.add(validator); } diff --git a/src/main/java/com/conveyal/gtfs/validator/CustomValidatorRequest.java b/src/main/java/com/conveyal/gtfs/validator/ValidatorCreator.java similarity index 80% rename from src/main/java/com/conveyal/gtfs/validator/CustomValidatorRequest.java rename to src/main/java/com/conveyal/gtfs/validator/ValidatorCreator.java index bec593778..8f693f256 100644 --- a/src/main/java/com/conveyal/gtfs/validator/CustomValidatorRequest.java +++ b/src/main/java/com/conveyal/gtfs/validator/ValidatorCreator.java @@ -7,12 +7,13 @@ * An interface with a callback to instantiate a custom GTFS Feed validator * using the Feed and SQLErrorStorage objects. */ -public interface CustomValidatorRequest { +@FunctionalInterface +public interface ValidatorCreator { /** * The callback that instantiates and returns instances of custom FeedValidator objects * constructed using the provided feed and error storage objects. * @param feed The feed being validated. * @param errorStorage The object that handles error storage. */ - FeedValidator apply(Feed feed, SQLErrorStorage errorStorage); + FeedValidator createValidator(Feed feed, SQLErrorStorage errorStorage); } diff --git a/src/test/java/com/conveyal/gtfs/GTFSTest.java b/src/test/java/com/conveyal/gtfs/GTFSTest.java index efec7e32c..81395969f 100644 --- a/src/test/java/com/conveyal/gtfs/GTFSTest.java +++ b/src/test/java/com/conveyal/gtfs/GTFSTest.java @@ -9,13 +9,12 @@ import com.conveyal.gtfs.storage.PersistenceExpectation; import com.conveyal.gtfs.storage.RecordExpectation; import com.conveyal.gtfs.util.InvalidNamespaceException; -import com.conveyal.gtfs.validator.CustomValidatorRequest; +import com.conveyal.gtfs.validator.ValidatorCreator; import com.conveyal.gtfs.validator.FeedValidator; import com.conveyal.gtfs.validator.MTCValidator; import com.conveyal.gtfs.validator.ValidationResult; import com.csvreader.CsvReader; import com.google.common.collect.ArrayListMultimap; -import com.google.common.collect.Lists; import com.google.common.collect.Multimap; import com.google.common.io.Files; import org.apache.commons.io.FileUtils; @@ -366,7 +365,7 @@ private boolean runIntegrationTestOnFolder( Matcher fatalExceptionExpectation, PersistenceExpectation[] persistenceExpectations, ErrorExpectation[] errorExpectations, - CustomValidatorRequest customValidatorRequest + ValidatorCreator customValidator ) { LOG.info("Running integration test on folder {}", folderName); // zip up test folder into temp zip file @@ -382,7 +381,7 @@ private boolean runIntegrationTestOnFolder( fatalExceptionExpectation, persistenceExpectations, errorExpectations, - customValidatorRequest + customValidator ); } @@ -418,7 +417,7 @@ private boolean runIntegrationTestOnZipFile( Matcher fatalExceptionExpectation, PersistenceExpectation[] persistenceExpectations, ErrorExpectation[] errorExpectations, - CustomValidatorRequest customValidatorRequest + ValidatorCreator customValidator ) { String testDBName = TestUtils.generateNewDB(); String dbConnectionUrl = String.join("/", JDBC_URL, testDBName); @@ -435,7 +434,7 @@ private boolean runIntegrationTestOnZipFile( // load and validate feed LOG.info("load and validate GTFS file {}", zipFileName); FeedLoadResult loadResult = GTFS.load(zipFileName, dataSource); - ValidationResult validationResult = GTFS.validate(loadResult.uniqueIdentifier, dataSource, customValidatorRequest); + ValidationResult validationResult = GTFS.validate(loadResult.uniqueIdentifier, dataSource, customValidator); assertThat(validationResult.fatalException, is(fatalExceptionExpectation)); namespace = loadResult.uniqueIdentifier; From d449b12ae5663d6f6bc6b0a4dc6fc1cb3701075e Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Fri, 3 Apr 2020 10:58:13 -0400 Subject: [PATCH 10/17] refactor(validator): refactor Feed class and add javadoc --- src/main/java/com/conveyal/gtfs/GTFS.java | 4 +- .../java/com/conveyal/gtfs/loader/Feed.java | 65 ++++++++----------- .../gtfs/validator/FeedValidatorCreator.java | 24 +++++++ .../gtfs/validator/ReversedTripValidator.java | 9 ++- .../gtfs/validator/ValidatorCreator.java | 19 ------ src/test/java/com/conveyal/gtfs/GTFSTest.java | 6 +- 6 files changed, 62 insertions(+), 65 deletions(-) create mode 100644 src/main/java/com/conveyal/gtfs/validator/FeedValidatorCreator.java delete mode 100644 src/main/java/com/conveyal/gtfs/validator/ValidatorCreator.java diff --git a/src/main/java/com/conveyal/gtfs/GTFS.java b/src/main/java/com/conveyal/gtfs/GTFS.java index bdb9eaa9b..fc1a006e9 100644 --- a/src/main/java/com/conveyal/gtfs/GTFS.java +++ b/src/main/java/com/conveyal/gtfs/GTFS.java @@ -7,7 +7,7 @@ import com.conveyal.gtfs.loader.JdbcGtfsSnapshotter; import com.conveyal.gtfs.loader.SnapshotResult; import com.conveyal.gtfs.util.InvalidNamespaceException; -import com.conveyal.gtfs.validator.ValidatorCreator; +import com.conveyal.gtfs.validator.FeedValidatorCreator; import com.conveyal.gtfs.validator.ValidationResult; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.io.Files; @@ -94,7 +94,7 @@ public static SnapshotResult makeSnapshot (String feedId, DataSource dataSource) /** * Once a feed has been loaded into the database, examine its contents looking for various problems and errors. */ - public static ValidationResult validate (String feedId, DataSource dataSource, ValidatorCreator... customValidators) { + public static ValidationResult validate (String feedId, DataSource dataSource, FeedValidatorCreator... customValidators) { Feed feed = new Feed(dataSource, feedId); ValidationResult result = feed.validate(customValidators); return result; diff --git a/src/main/java/com/conveyal/gtfs/loader/Feed.java b/src/main/java/com/conveyal/gtfs/loader/Feed.java index 1bad2a1cf..80bbb7146 100644 --- a/src/main/java/com/conveyal/gtfs/loader/Feed.java +++ b/src/main/java/com/conveyal/gtfs/loader/Feed.java @@ -1,6 +1,5 @@ package com.conveyal.gtfs.loader; -import com.conveyal.gtfs.error.GTFSError; import com.conveyal.gtfs.error.NewGTFSError; import com.conveyal.gtfs.error.SQLErrorStorage; import com.conveyal.gtfs.model.*; @@ -14,8 +13,6 @@ import javax.sql.DataSource; import java.sql.Connection; import java.sql.SQLException; -import java.util.ArrayList; -import java.util.Arrays; import java.util.List; import static com.conveyal.gtfs.error.NewGTFSErrorType.VALIDATOR_FAILED; @@ -33,20 +30,15 @@ public class Feed { // This may be the empty string if the feed is stored in the root ("public") schema. public final String tablePrefix; - public final TableReader agencies; - public final TableReader calendars; - public final TableReader calendarDates; + public final TableReader agencies; + public final TableReader calendars; + public final TableReader calendarDates; public final TableReader fareAttributes; - public final TableReader frequencies; - public final TableReader routes; - public final TableReader stops; - public final TableReader trips; -// public final TableReader shapePoints; - public final TableReader stopTimes; - - /* A place to accumulate errors while the feed is loaded. Tolerate as many errors as possible and keep on loading. */ - // TODO remove this and use only NewGTFSErrors in Validators, loaded into a JDBC table - public final List errors = new ArrayList<>(); + public final TableReader frequencies; + public final TableReader routes; + public final TableReader stops; + public final TableReader trips; + public final TableReader stopTimes; /** * Create a feed that reads tables over a JDBC connection. The connection should already be set to the right @@ -66,16 +58,19 @@ public Feed (DataSource dataSource, String tablePrefix) { routes = new JDBCTableReader(Table.ROUTES, dataSource, tablePrefix, EntityPopulator.ROUTE); stops = new JDBCTableReader(Table.STOPS, dataSource, tablePrefix, EntityPopulator.STOP); trips = new JDBCTableReader(Table.TRIPS, dataSource, tablePrefix, EntityPopulator.TRIP); -// shapePoints = new JDBCTableReader(Table.SHAPES, dataSource, tablePrefix, EntityPopulator.SHAPE_POINT); stopTimes = new JDBCTableReader(Table.STOP_TIMES, dataSource, tablePrefix, EntityPopulator.STOP_TIME); } /** + * Run the standard validation checks for this feed and store the validation errors in the database. Optionally, + * takes one or more {@link FeedValidatorCreator} in the form of lambda method refs (e.g., {@code MTCValidator::new}), + * which this method will instantiate and run after the standard validation checks have been completed. + * * TODO check whether validation has already occurred, overwrite results. - * TODO allow validation within feed loading process, so the same connection can be used, and we're certain loaded data is 100% visible. - * That would also avoid having to reconnect the error storage to the DB. + * TODO allow validation within feed loading process, so the same connection can be used, and we're certain loaded + * data is 100% visible. That would also avoid having to reconnect the error storage to the DB. */ - public ValidationResult validate (ValidatorCreator... additionalValidators) { + public ValidationResult validate (FeedValidatorCreator... additionalValidators) { long validationStartTime = System.currentTimeMillis(); // Create an empty validation result that will have its fields populated by certain validators. ValidationResult validationResult = new ValidationResult(); @@ -88,24 +83,19 @@ public ValidationResult validate (ValidatorCreator... additionalValidators) { throw new StorageException(ex); } int errorCountBeforeValidation = errorStorage.getErrorCount(); - List feedValidators = new ArrayList<>(); - // Add misplaced stop validator (takes non-standard args for constructor). - feedValidators.add(new MisplacedStopValidator(this, errorStorage, validationResult)); - // For other validators, add to list of validators to create. - List validatorsToCreate = Lists.newArrayList( - DuplicateStopsValidator::new, - FaresValidator::new, - FrequencyValidator::new, - TimeZoneValidator::new, - NewTripTimesValidator::new, - NamesValidator::new + // Create list of standard validators to run on every feed. + List feedValidators = Lists.newArrayList( + new MisplacedStopValidator(this, errorStorage, validationResult), + new DuplicateStopsValidator(this, errorStorage), + new FaresValidator(this, errorStorage), + new FrequencyValidator(this, errorStorage), + new TimeZoneValidator(this, errorStorage), + new NewTripTimesValidator(this, errorStorage), + new NamesValidator(this, errorStorage) ); - // Add any additional validators specified in this method's args. - validatorsToCreate.addAll(Arrays.asList(additionalValidators)); - // Instantiate validators and add to list of feed validators to run. - for (ValidatorCreator creator : validatorsToCreate) { - FeedValidator validator = creator.createValidator(this, errorStorage); - feedValidators.add(validator); + // Create additional validators specified in this method's args and add to list of feed validators to run. + for (FeedValidatorCreator creator : additionalValidators) { + feedValidators.add(creator.create(this, errorStorage)); } for (FeedValidator feedValidator : feedValidators) { @@ -113,7 +103,6 @@ public ValidationResult validate (ValidatorCreator... additionalValidators) { try { LOG.info("Running {}.", validatorName); int errorCountBefore = errorStorage.getErrorCount(); - // todo why not just pass the feed and errorstorage in here? feedValidator.validate(); LOG.info("{} found {} errors.", validatorName, errorStorage.getErrorCount() - errorCountBefore); } catch (Exception e) { diff --git a/src/main/java/com/conveyal/gtfs/validator/FeedValidatorCreator.java b/src/main/java/com/conveyal/gtfs/validator/FeedValidatorCreator.java new file mode 100644 index 000000000..39fbc67e4 --- /dev/null +++ b/src/main/java/com/conveyal/gtfs/validator/FeedValidatorCreator.java @@ -0,0 +1,24 @@ +package com.conveyal.gtfs.validator; + +import com.conveyal.gtfs.error.SQLErrorStorage; +import com.conveyal.gtfs.loader.Feed; + +/** + * A functional interface used to create {@link FeedValidator} instances. This interface supports the ability to pass + * validators by lambda method references ({@code SomeValidator::new}) to the {@link Feed#validate(FeedValidatorCreator...)} + * method in order to run special validation checks on specific feeds (e.g., {@link MTCValidator} should be run only on + * GTFS files loaded for those MTC operators). To instantiate the FeedValidator, simply call the + * {@link #create(Feed, SQLErrorStorage)} method, passing in the feed and errorStorage arguments. This is in lieu of + * instantiating the FeedValidator with those arguments in the constructor because these objects are not available before + * the validate method is invoked. + */ +@FunctionalInterface +public interface FeedValidatorCreator { + /** + * The callback that instantiates and returns instances of custom FeedValidator objects + * constructed using the provided feed and error storage objects. + * @param feed The feed being validated. + * @param errorStorage The object that handles error storage. + */ + FeedValidator create(Feed feed, SQLErrorStorage errorStorage); +} diff --git a/src/main/java/com/conveyal/gtfs/validator/ReversedTripValidator.java b/src/main/java/com/conveyal/gtfs/validator/ReversedTripValidator.java index ebe1c1d1c..b2a4c7f90 100644 --- a/src/main/java/com/conveyal/gtfs/validator/ReversedTripValidator.java +++ b/src/main/java/com/conveyal/gtfs/validator/ReversedTripValidator.java @@ -50,7 +50,8 @@ public boolean validate(Feed feed, boolean repair) { if (trip.shape_id == null) { isValid = false; if (missingShapeErrorCount < errorLimit) { - feed.errors.add(new MissingShapeError(trip)); + // FIXME store MissingShape errors + // feed.errors.add(new MissingShapeError(trip)); } missingShapeErrorCount++; continue; @@ -111,7 +112,8 @@ public boolean validate(Feed feed, boolean repair) { // check if first stop is x times closer to end of shape than the beginning or last stop is x times closer to start than the end if (distanceFirstStopToStart > (distanceFirstStopToEnd * distanceMultiplier) && distanceLastStopToEnd > (distanceLastStopToStart * distanceMultiplier)) { if (reversedTripShapeErrorCount < errorLimit) { - feed.errors.add(new ReversedTripShapeError(trip)); + // FIXME store ReversedTripShape errors + // feed.errors.add(new ReversedTripShapeError(trip)); } reversedTripShapeErrorCount++; isValid = false; @@ -120,7 +122,8 @@ public boolean validate(Feed feed, boolean repair) { if (missingCoordinatesErrorCount > 0) { for (Map.Entry> shapeError : missingShapesMap.entrySet()) { String[] tripIdList = shapeError.getValue().toArray(new String[shapeError.getValue().size()]); - feed.errors.add(new ShapeMissingCoordinatesError(shapeError.getKey(), tripIdList)); + // FIXME store ShapeMissingCoordinates errors + // feed.errors.add(new ShapeMissingCoordinatesError(shapeError.getKey(), tripIdList)); } } return isValid; diff --git a/src/main/java/com/conveyal/gtfs/validator/ValidatorCreator.java b/src/main/java/com/conveyal/gtfs/validator/ValidatorCreator.java deleted file mode 100644 index 8f693f256..000000000 --- a/src/main/java/com/conveyal/gtfs/validator/ValidatorCreator.java +++ /dev/null @@ -1,19 +0,0 @@ -package com.conveyal.gtfs.validator; - -import com.conveyal.gtfs.error.SQLErrorStorage; -import com.conveyal.gtfs.loader.Feed; - -/** - * An interface with a callback to instantiate a custom GTFS Feed validator - * using the Feed and SQLErrorStorage objects. - */ -@FunctionalInterface -public interface ValidatorCreator { - /** - * The callback that instantiates and returns instances of custom FeedValidator objects - * constructed using the provided feed and error storage objects. - * @param feed The feed being validated. - * @param errorStorage The object that handles error storage. - */ - FeedValidator createValidator(Feed feed, SQLErrorStorage errorStorage); -} diff --git a/src/test/java/com/conveyal/gtfs/GTFSTest.java b/src/test/java/com/conveyal/gtfs/GTFSTest.java index 81395969f..bcd6099d0 100644 --- a/src/test/java/com/conveyal/gtfs/GTFSTest.java +++ b/src/test/java/com/conveyal/gtfs/GTFSTest.java @@ -9,7 +9,7 @@ import com.conveyal.gtfs.storage.PersistenceExpectation; import com.conveyal.gtfs.storage.RecordExpectation; import com.conveyal.gtfs.util.InvalidNamespaceException; -import com.conveyal.gtfs.validator.ValidatorCreator; +import com.conveyal.gtfs.validator.FeedValidatorCreator; import com.conveyal.gtfs.validator.FeedValidator; import com.conveyal.gtfs.validator.MTCValidator; import com.conveyal.gtfs.validator.ValidationResult; @@ -365,7 +365,7 @@ private boolean runIntegrationTestOnFolder( Matcher fatalExceptionExpectation, PersistenceExpectation[] persistenceExpectations, ErrorExpectation[] errorExpectations, - ValidatorCreator customValidator + FeedValidatorCreator customValidator ) { LOG.info("Running integration test on folder {}", folderName); // zip up test folder into temp zip file @@ -417,7 +417,7 @@ private boolean runIntegrationTestOnZipFile( Matcher fatalExceptionExpectation, PersistenceExpectation[] persistenceExpectations, ErrorExpectation[] errorExpectations, - ValidatorCreator customValidator + FeedValidatorCreator customValidator ) { String testDBName = TestUtils.generateNewDB(); String dbConnectionUrl = String.join("/", JDBC_URL, testDBName); From b6aea3b284f04faf2d4e9026798a2b1c696859a8 Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Fri, 3 Apr 2020 11:40:32 -0400 Subject: [PATCH 11/17] refactor(validator): fix npe --- src/main/java/com/conveyal/gtfs/loader/Feed.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/conveyal/gtfs/loader/Feed.java b/src/main/java/com/conveyal/gtfs/loader/Feed.java index 80bbb7146..46b294b40 100644 --- a/src/main/java/com/conveyal/gtfs/loader/Feed.java +++ b/src/main/java/com/conveyal/gtfs/loader/Feed.java @@ -95,7 +95,7 @@ public ValidationResult validate (FeedValidatorCreator... additionalValidators) ); // Create additional validators specified in this method's args and add to list of feed validators to run. for (FeedValidatorCreator creator : additionalValidators) { - feedValidators.add(creator.create(this, errorStorage)); + if (creator != null) feedValidators.add(creator.create(this, errorStorage)); } for (FeedValidator feedValidator : feedValidators) { From fd249dab655a4335fe524d69a6abea533834b1c5 Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Fri, 3 Apr 2020 15:12:34 -0400 Subject: [PATCH 12/17] refactor: fix build/imports --- src/main/java/com/conveyal/gtfs/GTFS.java | 5 ----- src/test/java/com/conveyal/gtfs/GTFSTest.java | 6 ------ 2 files changed, 11 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/GTFS.java b/src/main/java/com/conveyal/gtfs/GTFS.java index c3e206d16..59e5ec83d 100644 --- a/src/main/java/com/conveyal/gtfs/GTFS.java +++ b/src/main/java/com/conveyal/gtfs/GTFS.java @@ -1,6 +1,5 @@ package com.conveyal.gtfs; -import com.conveyal.gtfs.error.SQLErrorStorage; import com.conveyal.gtfs.loader.Feed; import com.conveyal.gtfs.loader.FeedLoadResult; import com.conveyal.gtfs.loader.JdbcGtfsExporter; @@ -8,11 +7,7 @@ import com.conveyal.gtfs.loader.JdbcGtfsSnapshotter; import com.conveyal.gtfs.loader.SnapshotResult; import com.conveyal.gtfs.util.InvalidNamespaceException; -<<<<<<< HEAD import com.conveyal.gtfs.validator.FeedValidatorCreator; -======= -import com.conveyal.gtfs.validator.FeedValidator; ->>>>>>> field-length-validator import com.conveyal.gtfs.validator.ValidationResult; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.io.Files; diff --git a/src/test/java/com/conveyal/gtfs/GTFSTest.java b/src/test/java/com/conveyal/gtfs/GTFSTest.java index 0965f00b2..b7a22c5e2 100644 --- a/src/test/java/com/conveyal/gtfs/GTFSTest.java +++ b/src/test/java/com/conveyal/gtfs/GTFSTest.java @@ -2,8 +2,6 @@ import com.conveyal.gtfs.error.NewGTFSErrorType; -import com.conveyal.gtfs.error.SQLErrorStorage; -import com.conveyal.gtfs.loader.Feed; import com.conveyal.gtfs.loader.FeedLoadResult; import com.conveyal.gtfs.loader.SnapshotResult; import com.conveyal.gtfs.storage.ErrorExpectation; @@ -12,7 +10,6 @@ import com.conveyal.gtfs.storage.RecordExpectation; import com.conveyal.gtfs.util.InvalidNamespaceException; import com.conveyal.gtfs.validator.FeedValidatorCreator; -import com.conveyal.gtfs.validator.FeedValidator; import com.conveyal.gtfs.validator.MTCValidator; import com.conveyal.gtfs.validator.ValidationResult; import com.csvreader.CsvReader; @@ -38,12 +35,9 @@ import java.sql.Connection; import java.sql.ResultSet; import java.sql.SQLException; -import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Iterator; -import java.util.List; -import java.util.function.BiFunction; import java.util.zip.ZipEntry; import java.util.zip.ZipFile; From 28b1ddae631b9f41a189739c99414dbb6702886d Mon Sep 17 00:00:00 2001 From: binh-dam-ibigroup <56846598+binh-dam-ibigroup@users.noreply.github.com> Date: Fri, 3 Apr 2020 15:31:33 -0400 Subject: [PATCH 13/17] refactor(GTFSTest): Use FeedValidatorCreator... syntax to remove test method overrides. --- src/test/java/com/conveyal/gtfs/GTFSTest.java | 44 ++----------------- 1 file changed, 4 insertions(+), 40 deletions(-) diff --git a/src/test/java/com/conveyal/gtfs/GTFSTest.java b/src/test/java/com/conveyal/gtfs/GTFSTest.java index b7a22c5e2..a3da82bf1 100644 --- a/src/test/java/com/conveyal/gtfs/GTFSTest.java +++ b/src/test/java/com/conveyal/gtfs/GTFSTest.java @@ -331,24 +331,6 @@ public void canLoadFeedWithLongFieldValues () { ); } - /** - * Shorthand for next method. - */ - private boolean runIntegrationTestOnFolder( - String folderName, - Matcher fatalExceptionExpectation, - PersistenceExpectation[] persistenceExpectations, - ErrorExpectation[] errorExpectations - ) { - return runIntegrationTestOnFolder( - folderName, - fatalExceptionExpectation, - persistenceExpectations, - errorExpectations, - null - ); - } - /** * A helper method that will zip a specified folder in test/main/resources and call * {@link #runIntegrationTestOnZipFile} on that file. @@ -358,7 +340,7 @@ private boolean runIntegrationTestOnFolder( Matcher fatalExceptionExpectation, PersistenceExpectation[] persistenceExpectations, ErrorExpectation[] errorExpectations, - FeedValidatorCreator customValidator + FeedValidatorCreator... customValidators ) { LOG.info("Running integration test on folder {}", folderName); // zip up test folder into temp zip file @@ -374,25 +356,7 @@ private boolean runIntegrationTestOnFolder( fatalExceptionExpectation, persistenceExpectations, errorExpectations, - customValidator - ); - } - - /** - * Shorthand for next method. - */ - private boolean runIntegrationTestOnZipFile( - String zipFileName, - Matcher fatalExceptionExpectation, - PersistenceExpectation[] persistenceExpectations, - ErrorExpectation[] errorExpectations - ) { - return runIntegrationTestOnZipFile( - zipFileName, - fatalExceptionExpectation, - persistenceExpectations, - errorExpectations, - null + customValidators ); } @@ -410,7 +374,7 @@ private boolean runIntegrationTestOnZipFile( Matcher fatalExceptionExpectation, PersistenceExpectation[] persistenceExpectations, ErrorExpectation[] errorExpectations, - FeedValidatorCreator customValidator + FeedValidatorCreator... customValidators ) { String testDBName = TestUtils.generateNewDB(); String dbConnectionUrl = String.join("/", JDBC_URL, testDBName); @@ -427,7 +391,7 @@ private boolean runIntegrationTestOnZipFile( // load and validate feed LOG.info("load and validate GTFS file {}", zipFileName); FeedLoadResult loadResult = GTFS.load(zipFileName, dataSource); - ValidationResult validationResult = GTFS.validate(loadResult.uniqueIdentifier, dataSource, customValidator); + ValidationResult validationResult = GTFS.validate(loadResult.uniqueIdentifier, dataSource, customValidators); assertThat(validationResult.fatalException, is(fatalExceptionExpectation)); namespace = loadResult.uniqueIdentifier; From 707a119f1791a21357cc3e63b00a44f0ec8f3a77 Mon Sep 17 00:00:00 2001 From: binh-dam-ibigroup <56846598+binh-dam-ibigroup@users.noreply.github.com> Date: Mon, 6 Apr 2020 10:34:44 -0400 Subject: [PATCH 14/17] refactor(MTCValidator): Rename validation method + add overload, adjust comments. --- .../conveyal/gtfs/error/NewGTFSErrorType.java | 1 + .../conveyal/gtfs/validator/MTCValidator.java | 39 +++++++++++-------- .../gtfs/validator/MTCValidatorTest.java | 8 ++-- 3 files changed, 28 insertions(+), 20 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java b/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java index 2fd516d4d..458481808 100644 --- a/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java +++ b/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java @@ -11,6 +11,7 @@ public enum NewGTFSErrorType { URL_FORMAT(Priority.MEDIUM, "URL format should be ://?#"), LANGUAGE_FORMAT(Priority.LOW, "Language should be specified with a valid BCP47 tag."), ILLEGAL_FIELD_VALUE(Priority.MEDIUM, "Fields may not contain tabs, carriage returns or new lines."), + // The error type below is an MTC-specific requirement. FIELD_VALUE_TOO_LONG(Priority.MEDIUM, "Field value has too many characters."), INTEGER_FORMAT(Priority.MEDIUM, "Incorrect integer format."), FARE_TRANSFER_MISMATCH(Priority.MEDIUM, "A fare that does not permit transfers has a non-zero transfer duration."), diff --git a/src/main/java/com/conveyal/gtfs/validator/MTCValidator.java b/src/main/java/com/conveyal/gtfs/validator/MTCValidator.java index 710ed671d..476bf3517 100644 --- a/src/main/java/com/conveyal/gtfs/validator/MTCValidator.java +++ b/src/main/java/com/conveyal/gtfs/validator/MTCValidator.java @@ -4,12 +4,18 @@ import com.conveyal.gtfs.loader.Feed; import com.conveyal.gtfs.model.*; +import java.net.URL; + import static com.conveyal.gtfs.error.NewGTFSErrorType.FIELD_VALUE_TOO_LONG; /** - * MTCValidator checks in a GTFS feed that the length of certain field values - * do not exceed the 511 MTC guidelines. (TODO: add guidelines URL.) - * To refer to specific limits, search the guidelines for the word 'character'. + * MTCValidator runs a set of custom validation checks for GTFS feeds managed by MTC in Data Tools. + * The checks consist of validating field lengths at this time per the 511 MTC guidelines at + * https://github.com/ibi-group/datatools-ui/files/4438625/511.Transit_Data.Guidelines_V2.0_3-27-2020.pdf. + * For specific field lengths, search the guidelines for the word 'character'. + * + * Note that other validations, e.g. on GTFS+ files, are discussed in + * https://github.com/ibi-group/datatools-ui/issues/544. */ public class MTCValidator extends FeedValidator { @@ -20,37 +26,38 @@ public MTCValidator(Feed feed, SQLErrorStorage errorStorage) { @Override public void validate() { for (Agency agency : feed.agencies) { - fieldLengthShouldNotExceed(agency, agency.agency_id, 50); - fieldLengthShouldNotExceed(agency, agency.agency_name, 50); - fieldLengthShouldNotExceed(agency, agency.agency_url, 500); + validateFieldLength(agency, agency.agency_id, 50); + validateFieldLength(agency, agency.agency_name, 50); + validateFieldLength(agency, agency.agency_url, 500); } for (Stop stop : feed.stops) { - fieldLengthShouldNotExceed(stop, stop.stop_name, 100); + validateFieldLength(stop, stop.stop_name, 100); } for (Trip trip : feed.trips) { - fieldLengthShouldNotExceed(trip, trip.trip_headsign, 120); - fieldLengthShouldNotExceed(trip, trip.trip_short_name, 50); + validateFieldLength(trip, trip.trip_headsign, 120); + validateFieldLength(trip, trip.trip_short_name, 50); } - - // TODO: Handle calendar_attributes.txt? } /** - * Checks that the length of a string (or Object.toString()) does not exceed a length. + * Checks that the length of a string does not exceed a certain length. * Reports an error if the length is exceeded. * @param entity The containing GTFS entity (for error reporting purposes). - * @param objValue The value to check. + * @param value The String value to check. * @param maxLength The length to check, should be positive or zero. - * @return true if the length of objValue.toString() is maxLength or less or if objValue is null; false otherwise. + * @return true if value.length() is maxLength or less, or if value is null; false otherwise. */ - public boolean fieldLengthShouldNotExceed(Entity entity, Object objValue, int maxLength) { - String value = objValue != null ? objValue.toString() : ""; + public boolean validateFieldLength(Entity entity, String value, int maxLength) { if (value.length() > maxLength) { if (errorStorage != null) registerError(entity, FIELD_VALUE_TOO_LONG, "[over " + maxLength + " characters] " + value); return false; } return true; } + + public boolean validateFieldLength(Entity entity, URL url, int maxLength) { + return validateFieldLength(entity, url != null ? url.toString() : "", maxLength); + } } diff --git a/src/test/java/com/conveyal/gtfs/validator/MTCValidatorTest.java b/src/test/java/com/conveyal/gtfs/validator/MTCValidatorTest.java index ff2f18cde..4cd33521c 100644 --- a/src/test/java/com/conveyal/gtfs/validator/MTCValidatorTest.java +++ b/src/test/java/com/conveyal/gtfs/validator/MTCValidatorTest.java @@ -12,8 +12,8 @@ public class MTCValidatorTest { @Test public void fieldlLengthShouldNotExceed() { MTCValidator validator = new MTCValidator(null, null); - assertThat(validator.fieldLengthShouldNotExceed(null, "abcdefghijklmnopqrstwxyz1234567890", 20), is(false)); - assertThat(validator.fieldLengthShouldNotExceed(null, "abcdef", 20), is(true)); + assertThat(validator.validateFieldLength(null, "abcdefghijklmnopqrstwxyz1234567890", 20), is(false)); + assertThat(validator.validateFieldLength(null, "abcdef", 20), is(true)); } @Test @@ -22,7 +22,7 @@ public void fieldlLengthShouldNotExceed_usingObject() throws MalformedURLExcepti // You can also pass objects, in that case it will use toString(). URL url = new URL("http://www.gtfs.org"); - assertThat(validator.fieldLengthShouldNotExceed(null, url, 10), is(false)); - assertThat(validator.fieldLengthShouldNotExceed(null, url, 30), is(true)); + assertThat(validator.validateFieldLength(null, url, 10), is(false)); + assertThat(validator.validateFieldLength(null, url, 30), is(true)); } } From 7e97461985b53b2e9431f880ee7d3eb5f5308309 Mon Sep 17 00:00:00 2001 From: binh-dam-ibigroup <56846598+binh-dam-ibigroup@users.noreply.github.com> Date: Mon, 6 Apr 2020 12:19:05 -0400 Subject: [PATCH 15/17] fix(tests): Fix tests --- .../conveyal/gtfs/error/NewGTFSErrorType.java | 1 + .../conveyal/gtfs/validator/MTCValidator.java | 27 +++++++----- src/test/java/com/conveyal/gtfs/GTFSTest.java | 44 ++----------------- 3 files changed, 22 insertions(+), 50 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java b/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java index 2fd516d4d..458481808 100644 --- a/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java +++ b/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java @@ -11,6 +11,7 @@ public enum NewGTFSErrorType { URL_FORMAT(Priority.MEDIUM, "URL format should be ://?#"), LANGUAGE_FORMAT(Priority.LOW, "Language should be specified with a valid BCP47 tag."), ILLEGAL_FIELD_VALUE(Priority.MEDIUM, "Fields may not contain tabs, carriage returns or new lines."), + // The error type below is an MTC-specific requirement. FIELD_VALUE_TOO_LONG(Priority.MEDIUM, "Field value has too many characters."), INTEGER_FORMAT(Priority.MEDIUM, "Incorrect integer format."), FARE_TRANSFER_MISMATCH(Priority.MEDIUM, "A fare that does not permit transfers has a non-zero transfer duration."), diff --git a/src/main/java/com/conveyal/gtfs/validator/MTCValidator.java b/src/main/java/com/conveyal/gtfs/validator/MTCValidator.java index 710ed671d..56fce0567 100644 --- a/src/main/java/com/conveyal/gtfs/validator/MTCValidator.java +++ b/src/main/java/com/conveyal/gtfs/validator/MTCValidator.java @@ -4,12 +4,18 @@ import com.conveyal.gtfs.loader.Feed; import com.conveyal.gtfs.model.*; +import java.net.URL; + import static com.conveyal.gtfs.error.NewGTFSErrorType.FIELD_VALUE_TOO_LONG; /** - * MTCValidator checks in a GTFS feed that the length of certain field values - * do not exceed the 511 MTC guidelines. (TODO: add guidelines URL.) - * To refer to specific limits, search the guidelines for the word 'character'. + * MTCValidator runs a set of custom validation checks for GTFS feeds managed by MTC in Data Tools. + * The checks consist of validating field lengths at this time per the 511 MTC guidelines at + * https://github.com/ibi-group/datatools-ui/files/4438625/511.Transit_Data.Guidelines_V2.0_3-27-2020.pdf. + * For specific field lengths, search the guidelines for the word 'character'. + * + * Note that other validations, e.g. on GTFS+ files, are discussed in + * https://github.com/ibi-group/datatools-ui/issues/544. */ public class MTCValidator extends FeedValidator { @@ -33,24 +39,25 @@ public void validate() { fieldLengthShouldNotExceed(trip, trip.trip_headsign, 120); fieldLengthShouldNotExceed(trip, trip.trip_short_name, 50); } - - // TODO: Handle calendar_attributes.txt? } /** - * Checks that the length of a string (or Object.toString()) does not exceed a length. + * Checks that the length of a string does not exceed a certain length. * Reports an error if the length is exceeded. * @param entity The containing GTFS entity (for error reporting purposes). - * @param objValue The value to check. + * @param value The String value to check. * @param maxLength The length to check, should be positive or zero. - * @return true if the length of objValue.toString() is maxLength or less or if objValue is null; false otherwise. + * @return true if value.length() is maxLength or less, or if value is null; false otherwise. */ - public boolean fieldLengthShouldNotExceed(Entity entity, Object objValue, int maxLength) { - String value = objValue != null ? objValue.toString() : ""; + public boolean fieldLengthShouldNotExceed(Entity entity, String value, int maxLength) { if (value.length() > maxLength) { if (errorStorage != null) registerError(entity, FIELD_VALUE_TOO_LONG, "[over " + maxLength + " characters] " + value); return false; } return true; } + + public boolean fieldLengthShouldNotExceed(Entity entity, URL url, int maxLength) { + return fieldLengthShouldNotExceed(entity, url != null ? url.toString() : "", maxLength); + } } diff --git a/src/test/java/com/conveyal/gtfs/GTFSTest.java b/src/test/java/com/conveyal/gtfs/GTFSTest.java index b7a22c5e2..a3da82bf1 100644 --- a/src/test/java/com/conveyal/gtfs/GTFSTest.java +++ b/src/test/java/com/conveyal/gtfs/GTFSTest.java @@ -331,24 +331,6 @@ public void canLoadFeedWithLongFieldValues () { ); } - /** - * Shorthand for next method. - */ - private boolean runIntegrationTestOnFolder( - String folderName, - Matcher fatalExceptionExpectation, - PersistenceExpectation[] persistenceExpectations, - ErrorExpectation[] errorExpectations - ) { - return runIntegrationTestOnFolder( - folderName, - fatalExceptionExpectation, - persistenceExpectations, - errorExpectations, - null - ); - } - /** * A helper method that will zip a specified folder in test/main/resources and call * {@link #runIntegrationTestOnZipFile} on that file. @@ -358,7 +340,7 @@ private boolean runIntegrationTestOnFolder( Matcher fatalExceptionExpectation, PersistenceExpectation[] persistenceExpectations, ErrorExpectation[] errorExpectations, - FeedValidatorCreator customValidator + FeedValidatorCreator... customValidators ) { LOG.info("Running integration test on folder {}", folderName); // zip up test folder into temp zip file @@ -374,25 +356,7 @@ private boolean runIntegrationTestOnFolder( fatalExceptionExpectation, persistenceExpectations, errorExpectations, - customValidator - ); - } - - /** - * Shorthand for next method. - */ - private boolean runIntegrationTestOnZipFile( - String zipFileName, - Matcher fatalExceptionExpectation, - PersistenceExpectation[] persistenceExpectations, - ErrorExpectation[] errorExpectations - ) { - return runIntegrationTestOnZipFile( - zipFileName, - fatalExceptionExpectation, - persistenceExpectations, - errorExpectations, - null + customValidators ); } @@ -410,7 +374,7 @@ private boolean runIntegrationTestOnZipFile( Matcher fatalExceptionExpectation, PersistenceExpectation[] persistenceExpectations, ErrorExpectation[] errorExpectations, - FeedValidatorCreator customValidator + FeedValidatorCreator... customValidators ) { String testDBName = TestUtils.generateNewDB(); String dbConnectionUrl = String.join("/", JDBC_URL, testDBName); @@ -427,7 +391,7 @@ private boolean runIntegrationTestOnZipFile( // load and validate feed LOG.info("load and validate GTFS file {}", zipFileName); FeedLoadResult loadResult = GTFS.load(zipFileName, dataSource); - ValidationResult validationResult = GTFS.validate(loadResult.uniqueIdentifier, dataSource, customValidator); + ValidationResult validationResult = GTFS.validate(loadResult.uniqueIdentifier, dataSource, customValidators); assertThat(validationResult.fatalException, is(fatalExceptionExpectation)); namespace = loadResult.uniqueIdentifier; From be1fc6d3d19c63962584681279703c944e8c3438 Mon Sep 17 00:00:00 2001 From: binh-dam-ibigroup <56846598+binh-dam-ibigroup@users.noreply.github.com> Date: Mon, 6 Apr 2020 12:21:14 -0400 Subject: [PATCH 16/17] refactor(MTCValidator): Rename validateFieldLength; Add null check! --- .../conveyal/gtfs/validator/MTCValidator.java | 20 +++++++++---------- .../gtfs/validator/MTCValidatorTest.java | 8 ++++---- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/validator/MTCValidator.java b/src/main/java/com/conveyal/gtfs/validator/MTCValidator.java index 56fce0567..0453c32e0 100644 --- a/src/main/java/com/conveyal/gtfs/validator/MTCValidator.java +++ b/src/main/java/com/conveyal/gtfs/validator/MTCValidator.java @@ -26,18 +26,18 @@ public MTCValidator(Feed feed, SQLErrorStorage errorStorage) { @Override public void validate() { for (Agency agency : feed.agencies) { - fieldLengthShouldNotExceed(agency, agency.agency_id, 50); - fieldLengthShouldNotExceed(agency, agency.agency_name, 50); - fieldLengthShouldNotExceed(agency, agency.agency_url, 500); + validateFieldLength(agency, agency.agency_id, 50); + validateFieldLength(agency, agency.agency_name, 50); + validateFieldLength(agency, agency.agency_url, 500); } for (Stop stop : feed.stops) { - fieldLengthShouldNotExceed(stop, stop.stop_name, 100); + validateFieldLength(stop, stop.stop_name, 100); } for (Trip trip : feed.trips) { - fieldLengthShouldNotExceed(trip, trip.trip_headsign, 120); - fieldLengthShouldNotExceed(trip, trip.trip_short_name, 50); + validateFieldLength(trip, trip.trip_headsign, 120); + validateFieldLength(trip, trip.trip_short_name, 50); } } @@ -49,15 +49,15 @@ public void validate() { * @param maxLength The length to check, should be positive or zero. * @return true if value.length() is maxLength or less, or if value is null; false otherwise. */ - public boolean fieldLengthShouldNotExceed(Entity entity, String value, int maxLength) { - if (value.length() > maxLength) { + public boolean validateFieldLength(Entity entity, String value, int maxLength) { + if (value != null && value.length() > maxLength) { if (errorStorage != null) registerError(entity, FIELD_VALUE_TOO_LONG, "[over " + maxLength + " characters] " + value); return false; } return true; } - public boolean fieldLengthShouldNotExceed(Entity entity, URL url, int maxLength) { - return fieldLengthShouldNotExceed(entity, url != null ? url.toString() : "", maxLength); + public boolean validateFieldLength(Entity entity, URL url, int maxLength) { + return validateFieldLength(entity, url != null ? url.toString() : "", maxLength); } } diff --git a/src/test/java/com/conveyal/gtfs/validator/MTCValidatorTest.java b/src/test/java/com/conveyal/gtfs/validator/MTCValidatorTest.java index ff2f18cde..4cd33521c 100644 --- a/src/test/java/com/conveyal/gtfs/validator/MTCValidatorTest.java +++ b/src/test/java/com/conveyal/gtfs/validator/MTCValidatorTest.java @@ -12,8 +12,8 @@ public class MTCValidatorTest { @Test public void fieldlLengthShouldNotExceed() { MTCValidator validator = new MTCValidator(null, null); - assertThat(validator.fieldLengthShouldNotExceed(null, "abcdefghijklmnopqrstwxyz1234567890", 20), is(false)); - assertThat(validator.fieldLengthShouldNotExceed(null, "abcdef", 20), is(true)); + assertThat(validator.validateFieldLength(null, "abcdefghijklmnopqrstwxyz1234567890", 20), is(false)); + assertThat(validator.validateFieldLength(null, "abcdef", 20), is(true)); } @Test @@ -22,7 +22,7 @@ public void fieldlLengthShouldNotExceed_usingObject() throws MalformedURLExcepti // You can also pass objects, in that case it will use toString(). URL url = new URL("http://www.gtfs.org"); - assertThat(validator.fieldLengthShouldNotExceed(null, url, 10), is(false)); - assertThat(validator.fieldLengthShouldNotExceed(null, url, 30), is(true)); + assertThat(validator.validateFieldLength(null, url, 10), is(false)); + assertThat(validator.validateFieldLength(null, url, 30), is(true)); } } From 12ff728a55cced4ff70e03d22b85008af1dcb4ff Mon Sep 17 00:00:00 2001 From: binh-dam-ibigroup <56846598+binh-dam-ibigroup@users.noreply.github.com> Date: Tue, 7 Apr 2020 10:37:52 -0400 Subject: [PATCH 17/17] refactor(NewGTFSErrorType): Reorganize error types per PR comment. --- .../conveyal/gtfs/error/NewGTFSErrorType.java | 89 ++++++++++--------- 1 file changed, 46 insertions(+), 43 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java b/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java index 458481808..eb12337c2 100644 --- a/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java +++ b/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java @@ -3,78 +3,81 @@ import com.conveyal.gtfs.validator.model.Priority; public enum NewGTFSErrorType { - + // Standard errors. + BOOLEAN_FORMAT(Priority.MEDIUM, "A GTFS boolean field must contain the value 1 or 0."), + COLOR_FORMAT(Priority.MEDIUM, "A color should be specified with six-characters (three two-digit hexadecimal numbers)."), + COLUMN_NAME_UNSAFE(Priority.HIGH, "Column header contains characters not safe in SQL, it was renamed."), + CURRENCY_UNKNOWN(Priority.MEDIUM, "The currency code was not recognized."), DATE_FORMAT(Priority.MEDIUM, "Date format should be YYYYMMDD."), - DATE_RANGE(Priority.MEDIUM, "Date should is extremely far in the future or past."), DATE_NO_SERVICE(Priority.MEDIUM, "No service_ids were active on a date within the range of dates with defined service."), - TIME_FORMAT(Priority.MEDIUM, "Time format should be HH:MM:SS."), - URL_FORMAT(Priority.MEDIUM, "URL format should be ://?#"), - LANGUAGE_FORMAT(Priority.LOW, "Language should be specified with a valid BCP47 tag."), - ILLEGAL_FIELD_VALUE(Priority.MEDIUM, "Fields may not contain tabs, carriage returns or new lines."), - // The error type below is an MTC-specific requirement. - FIELD_VALUE_TOO_LONG(Priority.MEDIUM, "Field value has too many characters."), - INTEGER_FORMAT(Priority.MEDIUM, "Incorrect integer format."), - FARE_TRANSFER_MISMATCH(Priority.MEDIUM, "A fare that does not permit transfers has a non-zero transfer duration."), - FREQUENCY_PERIOD_OVERLAP(Priority.MEDIUM, "A frequency for a trip overlaps with another frequency defined for the same trip."), - FLOATING_FORMAT(Priority.MEDIUM, "Incorrect floating point number format."), - COLUMN_NAME_UNSAFE(Priority.HIGH, "Column header contains characters not safe in SQL, it was renamed."), - NUMBER_PARSING(Priority.MEDIUM, "Unable to parse number from value."), - NUMBER_NEGATIVE(Priority.MEDIUM, "Number was expected to be non-negative."), - NUMBER_TOO_SMALL(Priority.MEDIUM, "Number was below the allowed range."), - NUMBER_TOO_LARGE(Priority.MEDIUM, "Number was above the allowed range."), + DATE_RANGE(Priority.MEDIUM, "Date should is extremely far in the future or past."), + DEPARTURE_BEFORE_ARRIVAL(Priority.MEDIUM, "The vehicle departs from this stop before it arrives."), + DUPLICATE_HEADER(Priority.MEDIUM, "More than one column in a table had the same name in the header row."), DUPLICATE_ID(Priority.MEDIUM, "More than one entity in a table had the same ID."), - DUPLICATE_TRIP(Priority.MEDIUM, "More than one trip had an identical schedule and stops."), DUPLICATE_STOP(Priority.MEDIUM, "More than one stop was located in exactly the same place."), - DUPLICATE_HEADER(Priority.MEDIUM, "More than one column in a table had the same name in the header row."), - MISSING_TABLE(Priority.MEDIUM, "This table is required by the GTFS specification but is missing."), + DUPLICATE_TRIP(Priority.MEDIUM, "More than one trip had an identical schedule and stops."), + FARE_TRANSFER_MISMATCH(Priority.MEDIUM, "A fare that does not permit transfers has a non-zero transfer duration."), + FEED_TRAVEL_TIMES_ROUNDED(Priority.LOW, "All travel times in the feed are rounded to the minute, which may cause unexpected results in routing applications where travel times are zero."), + FLOATING_FORMAT(Priority.MEDIUM, "Incorrect floating point number format."), + FREQUENCY_PERIOD_OVERLAP(Priority.MEDIUM, "A frequency for a trip overlaps with another frequency defined for the same trip."), + ILLEGAL_FIELD_VALUE(Priority.MEDIUM, "Fields may not contain tabs, carriage returns or new lines."), + INTEGER_FORMAT(Priority.MEDIUM, "Incorrect integer format."), + LANGUAGE_FORMAT(Priority.LOW, "Language should be specified with a valid BCP47 tag."), + MISSING_ARRIVAL_OR_DEPARTURE(Priority.MEDIUM, "First and last stop times are required to have both an arrival and departure time."), MISSING_COLUMN(Priority.MEDIUM, "A required column was missing from a table."), - MISSING_SHAPE(Priority.MEDIUM, "???"), MISSING_FIELD(Priority.MEDIUM, "A required field was missing or empty in a particular row."), + MISSING_SHAPE(Priority.MEDIUM, "???"), + MISSING_TABLE(Priority.MEDIUM, "This table is required by the GTFS specification but is missing."), MULTIPLE_SHAPES_FOR_PATTERN(Priority.MEDIUM, "Multiple shapes found for a single unique sequence of stops (i.e, trip pattern)."), - WRONG_NUMBER_OF_FIELDS(Priority.MEDIUM, "A row did not have the same number of fields as there are headers in its table."), NO_SERVICE(Priority.HIGH, "There is no service defined on any day in this feed."), + NUMBER_NEGATIVE(Priority.MEDIUM, "Number was expected to be non-negative."), + NUMBER_PARSING(Priority.MEDIUM, "Unable to parse number from value."), + NUMBER_TOO_LARGE(Priority.MEDIUM, "Number was above the allowed range."), + NUMBER_TOO_SMALL(Priority.MEDIUM, "Number was below the allowed range."), OVERLAPPING_TRIP(Priority.MEDIUM, "Blocks?"), - SHAPE_REVERSED(Priority.MEDIUM, "A shape appears to be intended for vehicles running the opposite direction on the route."), - SHAPE_MISSING_COORDINATE(Priority.MEDIUM, "???"), - TABLE_IN_SUBDIRECTORY(Priority.HIGH, "Rather than being at the root of the zip file, a table was nested in a subdirectory."), - TABLE_MISSING_COLUMN_HEADERS(Priority.HIGH, "Table is missing column headers."), - TABLE_TOO_LONG(Priority.MEDIUM, "Table is too long to record line numbers with a 32-bit integer, overflow will occur."), - TIME_ZONE_FORMAT(Priority.MEDIUM, "Time zone format should match value from the Time Zone Database https://en.wikipedia.org/wiki/List_of_tz_database_time_zones."), + REFERENTIAL_INTEGRITY(Priority.HIGH, "This line references an ID that does not exist in the target table."), REQUIRED_TABLE_EMPTY(Priority.MEDIUM, "This table is required by the GTFS specification but is empty."), - FEED_TRAVEL_TIMES_ROUNDED(Priority.LOW, "All travel times in the feed are rounded to the minute, which may cause unexpected results in routing applications where travel times are zero."), ROUTE_DESCRIPTION_SAME_AS_NAME(Priority.LOW, "The description of a route is identical to its name, so does not add any information."), ROUTE_LONG_NAME_CONTAINS_SHORT_NAME(Priority.LOW, "The long name of a route should complement the short name, not include it."), ROUTE_SHORT_AND_LONG_NAME_MISSING(Priority.MEDIUM, "A route has neither a long nor a short name."), ROUTE_SHORT_NAME_TOO_LONG(Priority.MEDIUM, "The short name of a route is too long for display in standard GTFS consumer applications."), + 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."), 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."), STOP_DESCRIPTION_SAME_AS_NAME(Priority.LOW, "The description of a stop is identical to its name, so does not add any information."), + STOP_GEOGRAPHIC_OUTLIER(Priority.HIGH, "This stop is located very far from the middle 90% of stops in this feed."), STOP_LOW_POPULATION_DENSITY(Priority.HIGH, "A stop is located in a geographic area with very low human population density."), STOP_NAME_MISSING(Priority.MEDIUM, "A stop does not have a name."), - STOP_GEOGRAPHIC_OUTLIER(Priority.HIGH, "This stop is located very far from the middle 90% of stops in this feed."), STOP_TIME_UNUSED(Priority.LOW, "This stop time allows neither pickup nor drop off and is not a timepoint, so it serves no purpose and should be removed from trip."), STOP_UNUSED(Priority.MEDIUM, "This stop is not referenced by any trips."), + TABLE_IN_SUBDIRECTORY(Priority.HIGH, "Rather than being at the root of the zip file, a table was nested in a subdirectory."), + TABLE_MISSING_COLUMN_HEADERS(Priority.HIGH, "Table is missing column headers."), + TABLE_TOO_LONG(Priority.MEDIUM, "Table is too long to record line numbers with a 32-bit integer, overflow will occur."), + TIME_FORMAT(Priority.MEDIUM, "Time format should be HH:MM:SS."), + TIME_ZONE_FORMAT(Priority.MEDIUM, "Time zone format should match value from the Time Zone Database https://en.wikipedia.org/wiki/List_of_tz_database_time_zones."), TIMEPOINT_MISSING_TIMES(Priority.MEDIUM, "This stop time is marked as a timepoint, but is missing both arrival and departure times."), + TRAVEL_DISTANCE_ZERO(Priority.MEDIUM, "The vehicle does not cover any distance between the last stop and this one."), + TRAVEL_TIME_NEGATIVE(Priority.HIGH, "The vehicle arrives at this stop before it departs from the previous one."), + TRAVEL_TIME_ZERO(Priority.HIGH, "The vehicle arrives at this stop at the same time it departs from the previous stop."), + TRAVEL_TOO_FAST(Priority.MEDIUM, "The vehicle travels extremely fast to reach this stop from the previous one."), + TRAVEL_TOO_SLOW(Priority.MEDIUM, "The vehicle is traveling very slowly to reach this stop from the previous one."), TRIP_EMPTY(Priority.HIGH, "This trip is defined but has no stop times."), TRIP_HEADSIGN_CONTAINS_ROUTE_NAME(Priority.LOW, "A trip headsign contains the route name, but should only contain information to distinguish it from other trips for the route."), TRIP_HEADSIGN_SHOULD_DESCRIBE_DESTINATION_OR_WAYPOINTS(Priority.LOW, "A trip headsign begins with 'to' or 'towards', but should begin with destination or direction and optionally include waypoints with 'via'"), TRIP_NEVER_ACTIVE(Priority.MEDIUM, "A trip is defined, but its service is never running on any date."), - ROUTE_UNUSED(Priority.HIGH, "This route is defined but has no trips."), - TRAVEL_DISTANCE_ZERO(Priority.MEDIUM, "The vehicle does not cover any distance between the last stop and this one."), - TRAVEL_TIME_NEGATIVE(Priority.HIGH, "The vehicle arrives at this stop before it departs from the previous one."), - TRAVEL_TIME_ZERO(Priority.HIGH, "The vehicle arrives at this stop at the same time it departs from the previous stop."), - MISSING_ARRIVAL_OR_DEPARTURE(Priority.MEDIUM, "First and last stop times are required to have both an arrival and departure time."), - TRIP_TOO_FEW_STOP_TIMES(Priority.MEDIUM, "A trip must have at least two stop times to represent travel."), TRIP_OVERLAP_IN_BLOCK(Priority.MEDIUM, "A trip overlaps another trip and shares the same block_id."), - TRAVEL_TOO_SLOW(Priority.MEDIUM, "The vehicle is traveling very slowly to reach this stop from the previous one."), - TRAVEL_TOO_FAST(Priority.MEDIUM, "The vehicle travels extremely fast to reach this stop from the previous one."), + TRIP_TOO_FEW_STOP_TIMES(Priority.MEDIUM, "A trip must have at least two stop times to represent travel."), + URL_FORMAT(Priority.MEDIUM, "URL format should be ://?#"), VALIDATOR_FAILED(Priority.HIGH, "The specified validation stage failed due to an error encountered during loading. This is likely due to an error encountered during loading (e.g., a date or number field is formatted incorrectly.)."), - DEPARTURE_BEFORE_ARRIVAL(Priority.MEDIUM, "The vehicle departs from this stop before it arrives."), - REFERENTIAL_INTEGRITY(Priority.HIGH, "This line references an ID that does not exist in the target table."), - BOOLEAN_FORMAT(Priority.MEDIUM, "A GTFS boolean field must contain the value 1 or 0."), - COLOR_FORMAT(Priority.MEDIUM, "A color should be specified with six-characters (three two-digit hexadecimal numbers)."), - CURRENCY_UNKNOWN(Priority.MEDIUM, "The currency code was not recognized."), + WRONG_NUMBER_OF_FIELDS(Priority.MEDIUM, "A row did not have the same number of fields as there are headers in its table."), + + // MTC-specific errors. + FIELD_VALUE_TOO_LONG(Priority.MEDIUM, "Field value has too many characters."), + + // Unknown errors. OTHER(Priority.LOW, "Other errors."); public final Priority priority;