fix 878: require agency_id when more than one agency, warn when only one

RULES.md

@@ -1990,6 +1990,8 @@ The given field has no value in some input row, even though values are recommend
 
 #### References
 * [feed_info.txt best practices](https://gtfs.org/schedule/best-practices/#feed_infotxt)
+* [agency.txt best practices](https://gtfs.org/schedule/best-practices/#agencytxt)
+* [fare_attributes.txt best practices](https://gtfs.org/schedule/best-practices/#fare_attributestxt)
 <details>
 
 #### Notice fields description

main/src/main/java/org/mobilitydata/gtfsvalidator/validator/AgencyConsistencyValidator.java

@@ -20,10 +20,7 @@
 import java.util.Locale;
 import javax.inject.Inject;
 import org.mobilitydata.gtfsvalidator.annotation.GtfsValidator;
-import org.mobilitydata.gtfsvalidator.notice.MissingRequiredFieldNotice;
-import org.mobilitydata.gtfsvalidator.notice.NoticeContainer;
-import org.mobilitydata.gtfsvalidator.notice.SeverityLevel;
-import org.mobilitydata.gtfsvalidator.notice.ValidationNotice;
+import org.mobilitydata.gtfsvalidator.notice.*;
 import org.mobilitydata.gtfsvalidator.table.GtfsAgency;
 import org.mobilitydata.gtfsvalidator.table.GtfsAgencyTableContainer;
 
@@ -35,6 +32,7 @@
  *
  * <ul>
  *   <li>{@link MissingRequiredFieldNotice} - multiple agencies present but no agency_id set
+ *   <li>{@link MissingRecommendedFieldNotice} - single agency present but no agency_id set
  *   <li>{@link InconsistentAgencyTimezoneNotice} - inconsistent timezone among the agencies
  *   <li>{@link InconsistentAgencyLangNotice} - inconsistent language among the agencies
  * </ul>
@@ -51,18 +49,28 @@ public class AgencyConsistencyValidator extends FileValidator {
   @Override
   public void validate(NoticeContainer noticeContainer) {
     final int agencyCount = agencyTable.entityCount();
-    if (agencyCount < 2) {
+    if (agencyCount == 1) {
+      GtfsAgency agency = agencyTable.getEntities().get(0);
+      // agency_id is recommended even when there is only 1 agency
+      if (!agency.hasAgencyId()) {
+        noticeContainer.addValidationNotice(
+            new MissingRecommendedFieldNotice(
+                agencyTable.gtfsFilename(), agency.csvRowNumber(), agency.agencyName()));
+      }
+      // no further validation required
       return;
     }
 
-    for (GtfsAgency agency : agencyTable.getEntities()) {
-      // agency_id is required when there are 2 or more agencies.
-      if (!agency.hasAgencyId()) {
-        noticeContainer.addValidationNotice(
-            new MissingRequiredFieldNotice(
-                agencyTable.gtfsFilename(),
-                agency.csvRowNumber(),
-                GtfsAgency.AGENCY_ID_FIELD_NAME));
+    if (agencyCount > 1) {
+      for (GtfsAgency agency : agencyTable.getEntities()) {
+        // agency_id is required when there are 2 or more agencies.
+        if (!agency.hasAgencyId()) {
+          noticeContainer.addValidationNotice(
+              new MissingRequiredFieldNotice(
+                  agencyTable.gtfsFilename(),
+                  agency.csvRowNumber(),
+                  GtfsAgency.AGENCY_ID_FIELD_NAME));
+        }
       }
     }
 

main/src/main/java/org/mobilitydata/gtfsvalidator/validator/FareAttributeAgencyIdValidator.java

@@ -0,0 +1,72 @@
+/*
+ * Copyright 2020 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 javax.inject.Inject;
+import org.mobilitydata.gtfsvalidator.annotation.GtfsValidator;
+import org.mobilitydata.gtfsvalidator.notice.*;
+import org.mobilitydata.gtfsvalidator.table.*;
+
+/**
+ * Checks that agency_id field in "fare_attributes.txt" is defined for every row if there is more
+ * than 1 agency in the feed, recommended if only 1 agency.
+ *
+ * <p>Generated notice: {@link MissingRequiredFieldNotice}.
+ *
+ * <p>Generated notice: {@link MissingRecommendedFieldNotice}.
+ */
+@GtfsValidator
+public class FareAttributeAgencyIdValidator extends FileValidator {
+  private final GtfsAgencyTableContainer agencyTable;
+  private final GtfsFareAttributeTableContainer attributeTable;
+
+  @Inject
+  FareAttributeAgencyIdValidator(
+      GtfsAgencyTableContainer agencyTable, GtfsFareAttributeTableContainer attributeTable) {
+    this.agencyTable = agencyTable;
+    this.attributeTable = attributeTable;
+  }
+
+  @Override
+  public void validate(NoticeContainer noticeContainer) {
+
+    // routes.agency_id is required when there are multiple agencies
+    int totalAgencies = agencyTable.entityCount();
+
+    for (GtfsFareAttribute fare : attributeTable.getEntities()) {
+      if (!fare.hasAgencyId()) {
+        if (totalAgencies > 1) {
+          // add error notice if more than one agency
+          noticeContainer.addValidationNotice(
+              new MissingRequiredFieldNotice(
+                  attributeTable.gtfsFilename(),
+                  fare.csvRowNumber(),
+                  GtfsFareAttribute.AGENCY_ID_FIELD_NAME));
+        } else {
+          // add warning notice if only one agency
+          noticeContainer.addValidationNotice(
+              new MissingRecommendedFieldNotice(
+                  attributeTable.gtfsFilename(),
+                  fare.csvRowNumber(),
+                  GtfsFareAttribute.AGENCY_ID_FIELD_NAME));
+        }
+      }
+    }
+    // No need to check reference integrity because it is done by a validator generated from
+    // @ForeignKey annotation.
+  }
+}

main/src/main/java/org/mobilitydata/gtfsvalidator/validator/TripAgencyIdValidator.java → main/src/main/java/org/mobilitydata/gtfsvalidator/validator/RouteAgencyIdValidator.java

@@ -18,43 +18,50 @@
 
 import javax.inject.Inject;
 import org.mobilitydata.gtfsvalidator.annotation.GtfsValidator;
-import org.mobilitydata.gtfsvalidator.notice.MissingRequiredFieldNotice;
-import org.mobilitydata.gtfsvalidator.notice.NoticeContainer;
-import org.mobilitydata.gtfsvalidator.table.GtfsAgencyTableContainer;
-import org.mobilitydata.gtfsvalidator.table.GtfsRoute;
-import org.mobilitydata.gtfsvalidator.table.GtfsRouteTableContainer;
+import org.mobilitydata.gtfsvalidator.notice.*;
+import org.mobilitydata.gtfsvalidator.table.*;
 
 /**
  * Checks that agency_id field in "routes.txt" is defined for every row if there is more than 1
- * agency in the feed.
+ * agency in the feed, recommended if only 1 agency.
  *
  * <p>Generated notice: {@link MissingRequiredFieldNotice}.
+ *
+ * <p>Generated notice: {@link MissingRecommendedFieldNotice}.
  */
 @GtfsValidator
-public class TripAgencyIdValidator extends FileValidator {
+public class RouteAgencyIdValidator extends FileValidator {
   private final GtfsAgencyTableContainer agencyTable;
   private final GtfsRouteTableContainer routeTable;
 
   @Inject
-  TripAgencyIdValidator(GtfsAgencyTableContainer agencyTable, GtfsRouteTableContainer routeTable) {
+  RouteAgencyIdValidator(GtfsAgencyTableContainer agencyTable, GtfsRouteTableContainer routeTable) {
     this.agencyTable = agencyTable;
     this.routeTable = routeTable;
   }
 
   @Override
   public void validate(NoticeContainer noticeContainer) {
-    if (agencyTable.entityCount() < 2) {
-      // routes.agency_id is not required when there is a single agency.
-      return;
-    }
+
+    // routes.agency_id is required when there are multiple agencies
+    int totalAgencies = agencyTable.entityCount();
+
     for (GtfsRoute route : routeTable.getEntities()) {
       if (!route.hasAgencyId()) {
-        noticeContainer.addValidationNotice(
-            new MissingRequiredFieldNotice(
-                routeTable.gtfsFilename(), route.csvRowNumber(), GtfsRoute.AGENCY_ID_FIELD_NAME));
+        if (totalAgencies > 1) {
+          // add error notice if more than one agency
+          noticeContainer.addValidationNotice(
+              new MissingRequiredFieldNotice(
+                  routeTable.gtfsFilename(), route.csvRowNumber(), GtfsRoute.AGENCY_ID_FIELD_NAME));
+        } else {
+          // add warning notice if only one agency
+          noticeContainer.addValidationNotice(
+              new MissingRecommendedFieldNotice(
+                  routeTable.gtfsFilename(), route.csvRowNumber(), GtfsRoute.AGENCY_ID_FIELD_NAME));
+        }
       }
-      // No need to check reference integrity because it is done by a validator generated from
-      // @ForeignKey annotation.
     }
+    // No need to check reference integrity because it is done by a validator generated from
+    // @ForeignKey annotation.
   }
 }

main/src/test/java/org/mobilitydata/gtfsvalidator/validator/AgencyConsistencyValidatorTest.java

@@ -26,9 +26,7 @@
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
-import org.mobilitydata.gtfsvalidator.notice.MissingRequiredFieldNotice;
-import org.mobilitydata.gtfsvalidator.notice.NoticeContainer;
-import org.mobilitydata.gtfsvalidator.notice.ValidationNotice;
+import org.mobilitydata.gtfsvalidator.notice.*;
 import org.mobilitydata.gtfsvalidator.table.GtfsAgency;
 import org.mobilitydata.gtfsvalidator.table.GtfsAgencyTableContainer;
 import org.mobilitydata.gtfsvalidator.validator.AgencyConsistencyValidator.InconsistentAgencyLangNotice;
@@ -62,25 +60,44 @@ public static GtfsAgency createAgency(
   }
 
   @Test
-  public void multipleAgenciesPresentButNoAgencyIdSetShouldGenerateNotice() {
-    assertThat(
-            generateNotices(
-                ImmutableList.of(
-                    createAgency(
-                        0,
-                        "first agency",
-                        "agency name",
-                        "www.mobilitydata.org",
-                        ZoneId.of("America/Montreal"),
-                        Locale.CANADA),
-                    createAgency(
-                        1,
-                        null,
-                        "agency name",
-                        "www.mobilitydata.org",
-                        ZoneId.of("America/Montreal"),
-                        Locale.CANADA))))
+  public void singleAgencyPresentButNoAgencyIdSetShouldGenerateWarningNotice() {
+    List<ValidationNotice> notices =
+        generateNotices(
+            ImmutableList.of(
+                createAgency(
+                    0,
+                    null,
+                    "agency name",
+                    "www.mobilitydata.org",
+                    ZoneId.of("America/Montreal"),
+                    Locale.CANADA)));
+    assertThat(notices)
+        .containsExactly(new MissingRecommendedFieldNotice("agency.txt", 0, "agency name"));
+    assertThat(notices.get(0).getSeverityLevel()).isEqualTo(SeverityLevel.WARNING);
+  }
+
+  @Test
+  public void multipleAgenciesPresentButNoAgencyIdSetShouldGenerateErrorNotice() {
+    List<ValidationNotice> notices =
+        generateNotices(
+            ImmutableList.of(
+                createAgency(
+                    0,
+                    "first agency",
+                    "agency name",
+                    "www.mobilitydata.org",
+                    ZoneId.of("America/Montreal"),
+                    Locale.CANADA),
+                createAgency(
+                    1,
+                    null,
+                    "agency name",
+                    "www.mobilitydata.org",
+                    ZoneId.of("America/Montreal"),
+                    Locale.CANADA)));
+    assertThat(notices)
         .containsExactly(new MissingRequiredFieldNotice("agency.txt", 1, "agency_id"));
+    assertThat(notices.get(0).getSeverityLevel()).isEqualTo(SeverityLevel.ERROR);
   }
 
   @Test