From 37ca02b43ba557c15253b2bffc97bf7b605767b5 Mon Sep 17 00:00:00 2001 From: Brian Ferris Date: Fri, 10 Feb 2023 16:59:03 -0800 Subject: [PATCH 1/6] Move notice schema scanning into ClassGraphDiscovery for consistency with other ClassGraph methods. --- .../mobilitydata/gtfsvalidator/cli/Main.java | 3 +- .../gtfsvalidator/notice/Notice.java | 9 ++-- .../notice/NoticeSchemaGenerator.java | 47 +++---------------- .../validator/ClassGraphDiscovery.java | 32 +++++++++++++ .../notice/NoticeSchemaGeneratorTest.java | 25 ++-------- .../validator/ClassGraphDiscoveryTest.java | 27 +++++++++++ 6 files changed, 77 insertions(+), 66 deletions(-) create mode 100644 core/src/test/java/org/mobilitydata/gtfsvalidator/validator/ClassGraphDiscoveryTest.java diff --git a/cli/src/main/java/org/mobilitydata/gtfsvalidator/cli/Main.java b/cli/src/main/java/org/mobilitydata/gtfsvalidator/cli/Main.java index ded339e0b8..88cbac8761 100644 --- a/cli/src/main/java/org/mobilitydata/gtfsvalidator/cli/Main.java +++ b/cli/src/main/java/org/mobilitydata/gtfsvalidator/cli/Main.java @@ -28,6 +28,7 @@ import org.mobilitydata.gtfsvalidator.notice.NoticeSchemaGenerator; import org.mobilitydata.gtfsvalidator.runner.ValidationRunner; import org.mobilitydata.gtfsvalidator.util.VersionResolver; +import org.mobilitydata.gtfsvalidator.validator.ClassGraphDiscovery; /** The main entry point for GTFS Validator CLI. */ public class Main { @@ -85,7 +86,7 @@ private static void exportNoticeSchema(final Arguments args) { Paths.get(args.getOutputBase(), NOTICE_SCHEMA_JSON), gson.toJson( NoticeSchemaGenerator.jsonSchemaForPackages( - NoticeSchemaGenerator.DEFAULT_NOTICE_PACKAGES)) + ClassGraphDiscovery.DEFAULT_NOTICE_PACKAGES)) .getBytes(StandardCharsets.UTF_8)); } catch (IOException e) { logger.atSevere().withCause(e).log("Cannot store notice schema file"); diff --git a/core/src/main/java/org/mobilitydata/gtfsvalidator/notice/Notice.java b/core/src/main/java/org/mobilitydata/gtfsvalidator/notice/Notice.java index d4d149c22d..ab8db68cb6 100644 --- a/core/src/main/java/org/mobilitydata/gtfsvalidator/notice/Notice.java +++ b/core/src/main/java/org/mobilitydata/gtfsvalidator/notice/Notice.java @@ -68,17 +68,18 @@ public SeverityLevel getSeverityLevel() { * @return notice code, e.g., "foreign_key_violation". */ public String getCode() { - return getCode(getClass().getSimpleName()); + return getCode(getClass()); } /** - * Returns a descriptive type-specific name for this notice class simple name. + * Returns a descriptive type-specific name for this notice class. * * @return notice code, e.g., "foreign_key_violation". */ - static String getCode(String className) { + public static String getCode(Class noticeClass) { return CaseFormat.UPPER_CAMEL.to( - CaseFormat.LOWER_UNDERSCORE, StringUtils.removeEnd(className, NOTICE_SUFFIX)); + CaseFormat.LOWER_UNDERSCORE, + StringUtils.removeEnd(noticeClass.getSimpleName(), NOTICE_SUFFIX)); } /** diff --git a/core/src/main/java/org/mobilitydata/gtfsvalidator/notice/NoticeSchemaGenerator.java b/core/src/main/java/org/mobilitydata/gtfsvalidator/notice/NoticeSchemaGenerator.java index e80c4ab384..4121887545 100644 --- a/core/src/main/java/org/mobilitydata/gtfsvalidator/notice/NoticeSchemaGenerator.java +++ b/core/src/main/java/org/mobilitydata/gtfsvalidator/notice/NoticeSchemaGenerator.java @@ -17,31 +17,23 @@ package org.mobilitydata.gtfsvalidator.notice; import com.google.common.annotations.VisibleForTesting; -import com.google.common.collect.ImmutableList; import com.google.common.geometry.S2LatLng; import com.google.gson.JsonArray; import com.google.gson.JsonElement; import com.google.gson.JsonObject; import com.google.gson.JsonPrimitive; -import io.github.classgraph.ClassGraph; -import io.github.classgraph.ClassInfo; -import io.github.classgraph.ScanResult; import java.io.IOException; import java.lang.reflect.Field; -import java.util.ArrayList; import java.util.List; import java.util.Map; import java.util.TreeMap; import org.mobilitydata.gtfsvalidator.type.GtfsColor; import org.mobilitydata.gtfsvalidator.type.GtfsDate; import org.mobilitydata.gtfsvalidator.type.GtfsTime; +import org.mobilitydata.gtfsvalidator.validator.ClassGraphDiscovery; /** Exports schema describing all possible notices and their contexts. */ public class NoticeSchemaGenerator { - /** Default packages to find notices in open-source validator. */ - public static final ImmutableList DEFAULT_NOTICE_PACKAGES = - ImmutableList.of( - "org.mobilitydata.gtfsvalidator.notice", "org.mobilitydata.gtfsvalidator.validator"); /** * Exports JSON schema for all notices in given packages. This includes notices that are declared @@ -86,7 +78,7 @@ public class NoticeSchemaGenerator { * } * * @param packages List of packages where notices are declared. Use {@link - * #DEFAULT_NOTICE_PACKAGES} and add your custom Java packages here, if any + * ClassGraphDiscovery#DEFAULT_NOTICE_PACKAGES} and add your custom Java packages here, if any * @return a {@link JsonObject} describing all notices in given packages (see above) * @throws IOException */ @@ -94,8 +86,7 @@ public static JsonObject jsonSchemaForPackages(List packages) throws IOE JsonObject schema = new JsonObject(); for (Map.Entry>> entry : contextFieldsInPackages(packages).entrySet()) { - schema.add( - Notice.getCode(entry.getKey()), jsonSchemaForNotice(entry.getKey(), entry.getValue())); + schema.add(entry.getKey(), jsonSchemaForNotice(entry.getKey(), entry.getValue())); } return schema; } @@ -123,8 +114,9 @@ static Map>> contextFieldsInPackages(List p throws IOException { // Return a sorted TreeMap for stable results. Map>> contextFieldsByNotice = new TreeMap<>(); - for (Class noticeClass : findNoticeSubclasses(packages)) { - contextFieldsByNotice.put(noticeClass.getSimpleName(), contextFieldsForNotice(noticeClass)); + + for (Class noticeClass : ClassGraphDiscovery.discoverNoticeSubclasses(packages)) { + contextFieldsByNotice.put(Notice.getCode(noticeClass), contextFieldsForNotice(noticeClass)); } return contextFieldsByNotice; @@ -144,33 +136,6 @@ private static boolean isSubclassOf(Class parent, Class child) { return !child.equals(parent) && parent.isAssignableFrom(child); } - /** - * Finds all subclasses of {@link Notice} that belong to the given packages. - * - *

This function also dives into validator classes that may contain inner notice classes. - */ - @VisibleForTesting - static List> findNoticeSubclasses(List packages) throws IOException { - String[] packagesAsArray = packages.toArray(new String[] {}); - List> notices = new ArrayList<>(); - ClassGraph classGraph = - new ClassGraph() - .enableClassInfo() - .acceptPackages(packagesAsArray) - .ignoreClassVisibility() - .verbose(); - try (ScanResult scanResult = classGraph.scan()) { - for (ClassInfo classInfo : scanResult.getSubclasses(Notice.class)) { - if (classInfo.isAbstract()) { - continue; - } - Class clazz = classInfo.loadClass(); - notices.add((Class) clazz); - } - } - return notices; - } - private static final class JsonTypes { private static final String NUMBER = "number"; diff --git a/core/src/main/java/org/mobilitydata/gtfsvalidator/validator/ClassGraphDiscovery.java b/core/src/main/java/org/mobilitydata/gtfsvalidator/validator/ClassGraphDiscovery.java index 57ef78e259..f435c3e079 100644 --- a/core/src/main/java/org/mobilitydata/gtfsvalidator/validator/ClassGraphDiscovery.java +++ b/core/src/main/java/org/mobilitydata/gtfsvalidator/validator/ClassGraphDiscovery.java @@ -4,7 +4,9 @@ import io.github.classgraph.ClassGraph; import io.github.classgraph.ClassInfo; import io.github.classgraph.ScanResult; +import java.util.List; import org.mobilitydata.gtfsvalidator.annotation.GtfsValidator; +import org.mobilitydata.gtfsvalidator.notice.Notice; import org.mobilitydata.gtfsvalidator.table.GtfsTableDescriptor; /** Discovers GTFS table descriptor and validator classes in the given Java packages. */ @@ -12,6 +14,10 @@ public class ClassGraphDiscovery { public static final String DEFAULT_VALIDATOR_PACKAGE = "org.mobilitydata.gtfsvalidator.validator"; public static final String DEFAULT_TABLE_PACKAGE = "org.mobilitydata.gtfsvalidator.table"; + /** Default packages to find notices in open-source validator. */ + public static final ImmutableList DEFAULT_NOTICE_PACKAGES = + ImmutableList.of( + "org.mobilitydata.gtfsvalidator.notice", "org.mobilitydata.gtfsvalidator.validator"); private ClassGraphDiscovery() {} @@ -64,4 +70,30 @@ public static ImmutableList> discoverValidators( } return validatorClasses.build(); } + + /** + * Finds all subclasses of {@link Notice} that belong to the given packages. + * + *

This function also dives into validator classes that may contain inner notice classes. + */ + public static ImmutableList> discoverNoticeSubclasses(List packages) { + String[] packagesAsArray = packages.toArray(new String[] {}); + ImmutableList.Builder> notices = ImmutableList.builder(); + ClassGraph classGraph = + new ClassGraph() + .enableClassInfo() + .acceptPackages(packagesAsArray) + .ignoreClassVisibility() + .verbose(); + try (ScanResult scanResult = classGraph.scan()) { + for (ClassInfo classInfo : scanResult.getSubclasses(Notice.class)) { + if (classInfo.isAbstract()) { + continue; + } + Class clazz = classInfo.loadClass(); + notices.add((Class) clazz); + } + } + return notices.build(); + } } diff --git a/core/src/test/java/org/mobilitydata/gtfsvalidator/notice/NoticeSchemaGeneratorTest.java b/core/src/test/java/org/mobilitydata/gtfsvalidator/notice/NoticeSchemaGeneratorTest.java index e870dce223..453bccb89b 100644 --- a/core/src/test/java/org/mobilitydata/gtfsvalidator/notice/NoticeSchemaGeneratorTest.java +++ b/core/src/test/java/org/mobilitydata/gtfsvalidator/notice/NoticeSchemaGeneratorTest.java @@ -25,11 +25,7 @@ import com.google.gson.JsonElement; import java.io.IOException; import org.junit.Test; -import org.mobilitydata.gtfsvalidator.notice.testnotices.DoubleFieldNotice; -import org.mobilitydata.gtfsvalidator.notice.testnotices.GtfsTypesValidationNotice; import org.mobilitydata.gtfsvalidator.notice.testnotices.S2LatLngNotice; -import org.mobilitydata.gtfsvalidator.notice.testnotices.StringFieldNotice; -import org.mobilitydata.gtfsvalidator.notice.testnotices.TestValidator.TestInnerNotice; import org.mobilitydata.gtfsvalidator.type.GtfsColor; import org.mobilitydata.gtfsvalidator.type.GtfsDate; import org.mobilitydata.gtfsvalidator.type.GtfsTime; @@ -39,17 +35,6 @@ public class NoticeSchemaGeneratorTest { private static final String TEST_NOTICES_PACKAGE = "org.mobilitydata.gtfsvalidator.notice.testnotices"; - @Test - public void findNoticeSubclasses() throws IOException { - assertThat(NoticeSchemaGenerator.findNoticeSubclasses(ImmutableList.of(TEST_NOTICES_PACKAGE))) - .containsExactly( - DoubleFieldNotice.class, - TestInnerNotice.class, - GtfsTypesValidationNotice.class, - S2LatLngNotice.class, - StringFieldNotice.class); - } - @Test public void jsonSchemaForPackages_succeeds() throws IOException { assertThat( @@ -64,16 +49,16 @@ public void contextFieldsInPackages_testNotices() throws IOException { NoticeSchemaGenerator.contextFieldsInPackages(ImmutableList.of(TEST_NOTICES_PACKAGE))) .isEqualTo( ImmutableMap.of( - "DoubleFieldNotice", + "double_field", ImmutableMap.of("doubleField", double.class), - "TestInnerNotice", + "test_inner", ImmutableMap.of("intField", int.class), - "GtfsTypesValidationNotice", + "gtfs_types_validation", ImmutableMap.of( "color", GtfsColor.class, "date", GtfsDate.class, "time", GtfsTime.class), - "S2LatLngNotice", + "s2_lat_lng", ImmutableMap.of("point", S2LatLng.class), - "StringFieldNotice", + "string_field", ImmutableMap.of("someField", String.class))); } diff --git a/core/src/test/java/org/mobilitydata/gtfsvalidator/validator/ClassGraphDiscoveryTest.java b/core/src/test/java/org/mobilitydata/gtfsvalidator/validator/ClassGraphDiscoveryTest.java new file mode 100644 index 0000000000..eca5cda4ef --- /dev/null +++ b/core/src/test/java/org/mobilitydata/gtfsvalidator/validator/ClassGraphDiscoveryTest.java @@ -0,0 +1,27 @@ +package org.mobilitydata.gtfsvalidator.validator; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.collect.ImmutableList; +import org.junit.Test; +import org.mobilitydata.gtfsvalidator.notice.testnotices.DoubleFieldNotice; +import org.mobilitydata.gtfsvalidator.notice.testnotices.GtfsTypesValidationNotice; +import org.mobilitydata.gtfsvalidator.notice.testnotices.S2LatLngNotice; +import org.mobilitydata.gtfsvalidator.notice.testnotices.StringFieldNotice; +import org.mobilitydata.gtfsvalidator.notice.testnotices.TestValidator.TestInnerNotice; + +public class ClassGraphDiscoveryTest { + private static final String TEST_NOTICES_PACKAGE = + "org.mobilitydata.gtfsvalidator.notice.testnotices"; + + @Test + public void discoverNoticeSubclasses() { + assertThat(ClassGraphDiscovery.discoverNoticeSubclasses(ImmutableList.of(TEST_NOTICES_PACKAGE))) + .containsExactly( + DoubleFieldNotice.class, + TestInnerNotice.class, + GtfsTypesValidationNotice.class, + S2LatLngNotice.class, + StringFieldNotice.class); + } +} From 831c80aa1326f2e015fb463e0b83febcfc3c0985 Mon Sep 17 00:00:00 2001 From: Brian Ferris Date: Fri, 10 Feb 2023 16:59:34 -0800 Subject: [PATCH 2/6] Add a unit test that verifies that every Notice has an entry in RULES.md. --- RULES.md | 19 ++-- main/build.gradle | 9 +- .../validator/NoticeDocumentationTest.java | 89 +++++++++++++++++++ 3 files changed, 109 insertions(+), 8 deletions(-) create mode 100644 main/src/test/java/org/mobilitydata/gtfsvalidator/validator/NoticeDocumentationTest.java diff --git a/RULES.md b/RULES.md index 91c390f00d..da776fffb7 100644 --- a/RULES.md +++ b/RULES.md @@ -101,9 +101,9 @@ Each Notice is associated with a severity: `INFO`, `WARNING`, `ERROR`. | [`equal_shape_distance_same_coordinates`](#equal_shape_distance_same_coordinates) | Two consecutive points have equal `shape_dist_traveled` and the same lat/lon coordinates in `shapes.txt`. | | [`fast_travel_between_consecutive_stops`](#fast_travel_between_consecutive_stops) | A transit vehicle moves too fast between two consecutive stops. | | [`fast_travel_between_far_stops`](#fast_travel_between_far_stops) | A transit vehicle moves too fast between two far stops. | -| [`feed_expiration_date7_days`](#feed_expiration_date7_days) | Dataset should be valid for at least the next 7 days. | -| [`feed_expiration_date30_days`](#feed_expiration_date30_days) | Dataset should cover at least the next 30 days of service. | -| [`feed_info_lang_and_agency_mismatch`](#feed_info_lang_and_agency_mismatch) | Mismatching feed and agency language fields. | +| [`feed_expiration_date7_days`](#feed_expiration_date7_days) | Dataset should be valid for at least the next 7 days. | +| [`feed_expiration_date30_days`](#feed_expiration_date30_days) | Dataset should cover at least the next 30 days of service. | +| [`feed_info_lang_and_agency_lang_mismatch`](#feed_info_lang_and_agency_lang_mismatch) | Mismatching feed and agency language fields. | | [`inconsistent_agency_lang`](#inconsistent_agency_lang) | Inconsistent language among agencies. | | [`leading_or_trailing_whitespaces`](#leading_or_trailing_whitespaces) | The value in CSV file has leading or trailing whitespaces. | | [`missing_feed_info_date`](#missing_feed_info_date) | `feed_end_date` should be provided if `feed_start_date` is provided. `feed_start_date` should be provided if `feed_end_date` is provided. | @@ -408,7 +408,7 @@ A row from GTFS file `fare_transfer_rules.txt` has a defined `duration_limit_typ -### fare_transfer_rule_duration_limit_without_type +### fare_transfer_rule_duration_limit_without_type A row from GTFS file `fare_transfer_rules.txt` has a defined `duration_limit` field but no `duration_limit_type` specified. @@ -1870,7 +1870,7 @@ At any time, the GTFS dataset should cover at least the next 30 days of service, -### feed_info_lang_and_agency_mismatch +### feed_info_lang_and_agency_lang_mismatch 1. Files `agency.txt` and `feed_info.txt` should define matching `agency.agency_lang` and `feed_info.feed_lang`. The default language may be multilingual for datasets with the original text in multiple languages. In such cases, the feed_lang field should contain the language code mul defined by the norm ISO 639-2. * If `feed_lang` is not `mul` and does not match with `agency_lang`, that's an error @@ -1964,7 +1964,7 @@ Even though `feed_info.start_date` and `feed_info.end_date` are optional, if one - + ### missing_recommended_file @@ -1984,6 +1984,8 @@ A recommended file is missing. + + ### missing_recommended_field The given field has no value in some input row, even though values are recommended. @@ -2004,6 +2006,8 @@ The given field has no value in some input row, even though values are recommend + + ### missing_timepoint_column The `timepoint` column should be provided. @@ -2159,7 +2163,7 @@ A platform has no `parent_station` field set. -#### route_color_contrast +### route_color_contrast A route's color and `route_text_color` should be contrasting. @@ -2676,6 +2680,7 @@ Error in IO operation. | `exception` | The name of the exception. | String | | `message` | The error message that explains the reason for the exception. | String | + ### runtime_exception_in_loader_error diff --git a/main/build.gradle b/main/build.gradle index 51eb7b4bfe..43194c0ce6 100644 --- a/main/build.gradle +++ b/main/build.gradle @@ -58,9 +58,16 @@ dependencies { testImplementation 'org.mockito:mockito-core:4.5.1' } +// A custom task to copy RULES.md into the test resource directory so that we can reference it in +// unit tests. See NoticeDocumentationTest for more details. +tasks.register('copyRulesMarkdown', Copy) { + from "$rootDir/RULES.md" + into "$projectDir/build/resources/test" +} + test { // Always run tests, even when nothing changed. - dependsOn 'cleanTest' + dependsOn 'cleanTest', 'copyRulesMarkdown' // Show test results. testLogging { diff --git a/main/src/test/java/org/mobilitydata/gtfsvalidator/validator/NoticeDocumentationTest.java b/main/src/test/java/org/mobilitydata/gtfsvalidator/validator/NoticeDocumentationTest.java new file mode 100644 index 0000000000..99b941f5eb --- /dev/null +++ b/main/src/test/java/org/mobilitydata/gtfsvalidator/validator/NoticeDocumentationTest.java @@ -0,0 +1,89 @@ +package org.mobilitydata.gtfsvalidator.validator; + +import static com.google.common.truth.Truth.assertThat; + +import java.io.BufferedReader; +import java.io.IOException; +import java.io.InputStream; +import java.io.InputStreamReader; +import java.util.Optional; +import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; +import org.mobilitydata.gtfsvalidator.notice.Notice; + +@RunWith(JUnit4.class) +public class NoticeDocumentationTest { + private static Pattern MARKDOWN_NOTICE_SIMPLE_CLASS_NAME_ANCHOR_PATTERN = + Pattern.compile("^"); + + private static Pattern MARKDOWN_NOTICE_CODE_HEADER_PATTERN = + Pattern.compile("^### (([a-z0-9]+_)*[a-z0-9]+)"); + + /** + * If this test is failing, it likely means you need to update RULES.md in the project root + * directory to include an entry for a new notice. + */ + @Test + public void testThatRulesMarkdownContainsAnchorsForAllValidationNotices() throws IOException { + Set fromMarkdown = readNoticeSimpleClassNamesFromRulesMarkdown(); + Set fromSource = + discoverValidationNoticeClasses().map(Class::getSimpleName).collect(Collectors.toSet()); + + assertThat(fromMarkdown).isEqualTo(fromSource); + } + + /** + * If this test is failing, it likely means you need to update RULES.md in the project root + * directory to include an entry for a new notice. + */ + @Test + public void testThatRulesMarkdownContainsHeadersForAllValidationNotices() throws IOException { + Set fromMarkdown = readNoticeCodesFromRulesMarkdown(); + Set fromSource = + discoverValidationNoticeClasses().map(Notice::getCode).collect(Collectors.toSet()); + + assertThat(fromMarkdown).isEqualTo(fromSource); + } + + private static Stream> discoverValidationNoticeClasses() { + return ClassGraphDiscovery.discoverNoticeSubclasses(ClassGraphDiscovery.DEFAULT_NOTICE_PACKAGES) + .stream(); + } + + private static Set readNoticeSimpleClassNamesFromRulesMarkdown() throws IOException { + return readValuesFromRulesMarkdown(MARKDOWN_NOTICE_SIMPLE_CLASS_NAME_ANCHOR_PATTERN); + } + + private static Set readNoticeCodesFromRulesMarkdown() throws IOException { + return readValuesFromRulesMarkdown(MARKDOWN_NOTICE_CODE_HEADER_PATTERN); + } + + private static Set readValuesFromRulesMarkdown(Pattern pattern) throws IOException { + // RULES.md is copied into the main/build/resources/test resource directory by a custom copy + // rule in the main/build.gradle file. + try (InputStream in = NoticeDocumentationTest.class.getResourceAsStream("/RULES.md")) { + // Scan lines from the markdown file, find those that match our regex pattern, and pull out + // the matching group. + return new BufferedReader(new InputStreamReader(in)) + .lines() + .map(line -> maybeMatchAndExtract(pattern, line)) + .flatMap(Optional::stream) + .collect(Collectors.toSet()); + } + } + + private static Optional maybeMatchAndExtract(Pattern p, String line) { + Matcher m = p.matcher(line); + if (m.matches()) { + return Optional.of(m.group(1)); + } else { + return Optional.empty(); + } + } +} From fb88301ded5fa5f9ba2e499e3113e02b308109ec Mon Sep 17 00:00:00 2001 From: Brian Ferris Date: Thu, 16 Feb 2023 07:58:46 -0800 Subject: [PATCH 3/6] Include doc changes in the test_pack_doc.yml workflow. --- .github/workflows/test_pack_doc.yml | 4 ---- 1 file changed, 4 deletions(-) diff --git a/.github/workflows/test_pack_doc.yml b/.github/workflows/test_pack_doc.yml index a76403e84a..08c5ee20b8 100644 --- a/.github/workflows/test_pack_doc.yml +++ b/.github/workflows/test_pack_doc.yml @@ -3,12 +3,8 @@ name: Test Package Document on: push: branches: [ master ] - paths-ignore: - - '**.md' pull_request: branches: [ master ] - paths-ignore: - - '**.md' release: types: [ prereleased, released ] From 1e7493ba43e915d795092b5e0551f1d9edcd176e Mon Sep 17 00:00:00 2001 From: Brian Ferris Date: Thu, 16 Feb 2023 20:45:08 -0800 Subject: [PATCH 4/6] Update comment to match code. --- .../gtfsvalidator/notice/NoticeSchemaGenerator.java | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/org/mobilitydata/gtfsvalidator/notice/NoticeSchemaGenerator.java b/core/src/main/java/org/mobilitydata/gtfsvalidator/notice/NoticeSchemaGenerator.java index 4121887545..5a7142dec6 100644 --- a/core/src/main/java/org/mobilitydata/gtfsvalidator/notice/NoticeSchemaGenerator.java +++ b/core/src/main/java/org/mobilitydata/gtfsvalidator/notice/NoticeSchemaGenerator.java @@ -98,7 +98,7 @@ public static JsonObject jsonSchemaForPackages(List packages) throws IOE * * 

{@code
    * {
-   *   "AttributionWithoutRoleNotice": {
+   *   "attribution_without_role": {
    *     "attributionId": String.class,
    *     "csvRowNumber": Long.class,
    *   }
@@ -106,20 +106,21 @@ public static JsonObject jsonSchemaForPackages(List packages) throws IOE
    * }
* * @param packages List of packages where notices are declared - * @return a map describing all notices in given packages (see above) + * @return a map describing all notices in given packages, keyed by notice code (see above) * @throws IOException */ @VisibleForTesting static Map>> contextFieldsInPackages(List packages) throws IOException { // Return a sorted TreeMap for stable results. - Map>> contextFieldsByNotice = new TreeMap<>(); + Map>> contextFieldsByNoticeCode = new TreeMap<>(); for (Class noticeClass : ClassGraphDiscovery.discoverNoticeSubclasses(packages)) { - contextFieldsByNotice.put(Notice.getCode(noticeClass), contextFieldsForNotice(noticeClass)); + contextFieldsByNoticeCode.put( + Notice.getCode(noticeClass), contextFieldsForNotice(noticeClass)); } - return contextFieldsByNotice; + return contextFieldsByNoticeCode; } @VisibleForTesting From 500a7e299b66e7f8d30c6c297de665a5dc2ae581 Mon Sep 17 00:00:00 2001 From: Brian Ferris Date: Wed, 1 Mar 2023 06:27:58 -0800 Subject: [PATCH 5/6] Add a unit-test that enforces consistency of Notice field names by checking against an allow-list of existing field names. --- .../validator/NoticeFieldsTest.java | 181 ++++++++++++++++++ 1 file changed, 181 insertions(+) create mode 100644 main/src/test/java/org/mobilitydata/gtfsvalidator/validator/NoticeFieldsTest.java diff --git a/main/src/test/java/org/mobilitydata/gtfsvalidator/validator/NoticeFieldsTest.java b/main/src/test/java/org/mobilitydata/gtfsvalidator/validator/NoticeFieldsTest.java new file mode 100644 index 0000000000..ca5b7a5624 --- /dev/null +++ b/main/src/test/java/org/mobilitydata/gtfsvalidator/validator/NoticeFieldsTest.java @@ -0,0 +1,181 @@ +package org.mobilitydata.gtfsvalidator.validator; + +import static com.google.common.truth.Truth.assertThat; +import static java.util.Arrays.stream; + +import java.lang.reflect.Field; +import java.util.List; +import java.util.stream.Collectors; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class NoticeFieldsTest { + + /** + * Is this test failing? That likely means you've added a new field name to a `Notice` that hasn't + * been used before. That's not necessarily a bad thing, but we want to enforce some consistency + * amongst Notice field names because they are communicated and consumed externally. + * + *

Some general guidelines: + * + *

1) If there is an existing field name that is a good fit, re-use it. + * + *

2) When you need to reference two different entities in a Notice, favor a numeric suffix for + * field names. E.g. `fieldName1` and `fieldName2` (as opposed to `fieldNameA` and `fieldNameB`). + * + *

If no existing field name is a good match, then add your new field to the sorted list below. + */ + @Test + public void testNoticeClassFieldNames() { + // Keep the list of field names is in sorted order. + assertThat(discoverValidationNoticeFieldNames()) + .containsExactly( + "actual", + "actualDistanceBetweenShapePoints", + "agencyCsvRowNumber", + "agencyId", + "agencyLang", + "agencyName", + "amount", + "arrivalTime", + "arrivalTime2", + "attributionId", + "blockId", + "charIndex", + "childFieldName", + "childFilename", + "columnIndex", + "columnName", + "csvRowNumber", + "csvRowNumber1", + "csvRowNumber2", + "csvRowNumberA", + "csvRowNumberB", + "currCsvRowNumber", + "currStartTime", + "currentDate", + "departureTime", + "departureTime1", + "distanceKm", + "endFieldName", + "endValue", + "entityCount", + "entityId", + "exception", + "expected", + "expectedLocationType", + "expectedRouteId", + "feedEndDate", + "feedLang", + "fieldName", + "fieldName1", + "fieldName2", + "fieldType", + "fieldValue", + "fieldValue1", + "fieldValue2", + "filename", + "firstIndex", + "geoDistanceToShape", + "hasEntrance", + "hasExit", + "headerCount", + "index", + "intersection", + "latFieldName", + "latFieldValue", + "lineIndex", + "locationType", + "locationTypeName", + "locationTypeValue", + "lonFieldName", + "lonFieldValue", + "match", + "match1", + "match2", + "matchCount", + "message", + "newCsvRowNumber", + "oldCsvRowNumber", + "parentCsvRowNumber", + "parentFieldName", + "parentFilename", + "parentLocationType", + "parentStation", + "parentStopName", + "parsedContent", + "pathwayId", + "prevCsvRowNumber", + "prevEndTime", + "prevShapeDistTraveled", + "prevShapePtSequence", + "prevStopSequence", + "prevStopTimeDistTraveled", + "recordId", + "recordSubId", + "routeColor", + "routeCsvRowNumber", + "routeDesc", + "routeFieldName", + "routeId", + "routeId1", + "routeId2", + "routeLongName", + "routeShortName", + "routeTextColor", + "routeTypeValue", + "routeUrl", + "rowLength", + "rowNumber", + "secondIndex", + "serviceIdA", + "serviceIdB", + "shapeDistTraveled", + "shapeId", + "shapePtSequence", + "specifiedField", + "speedKph", + "startFieldName", + "startValue", + "stopCsvRowNumber", + "stopDesc", + "stopFieldName", + "stopId", + "stopId1", + "stopId2", + "stopIdFieldName", + "stopName", + "stopName1", + "stopName2", + "stopSequence", + "stopSequence1", + "stopSequence2", + "stopTimeCsvRowNumber", + "stopTimeCsvRowNumber1", + "stopTimeCsvRowNumber2", + "stopUrl", + "suggestedExpirationDate", + "tableName", + "transferCount", + "tripCsvRowNumber", + "tripFieldName", + "tripId", + "tripIdA", + "tripIdB", + "tripIdFieldName", + "validator", + "value"); + } + + private static List discoverValidationNoticeFieldNames() { + return ClassGraphDiscovery.discoverNoticeSubclasses(ClassGraphDiscovery.DEFAULT_NOTICE_PACKAGES) + .stream() + .flatMap(c -> stream(c.getDeclaredFields())) + .map(Field::getName) + .distinct() + .sorted() + .collect(Collectors.toList()); + } +} From 4a43ad4bed75c545e931c63cd24657bbac6769e8 Mon Sep 17 00:00:00 2001 From: Brian Ferris Date: Wed, 1 Mar 2023 06:37:13 -0800 Subject: [PATCH 6/6] Add copyright header. --- .../validator/NoticeFieldsTest.java | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/main/src/test/java/org/mobilitydata/gtfsvalidator/validator/NoticeFieldsTest.java b/main/src/test/java/org/mobilitydata/gtfsvalidator/validator/NoticeFieldsTest.java index ca5b7a5624..92c6bf5d9b 100644 --- a/main/src/test/java/org/mobilitydata/gtfsvalidator/validator/NoticeFieldsTest.java +++ b/main/src/test/java/org/mobilitydata/gtfsvalidator/validator/NoticeFieldsTest.java @@ -1,3 +1,19 @@ +/* + * Copyright 2023 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + package org.mobilitydata.gtfsvalidator.validator; import static com.google.common.truth.Truth.assertThat;