Skip to content

Allow http headers to be configured for bike rental updaters#3533

Merged
gmellemstrand merged 10 commits into
opentripplanner:dev-2.xfrom
entur:otp2_http_headers
Jul 1, 2021
Merged

Allow http headers to be configured for bike rental updaters#3533
gmellemstrand merged 10 commits into
opentripplanner:dev-2.xfrom
entur:otp2_http_headers

Conversation

@gmellemstrand

@gmellemstrand gmellemstrand commented Jun 11, 2021

Copy link
Copy Markdown
Contributor

Summary

This allows arbitrary http headers to be added to bike rental updaters. At Entur this is useful, because we read headers in other components to know where the traffic is coming from.

I have changed the bike rental updater classes and HttpUtils to accept a map of headers. The BikeRentalServiceDirectoryFetcher sandbox feature is also changed to make headers configureable in router-config. If needed, this could also be configurable for individual updaters.

Issue

No issue

Unit tests

Tested manually

Documentation

Documented with javadoc

Changelog

Added to changelog

@gmellemstrand gmellemstrand added the Entur Test This is currently being tested at Entur label Jun 11, 2021
@gmellemstrand
gmellemstrand requested a review from a team as a code owner June 11, 2021 08:35
@t2gran t2gran added this to the 2.1 milestone Jun 24, 2021
@flaktack

Copy link
Copy Markdown
Contributor

#2544 adds headers to the GTFS-rt updaters, it might be useful to integrate that functionality here.

t2gran
t2gran previously requested changes Jun 24, 2021

@t2gran t2gran left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess the discussion here is if we should add the ability to configure all bike updaters with a map of headers. This PR make all the necessary steps - but not the tiny last one, so if we allow for this code change with extra the complexity, then it also seems fare to add the ability to the other updaters as well.

Comment thread docs/Changelog.md
Comment thread src/main/java/org/opentripplanner/util/HttpUtils.java Outdated
@t2gran

t2gran commented Jun 24, 2021

Copy link
Copy Markdown
Member

I vote for adding headers to bike-updaters, that would pave the way for adding it to other updaters later, like the GTFS-RT updater in #2544. If we do so, the documentation also needs updating.

network(),
routeAsCar(),
allowKeepingBicycleRentalsAtDestination(),
c.asMap("headers", NodeAdapter::asText)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

All the other parameters have nice little methods here, it would be nice to follow the same pattern - giving the parameter a name. I think the parameter should be named httpHeader since the context is a BikeRentalDataSource.

return apiKey;
}

public Map<String, String> getHttpHeaders() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this should be @Nonnull so we do not have to check for null later. Add @Nonnull to the constructor parameter as well.

super(
config,
"stations",
config.getApiKey() != null ? Map.of("Client-Identifier", config.getApiKey()) : Map.of()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it now possible to remove the Entur specific ApiKey? An replace it just by adding config in our deployment ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That would be possible, but we are not using this updater anymore, and I think it might be removed by @leonardehrenfried soon?

@flaktack flaktack left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One minor thing: could you add the headers to the configureUrls / GbfsAutoDiscoveryDataSource so that is is also used there?

configureUrls(parameters.getUrl());
this.networkName = parameters.getNetwork(DEFAULT_NETWORK_NAME);
}
private void configureUrls(String url) {
GbfsAutoDiscoveryDataSource gbfsAutoDiscoveryDataSource = new GbfsAutoDiscoveryDataSource(url);

@gmellemstrand
gmellemstrand merged commit b9252bd into opentripplanner:dev-2.x Jul 1, 2021
@gmellemstrand
gmellemstrand deleted the otp2_http_headers branch July 1, 2021 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Entur Test This is currently being tested at Entur

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants