Skip to content
This repository has been archived by the owner on May 27, 2024. It is now read-only.

feat: Add new OTP2 pojos #34

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

BumbleFlash
Copy link
Contributor

To add new pojos to handle recent changes to the OTP2 Planner API.

Copy link
Member

@barbeau barbeau left a comment

Choose a reason for hiding this comment

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

Thanks @BumbleFlash! Some comments in line. Before we merge this we will need to test the classes to make sure they actually work with a real OTP v2 server using something like Jackson, and we'll also need to do a v1 release of the current master branch code.

pom.xml Outdated


<properties>
<kotlin.version>1.5.0-M1</kotlin.version>
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this isn't a final release (it's a milestone release)? Can we use the most recent final stable release instead? If we can we should be able to get rid of the Jetbrains EAP repo too.

src/edu/usf/cutr/otp/model/DebugOutput.kt Outdated Show resolved Hide resolved
@barbeau
Copy link
Member

barbeau commented Mar 23, 2021

Also, the build is failing on Travis CI.

@BumbleFlash
Copy link
Contributor Author

@barbeau I think the Travis CI wants to compile using JDK 11. This would be a major conflict between the two projects we're working on. Should I update Java 11 to all our projects since OTP2 also uses java 11?

pom.xml Outdated
</dependency>
</dependency>
<dependency>
<groupId>javax</groupId>
Copy link
Member

Choose a reason for hiding this comment

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

I think you can get rid of this javaee-api dependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One of the pojos in the older code was using this dependency and it was throwing a compile error on the CI earlier.

@barbeau
Copy link
Member

barbeau commented Mar 24, 2021

Looks like updating Travis to explicitly run on Java 8 fixed this - #35 for now. I'm guessing it's an issue with an older version of Lombok on Java 11. I recall having to bump the Lombok version before to get it to run on newer Java versions (probably 8 or 9 at the time). However, we should be able to get rid of Lombok entirely from the dependencies in this PR as well when you delete all the old Java files (Lombok just generates getters and setters automatically, which Kotlin already takes care of).

@BumbleFlash
Copy link
Contributor Author

@barbeau It looks like there's a significant difference between the model classes in OTP1 and OTP2. I can only see 2 classes Itinerary and Leg that are similar and the other classes are completely different from the two versions.

@barbeau
Copy link
Member

barbeau commented Mar 24, 2021

I'd say delete the old OTP1 classes entirely in this PR, and any dependencies like Lombok that they need can go too.

@BumbleFlash
Copy link
Contributor Author

@barbeau Just to clarify, should I delete the whole org.opentripplanner package?

@BumbleFlash BumbleFlash requested a review from barbeau March 24, 2021 22:40
@barbeau
Copy link
Member

barbeau commented Mar 24, 2021

Yes, delete all existing classes, and we'll replace them with the contents of this PR. We'll then release a new v2 that's just compatible with OTP v2.

@barbeau
Copy link
Member

barbeau commented Mar 24, 2021

Note that you'll need to make sure there are new parallel classes in the library for all the one's you're deleting. I believe that includes classes other than just the /plan API - I think the GraphMetadata is another one? https://github.com/CUTR-at-USF/opentripplanner-pojos/blob/master/src/org/opentripplanner/api/ws/GraphMetadata.java You'll need to look at the OTP REST API docs.

@barbeau
Copy link
Member

barbeau commented Mar 24, 2021

@barbeau
Copy link
Member

barbeau commented Mar 24, 2021

And the bike station API https://github.com/CUTR-at-USF/opentripplanner-pojos/tree/master/src/org/opentripplanner/routing/bike_rental. But I don't think that's supported yet in OTP2? If not we should open an issue here pointing to the OTP issue so we can implement support for v2 in this library when it's supported in OTP2.

Copy link
Member

@barbeau barbeau left a comment

Choose a reason for hiding this comment

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

Thanks @BumbleFlash, some quick comments.

Let me know after you've tested the code in this PR to actually consume OTP2 API, and then I'll work on the v1 tag/release and we can merge this PR for a v2 release.

pom.xml Outdated

<build>

<repositories>
Copy link
Member

Choose a reason for hiding this comment

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

Can we get rid of these two EAP repositories? Typically releases should be on Maven Central and we should be able to pull from there.

@BumbleFlash
Copy link
Contributor Author

@barbeau I added support to ServerInfo and BikeRental APIs but I don't think GraphMetaData exists in the v2. I have also updated the Request parameters to reflect the previous version with some new changes to property names. I'll test the pojos and let you know how it works!

@BumbleFlash BumbleFlash requested a review from barbeau March 25, 2021 22:33
@barbeau
Copy link
Member

barbeau commented Mar 25, 2021

Thanks @BumbleFlash! When testing this, please save the .json responses from OTP2 in a main/src/test/resources directory in this project. Let's also write some unit tests in this project to confirm that the data being parsed is the data being retrieved. One way we've done this is the past is to push the .json files to GitHub, and then actually execute HTTPS requests in the unit tests to GitHub using the raw URLs to do a live HTTP request in the test. See this project for details - https://github.com/CUTR-at-USF/pelias-client-library/tree/master/src/test.

I'm open to other ways of testing this too (using Jackson to parse the files off the file path locally?) if you have them. This library itself doesn't execute HTTP requests so I don't think we need them for end-to-end tests to ensure the data is being accurately captured from the JSON files.

@BumbleFlash
Copy link
Contributor Author

@barbeau Thanks for the details. I think the former sounds like a better idea since we get to test both requests and responses at the same time.

@barbeau
Copy link
Member

barbeau commented Mar 26, 2021

@BumbleFlash So the more I think about this, the more I realize that we're better off splitting the work in this PR into a new project that covers the POJOs and a Java client for making the HTTP request and parsing the response for OTP2, since we're already going to need that in more than one project.

I've created a new repo and opened these issues to cover the concept:

  1. POJOs - Implement OTP v2 POJOs as Kotlin Multiplatform Mobile opentripplanner-client-library#1.
  2. Client code - Implement OTP v2 HTTP client code opentripplanner-client-library#2

Could you please move the new model classes you created in this repo into a new PR there? We can then merge and iterate on them in that repo, and implement the client code in another PR.

And we'll deprecate this repo as being OTP1-only.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants