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

Jump to bottom

Repair Shared Stops Validator #584

Merged
merged 8 commits into from
Jan 16, 2024
Merged

Repair Shared Stops Validator #584

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b2e6755
correct shared stops validator behaviors
miles-grant-ibigroup Dec 25, 2023
6ad9abf
fix formatting
miles-grant-ibigroup Dec 25, 2023
8eabd13
refactor: address pr feedback
miles-grant-ibigroup Jan 5, 2024
a55e37e
deps: update gtfs-lib
miles-grant-ibigroup Jan 5, 2024
7521cc7
extract testable functionality to unit tests
miles-grant-ibigroup Jan 11, 2024
a2bdbff
refactor(Updates to enum use and parameterized unit tests):
br648 Jan 12, 2024
dbf47d3
Merge pull request #585 from ibi-group/shared-stops-validator-repair-rb
miles-grant-ibigroup Jan 16, 2024
509cec4
update ci node
miles-grant-ibigroup Jan 16, 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
71 changes: 50 additions & 21 deletions src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,8 @@

import java.io.IOException;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Set;


public class SharedStopsValidator extends FeedValidator {
Expand Down Expand Up @@ -58,11 +56,40 @@ public void validate() {

CsvReader configReader = CsvReader.parse(config);

// TODO: pull indicies from the CSV header
final int STOP_GROUP_ID_INDEX = 0;
final int STOP_ID_INDEX = 2;
final int IS_PRIMARY_INDEX = 3;
final int FEED_ID_INDEX = 1;
int STOP_GROUP_ID_INDEX = -1;
int STOP_ID_INDEX = -1;
int IS_PRIMARY_INDEX = -1;
int FEED_ID_INDEX = -1;

try {
configReader.setHeaders(new String[]{"stop_group_id", "feed_id", "stop_id", "is_primary"});
Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding is that you are setting the headers (and presumably the order) and then checking the index of each. I think the index will always be the same for each header as defined here.

Also, if the order in project.sharedStopsConfig doesn't match this, the headers will be assigned to the wrong data column. To fix (assuming my understanding of this is correct) is to remove this line and therefore use the headers as they are defined (rightly or wrongly) in project.sharedStopsConfig. A unit test or two around this would be really useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we have examples of unit testing other validators? I've manually created some tests to fix the behavior issues you're talking about but I can't find a good way to split things to reasonably test them. Either way the issues you've mentioned have been fixed. Using split.

Copy link
Contributor

@br648 br648 Jan 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@miles-grant-ibigroup Look to move the contents of validate() method into a static method or two so these can be called from a unit test to avoid having to go through the whole feed/validation process.

See https://github.com/ibi-group/gtfs-lib/blob/dev-flex/src/main/java/com/conveyal/gtfs/validator/FlexValidator.java and https://github.com/ibi-group/gtfs-lib/blob/dev-flex/src/test/java/com/conveyal/gtfs/validator/FlexValidatorTest.java.

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've given this an attempt. Let me know what you think

Copy link
Contributor

@br648 br648 Jan 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@miles-grant-ibigroup I hope you don't mind, but I have created a PR off of this PR: #585 and made a few changes. Let me know if you have any questions. If not, you can merge that back into this PR.

String[] headers = configReader.getHeaders();
for (int i = 0; i < headers.length; i++) {
String header = headers[i];
switch(header) {
miles-grant-ibigroup marked this conversation as resolved.
Show resolved Hide resolved
case "stop_group_id":
STOP_GROUP_ID_INDEX = i;
break;
case "feed_id":
FEED_ID_INDEX = i;
break;
case "stop_id":
STOP_ID_INDEX = i;
break;
case "is_primary":
IS_PRIMARY_INDEX = i;
break;
default:
throw new RuntimeException("shared_stops.csv contained invalid headers!");
}
}

if (STOP_GROUP_ID_INDEX == -1 || FEED_ID_INDEX == -1 || STOP_ID_INDEX == -1 || IS_PRIMARY_INDEX == -1) {
throw new RuntimeException("shared_stops.csv is missing headers!");
}
} catch (IOException e) {
throw new RuntimeException("shared_stops.csv was invalid! " + e.toString());
miles-grant-ibigroup marked this conversation as resolved.
Show resolved Hide resolved
}

// Build list of stop Ids.
List<String> stopIds = new ArrayList<>();
Expand All @@ -74,7 +101,7 @@ public void validate() {
}

// Initialize hashmaps to hold duplication info
HashMap<String, Set<String>> stopGroupStops = new HashMap<>();
HashSet<String> seenStopIds = new HashSet<>();
HashSet<String> stopGroupsWithPrimaryStops = new HashSet<>();

try {
Expand All @@ -88,26 +115,28 @@ public void validate() {
continue;
}

stopGroupStops.putIfAbsent(stopGroupId, new HashSet<>());
Set<String> seenStopIds = stopGroupStops.get(stopGroupId);

// Check for SS_01 (stop id appearing in multiple stop groups)
// Make sure this error is only returned if we are inside the feed that is being checked
if (seenStopIds.contains(stopId)) {
registerError(stops
.stream()
.filter(stop -> stop.stop_id.equals(stopId))
.findFirst()
.orElse(stops.get(0)), NewGTFSErrorType.MULTIPLE_SHARED_STOPS_GROUPS
);
if (this.feedId.equals(sharedStopFeedId)) {
miles-grant-ibigroup marked this conversation as resolved.
Show resolved Hide resolved
registerError(stops
.stream()
.filter(stop -> stop.stop_id.equals(stopId))
.findFirst()
.orElse(new Stop()), NewGTFSErrorType.MULTIPLE_SHARED_STOPS_GROUPS
);
}
} else {
seenStopIds.add(stopId);
}

// Check for SS_02 (multiple primary stops per stop group)
if (stopGroupsWithPrimaryStops.contains(stopGroupId)) {
registerError(NewGTFSError.forFeed(NewGTFSErrorType.SHARED_STOP_GROUP_MUTLIPLE_PRIMARY_STOPS, stopGroupId));
} else if (configReader.get(IS_PRIMARY_INDEX).equals("true")) {
stopGroupsWithPrimaryStops.add(stopGroupId);
if (configReader.get(IS_PRIMARY_INDEX).equals("1") || configReader.get(IS_PRIMARY_INDEX).equals("true")) {
if (stopGroupsWithPrimaryStops.contains(stopGroupId)) {
registerError(NewGTFSError.forFeed(NewGTFSErrorType.SHARED_STOP_GROUP_MUTLIPLE_PRIMARY_STOPS, stopGroupId));
miles-grant-ibigroup marked this conversation as resolved.
Show resolved Hide resolved
} else {
stopGroupsWithPrimaryStops.add(stopGroupId);
}
}

// Check for SS_03 (stop_id referenced doesn't exist)
Expand Down
Loading