Merge pull request #5504 from entur/otp2_fix_st_bug

Allow early creation of StopModelBuilder

src/main/java/org/opentripplanner/transit/service/StopModel.java

@@ -5,6 +5,7 @@
 import java.util.Collection;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.atomic.AtomicInteger;
 import javax.annotation.Nullable;
 import org.locationtech.jts.geom.Envelope;
 import org.opentripplanner.framework.collection.CollectionsView;
@@ -28,10 +29,8 @@
 public class StopModel implements Serializable {
 
   private static final Logger LOG = LoggerFactory.getLogger(StopModel.class);
-  private static final int NO_PARENT = -1;
 
-  private final int parentHash;
-  private final int stopIndexSize;
+  private final AtomicInteger stopIndexCounter;
   private final Map<FeedScopedId, RegularStop> regularStopById;
   private final Map<FeedScopedId, Station> stationById;
   private final Map<FeedScopedId, MultiModalStation> multiModalStationById;
@@ -42,8 +41,7 @@ public class StopModel implements Serializable {
 
   @Inject
   public StopModel() {
-    this.parentHash = NO_PARENT;
-    this.stopIndexSize = 0;
+    this.stopIndexCounter = new AtomicInteger(0);
     this.regularStopById = Map.of();
     this.stationById = Map.of();
     this.multiModalStationById = Map.of();
@@ -54,8 +52,7 @@ public StopModel() {
   }
 
   StopModel(StopModelBuilder builder) {
-    this.parentHash = builder.original().hashCode();
-    this.stopIndexSize = builder.stopIndexSize();
+    this.stopIndexCounter = builder.stopIndexCounter();
     this.regularStopById = builder.regularStopsById().asImmutableMap();
     this.stationById = builder.stationById().asImmutableMap();
     this.multiModalStationById = builder.multiModalStationById().asImmutableMap();
@@ -71,14 +68,7 @@ public StopModel() {
    * this feature is normally not allowed, but not enforced here.
    */
   private StopModel(StopModel main, StopModel child) {
-    if (main.hashCode() != child.parentHash) {
-      throw new IllegalArgumentException(
-        "A Stop model can only be merged with its parent, this is done to avoid duplicates/gaps " +
-        "in the stopIndex."
-      );
-    }
-    this.parentHash = NO_PARENT;
-    this.stopIndexSize = child.stopIndexSize();
+    this.stopIndexCounter = assertSameStopIndexCounterIsUsedToCreateBothModels(main, child);
     this.areaStopById = MapUtils.combine(main.areaStopById, child.areaStopById);
     this.regularStopById = MapUtils.combine(main.regularStopById, child.regularStopById);
     this.groupOfStationsById =
@@ -91,14 +81,15 @@ private StopModel(StopModel main, StopModel child) {
   }
 
   /**
-   * Create a new builder based on a empty model. This is useful in unit-tests, but should
-   * NOT be used in the main code.
+   * Create a new builder based on an empty model. This is useful in unit-tests, but should
+   * NOT be used in the main code. It is not possible to merge the result with another
+   * {@link StopModel}, because they do not share the same context(stopIndexCounter).
    * <p>
-   * In the application code the correct way is to retrieve a model instance and the n use the
+   * In the application code the correct way is to retrieve a model instance and then use the
    * {@link #withContext()} method to create a builder.
    */
   public static StopModelBuilder of() {
-    return new StopModelBuilder(new StopModel());
+    return new StopModelBuilder(new AtomicInteger(0));
   }
 
   /**
@@ -113,12 +104,7 @@ public static StopModelBuilder of() {
    * StopModel and the {@link #of()} method should be used if a stop-model in not needed.
    */
   public StopModelBuilder withContext() {
-    return new StopModelBuilder(this);
-  }
-
-  @Deprecated
-  public StopModelBuilder copy() {
-    return withContext().addAll(this);
+    return new StopModelBuilder(this.stopIndexCounter);
   }
 
   /**
@@ -305,7 +291,7 @@ private StopModelIndex createIndex() {
       areaStopById.values(),
       groupStopById.values(),
       multiModalStationById.values(),
-      stopIndexSize
+      stopIndexCounter.get()
     );
   }
 
@@ -323,4 +309,22 @@ private static <V> V getById(FeedScopedId id, Map<FeedScopedId, ? extends V>...
     }
     return null;
   }
+
+  /**
+   * The 'stopIndexCounter' must be the same instance, hence the '!=' operator.
+   */
+  @SuppressWarnings("NumberEquality")
+  private static AtomicInteger assertSameStopIndexCounterIsUsedToCreateBothModels(
+    StopModel main,
+    StopModel child
+  ) {
+    if (main.stopIndexCounter != child.stopIndexCounter) {
+      throw new IllegalArgumentException(
+        "Two Stop models can only be merged if they are created with the same stopIndexCounter. " +
+        "This is archived by using the 'StopModel.withContext()' method. We do this to avoid " +
+        "duplicates/gaps in the stopIndex."
+      );
+    }
+    return main.stopIndexCounter;
+  }
 }

src/main/java/org/opentripplanner/transit/service/StopModelBuilder.java

@@ -19,8 +19,7 @@
 
 public class StopModelBuilder {
 
-  private final StopModel original;
-  private final AtomicInteger indexCounter;
+  private final AtomicInteger stopIndexCounter;
 
   private final EntityById<RegularStop> regularStopById = new DefaultEntityById<>();
   private final EntityById<AreaStop> areaStopById = new DefaultEntityById<>();
@@ -29,25 +28,16 @@ public class StopModelBuilder {
   private final EntityById<MultiModalStation> multiModalStationById = new DefaultEntityById<>();
   private final EntityById<GroupOfStations> groupOfStationById = new DefaultEntityById<>();
 
-  StopModelBuilder(StopModel original) {
-    this.original = original;
-    this.indexCounter = new AtomicInteger(original.stopIndexSize());
-  }
-
-  public StopModel original() {
-    return original;
-  }
-
-  int stopIndexSize() {
-    return indexCounter.get();
+  StopModelBuilder(AtomicInteger stopIndexCounter) {
+    this.stopIndexCounter = stopIndexCounter;
   }
 
   public ImmutableEntityById<RegularStop> regularStopsById() {
     return regularStopById;
   }
 
   public RegularStopBuilder regularStop(FeedScopedId id) {
-    return RegularStop.of(id, indexCounter::getAndIncrement);
+    return RegularStop.of(id, stopIndexCounter::getAndIncrement);
   }
 
   public RegularStop computeRegularStopIfAbsent(
@@ -104,7 +94,7 @@ public StopModelBuilder withGroupOfStation(GroupOfStations station) {
   }
 
   public AreaStopBuilder areaStop(FeedScopedId id) {
-    return AreaStop.of(id, indexCounter::getAndIncrement);
+    return AreaStop.of(id, stopIndexCounter::getAndIncrement);
   }
 
   public ImmutableEntityById<AreaStop> areaStopById() {
@@ -122,7 +112,7 @@ public StopModelBuilder withAreaStops(Collection<AreaStop> stops) {
   }
 
   public GroupStopBuilder groupStop(FeedScopedId id) {
-    return GroupStop.of(id, indexCounter::getAndIncrement);
+    return GroupStop.of(id, stopIndexCounter::getAndIncrement);
   }
 
   public ImmutableEntityById<GroupStop> groupStopById() {
@@ -156,4 +146,8 @@ public StopModelBuilder addAll(StopModel other) {
   public StopModel build() {
     return new StopModel(this);
   }
+
+  AtomicInteger stopIndexCounter() {
+    return stopIndexCounter;
+  }
 }

src/test/java/org/opentripplanner/routing/graph/GraphSerializationTest.java

@@ -9,6 +9,7 @@
 import java.util.BitSet;
 import java.util.Set;
 import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.jar.JarFile;
 import org.geotools.util.WeakValueHashMap;
 import org.jets3t.service.io.TempFile;
@@ -42,6 +43,8 @@ public class GraphSerializationTest {
 
   static Class<?>[] IGNORED_CLASSES = Set
     .of(
+      // Skip AtomicInteger, it does not implement equals/hashCode
+      AtomicInteger.class,
       ThreadPoolExecutor.class,
       WeakValueHashMap.class,
       Method.class,