-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Implement SIRI Lite #6284
base: dev-2.x
Are you sure you want to change the base?
Implement SIRI Lite #6284
Conversation
6afe592
to
bf2fcf2
Compare
9e15f52
to
538a7dd
Compare
@habrahamsson-skanetrafiken Can I ask you to review this one? If not let me know and I will reassign. |
It never occurred to me until yesterday's meeting that I also could have made the current updater/loader configurable. Let me know if you feel that's the better option, @t2gran. |
538a7dd
to
6703d62
Compare
63687ed
to
86a447f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. Just a few typos.
application/src/main/java/org/opentripplanner/updater/siri/updater/SiriETUpdater.java
Show resolved
Hide resolved
@Override | ||
public Optional<Siri> getUpdates() { | ||
try { | ||
var siri = siriLoader.fetchETFeed(DUMMY_REQUESTOR_REF); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This dummy parameter is a little bit of a code smell. But it seems to be the simplest solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative would be to make the parameter nullable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue seems to be that it's required for one implementation and not required for the other implementations. So making it nullable isn't 100% accurate either. IMO sending a dummy value is a good enough solution.
.../main/java/org/opentripplanner/updater/siri/updater/lite/SiriETLiteHttpTripUpdateSource.java
Outdated
Show resolved
Hide resolved
.../main/java/org/opentripplanner/updater/siri/updater/lite/SiriETLiteHttpTripUpdateSource.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Henrik Abrahamsson <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read through most of the main code - not all. The wiring can be improved, so I will wait with the rest of the review to that is done.
application/src/main/java/org/opentripplanner/updater/siri/updater/SiriETUpdater.java
Outdated
Show resolved
Hide resolved
/** | ||
* Feed id that is used to match trip ids in the TripUpdates | ||
*/ | ||
private final String feedId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this unused, or is it the SIRI namespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was a reference to the actual id of the feed and unused.
...ation/src/main/java/org/opentripplanner/updater/siri/updater/SiriETHttpTripUpdateSource.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/opentripplanner/updater/siri/updater/SiriSXUpdater.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/opentripplanner/updater/siri/updater/SiriETUpdater.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/opentripplanner/updater/siri/updater/SiriETUpdater.java
Outdated
Show resolved
Hide resolved
.toString(); | ||
} | ||
|
||
private static SiriLoader createLoader(Parameters parameters) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DRY - This can be refactored out. Either create a SiriLoaderFactory or create it in the UpdatorConfigurator and pass it in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done both in c7bf5ed
…ater/SiriETUpdater.java Co-authored-by: Thomas Gran <[email protected]>
…ater/SiriETHttpTripUpdateSource.java Co-authored-by: Thomas Gran <[email protected]>
1ab52da
to
c7bf5ed
Compare
c7bf5ed
to
790bdc6
Compare
18dfc6d
to
ec9e724
Compare
Here is what I had in mind with respect to manual Dependency Injection: package org.opentripplanner.updater.siri.configure;
/** This is doing manually dependency injection and work as a factory to create the updater components. */
class SiriUpdaterModule {
public static SiriETUpdater createSiriETUpdater(...) {
var loader = createLoader(...);
var source = createSource(loader, ...);
return createETUpdater(source, ...);
}
private static createLoader(...);
private static createSource(...);
private static createUpdater(...);
} Each component define the parameters as an interface inside the class. The same parameters can be repeated, but not inherited. For example if The factory for creating the Loader can be inlined here. The |
Summary
Implements the SIRI Light flavour (as opposed to the request/response one that Entur and Skanetrafiken uses). This means that the entire SIRI feed is downloaded in a single GET request.
Issue
Closes #6263
Documentation
The new updater is fully documented and I took the liberty to make all references to SIRI consistent in the docs.
Unit tests
Some added, but it's mostly wiring. I'm happy to add more if requested but generally we don't test the HTTP layer a lot.
cc @rcavaliere @Jouca