Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add an argument for the cli (--date) that specifies the date to use during verification. #1628

Merged
merged 9 commits into from
Jan 17, 2024
Merged

feat: Add an argument for the cli (--date) that specifies the date to use during verification. #1628

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
44588f7
Add --date argument to cli to override the date, and include it in ou…
bradyhunsaker Jan 2, 2024
4b33022
Correct formatting with google-java-format
bradyhunsaker Jan 2, 2024
9b08cc1
Include short argument form -d
bradyhunsaker Jan 7, 2024
945a2d2
Add --date argument to cli to override the date, and include it in ou…
bradyhunsaker Jan 2, 2024
48d7014
Correct formatting with google-java-format
bradyhunsaker Jan 2, 2024
b3d4148
Include short argument form -d
bradyhunsaker Jan 7, 2024
86e7a9f
Trying to reconcile branches
bradyhunsaker Jan 15, 2024
5f481fa
Changes to use a LocalDate and the name dateForValidation
bradyhunsaker Jan 16, 2024
56568e3
Merge branch 'master' into override-date
jcpitre Jan 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions cli/src/main/java/org/mobilitydata/gtfsvalidator/cli/Arguments.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
import java.net.URI;
import java.net.URISyntaxException;
import java.nio.file.Path;
import java.time.LocalDate;
import java.time.format.DateTimeFormatter;
import org.mobilitydata.gtfsvalidator.input.CountryCode;
import org.mobilitydata.gtfsvalidator.runner.ValidationRunnerConfig;

Expand Down Expand Up @@ -52,6 +54,14 @@ public class Arguments {
+ "It must be a two-letter country code (ISO 3166-1 alpha-2)")
private String countryCode;

@Parameter(
names = {"-d", "--date"},
description =
"Date to simulate when validating, in ISO_LOCAL_DATE format like "
+ "'2001-01-30'. By default, the current date is used. "
+ "This option enables debugging rules like feed expiration.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bradyhunsaker It's a bit late, but I was wondering about this sentence. I don't see where these debugging rules are enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear. I meant that the main purpose of this option is debugging. For example, it could be used to debug an error with a rule like feed expiration.

There are not special debugging rules associated with it.
I think the use of the word "enables" was a bad choice. Feel free to remove or edit the description.

private String dateString;

@Parameter(
names = {"-u", "--url"},
description = "Fully qualified URL to download GTFS archive")
Expand Down Expand Up @@ -111,6 +121,9 @@ ValidationRunnerConfig toConfig() throws URISyntaxException {
if (countryCode != null) {
builder.setCountryCode(CountryCode.forStringOrUnknown(countryCode));
}
if (dateString != null) {
builder.setDateForValidation(LocalDate.parse(dateString, DateTimeFormatter.ISO_LOCAL_DATE));
}
if (validationReportName != null) {
builder.setValidationReportFileName(validationReportName);
}
Expand Down
4 changes: 3 additions & 1 deletion cli/src/test/java/org/mobilitydata/gtfsvalidator/cli/ArgumentsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ public void longNameShouldInitializeArguments() throws URISyntaxException {
"--input", "/tmp/gtfs.zip",
"--output_base", "/tmp/output",
"--country_code", "ca",
"--threads", "4"
"--threads", "4",
"--date", "2020-01-02"
};

Arguments underTest = new Arguments();
Expand All @@ -100,6 +101,7 @@ public void longNameShouldInitializeArguments() throws URISyntaxException {
assertThat(config.validationReportFileName()).matches("report.json");
assertThat(config.htmlReportFileName()).matches("report.html");
assertThat(config.systemErrorsReportFileName()).matches("system_errors.json");
assertThat(config.dateForValidation().toString()).matches("2020-01-02");
}

@Test
Expand Down
4 changes: 3 additions & 1 deletion main/src/main/java/org/mobilitydata/gtfsvalidator/report/HtmlReportGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ public void generateReport(
ValidationRunnerConfig config,
VersionInfo versionInfo,
Path reportPath,
String date)
String date,
boolean is_different_date)
throws IOException {
TemplateEngine templateEngine = new TemplateEngine();
ClassLoaderTemplateResolver templateResolver = new ClassLoaderTemplateResolver();
Expand All @@ -54,6 +55,7 @@ public void generateReport(
context.setVariable("summary", summary);
context.setVariable("config", config);
context.setVariable("date", date);
context.setVariable("is_different_date", is_different_date);

try (FileWriter writer = new FileWriter(reportPath.toFile())) {
templateEngine.process("report.html", context, writer);
Expand Down
2 changes: 2 additions & 0 deletions main/src/main/java/org/mobilitydata/gtfsvalidator/report/JsonReportSummary.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ public class JsonReportSummary {
private String validationReportName;
private String htmlReportName;
private String countryCode;
private String dateForValidation;
private JsonReportFeedInfo feedInfo;
private List<JsonReportAgencyMetadata> agencies;
private Set<String> files;
Expand All @@ -55,6 +56,7 @@ public JsonReportSummary(
this.validationReportName = config.validationReportFileName();
this.htmlReportName = config.htmlReportFileName();
this.countryCode = config.countryCode().getCountryCode();
this.dateForValidation = config.dateForValidation().toString();
} else {
logger.atSevere().log(
"No validation configuration for JSON report, there will be missing data in the report.");
Expand Down
16 changes: 8 additions & 8 deletions main/src/main/java/org/mobilitydata/gtfsvalidator/runner/ValidationRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,9 @@
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.text.SimpleDateFormat;
import java.time.Duration;
import java.time.LocalDate;
import java.util.Date;
import java.time.ZonedDateTime;
import java.time.format.DateTimeFormatter;
import java.util.List;
import java.util.stream.Collectors;
import org.mobilitydata.gtfsvalidator.input.DateForValidation;
Expand Down Expand Up @@ -120,7 +119,7 @@ public Status run(ValidationRunnerConfig config) {
ValidationContext validationContext =
ValidationContext.builder()
.setCountryCode(config.countryCode())
.setDateForValidation(new DateForValidation(LocalDate.now()))
.setDateForValidation(new DateForValidation(config.dateForValidation()))
.build();
try {
feedContainer =
Expand Down Expand Up @@ -267,9 +266,9 @@ public static void exportReport(
"Error creating output directory: %s", config.outputDirectory());
}
}
SimpleDateFormat formatter = new SimpleDateFormat("yyyy-MM-dd 'at' HH:mm:ss z");
Date now = new Date(System.currentTimeMillis());
String date = formatter.format(now);
ZonedDateTime now = ZonedDateTime.now();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would think now will pretty much always be different from the date specified on the command line, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. There have always been two times at play: the time used for validation and the time indicated in the report. In the current code (before this PR), only the report generation time is included in the output.

Note that this part of the code is a refactor. The existing code uses System.currentTimeMillis() while the new code uses ZonedDateTime.now(). In either case, this time included in the report is the time that the report is generated (after validation steps have been done).

The time used for validation is set at the start of processing if the new --date option isn't used. In that common case the two times differ by the processing time, which could be a fraction of a second or could be many minutes for large feeds (maybe even hours? I don't know how long the biggest feeds take).

That was already true and remains true. A difference now is that both times are included in the JSON report, and both are included in the HTML report if the date differs.

I'm open to changing any of that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification.
So config.zonedDateTime() is the time specified on the command line, right?
If it's the case, my point is that it's almost impossible to specify the exact same time as now, so is_different_date will pretty much always be true. In that case why check at all?
It's no big deal because the net result will that the it always prints the dateForValidation, which is OK IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I misunderstood your original question. Date and time handling is confusing.

The computation of is_different_date converts both ZonedDateTime variables to LocalDate before comparing. It's looking to see if the date (not the time part) is different. In the common case that --date is not specified and the validation run completes on the same calendar date as it starts, is_different_date will be false and the HTML report will not have anything added. As you say, the date+time will always be different, but that isn't what is checked. My goal was to be conservative about changing the human-readable HTML output (although I'm open-minded).

Part of why this is confusing is that I called the argument --date, but you must specify a time as well, so it's really a date+time.

I have a suggestion to reduce the confusion: Refactor the code by replacing "currentDateTime" in ValidationContext with a LocalDate (no time). Currently the type is a ZonedDateTime, which is why I used that type in this PR and require that a time be specified along with a date. But I checked the uses of the ValidationContext, and the rules all convert to LocalDate when using currentDateTime anyway. (example:

gtfs-validator/main/src/main/java/org/mobilitydata/gtfsvalidator/validator/DateTripsValidator.java

Line 76 in 3d5ef6f

LocalDate now = currentDateTime.getNow().toLocalDate();
)

I'll create a PR with that refactor so you see what I mean. It won't change the current functionality at all.
After that change (if you agree), then I'll change this PR so that only a date is specified by --date and hopefully it makes everything simpler and clearer. That will also resolve the issue about different timestamp formats; with the change the new one will just be a date. (Though I'll still follow up separately about making the existing one machine readable.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I missed that the comparison was based only on the date.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just created PR #1636, a refactor to simplify and clarify the existing date handling. I'll wait to see if that is accepted and merged. If so, it will simplify this PR.

String date = now.format(DateTimeFormatter.ofPattern("yyyy-MM-dd 'at' HH:mm:ss z"));
boolean is_different_date = !now.toLocalDate().equals(config.dateForValidation());

Gson gson = createGson(config.prettyJson());
HtmlReportGenerator htmlGenerator = new HtmlReportGenerator();
Expand All @@ -291,7 +290,8 @@ public static void exportReport(
config,
versionInfo,
config.outputDirectory().resolve(config.htmlReportFileName()),
date);
date,
is_different_date);
Files.write(
config.outputDirectory().resolve(config.systemErrorsReportFileName()),
gson.toJson(noticeContainer.exportSystemErrors()).getBytes(StandardCharsets.UTF_8));
Expand Down
9 changes: 8 additions & 1 deletion main/src/main/java/org/mobilitydata/gtfsvalidator/runner/ValidationRunnerConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import com.google.auto.value.AutoValue;
import java.net.URI;
import java.nio.file.Path;
import java.time.LocalDate;
import java.util.Optional;
import org.mobilitydata.gtfsvalidator.input.CountryCode;

Expand Down Expand Up @@ -56,6 +57,9 @@ public Path systemErrorsReportPath() {
// validated.
public abstract CountryCode countryCode();

// The date to use for validation.
public abstract LocalDate dateForValidation();

// If true, any output json will be pretty-printed.
public abstract boolean prettyJson();

Expand All @@ -67,7 +71,8 @@ public static Builder builder() {
.setSystemErrorsReportFileName("system_errors.json")
.setNumThreads(1)
.setPrettyJson(false)
.setCountryCode(CountryCode.forStringOrUnknown(CountryCode.ZZ));
.setCountryCode(CountryCode.forStringOrUnknown(CountryCode.ZZ))
.setDateForValidation(LocalDate.now());
}

@AutoValue.Builder
Expand All @@ -88,6 +93,8 @@ public abstract static class Builder {

public abstract Builder setCountryCode(CountryCode countryCode);

public abstract Builder setDateForValidation(LocalDate dateForValidation);

public abstract Builder setPrettyJson(boolean prettyJson);

public abstract ValidationRunnerConfig build();
Expand Down
1 change: 1 addition & 0 deletions main/src/main/resources/report.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,7 @@ <h1>GTFS Schedule Validation Report</h1>
<span th:text="${config.gtfsSource}"></span><span
th:text="${config.countryCode.isUnknown()} ? '. No country code was provided.' : ', with the country code: ' + ${config.countryCode} + '.'"></span>
</br>
<span th:if="${is_different_date}" th:text="'The date used during validation was ' + ${config.dateForValidation} + '.'"></span></p>

<p>Use this report alongside our <a href="https://gtfs-validator.mobilitydata.org/rules.html">documentation</a>.</p>

Expand Down
6 changes: 5 additions & 1 deletion main/src/test/java/org/mobilitydata/gtfsvalidator/report/model/JsonReportSummaryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import com.google.gson.JsonParser;
import java.net.URI;
import java.nio.file.Path;
import java.time.LocalDate;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Map;
Expand Down Expand Up @@ -41,6 +42,7 @@ private static ValidationRunnerConfig generateValidationRunnerConfig() throws Ex
builder.setPrettyJson(true);
builder.setSystemErrorsReportFileName("some_error_filename");
builder.setValidationReportFileName("some_report_filename");
builder.setDateForValidation(LocalDate.parse("2020-01-02"));

return builder.build();
}
Expand Down Expand Up @@ -91,7 +93,8 @@ public void noFeedMetadataWithConfigTest() throws Exception {
+ "\"systemErrorsReportName\":\"some_error_filename\","
+ "\"validationReportName\":\"some_report_filename\","
+ "\"htmlReportName\":\"some_html_filename\","
+ "\"countryCode\":\"GB\"}";
+ "\"countryCode\":\"GB\","
+ "\"dateForValidation\":\"2020-01-02\"}";

assertEquals(JsonParser.parseString(expected), gson.toJsonTree(reportSummary));
}
Expand All @@ -113,6 +116,7 @@ public void withFeedMetadataWithConfigTest() throws Exception {
+ "\"validationReportName\":\"some_report_filename\","
+ "\"htmlReportName\":\"some_html_filename\","
+ "\"countryCode\":\"GB\","
+ "\"dateForValidation\":\"2020-01-02\","
+ "\"feedInfo\":{\"publisherName\":\"value1\",\"publisherUrl\":\"value2\"},"
+ "\"agencies\":["
+ "{\"name\":\"agency1\",\"url\":\"some URL 1\",\"phone\":\"phone1\",\"email\":\"email1\"},"
Expand Down
Loading