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.

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

Refactor OTP to have new OTP classes to replace the OBA GTFS classes #2494

Closed
t2gran opened this issue Sep 12, 2017 · 6 comments
Closed

Refactor OTP to have new OTP classes to replace the OBA GTFS classes #2494

t2gran opened this issue Sep 12, 2017 · 6 comments

Comments

@t2gran
Copy link
Member

t2gran commented Sep 12, 2017

Goal

Remove the coupling from OTP's internal model and OTP's API to Conveyal´s fork of OneBusAway (OBA) GTFS classes. The OBA GTFS library should still be used for importing GTFS files. This change will make it easier in the feature to:

  • Improve the internal model and extend it with new functionality. New concepts can be added by adding properties or relations to the existing classes, avoiding the decoration that is required today.
  • Separate concerns, the GTFS model is made for data transfer while the internal OTP model have other concerns (finding the optimal route as fast as possible).

We want to do this with as few changes to the rest of the code as possible.

Proposed solution

  1. Copy the remaining GTFS classes that are being used internally by OTP classes - into OTP.
  2. Refactor OTP to use the copied classes (except for classes used by GTFS parsing)
  3. Map GTFS classes to the new (copied) classes in the internal model.
  4. Refactor the copied classes, remove CSV serialization and other dead code.
  5. Write unit tests for the new mapping code.

Id generation is done today by the GTFS lib. For now we will also do id generation inside the "new" Dao. For consistency this should be done in the OTP import, not in the data transfer classes; We have kept the id generation in the "new" classes in the POC below. This ensure id generation for any new data format import.

Proof of concept

This is no longer a POC, but part of our NETEX solution as Entur.
See PR-2495
More on the design is described in the developer discussion group.

Java Packages

The GFTS classes are copied into OTP. Some classes are renamed to better describe their role, and put in the following packages:

  • Model classes: Agency, Stop, Trip, Route ..
    Moved to package: org.opentripplanner.model

  • Service interfaces: OtpTransitService(GtfsDao), calendar.CalendarService) Moved to package: org.opentripplanner.model`
    These two services play a role together with the model classes; their implementation on the other hand is hidden away in their own packages.

  • Service implementation (GtfsDaoImpl, exceptions and new builder)
    Moved to package: org.opentripplanner.model.impl

  • calendar.CalendarServiceImpl (move together with Factory and MultiCalendarServiceImpl)
    Moved to package: org.opentripplanner.calendar.impl

  • Mapping classes (AgencyMapper, StopMapper, TripMapper, RouteMapper)
    New package: org.opentripplanner.gtfs.mapping

  • MockGtfs. (Used for testing, read in GTFS CSV files and map to internal model).
    New package: org.opentripplanner.gtfs

  • TimeToStringConverter and InvalidTimeException. Used in org.otp.updater and model classes.
    New package: org.opentripplanner.util

Renaming

The following names would be misleading after a refactoring, these should changed:

  • Rename GtfsDaoto OtpTransitService. This apply to the Ìmpl` as well.

Dao Inheritance structure

The GTFS lib have a very complex inheritance structure without any polymorphy, this can easily be simplified and flattened into just one interface OtpTransitService (replace GtfsDao).

Version of OTP used

1.3-SNAPSHOT (dd48797) 2017-09-12

