Add support for HTTPS datasource for Graph Building #4482

vpaturet · 2022-09-29T10:52:12Z

Summary

This PR implements support for reading data sources from an HTTPS resource.
The data source provides only read operations.

Issue

Implements #4481

Unit tests

Added unit tests

Documentation

Updated BuildConfiguration.md

src/main/java/org/opentripplanner/datastore/https/HttpsDataSourceRepository.java

src/main/java/org/opentripplanner/datastore/https/HttpsFileDataSource.java

leonardehrenfried · 2022-09-29T11:46:59Z

src/main/java/org/opentripplanner/datastore/https/HttpsDataSourceRepository.java

+  /* private methods */
+
+  private static boolean skipUri(URI uri) {
+    return !"https".equals(uri.getScheme());


I doesn't happen a lot but do we not want to support HTTP without TLS?

I would rather not implement it right away (for security reasons). This could be easily added later if someone asks for it.

codecov-commenter · 2022-09-29T16:12:07Z

Codecov Report

Base: 58.48% // Head: 58.62% // Increases project coverage by +0.13% 🎉

Coverage data is based on head (d396094) compared to base (1fb4d2f).
Patch coverage: 76.20% of modified lines in pull request are covered.

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #4482      +/-   ##
=============================================
+ Coverage      58.48%   58.62%   +0.13%     
- Complexity     11431    11548     +117     
=============================================
  Files           1512     1520       +8     
  Lines          60102    60611     +509     
  Branches        6829     6890      +61     
=============================================
+ Hits           35152    35534     +382     
- Misses         22887    22986      +99     
- Partials        2063     2091      +28

Impacted Files	Coverage Δ
...anner/ext/reportapi/model/BicycleSafetyReport.java	`0.00% <0.00%> (ø)`
...planner/ext/reportapi/resource/ReportResource.java	`0.00% <0.00%> (ø)`
.../org/opentripplanner/datastore/api/DataSource.java	`9.09% <ø> (ø)`
...lanner/graph_builder/module/osm/WayProperties.java	`75.00% <ø> (-1.48%)`	⬇️
...ipplanner/routing/graph/SerializedGraphObject.java	`72.22% <ø> (-0.31%)`	⬇️
...lanner/transit/model/basic/NonLocalizedString.java	`71.42% <0.00%> (-1.91%)`	⬇️
.../main/java/org/opentripplanner/util/HttpUtils.java	`12.19% <0.00%> (-3.43%)`	⬇️
...ipplanner/datastore/https/HttpsFileDataSource.java	`42.10% <42.10%> (ø)`
...opentripplanner/standalone/config/BuildConfig.java	`87.50% <50.00%> (-1.14%)`	⬇️
...g/opentripplanner/standalone/config/OtpConfig.java	`88.88% <50.00%> (ø)`
... and 106 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

leonardehrenfried · 2022-09-30T12:33:17Z

src/main/java/org/opentripplanner/datastore/https/HttpsDataSourceMetadata.java

+    if (lastModifiedHeader != null) {
+      Date lastModifiedDate = DateUtils.parseDate(lastModifiedHeader);
+      if (lastModifiedDate != null) {
+        return lastModifiedDate.getTime();
+      }
+    }
+    return 0;


It's very much a matter of personal taste but I dislike repeated null checks (an null checks in general).

How to you like this?

Suggested change

if (lastModifiedHeader != null) {

Date lastModifiedDate = DateUtils.parseDate(lastModifiedHeader);

if (lastModifiedDate != null) {

return lastModifiedDate.getTime();

}

}

return 0;

return Optional

.ofNullable(lastModifiedHeader)

.map(DateUtils::parseDate)

.map(Date::getTime)

.orElse(0L);

I have no strong opinion about this, but it seems @t2gran has :-)
#4480 (comment)

I would just hope that there would be a clean syntax for this, similar to how optional chaining in JavaScript is. I really dislike how the repeated null-checks look

Maybe you could also chime in here: #4480 (comment)

The Optional is not made for this, and it reduces readability - I wish Java had support for a safe operator like Kotlin(?.) as well, but until then, use the language feature, not a library feature intended for something else. I would probably written the above code like this, to avoid nested ifs - but this is just personal taste:

if (lastModifiedHeader == null) { return -1; } Date lastModifiedDate = DateUtils.parseDate(lastModifiedHeader); if (lastModifiedDate != null) { return -1; } return lastModifiedDate.getTime();

PS ! The method should return -1 not 0 if the value does not exist

OpenTripPlanner/src/main/java/org/opentripplanner/datastore/api/DataSource.java

Line 67 in efe8374

* @return last modified timestamp in ms, if unknown returns {@code -1}

leonardehrenfried · 2022-09-30T12:36:27Z

src/main/java/org/opentripplanner/util/HttpUtils.java

@@ -85,4 +98,28 @@ public static InputStream openInputStream(URI uri, Map<String, String> headers)
      return downloadUrl.openStream();
    }
  }
+
+  private static HttpResponse getResponse(


This has nothing to do with your PR but now that we have this method we can actually use it in SiriHttpUtils.

leonardehrenfried

I've ran the code and it works great but I have a suggestion about the null checks.

What I want to add in the future is progress bar and local caching of files (if the ETag/Last-Modified) hasn't changed.

leonardehrenfried

I would love to see the (in my view ugly) null checks go but the code is solid otherwise.

t2gran · 2022-10-03T09:02:15Z

src/main/java/org/opentripplanner/datastore/https/HttpsDataSourceMetadata.java

+    try {
+      return Long.parseLong(header);
+    } catch (Exception e) {
+      return 0;


OpenTripPlanner/src/main/java/org/opentripplanner/datastore/api/DataSource.java

Line 60 in efe8374

* @return size in bytes, if unknown returns {@code -1}

This 0 end up as the returned value for the HttpDataSource#size(). The JavaDoc is either wrong or this is.

The nice fix to this would be to change the DataSource interface and return an OptionalLong for size and lastModified. If you don´t want to do that in this PR, I can fix it in a separate PR later.

I updated the two methods to be consistent with the Javadoc.
Updating the DataSource interface should probably be done in a separate PR.

hannesj · 2022-10-03T21:53:05Z

docs/BuildConfiguration.md

-Google Cloud Storage files: `"gs://otp-test-bucket/a/b/graph.obj"`
+Local files: `"file:///Users/kelvin/otp/streetGraph.obj"`  
+HTTPS resources: `"https://download.geofabrik.de/europe/norway-latest.osm.pbf"`  
+Google Cloud Storage files: `"gs://otp-test-bucket/a/b/graph.obj"`  


Should we add a mention of osmCacheDataInMem somewhere, or even enable it by default, if it is a https source?

We'll do this later

hannesj · 2022-10-03T21:55:32Z

src/main/java/org/opentripplanner/datastore/https/HttpsDataSourceMetadata.java

+/**
+ * HTTPS data source metadata returned by the HTTP server (HTTP headers).
+ */
+public class HttpsDataSourceMetadata {


This should probably have a toString, as it is being logged

Add support for HTTPS datasource

501b09a

hannesj reviewed Sep 29, 2022

View reviewed changes

src/main/java/org/opentripplanner/datastore/https/HttpsDataSourceRepository.java Outdated Show resolved Hide resolved

hannesj reviewed Sep 29, 2022

View reviewed changes

src/main/java/org/opentripplanner/datastore/https/HttpsFileDataSource.java Outdated Show resolved Hide resolved

leonardehrenfried reviewed Sep 29, 2022

View reviewed changes

t2gran self-requested a review September 29, 2022 13:50

t2gran added this to the 2.2 milestone Sep 29, 2022

t2gran added New Feature bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR and removed bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR labels Sep 29, 2022

t2gran changed the title ~~Add support for HTTPS datasource~~ Add support for HTTPS datasource for Graph Building Sep 29, 2022

vpaturet added 3 commits September 29, 2022 17:27

Retrieve HTTPS datasource metadata from HTTP server

0f023fa

Fix formatting

edb8a0a

Filter HTTP headers

8028c46

vpaturet added 6 commits September 29, 2022 18:26

Sanitize HTTP query parameters in log messages

74fa24a

Fix handling of HTTP metadata

9871331

Fix resource name and directory

fbf0e57

Updated documentation

c0d8c8e

Added unit tests

6af0658

Code cleanup

c719cad

vpaturet marked this pull request as ready for review September 30, 2022 10:03

vpaturet requested a review from a team as a code owner September 30, 2022 10:03

vpaturet requested review from leonardehrenfried and hannesj September 30, 2022 10:04

leonardehrenfried reviewed Sep 30, 2022

View reviewed changes

leonardehrenfried requested changes Sep 30, 2022

View reviewed changes

leonardehrenfried previously approved these changes Sep 30, 2022

View reviewed changes

t2gran reviewed Oct 3, 2022

View reviewed changes

Fix default value for missing HTTP server metadata

d396094

vpaturet dismissed leonardehrenfried’s stale review via d396094 October 3, 2022 09:53

vpaturet requested review from t2gran and leonardehrenfried October 3, 2022 10:51

hannesj reviewed Oct 3, 2022

View reviewed changes

hannesj previously approved these changes Oct 3, 2022

View reviewed changes

Added toString() method on HttpsDataSourceMetadata

3785c44

vpaturet dismissed hannesj’s stale review via 3785c44 October 4, 2022 11:57

hannesj approved these changes Oct 4, 2022

View reviewed changes

leonardehrenfried approved these changes Oct 4, 2022

View reviewed changes

t2gran mentioned this pull request Oct 4, 2022

Otp2 use optional in data source #4497

Merged

vpaturet merged commit 495f1f5 into opentripplanner:dev-2.x Oct 4, 2022

t2gran pushed a commit that referenced this pull request Oct 4, 2022

Add changelog entry for #4482 [ci skip]

0ace8ba

t2gran deleted the otp2_add_support_for_https_datasource branch March 31, 2023 09:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for HTTPS datasource for Graph Building #4482

Add support for HTTPS datasource for Graph Building #4482

vpaturet commented Sep 29, 2022 •

edited

Loading

leonardehrenfried Sep 29, 2022

vpaturet Sep 29, 2022 •

edited

Loading

codecov-commenter commented Sep 29, 2022 •

edited

Loading

leonardehrenfried Sep 30, 2022

vpaturet Sep 30, 2022

hannesj Sep 30, 2022 •

edited

Loading

leonardehrenfried Sep 30, 2022

t2gran Oct 3, 2022 •

edited

Loading

leonardehrenfried Sep 30, 2022

leonardehrenfried left a comment

leonardehrenfried left a comment

t2gran Oct 3, 2022

t2gran Oct 3, 2022

t2gran Oct 3, 2022 •

edited

Loading

vpaturet Oct 3, 2022

hannesj Oct 3, 2022

hannesj Oct 4, 2022

hannesj Oct 3, 2022

Add support for HTTPS datasource for Graph Building #4482

Add support for HTTPS datasource for Graph Building #4482

Conversation

vpaturet commented Sep 29, 2022 • edited Loading

Summary

Issue

Unit tests

Documentation

Choose a reason for hiding this comment

vpaturet Sep 29, 2022 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Sep 29, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannesj Sep 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t2gran Oct 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonardehrenfried left a comment

Choose a reason for hiding this comment

leonardehrenfried left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t2gran Oct 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpaturet commented Sep 29, 2022 •

edited

Loading

vpaturet Sep 29, 2022 •

edited

Loading

codecov-commenter commented Sep 29, 2022 •

edited

Loading

hannesj Sep 30, 2022 •

edited

Loading

t2gran Oct 3, 2022 •

edited

Loading

t2gran Oct 3, 2022 •

edited

Loading