doc: Add design doc and more TODOs

t2gran · t2gran · commit b12e94269911 · 2023-01-17T20:07:35.000+01:00
diff --git a/src/main/java/org/opentripplanner/transit/model/OTPModelOverview.png b/src/main/java/org/opentripplanner/transit/model/OTPModelOverview.png
diff --git a/src/main/java/org/opentripplanner/transit/model/api/TransitRoutingRequest.java b/src/main/java/org/opentripplanner/transit/model/api/TransitRoutingRequest.java
@@ -4,6 +4,10 @@
 import org.opentripplanner.routing.algorithm.raptoradapter.transit.cost.GeneralizedCostParameters;
 import org.opentripplanner.transit.model.trip.TripOnDate;
 
+
+/**
+ * TODO RTM - This has nothing to do with the transit model, move somewhere else
+ */
 public record TransitRoutingRequest(
   RaptorRequest<TripOnDate> raptorRequest,
   GeneralizedCostParameters generalizedCostParams,
diff --git a/src/main/java/org/opentripplanner/transit/model/basic/EntityNotFoundException.java b/src/main/java/org/opentripplanner/transit/model/basic/EntityNotFoundException.java
diff --git a/src/main/java/org/opentripplanner/transit/model/basic/LocalizedMoney.java b/src/main/java/org/opentripplanner/transit/model/basic/LocalizedMoney.java
@@ -3,6 +3,10 @@
 import java.util.Locale;
 import org.opentripplanner.framework.i18n.I18NString;
 
+/**
+ * TODO RTM - This has nothing to do with the transit model, move somewhere else
+ *          - No entity in the model have a reference to this.
+ */
 public record LocalizedMoney(Money money) implements I18NString {
   @Override
   public String toString(Locale locale) {
diff --git a/src/main/java/org/opentripplanner/transit/model/basic/Money.java b/src/main/java/org/opentripplanner/transit/model/basic/Money.java
@@ -5,6 +5,9 @@
 import java.util.Locale;
 
 /**
+ * TODO RTM - This has nothing to do with the transit model, move somewhere else
+ *          - No entity in the model have a reference to this.
+ *
  * @param currency The currency of the money.
  * @param cents    The actual currency value in decimal fixed-point, with the default number of
  *                 fraction digits from currency after the decimal point.
diff --git a/src/main/java/org/opentripplanner/transit/model/calendar/OperatingDay.java b/src/main/java/org/opentripplanner/transit/model/calendar/OperatingDay.java
@@ -35,6 +35,11 @@ public ZonedDateTime startTime() {
     return startTime;
   }
 
+
+  public ZonedDateTime toTime(int time) {
+    return startTime.plusSeconds(time);
+  }
+
   public int lengthSeconds() {
     return lengthSeconds;
   }
diff --git a/src/main/java/org/opentripplanner/transit/model/calendar/PatternForDay.java b/src/main/java/org/opentripplanner/transit/model/calendar/PatternForDay.java
@@ -9,6 +9,8 @@ public class PatternForDay {
   private final RoutingTripPattern pattern;
   private final Timetable timetable;
 
+  // TODO RTM  - Add next/prev
+
   public PatternForDay(OperatingDay day, RoutingTripPattern pattern, Timetable timetable) {
     this.day = day;
     this.pattern = pattern;
diff --git a/src/main/java/org/opentripplanner/transit/model/calendar/PatternsForDay.java b/src/main/java/org/opentripplanner/transit/model/calendar/PatternsForDay.java
@@ -1,4 +1,8 @@
 package org.opentripplanner.transit.model.calendar;
 
 // TODO RTM : For a given day, keep a reference to all patterns stating on that day
-public class PatternsForDay {}
+public class PatternsForDay {
+
+
+
+}
diff --git a/src/main/java/org/opentripplanner/transit/model/framework/Deduplicator.java b/src/main/java/org/opentripplanner/transit/model/framework/Deduplicator.java
@@ -17,6 +17,9 @@
 import org.opentripplanner.framework.tostring.ToStringBuilder;
 
 /**
+ * TODO RTM - This class should be split in a generic on (o.o.framework) and a transit
+ *          - specific one witch delagate the to generic one(single instance needed).
+ *
  * Does the same thing as String.intern, but for several different types. Java's String.intern uses
  * perm gen space and is broken anyway.
  */
diff --git a/src/main/java/org/opentripplanner/transit/model/framework/FeedScopedId.java b/src/main/java/org/opentripplanner/transit/model/framework/FeedScopedId.java
@@ -11,6 +11,10 @@
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 
+/**
+ * TODO RTM - This is used in the street mdel as well, so we should move it to
+ *          - 'o.o.framework.entity'
+ */
 public final class FeedScopedId implements Serializable, Comparable<FeedScopedId> {
 
   /**
diff --git a/src/main/java/org/opentripplanner/transit/model/framework/Result.java b/src/main/java/org/opentripplanner/transit/model/framework/Result.java
@@ -5,6 +5,9 @@
 import javax.annotation.Nonnull;
 
 /**
+ * TODO RTM - No need for this inside the transit model, move to realtime updater framework
+ *          - (unsure if package exist)
+ *
  * A type for containing either a success or a failure type as the result of a computation.
  * <p>
  * It's very similar to the Either or Validation type found in functional programming languages.
diff --git a/src/main/java/org/opentripplanner/transit/model/framework/package.md b/src/main/java/org/opentripplanner/transit/model/framework/package.md
@@ -0,0 +1,59 @@
+### Transit Entities
+
+All transit entities must have an ID. Transit entities ar "root" level are considered _aggregate
+roots_.
+
+
+#### @Nonnull and @Nullable entity fields
+
+All fields getters(except primitive types) should be annotated with `@Nullable` or `@Nonnull`. None
+null field should be enforced in the Entity constructor by using `Objects.requireNonNull`,
+`Objects.requireNonNullElse` or `ObjectUtils.ifNotNull`. We should enforce this for all fields
+required in both GTFS and in the Nordic NeTEx Profile. For enumeration types using a special value
+like `UNKNOWN` is preferred over making the field optional.
+
+
+#### sameAs(), equals() and hashCode()
+
+`equals()` and `hashCode` uses type and `id` only, while `sameAs` is all fields including
+`id`. The `id` is used for looking up entities in indexes and for references between entities,
+while `sameAs()` is used for modification detection.
+
+
+#### logName() and toString()
+
+We want a sensible toString() method implementation for all classes in the transit model. The
+toString is used for testing, logging and debugging, and should not be used for any domain feature
+or exposed on the APIs.
+
+Note! The `toString()` should not include nested entities, this may lead to extensive logging and
+cyclic references.
+
+For all entities the `toString()` is implemented in `AbstractTransitEntity` in the following format:
+
+```
+SIMPLE_CLASS_NAME{ID LOG_NAME?}
+```
+
+The `LOG_NAME` is optional. An entity can implement `LogInfo` to provide extra information in the
+`toString()` method. This is useful for including human recognizable information. The `LOG_NAME`
+does not need to identify the entity uniquely.
+
+
+### Value Objects
+
+A value object is something whose equality is **not** based on identity(`id`). In theory enum classes
+is also value objects, but for simplicity we do not include them in this discussion.
+
+For value objects `equals()` and `sameAs()` should be equivalent.
+
+The `toString` should include all fields, unless this is too much, then only include enough
+information to identify the value object within the context it exists.
+
+Values object can **not** reference entities, but may reference nested value objects - avoid
+cyclic references.
+
+
+### References
+
+1. [Domain Driven Design](https://en.wikipedia.org/wiki/Domain-driven_design)
diff --git a/src/main/java/org/opentripplanner/transit/model/package.md b/src/main/java/org/opentripplanner/transit/model/package.md
@@ -1,78 +1,92 @@
-## Transit Model Design
+# Transit Model
 
-TODO RTM - Add framework class diagram
+DRAFT! THIS DOCUMENTATION IS NOT COMPLETE AND MIGHT BE OUT OF DATE COMPARED WITH THE CODE, AND 
+VICE VERSA. 
 
-### Services and its context
-
-The `TransitService` is the main entry point for accessing all transit model objects. It may have 
-nested services or provide read-only access to key model classes. For changing the model a 
-`TransitEditorService` is created(obtained from the service). The editor can then be used to get a
-builder for all _aggregate roots_[1]. For simplicity, we only allow _one_ editor to be active at 
-any given time. 
-
-[TODO RTM, clarify] Modifying, adding and deleting entities are **not** synchronized, so if more 
-than one thread are doing updates at concurrent, then the synchronization responsibility is put on 
-the client (graph builders and RealTimeSnapshot). When all modification are complete, then changes 
-are made active by calling the `commit()` on the context. Before the `commit()` the changes are not 
-visible by e.g. the routing. [TODO Diagram]
+ - The draft design will help us implement the new transit model, but it is too much work to keep 
+   it "up to date". The audience is the developers actively working on refactoring the model.
+ - We will use this doc as an "analyses and design" workbook. 
+ - We will go over and make a "none draft" when it is time for review.
 
 
-### Transit Entities
+![OTP Model Overview](OTPModelOverview.png)
 
-All transit entities must have an ID. Transit entities ar "root" level are considered _aggregate
-roots_.
+(THIS IS WORK IN PROGRESS! The packages are probably almost ok, but the dependencies are not...)
 
 
-#### @Nonnull and @Nullable entity fields
+## Components
 
-All fields getters(except primitive types) should be annotated with `@Nullable` or `@Nonnull`. None 
-null field should be enforced in the Entity constructor by using `Objects.requireNonNull`, 
-`Objects.requireNonNullElse` or `ObjectUtils.ifNotNull`. We should enforce this for all fields 
-required in both GTFS and in the Nordic NeTEx Profile. For enumeration types using a special value
-like `UNKNOWN` is preferred over making the field optional.
+This is the top level package structure
 
+ - [framework](framework\package.md) - framework to support the transit model 
+ - basic - Value objects used across multiple packages.  
+ - organization
+ - site - StopLocation, RegularStop and Station +++ 
+ - organization - Authority, Operator and booking info
+ - network - Route
+ - trip - Trip and TripOnDate
+ - transfers - Regular and constrained transfer information
+ - calendar - Operating days and patterns on day
+ - timetable - Trip times for one operating day and trip schedule search
+ - trip schedule??? To avoid circular dependencies we might need to join trip and timetable
+ - plan - Support trip planning by implementing Raptor SPI. This is the main entry point. 
 
-#### sameAs(), equals() and hashCode()
+## Plan query
+The plan query is number ONE reason why OTP exist, and the primary design goal of the transit model
+is to support the plan query to be as efficient "as possible".
 
-`equals()` and `hashCode` uses type and `id` only, while `sameAs` is all fields including
-`id`. The `id` is used for looking up entities in indexes and for references between entities, 
-while `sameAs()` is used for modification detection.
 
+TODO RTM - Add an object diagram witch show the main types involved in a Raptor trip plan query.
 
-#### logName() and toString()
 
-We want a sensible toString() method implementation for all classes in the transit model. The
-toString is used for testing, logging and debugging, and should not be used for any domain feature
-or exposed on the APIs.
+## Services and its context
 
-Note! The `toString()` should not include nested entities, this may lead to extensive logging and
-cyclic references.
-
-For all entities the `toString()` is implemented in `AbstractTransitEntity` in the following format:
-
-```
-SIMPLE_CLASS_NAME{ID LOG_NAME?}
-```
+The `TransitService` is the main entry point for accessing all transit model objects. It may have 
+nested services or provide read-only access to key model classes. For changing the model a 
+`TransitEditorService` is created(obtained from the service). The editor can then be used to get a
+builder for all _aggregate roots_[1]. For simplicity, we only allow _one_ editor to be active at 
+any given time. 
 
-The `LOG_NAME` is optional. An entity can implement `LogInfo` to provide extra information in the
-`toString()` method. This is useful for including human recognizable information. The `LOG_NAME`
-does not need to identify the entity uniquely.
+[TODO RTM, clarify] Modifying, adding and deleting entities are **not** synchronized, so if more 
+than one thread are doing updates at concurrent, then the synchronization responsibility is put on 
+the client (graph builders and RealTimeSnapshot). When all modification are complete, then changes 
+are made active by calling the `commit()` on the context. Before the `commit()` the changes are not 
+visible by e.g. the routing. [TODO Diagram]
 
 
-### Value Objects
+## Support for real-time updates
 
-A value object is something whose equality is not based on identity(`id`). In theory enum classes 
-is also value objects, but for simplicity we do not include them in this discussion.      
+### RT-Notes
+ - The real time information will be part of the TransitModel, not a separate Snapshot. So a trip
+   will have both scheduled and realtime data. The RealTime updaters will copy parts of the model,
+   change it and post it back tho the service, which apply the changes in a thread-safe way.
+ - The data needed by for the routing will be put in a data structure optimized for routing. The 
+   entities will not hold this data as fields, but be views of the data in the optimized model. 
+   Fields/attributes not needed for trip routing will be part of the TransitModel objects.
 
-For value objects `equals()` and `sameAs()` should be equivalent. 
+### Design
+This is a very early suggestion on how we want this to look - it is not based on a realistic 
+use-case. The next step is to describe a couple RT updater use-cases - do a design for these and
+test the design.
 
-The `toString` should include all fields, unless this is too much, then only include enough 
-information to identify the value object within the context it exists. 
+```Java
+TransitModelEditor ctx = transitService().editTransitModel();
 
-Values object can **not** reference entities, but may reference nested value objects - avoid 
-cyclic references.
+// To modify a trip we get the builder, change it, and call save() 
+ctx.trip(id).withPrivateCode("CBP").save();
 
+// A more common case would be to perform a change to a trip schedule on a given date
+// We would like something like this. Here a combination of command and entity builders are used        
+ctx.on(date).changePattern(patternMatcher).removeStop(stopId).save();
+ctx.on(date).cancelTrip(tripMatcher).save();
+ctx.on(date).updateTrip(matchId(tripId)).arrivalTime(stopId, arrTime).depatureTime(stopId, depTime).save();
 
-### References
+// Revert an earlier update by going back to the planed version and apply new changes
+// - for a given trip on date  
+ctx.on(date).trip(id).revertToPlanned().withDelay(5m).save();
+// - for all data for an agency
+ctx.on(date).agency(id).revertToPlanned().trip(id).withDelay(5m).save();
 
-1. [Domain Driven Design](https://en.wikipedia.org/wiki/Domain-driven_design)
+// To update the model and make the changes visible to other threads
+ctx.commit();
+```

Original file line number	Diff line number	Diff line change
`@@ -35,6 +35,11 @@ public ZonedDateTime startTime() {`
`35`	`35`	`return startTime;`
`36`	`36`	`}`
`37`	`37`
	`38`	`+`
	`39`	`+ public ZonedDateTime toTime(int time) {`
	`40`	`+ return startTime.plusSeconds(time);`
	`41`	`+ }`
	`42`	`+`
`38`	`43`	`public int lengthSeconds() {`
`39`	`44`	`return lengthSeconds;`
`40`	`45`	`}`