Make vehicle rental stations feed scoped by hannesj · Pull Request #3601 · opentripplanner/OpenTripPlanner

hannesj · 2021-09-03T11:32:29Z

Summary

Remove possibility to import vehicle rental stations from OSM
Make vehicle rental stations feed scoped, making inter-feed collisions impossible
Add method for fetching vehicle rental station by id from VehicleRentalStationService

Issue

Unit tests

Adapted existing tests. Added loading vehicle rental stations from CSV file, as static rental stations from OSM were previously used in tests.

Documentation

Removed reference to static vehicle rental stations from configuration.

Changelog

Updated

Move network information to feed. Map back to station id in all APIs.

optionsome · 2021-09-03T11:42:13Z

Closes #3255 also

leonardehrenfried · 2021-09-03T11:53:31Z

This looks like a really good refactoring.

leonardehrenfried · 2021-09-03T11:54:12Z

I wondered for a long time why a station could have multiple networks. Was it only because it could be like that in OSM?

hannesj · 2021-09-03T11:54:40Z

I wondered for a long time why a station could have multiple networks. Was it only because it could be like that in OSM?

Yes, That was the only usage for this possibility

leonardehrenfried · 2021-09-03T11:55:55Z

    public Boolean rentedVehicle;

-   public List<String> vehicleRentalNetworks = new ArrayList<>();
+    public String vehicleRentalNetwork;


I would be the right thing to do but does this change the REST API?

No, that class is an internal class, this is the model for the REST API

leonardehrenfried · 2021-09-03T11:58:22Z

    @Override
    public DataFetcher<Iterable<String>> networks() {
-        return environment -> getSource(environment).networks;
+        return environment -> List.of(getSource(environment).getNetwork());


Good backwards-compatibility. 👍

leonardehrenfried · 2021-09-03T12:02:13Z

+        new T2<>("id", station.getStationId()),
        new T2<>("name", station.name),
-        new T2<>("networks", String.join(",", station.networks))
+        new T2<>("networks", station.getNetwork())


Backwards-compatible. 👍

optionsome

The changes to smoove updater looked fine.

optionsome · 2021-09-03T12:24:13Z

-         * networks ("catch-all" network defined).
+         * Special case for "null" networks ("catch-all" network defined).
         */
-        if (stationNetworks == null || rentedNetworks == null)


Is it possible that there are stations with null network and stations with network information at the same time or should all stations have network information if one has?

stationNetworks is never null. It could be previously, if an OSM station did not have it set. rentedNetworks however can be null, when travelling backwards in time, searching for a floating vehicle.

leonardehrenfried

This looks like a good PR and I would love to approve it but after having run it with our data set I get the following error when using the REST API:

15:23:57.622 ERROR (PlannerResource.java:96) System error
java.lang.NullPointerException: null
	at org.opentripplanner.api.mapping.PlaceMapper.mapPlace(PlaceMapper.java:44) ~[otp.jar:1.1]
	at org.opentripplanner.api.mapping.TripPlanMapper.mapTripPlan(TripPlanMapper.java:20) ~[otp.jar:1.1]
	at org.opentripplanner.api.resource.PlannerResource.plan(PlannerResource.java:81) ~[otp.jar:1.1]
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[na:na]
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) ~[na:na]
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[na:na]
	at java.base/java.lang.reflect.Method.invoke(Method.java:566) ~[na:na]
	at org.glassfish.jersey.server.model.internal.ResourceMethodInvocationHandlerFactory$1.invoke(ResourceMethodInvocationHandlerFactory.java:81) ~[otp.jar:1.1]
	at org.glassfish.jersey.server.model.internal.AbstractJavaResourceMethodDispatcher$1.run(AbstractJavaResourceMethodDispatcher.java:144) ~[otp.jar:1.1]
	at org.glassfish.jersey.server.model.internal.AbstractJavaResourceMethodDispatcher.invoke(AbstractJavaResourceMethodDispatcher.java:161) ~[otp.jar:1.1]
	at org.glassfish.jersey.server.model.internal.JavaResourceMethodDispatcherProvider$TypeOutInvoker.doDispatch(JavaResourceMethodDispatcherProvider.java:205) ~[otp.jar:1.1]
	at org.glassfish.jersey.server.model.internal.AbstractJavaResourceMethodDispatcher.dispatch(AbstractJavaResourceMethodDispatcher.java:99) ~[otp.jar:1.1]
	at org.glassfish.jersey.server.model.ResourceMethodInvoker.invoke(ResourceMethodInvoker.java:389) ~[otp.jar:1.1]
	at org.glassfish.jersey.server.model.ResourceMethodInvoker.apply(ResourceMethodInvoker.java:347) ~[otp.jar:1.1]
	at org.glassfish.jersey.server.model.ResourceMethodInvoker.apply(ResourceMethodInvoker.java:102) ~[otp.jar:1.1]
	at org.glassfish.jersey.server.ServerRuntime$2.run(ServerRuntime.java:308) ~[otp.jar:1.1]
	at org.glassfish.jersey.internal.Errors$1.call(Errors.java:271) ~[otp.jar:1.1]
	at org.glassfish.jersey.internal.Errors$1.call(Errors.java:267) ~[otp.jar:1.1]
	at org.glassfish.jersey.internal.Errors.process(Errors.java:315) ~[otp.jar:1.1]
	at org.glassfish.jersey.internal.Errors.process(Errors.java:297) ~[otp.jar:1.1]
	at org.glassfish.jersey.internal.Errors.process(Errors.java:267) ~[otp.jar:1.1]
	at org.glassfish.jersey.process.internal.RequestScope.runInScope(RequestScope.java:317) ~[otp.jar:1.1]
	at org.glassfish.jersey.server.ServerRuntime.process(ServerRuntime.java:291) ~[otp.jar:1.1]
	at org.glassfish.jersey.server.ApplicationHandler.handle(ApplicationHandler.java:1140) ~[otp.jar:1.1]
	at org.glassfish.jersey.grizzly2.httpserver.GrizzlyHttpContainer.service(GrizzlyHttpContainer.java:375) ~[otp.jar:1.1]
	at org.glassfish.grizzly.http.server.HttpHandler$1.run(HttpHandler.java:224) ~[otp.jar:1.1]
	at org.glassfish.grizzly.threadpool.AbstractThreadPool$Worker.doWork(AbstractThreadPool.java:565) ~[otp.jar:1.1]
	at org.glassfish.grizzly.threadpool.AbstractThreadPool$Worker.run(AbstractThreadPool.java:545) ~[otp.jar:1.1]
	at java.base/java.lang.Thread.run(Thread.java:829) ~[na:na]
15:24:03.405 ERROR (PlannerResource.java:96) System error
java.lang.NullPointerException: null
	at org.opentripplanner.api.mapping.PlaceMapper.mapPlace(PlaceMapper.java:44) ~[otp.jar:1.1]
	at org.opentripplanner.api.mapping.TripPlanMapper.mapTripPlan(TripPlanMapper.java:20) ~[otp.jar:1.1]
	at org.opentripplanner.api.resource.PlannerResource.plan(PlannerResource.java:81) ~[otp.jar:1.1]
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[na:na]
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) ~[na:na]
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[na:na]
	at java.base/java.lang.reflect.Method.invoke(Method.java:566) ~[na:na]
	at org.glassfish.jersey.server.model.internal.ResourceMethodInvocationHandlerFactory$1.invoke(ResourceMethodInvocationHandlerFactory.java:81) ~[otp.jar:1.1]
	at org.glassfish.jersey.server.model.internal.AbstractJavaResourceMethodDispatcher$1.run(AbstractJavaResourceMethodDispatcher.java:144) ~[otp.jar:1.1]
	at org.glassfish.jersey.server.model.internal.AbstractJavaResourceMethodDispatcher.invoke(AbstractJavaResourceMethodDispatcher.java:161) ~[otp.jar:1.1]
	at org.glassfish.jersey.server.model.internal.JavaResourceMethodDispatcherProvider$TypeOutInvoker.doDispatch(JavaResourceMethodDispatcherProvider.java:205) ~[otp.jar:1.1]
	at org.glassfish.jersey.server.model.internal.AbstractJavaResourceMethodDispatcher.dispatch(AbstractJavaResourceMethodDispatcher.java:99) ~[otp.jar:1.1]
	at org.glassfish.jersey.server.model.ResourceMethodInvoker.invoke(ResourceMethodInvoker.java:389) ~[otp.jar:1.1]
	at org.glassfish.jersey.server.model.ResourceMethodInvoker.apply(ResourceMethodInvoker.java:347) ~[otp.jar:1.1]
	at org.glassfish.jersey.server.model.ResourceMethodInvoker.apply(ResourceMethodInvoker.java:102) ~[otp.jar:1.1]
	at org.glassfish.jersey.server.ServerRuntime$2.run(ServerRuntime.java:308) ~[otp.jar:1.1]
	at org.glassfish.jersey.internal.Errors$1.call(Errors.java:271) ~[otp.jar:1.1]
	at org.glassfish.jersey.internal.Errors$1.call(Errors.java:267) ~[otp.jar:1.1]
	at org.glassfish.jersey.internal.Errors.process(Errors.java:315) ~[otp.jar:1.1]
	at org.glassfish.jersey.internal.Errors.process(Errors.java:297) ~[otp.jar:1.1]
	at org.glassfish.jersey.internal.Errors.process(Errors.java:267) ~[otp.jar:1.1]
	at org.glassfish.jersey.process.internal.RequestScope.runInScope(RequestScope.java:317) ~[otp.jar:1.1]
	at org.glassfish.jersey.server.ServerRuntime.process(ServerRuntime.java:291) ~[otp.jar:1.1]
	at org.glassfish.jersey.server.ApplicationHandler.handle(ApplicationHandler.java:1140) ~[otp.jar:1.1]
	at org.glassfish.jersey.grizzly2.httpserver.GrizzlyHttpContainer.service(GrizzlyHttpContainer.java:375) ~[otp.jar:1.1]
	at org.glassfish.grizzly.http.server.HttpHandler$1.run(HttpHandler.java:224) ~[otp.jar:1.1]
	at org.glassfish.grizzly.threadpool.AbstractThreadPool$Worker.doWork(AbstractThreadPool.java:565) ~[otp.jar:1.1]
	at org.glassfish.grizzly.threadpool.AbstractThreadPool$Worker.run(AbstractThreadPool.java:545) ~[otp.jar:1.1]
	at java.base/java.lang.Thread.run(Thread.java:829) ~[na:na]

