diff --git a/src/main/java/com/conveyal/datatools/manager/jobs/ProcessSingleFeedJob.java b/src/main/java/com/conveyal/datatools/manager/jobs/ProcessSingleFeedJob.java index f26eba655..63bbcfedb 100644 --- a/src/main/java/com/conveyal/datatools/manager/jobs/ProcessSingleFeedJob.java +++ b/src/main/java/com/conveyal/datatools/manager/jobs/ProcessSingleFeedJob.java @@ -13,6 +13,7 @@ import com.conveyal.datatools.manager.models.transform.FeedTransformRules; import com.conveyal.datatools.manager.models.transform.FeedTransformZipTarget; import com.conveyal.datatools.manager.models.transform.ZipTransformation; +import com.conveyal.gtfs.validator.ValidationResult; import com.fasterxml.jackson.annotation.JsonIgnore; import com.fasterxml.jackson.annotation.JsonProperty; import org.slf4j.Logger; @@ -198,15 +199,20 @@ private String getErrorReasonMessage() { public String getNotificationMessage() { StringBuilder message = new StringBuilder(); if (!status.error) { - message.append(String.format("New feed version created for %s (valid from %s - %s). ", - feedSource.name, - feedVersion.validationResult.firstCalendarDate, - feedVersion.validationResult.lastCalendarDate)); - if (feedVersion.validationResult.errorCount > 0) { - message.append(String.format("During validation, we found %s issue(s)", - feedVersion.validationResult.errorCount)); + ValidationResult validationResult = feedVersion.validationResult; + if (validationResult != null) { + message.append(String.format("New feed version created for %s (valid from %s - %s).", + feedSource.name, + validationResult.firstCalendarDate, + validationResult.lastCalendarDate + )); + if (validationResult.errorCount > 0) { + message.append(String.format(" During validation, we found %s issue(s)", validationResult.errorCount)); + } else { + message.append(" The validation check found no issues with this new dataset!"); + } } else { - message.append("The validation check found no issues with this new dataset!"); + message.append(String.format("New feed version created for %s.", feedSource.name)); } } else { // Processing did not complete. Depending on which sub-task this occurred in, diff --git a/src/main/java/com/conveyal/datatools/manager/models/FeedVersion.java b/src/main/java/com/conveyal/datatools/manager/models/FeedVersion.java index c985170cf..a69250cd0 100644 --- a/src/main/java/com/conveyal/datatools/manager/models/FeedVersion.java +++ b/src/main/java/com/conveyal/datatools/manager/models/FeedVersion.java @@ -582,7 +582,9 @@ public void delete() { feedStore.deleteFeed(id); // Delete feed version tables in GTFS database - GTFS.delete(this.namespace, DataManager.GTFS_DATA_SOURCE); + if (this.namespace != null) { + GTFS.delete(this.namespace, DataManager.GTFS_DATA_SOURCE); + } LOG.info("Dropped version's GTFS tables from Postgres."); // Remove this FeedVersion from all Deployments associated with this FeedVersion's FeedSource's Project // TODO TEST THOROUGHLY THAT THIS UPDATE EXPRESSION IS CORRECT diff --git a/src/main/java/com/conveyal/datatools/manager/models/transform/FeedTransformation.java b/src/main/java/com/conveyal/datatools/manager/models/transform/FeedTransformation.java index e63001b37..b7a275cf1 100644 --- a/src/main/java/com/conveyal/datatools/manager/models/transform/FeedTransformation.java +++ b/src/main/java/com/conveyal/datatools/manager/models/transform/FeedTransformation.java @@ -2,6 +2,7 @@ import com.conveyal.datatools.common.status.MonitorableJob; import com.conveyal.datatools.manager.utils.GtfsUtils; +import com.conveyal.gtfs.loader.Table; import com.fasterxml.jackson.annotation.JsonIgnoreProperties; import com.fasterxml.jackson.annotation.JsonSubTypes; import com.fasterxml.jackson.annotation.JsonTypeInfo; @@ -100,8 +101,16 @@ public void doTransform(FeedTransformTarget target, MonitorableJob.Status status */ protected void validateTableName(MonitorableJob.Status status) { // Validate fields before running transform. - if (GtfsUtils.getGtfsTable(table) == null) { - status.fail("Table must be valid GTFS spec table name (without .txt)."); + if (GtfsUtils.getGtfsTable(table) == null && !Table.LOCATION_GEO_JSON_FILE_NAME.equals(table)) { + status.fail(String.format("Table must be valid GTFS spec table name (without .txt) or %s.", Table.LOCATION_GEO_JSON_FILE_NAME)); } } + + protected String getTableName() { + return Table.LOCATION_GEO_JSON_FILE_NAME.equals(table) ? table : table + ".txt"; + } + + protected String getTableSuffix() { + return Table.LOCATION_GEO_JSON_FILE_NAME.equals(table) ? ".geojson" : ".txt"; + } } diff --git a/src/main/java/com/conveyal/datatools/manager/models/transform/NormalizeFieldTransformation.java b/src/main/java/com/conveyal/datatools/manager/models/transform/NormalizeFieldTransformation.java index e9c4d6ce7..f06840ed8 100644 --- a/src/main/java/com/conveyal/datatools/manager/models/transform/NormalizeFieldTransformation.java +++ b/src/main/java/com/conveyal/datatools/manager/models/transform/NormalizeFieldTransformation.java @@ -179,7 +179,12 @@ public static List getInvalidSubstitutionPatterns(List sub @Override public void transform(FeedTransformZipTarget zipTarget, MonitorableJob.Status status) { - String tableName = table + ".txt"; + if (Table.LOCATION_GEO_JSON_FILE_NAME.equals(table)) { + // It's not possible to select the locations.geojson file from the normalize field transformation list. This + // is here in case that changes. + throw new UnsupportedOperationException("It is not possible to normalize geo json fields."); + } + String tableName = getTableName(); try( // Hold output before writing to ZIP StringWriter stringWriter = new StringWriter(); diff --git a/src/main/java/com/conveyal/datatools/manager/models/transform/PreserveCustomFieldsTransformation.java b/src/main/java/com/conveyal/datatools/manager/models/transform/PreserveCustomFieldsTransformation.java index e247d2db3..5bf92e327 100644 --- a/src/main/java/com/conveyal/datatools/manager/models/transform/PreserveCustomFieldsTransformation.java +++ b/src/main/java/com/conveyal/datatools/manager/models/transform/PreserveCustomFieldsTransformation.java @@ -60,14 +60,14 @@ private static HashMap> createCsvHashMap(CsvMapReade @Override public void transform(FeedTransformZipTarget zipTarget, MonitorableJob.Status status) throws Exception{ - String tableName = table + ".txt"; + String tableName = getTableName(); Path targetZipPath = Paths.get(zipTarget.gtfsFile.getAbsolutePath()); Optional streamResult = Arrays.stream(Table.tablesInOrder) .filter(t -> t.name.equals(table)) .findFirst(); - if (!streamResult.isPresent()) { - throw new Exception(String.format("could not find specTable for table %s", table)); + if (streamResult.isEmpty()) { + throw new IOException(String.format("could not find specTable for table %s", table)); } Table specTable = streamResult.get(); @@ -77,8 +77,8 @@ public void transform(FeedTransformZipTarget zipTarget, MonitorableJob.Status st Path targetTxtFilePath = getTablePathInZip(tableName, targetZipFs); - final File tempFile = File.createTempFile(tableName + "-temp", ".txt"); - File output = File.createTempFile(tableName + "-output-temp", ".txt"); + final File tempFile = File.createTempFile(tableName + "-temp", getTableSuffix()); + File output = File.createTempFile(tableName + "-output-temp", getTableSuffix()); int rowsModified = 0; List customFields; diff --git a/src/main/java/com/conveyal/datatools/manager/models/transform/ReplaceFileFromVersionTransformation.java b/src/main/java/com/conveyal/datatools/manager/models/transform/ReplaceFileFromVersionTransformation.java index 53963fb16..76234ae3e 100644 --- a/src/main/java/com/conveyal/datatools/manager/models/transform/ReplaceFileFromVersionTransformation.java +++ b/src/main/java/com/conveyal/datatools/manager/models/transform/ReplaceFileFromVersionTransformation.java @@ -42,7 +42,7 @@ public void validateParameters(MonitorableJob.Status status) { @Override public void transform(FeedTransformZipTarget zipTarget, MonitorableJob.Status status) { FeedVersion sourceVersion = getSourceVersion(); - String tableName = table + ".txt"; + String tableName = getTableName(); // Run the replace transformation Path sourceZipPath = Paths.get(sourceVersion.retrieveGtfsFile().getAbsolutePath()); try (FileSystem sourceZipFs = FileSystems.newFileSystem(sourceZipPath, (ClassLoader) null)) { diff --git a/src/main/java/com/conveyal/datatools/manager/models/transform/StringTransformation.java b/src/main/java/com/conveyal/datatools/manager/models/transform/StringTransformation.java index c845d4f37..f822eb6b9 100644 --- a/src/main/java/com/conveyal/datatools/manager/models/transform/StringTransformation.java +++ b/src/main/java/com/conveyal/datatools/manager/models/transform/StringTransformation.java @@ -32,7 +32,7 @@ public void validateParameters(MonitorableJob.Status status) { @Override public void transform(FeedTransformZipTarget zipTarget, MonitorableJob.Status status) { - String tableName = table + ".txt"; + String tableName = getTableName(); Path targetZipPath = Paths.get(zipTarget.gtfsFile.getAbsolutePath()); try ( FileSystem targetZipFs = FileSystems.newFileSystem(targetZipPath, (ClassLoader) null); diff --git a/src/test/java/com/conveyal/datatools/manager/jobs/NormalizeFieldTransformJobTest.java b/src/test/java/com/conveyal/datatools/manager/jobs/NormalizeFieldTransformJobTest.java index 6a35edb56..85f8f3599 100644 --- a/src/test/java/com/conveyal/datatools/manager/jobs/NormalizeFieldTransformJobTest.java +++ b/src/test/java/com/conveyal/datatools/manager/jobs/NormalizeFieldTransformJobTest.java @@ -19,12 +19,18 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import java.io.BufferedReader; import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; import java.util.Date; +import java.util.stream.Stream; import java.util.zip.ZipEntry; import java.util.zip.ZipFile; @@ -35,13 +41,11 @@ public class NormalizeFieldTransformJobTest extends DatatoolsTest { - private static final String TABLE_NAME = "routes"; - private static final String FIELD_NAME = "route_long_name"; + private static final Logger LOG = LoggerFactory.getLogger(NormalizeFieldTransformJobTest.class); private static Project project; private static FeedSource feedSource; private FeedVersion targetVersion; - /** * Initialize Data Tools and set up a simple feed source and project. */ @@ -78,42 +82,51 @@ public void tearDownTest() { * Test that a {@link NormalizeFieldTransformation} will successfully complete. * FIXME: On certain Windows machines, this test fails. */ - @Test - public void canNormalizeField() throws IOException { - // Create transform. - // In this test, as an illustration, replace "Route" with the "Rte" abbreviation in routes.txt. - FeedTransformation transformation = NormalizeFieldTransformationTest.createTransformation( - TABLE_NAME, FIELD_NAME, null, Lists.newArrayList( - new Substitution("Route", "Rte") - ) - ); - initializeFeedSource(transformation); + @ParameterizedTest + @MethodSource("createNormalizedFieldCases") + void canNormalizeField(TransformationCase transformationCase) throws IOException { + initializeFeedSource(transformationCase.table, createTransformation(transformationCase)); // Create target version that the transform will operate on. targetVersion = createFeedVersion( feedSource, - zipFolderFiles("fake-agency-with-only-calendar") + zipFolderFiles("fake-agency-for-field-normalizing") ); try (ZipFile zip = new ZipFile(targetVersion.retrieveGtfsFile())) { - // Check that new version has routes table modified. - ZipEntry entry = zip.getEntry(TABLE_NAME + ".txt"); - assertNotNull(entry); - - // Scan the first data row in routes.txt and check that the substitution - // that was defined in setUp was done. - try ( - InputStream stream = zip.getInputStream(entry); - InputStreamReader streamReader = new InputStreamReader(stream); - BufferedReader reader = new BufferedReader(streamReader) - ) { - String[] columns = reader.readLine().split(","); - int fieldIndex = ArrayUtils.indexOf(columns, FIELD_NAME); - - String row1 = reader.readLine(); - String[] row1Fields = row1.split(","); - assertTrue(row1Fields[fieldIndex].startsWith("Rte "), row1); - } + // Check that new version has expected modifications. + checkTableForModification(zip, transformationCase); + } + } + + private static Stream createNormalizedFieldCases() { + return Stream.of( + Arguments.of(new TransformationCase("routes", "route_long_name", "Route", "Rte")), + Arguments.of(new TransformationCase("booking_rules", "pickup_message", "Message", "Msg")), + Arguments.of(new TransformationCase("areas", "area_name", "Area", "Place")) + ); + } + + private void checkTableForModification(ZipFile zip, TransformationCase transformationCase) throws IOException { + String tableName = transformationCase.table + ".txt"; + LOG.info("Getting table {} from zip {}", tableName, zip.getName()); + // Check that the new version has been modified. + ZipEntry entry = zip.getEntry(tableName); + assertNotNull(entry); + + // Scan the first data row and check that the substitution that was defined in the set-up was done. + try ( + InputStream stream = zip.getInputStream(entry); + InputStreamReader streamReader = new InputStreamReader(stream); + BufferedReader reader = new BufferedReader(streamReader) + ) { + String[] columns = reader.readLine().split(","); + int fieldIndex = ArrayUtils.indexOf(columns, transformationCase.fieldName); + + String rowOne = reader.readLine(); + assertNotNull(rowOne, String.format("First row in table %s is null!", transformationCase.table)); + String[] row1Fields = rowOne.split(","); + assertTrue(row1Fields[fieldIndex].contains(transformationCase.replacement), rowOne); } } @@ -121,16 +134,16 @@ public void canNormalizeField() throws IOException { * Test that a {@link NormalizeFieldTransformation} will fail if invalid substitution patterns are provided. */ @Test - public void canHandleInvalidSubstitutionPatterns() throws IOException { + void canHandleInvalidSubstitutionPatterns() throws IOException { // Create transform. // In this test, we provide an invalid pattern '\Cir\b' (instead of '\bCir\b'), // when trying to replace e.g. 'Piedmont Cir' with 'Piedmont Circle'. FeedTransformation transformation = NormalizeFieldTransformationTest.createTransformation( - TABLE_NAME, FIELD_NAME, null, Lists.newArrayList( + "routes", "route_long_name", null, Lists.newArrayList( new Substitution("\\Cir\\b", "Circle") ) ); - initializeFeedSource(transformation); + initializeFeedSource("routes", transformation); // Create target version that the transform will operate on. targetVersion = createFeedVersion( @@ -143,16 +156,27 @@ public void canHandleInvalidSubstitutionPatterns() throws IOException { assertTrue(targetVersion.hasCriticalErrors()); } + private FeedTransformation createTransformation(TransformationCase transformationCase) { + return NormalizeFieldTransformationTest + .createTransformation( + transformationCase.table, + transformationCase.fieldName, + null, + Lists.newArrayList( + new Substitution(transformationCase.pattern, transformationCase.replacement) + ) + ); + } + /** * Create and persist a feed source using the given transformation. */ - private static void initializeFeedSource(FeedTransformation transformation) { - FeedTransformRules transformRules = new FeedTransformRules(transformation); + private void initializeFeedSource(String table, FeedTransformation transformation) { // Create feed source with above transform. - feedSource = new FeedSource("Normalize Field Test Feed", project.id, FeedRetrievalMethod.MANUALLY_UPLOADED); + feedSource = new FeedSource(table + " Normalize Field Test Feed", project.id, FeedRetrievalMethod.MANUALLY_UPLOADED); feedSource.deployable = false; - feedSource.transformRules.add(transformRules); + feedSource.transformRules.add(new FeedTransformRules(transformation)); Persistence.feedSources.create(feedSource); } @@ -160,8 +184,22 @@ private static void initializeFeedSource(FeedTransformation