-
Notifications
You must be signed in to change notification settings - Fork 100
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.
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
fix: False positives for StopTimeTimepointWithoutTimesNotice #1044
Changes from all commits
a72b888
febb426
c2fa449
b3ec654
fc14880
d44332a
fa647ef
8e91071
c1d8fc3
7497e16
bab59bf
df1f702
a2b5e3b
ddf420b
1135049
539fc1f
2c62b0a
62c64c1
afa44dc
4420e7d
b9dd33f
22c59aa
e33c960
dc6462c
0a74e40
84926f4
35f47ce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -16,11 +16,13 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
package org.mobilitydata.gtfsvalidator.validator; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import javax.inject.Inject; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import org.mobilitydata.gtfsvalidator.annotation.GtfsValidator; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import org.mobilitydata.gtfsvalidator.notice.NoticeContainer; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import org.mobilitydata.gtfsvalidator.notice.SeverityLevel; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import org.mobilitydata.gtfsvalidator.notice.ValidationNotice; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import org.mobilitydata.gtfsvalidator.table.GtfsStopTime; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import org.mobilitydata.gtfsvalidator.table.GtfsStopTimeTableContainer; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import org.mobilitydata.gtfsvalidator.table.GtfsStopTimeTableLoader; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import org.mobilitydata.gtfsvalidator.table.GtfsStopTimeTimepoint; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -32,56 +34,102 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* <ul> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* <li>{@link StopTimeTimepointWithoutTimesNotice} - a timepoint does not specifies arrival_time | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* or departure_time | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* <li>{@link MissingTimepointValueNotice} - value for {@code stop_times.timepoint} is missing | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* <li>{@link MissingTimepointColumnNotice} - field {@code stop_times.timepoint} is missing | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* </ul> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
@GtfsValidator | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public class TimepointTimeValidator extends SingleEntityValidator<GtfsStopTime> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public class TimepointTimeValidator extends FileValidator { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
private final GtfsStopTimeTableContainer stopTimes; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
@Inject | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
TimepointTimeValidator(GtfsStopTimeTableContainer stopTimes) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
this.stopTimes = stopTimes; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
@Override | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public void validate(GtfsStopTime stopTime, NoticeContainer noticeContainer) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (!isTimepoint(stopTime)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public void validate(NoticeContainer noticeContainer) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (!stopTimes.hasColumn(GtfsStopTimeTableLoader.TIMEPOINT_FIELD_NAME)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// legacy datasets do not use timepoint column in stop_times.txt as a result: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// - this should be flagged; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// - but also no notice regarding the absence of arrival_time or departure_time should be | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// generated | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
noticeContainer.addValidationNotice(new MissingTimepointColumnNotice()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (!stopTime.hasArrivalTime()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
noticeContainer.addValidationNotice( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
new StopTimeTimepointWithoutTimesNotice( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
stopTime.csvRowNumber(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
stopTime.tripId(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
stopTime.stopSequence(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
String.format(GtfsStopTimeTableLoader.ARRIVAL_TIME_FIELD_NAME))); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (!stopTime.hasDepartureTime()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
noticeContainer.addValidationNotice( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
new StopTimeTimepointWithoutTimesNotice( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
stopTime.csvRowNumber(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
stopTime.tripId(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
stopTime.stopSequence(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GtfsStopTimeTableLoader.DEPARTURE_TIME_FIELD_NAME)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
for (GtfsStopTime stopTime : stopTimes.getEntities()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (!stopTime.hasTimepoint()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
noticeContainer.addValidationNotice(new MissingTimepointValueNotice(stopTime)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (isTimepoint(stopTime)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (!stopTime.hasArrivalTime()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
noticeContainer.addValidationNotice( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
new StopTimeTimepointWithoutTimesNotice( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
stopTime, GtfsStopTimeTableLoader.ARRIVAL_TIME_FIELD_NAME)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (!stopTime.hasDepartureTime()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
noticeContainer.addValidationNotice( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
new StopTimeTimepointWithoutTimesNotice( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
stopTime, GtfsStopTimeTableLoader.DEPARTURE_TIME_FIELD_NAME)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
private boolean isTimepoint(GtfsStopTime stopTime) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return stopTime.timepoint().equals(GtfsStopTimeTimepoint.EXACT); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return stopTime.hasTimepoint() && stopTime.timepoint().equals(GtfsStopTimeTimepoint.EXACT); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. usage of [0] gtfs-validator/main/src/main/java/org/mobilitydata/gtfsvalidator/table/GtfsStopTimeSchema.java Line 77 in 5243445
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually because of the fact that Bottom line, a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, in this case should we remove default value instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would make sense, thoughts @isabelle-dr @barbeau ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed to remove default value. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@asvechnikov2 I was trying to trace the execution for this too. Where do you see this assignment? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's this one Lines 134 to 160 in 903e9af
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @asvechnikov2 Ah, thanks! Looks like you're right. I just ran this through the debugger with two different datasets to confirm: This code is what auto-generates the So the current behavior is:
So the default enum value is the value defined for Back to the original question - if we remove IMHO the best solution is still having the default is set to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
After discussion with @isabelle-dr, we agreed on keeping the If you agree with that, I'll ask one of you their approval on this PR if the code looks good to you :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I'd add a comment to clarify the behaviour, here and probably in the class docstring as well. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* Timepoint without time | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* <p>Severity: {@code SeverityLevel.WARNING} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* <p>Severity: {@code SeverityLevel.WARNING} - to be upgraded as {@code SeverityLevel.ERROR} see | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* https://github.com/MobilityData/gtfs-validator/issues/1017 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
static class StopTimeTimepointWithoutTimesNotice extends ValidationNotice { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
private final long csvRowNumber; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
private final String tripId; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
private final long stopSequence; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
private final String specifiedField; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
StopTimeTimepointWithoutTimesNotice( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
long csvRowNumber, String tripId, long stopSequence, String specifiedField) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
StopTimeTimepointWithoutTimesNotice(GtfsStopTime stopTime, String specifiedField) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
super(SeverityLevel.WARNING); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
this.csvRowNumber = csvRowNumber; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
this.tripId = tripId; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
this.stopSequence = stopSequence; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
this.csvRowNumber = stopTime.csvRowNumber(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
this.tripId = stopTime.tripId(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
this.stopSequence = stopTime.stopSequence(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
this.specifiedField = specifiedField; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* {@code stop_times.timepoint} value is missing | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* <p>Severity: {@code SeverityLevel.WARNING} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
static class MissingTimepointValueNotice extends ValidationNotice { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
private final long csvRowNumber; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
private final String tripId; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
private final long stopSequence; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
MissingTimepointValueNotice(GtfsStopTime stopTime) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
super(SeverityLevel.WARNING); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
this.csvRowNumber = stopTime.csvRowNumber(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
this.tripId = stopTime.tripId(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
this.stopSequence = stopTime.stopSequence(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* Column {@code stop_times.timepoint} is missing. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* <p>Severity: {@code SeverityLevel.WARNING} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
static class MissingTimepointColumnNotice extends ValidationNotice { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
private final String filename; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
MissingTimepointColumnNotice() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
super(SeverityLevel.WARNING); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
this.filename = GtfsStopTimeTableLoader.FILENAME; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specification says
1 or empty - Times are considered exact.
. Are we going to change specification?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will advocate for @barbeau's recommendation in google/transit#61. If you support this recommendation or have other ideas, feel free to leave a comment there so we make a PR accordingly :)