Presumably a Place with a bikeShareId of null is being mapped.

Previously automatically mapped

leonardehrenfried

It works now. Thanks, @hannesj.

t2gran

The code changes look good from a code style and none functional requirements (NPE, threading, readability and so on).

t2gran · 2021-09-07T11:18:24Z

This PR was discussed at todays developer meeting. @leonardehrenfried should test it and @t2gran should provide a code review - both tasks are done and this is reddy to be merged.

hannesj added 5 commits September 3, 2021 14:25

Remove staticBikeRental

002fbdd

Make vehicleRentalNetworks singular

3a4b84a

Fix tests

8b519c8

Make VehicleRentalStation feed scoped

3407b50

Move network information to feed. Map back to station id in all APIs.

Add method for fetching vehicle rental station by id

4b30464

hannesj requested a review from a team as a code owner September 3, 2021 11:32

Update changelog

342d4ab

hannesj force-pushed the otp2_feed_scoped_vehicle_rental_station branch from 0850b6c to 342d4ab Compare September 3, 2021 11:33

Update snapshots with feed scoped ids

3a9961f

leonardehrenfried reviewed Sep 3, 2021

View reviewed changes

Bump graph version

bc602e2

optionsome reviewed Sep 3, 2021

View reviewed changes

leonardehrenfried requested changes Sep 6, 2021

View reviewed changes

Fix NPE when mapping non-vehicle rental stations

c434c24

hannesj requested a review from leonardehrenfried September 6, 2021 19:59

Add data fetcher for bikeRentalNetworks

e82bcc7

Previously automatically mapped

t2gran added this to the 2.1 milestone Sep 7, 2021

t2gran added the !Improvement A functional improvement or micro feature label Sep 7, 2021

leonardehrenfried approved these changes Sep 7, 2021

View reviewed changes

t2gran approved these changes Sep 7, 2021

View reviewed changes

hannesj merged commit 27951e8 into opentripplanner:dev-2.x Sep 7, 2021

hannesj deleted the otp2_feed_scoped_vehicle_rental_station branch September 7, 2021 11:20

Conversation

hannesj commented Sep 3, 2021

Summary

Issue

Unit tests

Documentation

Changelog

Uh oh!

optionsome commented Sep 3, 2021

Uh oh!

leonardehrenfried commented Sep 3, 2021

Uh oh!

leonardehrenfried commented Sep 3, 2021

Uh oh!

hannesj commented Sep 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leonardehrenfried Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

hannesj Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

optionsome left a comment

Choose a reason for hiding this comment

Uh oh!

optionsome Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

hannesj Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried left a comment

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried left a comment

Choose a reason for hiding this comment

Uh oh!

t2gran left a comment

Choose a reason for hiding this comment

Uh oh!

t2gran commented Sep 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hannesj commented Sep 3, 2021 •

edited

Loading