t2gran added a commit to entur/OpenTripPlanner that referenced this issue Sep 12, 2017
t2gran pushed a commit to entur/OpenTripPlanner that referenced this issue Sep 12, 2017
…ebusaway2'. (Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
t2gran pushed a commit to entur/OpenTripPlanner that referenced this issue Sep 12, 2017
…g package is done in 2 commits to preserve git history. (Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
t2gran pushed a commit to entur/OpenTripPlanner that referenced this issue Sep 12, 2017
… best place to map between the to models. (Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
t2gran pushed a commit to entur/OpenTripPlanner that referenced this issue Sep 12, 2017
… serialization.

(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
t2gran pushed a commit to entur/OpenTripPlanner that referenced this issue Sep 12, 2017
…resentation). This is probably a god idea, but it was not used.

(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
t2gran pushed a commit to entur/OpenTripPlanner that referenced this issue Sep 12, 2017
…nd after refactoring.

Remove unused code, collapse DAO inheritance structure to get rid of unnecessary complexity.
(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
t2gran pushed a commit to entur/OpenTripPlanner that referenced this issue Sep 12, 2017
t2gran pushed a commit to entur/OpenTripPlanner that referenced this issue Sep 12, 2017
t2gran pushed a commit to entur/OpenTripPlanner that referenced this issue Sep 12, 2017
Fixed member formatting, removing prefix: '_', and changing constant names to upper case.
(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
t2gran pushed a commit to entur/OpenTripPlanner that referenced this issue Sep 12, 2017
Fixed member formatting, removing prefix: '_', and changing constant names to upper case.
(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
t2gran pushed a commit to entur/OpenTripPlanner that referenced this issue Sep 19, 2017
  - Spelling errors fixed.
  - All new code formatted with the project profile.
  - Minor code style improvements, like Java 8 style generics.
(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
t2gran pushed a commit to entur/OpenTripPlanner that referenced this issue Sep 20, 2017
CalendarServiceDataFactoryImpl into one.
(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
t2gran pushed a commit to entur/OpenTripPlanner that referenced this issue Sep 20, 2017
CalendarServiceDataFactoryImpl into one.
(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
t2gran pushed a commit to entur/OpenTripPlanner that referenced this issue Sep 20, 2017
onebusaway2 into package 'org.opentripplanner.calendar.impl'.
(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
t2gran pushed a commit to entur/OpenTripPlanner that referenced this issue Oct 4, 2017
…ceInterval.java

(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
t2gran pushed a commit to entur/OpenTripPlanner that referenced this issue Oct 30, 2017
(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
t2gran added a commit to entur/OpenTripPlanner that referenced this issue Oct 30, 2017
…P transit model.

(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
t2gran added a commit to entur/OpenTripPlanner that referenced this issue Oct 30, 2017
…fy it.

(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
t2gran added a commit to entur/OpenTripPlanner that referenced this issue Oct 30, 2017
…lanner.util'.

(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
t2gran added a commit to entur/OpenTripPlanner that referenced this issue Oct 30, 2017
…ner.model'.

(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
t2gran added a commit to entur/OpenTripPlanner that referenced this issue Oct 30, 2017
…ner.model'.

(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
t2gran added a commit to entur/OpenTripPlanner that referenced this issue Oct 30, 2017
… to 'org.opentripplanner.model.impl'.

(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
t2gran added a commit to entur/OpenTripPlanner that referenced this issue Oct 30, 2017
(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
t2gran pushed a commit to entur/OpenTripPlanner that referenced this issue Nov 27, 2017
(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
t2gran pushed a commit to entur/OpenTripPlanner that referenced this issue Nov 27, 2017
…P Tranist model - Note these objects do

not have ids in the GTFS model.
(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
@t2gran
Copy link
Member Author

t2gran commented Nov 28, 2017

We now consider this work complete and we have implemented the NETEX import with several NETEX specific features on top of this PR - we are currently doing the system test before the final release.

@skinkie
Copy link
Contributor

skinkie commented Nov 28, 2017

So, if I would provide a Dutch NeTEx file as we speak, would that work?

@t2gran
Copy link
Member Author

t2gran commented Nov 28, 2017

We have implemented support for the Norwegian profile of the NeTEx profile only. But, we have designed it with support for other ´profiles´ in mind, so there is two possible solutions:

  • We can extend our implementation to also support your needs, or
  • We can write a new Netex Module to import your needs.

The good news is that the import is now separated from the internal model, and writing a new import is just a matter of mapping. Functionality like the PatternHopFactory is generfied to support both GTFS and out new NeTEx import. Among the work we have done in the NeTEx import is to extract TripPattern generation out of the PatternHopFactory, so this step is only run in the GTFSModule.

We have not made a PR of the NeTEx work jet, we await the PR #2495 to be accepted. I will write a little about our thought and rodmap in a new post on the forum later(this week).

@abyrd
Copy link
Member

abyrd commented Nov 28, 2017

Hi @t2gran, I appreciate all your work on this and have been working through it and the accompanying email chains. I hope to provide some responses today or tomorrow.

@t2gran
Copy link
Member Author

t2gran commented Nov 28, 2017

@abyrd thank you, I look forward for your response.

t2gran pushed a commit to entur/OpenTripPlanner that referenced this issue Nov 28, 2017
(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
t2gran pushed a commit to entur/OpenTripPlanner that referenced this issue Nov 28, 2017
…P Tranist model - Note these objects do

not have ids in the GTFS model.
(Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner#2494)
@t2gran
Copy link
Member Author

t2gran commented Apr 9, 2019

The PR is merged and every thing above is resolved so I will close this issue.

@t2gran t2gran closed this as completed Apr 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants