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

Report issue for duplicate journey patterns in NeTEx #5571

Merged
merged 2 commits into from
Jan 9, 2024
Merged

Report issue for duplicate journey patterns in NeTEx #5571

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f2241c9
Report issue for duplicate journey patterns in NeTEx
habrahamsson-skanetrafiken Dec 13, 2023
2c86a46
Separate code into method
habrahamsson-skanetrafiken Dec 18, 2023
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
41 changes: 30 additions & 11 deletions src/main/java/org/opentripplanner/netex/mapping/NetexMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
import org.opentripplanner.graph_builder.issue.api.DataImportIssueStore;
import org.opentripplanner.model.StopTime;
import org.opentripplanner.model.calendar.ServiceCalendar;
Expand Down Expand Up @@ -460,18 +461,36 @@ private void mapTripPatterns(Map<String, FeedScopedId> serviceIds) {
for (JourneyPattern_VersionStructure journeyPattern : currentNetexIndex
.getJourneyPatternsById()
.localValues()) {
TripPatternMapperResult result = tripPatternMapper.mapTripPattern(journeyPattern);
tripPatternMapper
.mapTripPattern(journeyPattern)
.ifPresent(result -> {
Copy link
Member

Choose a reason for hiding this comment

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

Calling a none-trivial code block inside a function chain is not clean. Consider making a private method for the code block or using Java language features like if/for.

var journeyPatternExists = transitBuilder
.getTripPatterns()
.get(result.tripPattern().getStopPattern())
.stream()
.anyMatch(tripPattern -> result.tripPattern().getId().equals(tripPattern.getId()));
if (journeyPatternExists) {
issueStore.add(
"DuplicateJourneyPattern",
"Duplicate of JourneyPattern %s found",
journeyPattern.getId()
);
}

for (Map.Entry<Trip, List<StopTime>> it : result.tripStopTimes.entrySet()) {
transitBuilder.getStopTimesSortedByTrip().put(it.getKey(), it.getValue());
transitBuilder.getTripsById().add(it.getKey());
}
for (var it : result.tripPatterns.entries()) {
transitBuilder.getTripPatterns().put(it.getKey(), it.getValue());
}
currentMapperIndexes.addStopTimesByNetexId(result.stopTimeByNetexId);
groupMapper.scheduledStopPointsIndex.putAll(Multimaps.asMap(result.scheduledStopPointsIndex));
transitBuilder.getTripOnServiceDates().addAll(result.tripOnServiceDates);
for (Map.Entry<Trip, List<StopTime>> it : result.tripStopTimes().entrySet()) {
transitBuilder.getStopTimesSortedByTrip().put(it.getKey(), it.getValue());
transitBuilder.getTripsById().add(it.getKey());
}

transitBuilder
.getTripPatterns()
.put(result.tripPattern().getStopPattern(), result.tripPattern());
currentMapperIndexes.addStopTimesByNetexId(result.stopTimeByNetexId());
groupMapper.scheduledStopPointsIndex.putAll(
Multimaps.asMap(result.scheduledStopPointsIndex())
);
transitBuilder.getTripOnServiceDates().addAll(result.tripOnServiceDates());
});
}
}

Expand Down
71 changes: 40 additions & 31 deletions src/main/java/org/opentripplanner/netex/mapping/TripPatternMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,12 @@
import jakarta.xml.bind.JAXBElement;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.stream.Collectors;
import org.opentripplanner.graph_builder.issue.api.DataImportIssueStore;
import org.opentripplanner.model.StopTime;
Expand Down Expand Up @@ -84,8 +86,6 @@ class TripPatternMapper {

private final Deduplicator deduplicator;

private TripPatternMapperResult result;

TripPatternMapper(
DataImportIssueStore issueStore,
FeedScopedIdFactory idFactory,
Expand Down Expand Up @@ -157,9 +157,7 @@ class TripPatternMapper {
}
}

TripPatternMapperResult mapTripPattern(JourneyPattern_VersionStructure journeyPattern) {
// Make sure the result is clean, by creating a new object.
result = new TripPatternMapperResult();
Optional<TripPatternMapperResult> mapTripPattern(JourneyPattern_VersionStructure journeyPattern) {
Copy link
Member

Choose a reason for hiding this comment

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

Observation: The original result class did handle the empty case, so this is weakening the existing functionality. The good thing is that a it is a bit less code to maintain, but since this does not reduce the code in the caller - it complicates it, than gain is minimal. I do not suggest to change it, but just wanted to mention it.

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 wanted to make it explicit that if we return a result then it will have a single TripPattern. I could have added an optional/nullable tripPattern field to the result class instead but that wouldn't really be simpler and i think this better expresses the actual behaviour of the mapTripPattern method.

Now the caller has to handle the case where the result is empty, it's a bit more complicated at the call site but IMO it's worth it.

Copy link
Member

@t2gran t2gran Dec 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

My point is that you are wrapping a result wrapper with another result wrapper - Optional and TripPatternMapperResult have some of the same responsibilities. You should not wrap the field inside TripPatternMapperResult with optional. We are return a collection here, not a single instance - you could provide a isEmpty() or ifPresent() method to make it explicit, but it was not needed; hence not added. The pattern we follow in OTP for collections is to return empty collections, not null or Optional.

Copy link
Member

@t2gran t2gran Dec 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Another thing is that we avoid using records with arrays and collections. Arrays can not be protected, while collections are hard to protect - classes are simpler/less error prune. Even if the TripPatternMapperResult is just a DTO here, people have a tendency to copy code/and ways of doing things - so I want it to be clean everywhere - even if it in this case is ok/no risk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My point is that you are wrapping a result wrapper with another result wrapper - Optional and TripPatternMapperResult have some of the same responsibilities.

In this PR my idea is to change the responsibilities so that the result class is only responsible for representing an actual result. The Optional is responsible for representing presence/absence of a result. I think this is a pretty common pattern. And I think it is a good pattern since the result class would only have to concern itself with a single responsibility.

We are return a collection here, not a single instance - you could provide a isEmpty() or ifPresent() method to make it explicit, but it was not needed; hence not added. The pattern we follow in OTP for collections is to return empty collections, not null or Optional.

The thing is that I want the result to have a single instance of a TripPattern instead of a collection of TripPatterns. Because I want the caller to be able to see what the resulting id is and I wanted it to be obvous that the issue for duplicated ids is only reported once.

In this codebase, would it be more idiomatic to put a List<TripPattern> in the result and have it be implicit that this is either empty or has a single entry?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another thing is that we avoid using records with arrays and collections. Arrays can not be protected, while collections are hard to protect - classes are simpler/less error prune. Even if the TripPatternMapperResult is just a DTO here, people have a tendency to copy code/and ways of doing things - so I want it to be clean everywhere - even if it in this case is ok/no risk.

Is this in order to protect the collections from mutation? I think that's a good concern. But then having collections as public attributes should be just as bad, or am I missing something? In this refactored code it wouldn't be that hard to make the collections unmodifiable when creating the record to protect them from modification. It's not perfect but I think it's a win for this style.

Copy link
Member

Choose a reason for hiding this comment

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

In this codebase, would it be more idiomatic to put a List in the result and have it be implicit that this is either empty or has a single entry?

Of cause not. The TripPatternMapperResult has a single-responsibility - to carry the result of one method. The result has many different states, which you toke just one out.

But we can agree to disagree - that is fine. I will approve the PR.

Collection<ServiceJourney> serviceJourneys = serviceJourniesByPatternId.get(
journeyPattern.getId()
);
Expand All @@ -170,10 +168,14 @@ TripPatternMapperResult mapTripPattern(JourneyPattern_VersionStructure journeyPa
"ServiceJourneyPattern %s does not contain any serviceJourneys.",
journeyPattern.getId()
);
return result;
return Optional.empty();
}

List<Trip> trips = new ArrayList<>();
ArrayListMultimap<String, String> scheduledStopPointsIndex = ArrayListMultimap.create();
HashMap<Trip, List<StopTime>> tripStopTimes = new HashMap<>();
Map<String, StopTime> stopTimeByNetexId = new HashMap<>();
ArrayList<TripOnServiceDate> tripOnServiceDates = new ArrayList<>();

for (ServiceJourney serviceJourney : serviceJourneys) {
Trip trip = mapTrip(journeyPattern, serviceJourney);
Expand All @@ -184,7 +186,7 @@ TripPatternMapperResult mapTripPattern(JourneyPattern_VersionStructure journeyPa
}

// Add the dated service journey to the model for this trip [if it exists]
mapDatedServiceJourney(journeyPattern, serviceJourney, trip);
tripOnServiceDates.addAll(mapDatedServiceJourney(journeyPattern, serviceJourney, trip));

StopTimesMapperResult stopTimes = stopTimesMapper.mapToStopTimes(
journeyPattern,
Expand All @@ -198,25 +200,22 @@ TripPatternMapperResult mapTripPattern(JourneyPattern_VersionStructure journeyPa
continue;
}

result.scheduledStopPointsIndex.putAll(
serviceJourney.getId(),
stopTimes.scheduledStopPointIds
);
result.tripStopTimes.put(trip, stopTimes.stopTimes);
result.stopTimeByNetexId.putAll(stopTimes.stopTimeByNetexId);
scheduledStopPointsIndex.putAll(serviceJourney.getId(), stopTimes.scheduledStopPointIds);
tripStopTimes.put(trip, stopTimes.stopTimes);
stopTimeByNetexId.putAll(stopTimes.stopTimeByNetexId);

trips.add(trip);
}

// No trips successfully mapped
if (trips.isEmpty()) {
return result;
return Optional.empty();
}

// Create StopPattern from any trip (since they are part of the same JourneyPattern)
StopPattern stopPattern = deduplicator.deduplicateObject(
StopPattern.class,
new StopPattern(result.tripStopTimes.get(trips.get(0)))
new StopPattern(tripStopTimes.get(trips.get(0)))
);

var tripPatternModes = new HashSet<TransitMode>();
Expand Down Expand Up @@ -246,7 +245,7 @@ TripPatternMapperResult mapTripPattern(JourneyPattern_VersionStructure journeyPa
);
}

TripPatternBuilder tripPatternBuilder = TripPattern
var tripPattern = TripPattern
.of(idFactory.createId(journeyPattern.getId()))
.withRoute(lookupRoute(journeyPattern))
.withStopPattern(stopPattern)
Expand All @@ -256,30 +255,35 @@ TripPatternMapperResult mapTripPattern(JourneyPattern_VersionStructure journeyPa
.withName(journeyPattern.getName() == null ? "" : journeyPattern.getName().getValue())
.withHopGeometries(
serviceLinkMapper.getGeometriesByJourneyPattern(journeyPattern, stopPattern)
);

TripPattern tripPattern = tripPatternBuilder.build();
createTripTimes(trips, tripPattern);

result.tripPatterns.put(stopPattern, tripPattern);

return result;
)
.build();
createTripTimes(trips, tripStopTimes).forEach(tripPattern::add);

return Optional.of(
new TripPatternMapperResult(
tripPattern,
scheduledStopPointsIndex,
tripStopTimes,
stopTimeByNetexId,
tripOnServiceDates
)
);
}

private void mapDatedServiceJourney(
private ArrayList<TripOnServiceDate> mapDatedServiceJourney(
JourneyPattern_VersionStructure journeyPattern,
ServiceJourney serviceJourney,
Trip trip
) {
var tripsOnServiceDates = new ArrayList<TripOnServiceDate>();
if (datedServiceJourneysBySJId.containsKey(serviceJourney.getId())) {
for (DatedServiceJourney datedServiceJourney : datedServiceJourneysBySJId.get(
serviceJourney.getId()
)) {
result.tripOnServiceDates.add(
mapDatedServiceJourney(journeyPattern, trip, datedServiceJourney)
);
tripsOnServiceDates.add(mapDatedServiceJourney(journeyPattern, trip, datedServiceJourney));
}
}
return tripsOnServiceDates;
}

private TripOnServiceDate mapDatedServiceJourney(
Expand Down Expand Up @@ -360,9 +364,13 @@ private org.opentripplanner.transit.model.network.Route lookupRoute(
return otpRouteById.get(idFactory.createId(lineId));
}

private void createTripTimes(List<Trip> trips, TripPattern tripPattern) {
private List<TripTimes> createTripTimes(
List<Trip> trips,
Map<Trip, List<StopTime>> tripStopTimes
) {
var tripTimesResult = new ArrayList<TripTimes>();
for (Trip trip : trips) {
List<StopTime> stopTimes = result.tripStopTimes.get(trip);
List<StopTime> stopTimes = tripStopTimes.get(trip);
if (stopTimes.isEmpty()) {
issueStore.add(
"TripWithoutTripTimes",
Expand All @@ -372,12 +380,13 @@ private void createTripTimes(List<Trip> trips, TripPattern tripPattern) {
} else {
try {
TripTimes tripTimes = TripTimesFactory.tripTimes(trip, stopTimes, deduplicator);
tripPattern.add(tripTimes);
tripTimesResult.add(tripTimes);
} catch (DataValidationException e) {
issueStore.add(e.error());
}
}
}
return tripTimesResult;
}

private Trip mapTrip(
Expand Down
31 changes: 11 additions & 20 deletions src/main/java/org/opentripplanner/netex/mapping/TripPatternMapperResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,24 +13,15 @@
import org.opentripplanner.transit.model.timetable.TripOnServiceDate;

/**
* This mapper returnes two collections, so we need to use a simple wraper to be able to return the
* result from the mapping method.
* Wrapper class for the result of TripPatternMapper
*
* @param scheduledStopPointsIndex A map from trip/serviceJourney id to an ordered list of scheduled stop point ids.
* @param stopTimeByNetexId stopTimes by the timetabled-passing-time id
*/
class TripPatternMapperResult {

/**
* A map from trip/serviceJourney id to an ordered list of scheduled stop point ids.
*/
final ArrayListMultimap<String, String> scheduledStopPointsIndex = ArrayListMultimap.create();

final Map<Trip, List<StopTime>> tripStopTimes = new HashMap<>();

final Multimap<StopPattern, TripPattern> tripPatterns = ArrayListMultimap.create();

/**
* stopTimes by the timetabled-passing-time id
*/
final Map<String, StopTime> stopTimeByNetexId = new HashMap<>();

final ArrayList<TripOnServiceDate> tripOnServiceDates = new ArrayList<>();
}
record TripPatternMapperResult(
TripPattern tripPattern,
ArrayListMultimap<String, String> scheduledStopPointsIndex,
Map<Trip, List<StopTime>> tripStopTimes,
Map<String, StopTime> stopTimeByNetexId,
ArrayList<TripOnServiceDate> tripOnServiceDates
) {}
45 changes: 26 additions & 19 deletions src/test/java/org/opentripplanner/netex/mapping/TripPatternMapperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
package org.opentripplanner.netex.mapping;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;

import com.google.common.collect.ArrayListMultimap;
import java.util.Map;
import java.util.Optional;
import org.junit.jupiter.api.Test;
import org.opentripplanner.graph_builder.issue.api.DataImportIssueStore;
import org.opentripplanner.netex.index.hierarchy.HierarchicalMap;
Expand All @@ -12,7 +14,6 @@
import org.opentripplanner.transit.model.framework.Deduplicator;
import org.opentripplanner.transit.model.framework.DefaultEntityById;
import org.opentripplanner.transit.model.framework.FeedScopedId;
import org.opentripplanner.transit.model.network.TripPattern;
import org.opentripplanner.transit.model.timetable.Trip;
import org.opentripplanner.transit.model.timetable.TripAlteration;
import org.opentripplanner.transit.model.timetable.TripOnServiceDate;
Expand Down Expand Up @@ -55,26 +56,28 @@ public void testMapTripPattern() {
150
);

TripPatternMapperResult r = tripPatternMapper.mapTripPattern(sample.getJourneyPattern());
Optional<TripPatternMapperResult> res = tripPatternMapper.mapTripPattern(
sample.getJourneyPattern()
);

assertEquals(1, r.tripPatterns.size());
assertTrue(res.isPresent());

TripPattern tripPattern = r.tripPatterns.values().stream().findFirst().orElseThrow();
TripPatternMapperResult r = res.get();

assertEquals(4, tripPattern.numberOfStops());
assertEquals(1, tripPattern.scheduledTripsAsStream().count());
assertEquals(4, r.tripPattern().numberOfStops());
assertEquals(1, r.tripPattern().scheduledTripsAsStream().count());

Trip trip = tripPattern.scheduledTripsAsStream().findFirst().get();
Trip trip = r.tripPattern().scheduledTripsAsStream().findFirst().get();

assertEquals("RUT:ServiceJourney:1", trip.getId().getId());
assertEquals("NSR:Quay:1", tripPattern.getStop(0).getId().getId());
assertEquals("NSR:Quay:2", tripPattern.getStop(1).getId().getId());
assertEquals("NSR:Quay:3", tripPattern.getStop(2).getId().getId());
assertEquals("NSR:Quay:4", tripPattern.getStop(3).getId().getId());
assertEquals("NSR:Quay:1", r.tripPattern().getStop(0).getId().getId());
assertEquals("NSR:Quay:2", r.tripPattern().getStop(1).getId().getId());
assertEquals("NSR:Quay:3", r.tripPattern().getStop(2).getId().getId());
assertEquals("NSR:Quay:4", r.tripPattern().getStop(3).getId().getId());

assertEquals(1, tripPattern.getScheduledTimetable().getTripTimes().size());
assertEquals(1, r.tripPattern().getScheduledTimetable().getTripTimes().size());

TripTimes tripTimes = tripPattern.getScheduledTimetable().getTripTimes().get(0);
TripTimes tripTimes = r.tripPattern().getScheduledTimetable().getTripTimes().get(0);

assertEquals(4, tripTimes.getNumStops());

Expand Down Expand Up @@ -115,15 +118,19 @@ public void testMapTripPattern_datedServiceJourney() {
150
);

TripPatternMapperResult r = tripPatternMapper.mapTripPattern(sample.getJourneyPattern());
Optional<TripPatternMapperResult> res = tripPatternMapper.mapTripPattern(
sample.getJourneyPattern()
);

assertTrue(res.isPresent());

var r = res.get();

assertEquals(1, r.tripPatterns.size());
assertEquals(2, r.tripOnServiceDates.size());
assertEquals(2, r.tripOnServiceDates().size());

TripPattern tripPattern = r.tripPatterns.values().stream().findFirst().orElseThrow();
Trip trip = tripPattern.scheduledTripsAsStream().findFirst().get();
Trip trip = r.tripPattern().scheduledTripsAsStream().findFirst().get();

for (TripOnServiceDate tripOnServiceDate : r.tripOnServiceDates) {
for (TripOnServiceDate tripOnServiceDate : r.tripOnServiceDates()) {
assertEquals(trip, tripOnServiceDate.getTrip());
assertEquals(TripAlteration.PLANNED, tripOnServiceDate.getTripAlteration());
assertEquals(
Expand Down
Loading