Add support for `rust-s3` backend. #30

lseelenbinder · 2024-01-30T10:42:46Z

Adds support for a S3 backend (which is optionally authenticated)

Fixes #11 and could be used to address maplibre/martin#1125.

nyurik

looks good! A few minor nits, and we need to re-export the underlying s3 crate somehow, but it conflicts with the s3.rs file. I would recommend NOT exporting the s3, http, and mmap namespaces. Instead, publish their content directly.

// --- this is lib.rs ---

pub use s3;  // re-export s3 crate as pmtiles::s3

mod s3_backend;
pub use s3_backend S3Backend;

mod http;
pub use http::HttpBackend;

nyurik · 2024-01-30T20:23:56Z

Cargo.toml

@@ -13,6 +13,8 @@ categories = ["science::geo"]
 [features]
 default = []
 http-async = ["dep:tokio", "dep:reqwest"]
+s3-async = ["dep:tokio", "dep:rust-s3", "rust-s3/tokio-native-tls"]


maybe call it s3-async-native ?

Good call. Done.

nyurik · 2024-01-30T20:26:14Z

src/error.rs

@@ -32,6 +32,9 @@ pub enum PmtError {
    #[cfg(feature = "http-async")]
    #[error("{0}")]
    Http(#[from] PmtHttpError),
+    #[cfg(any(feature = "s3-async-rustls", feature = "s3-async"))]
+    #[error("{0}")]


And above as well

Suggested change

#[error("{0}")]

#[error(transparent)]

nyurik · 2024-01-30T20:28:06Z

src/s3.rs

+        let response_bytes = response.bytes();
+
+        if response_bytes.len() > length {
+            Err(crate::error::PmtS3Error::ResponseBodyTooLong(response_bytes.len(), length).into())


shorten it with use ... at the top?

lseelenbinder · 2024-01-31T11:37:37Z

looks good! A few minor nits, and we need to re-export the underlying s3 crate somehow, but it conflicts with the s3.rs file. I would recommend NOT exporting the s3, http, and mmap namespaces. Instead, publish their content directly.
// --- this is lib.rs ---

pub use s3;  // re-export s3 crate as pmtiles::s3

mod s3_backend;
pub use s3_backend S3Backend;

mod http;
pub use http::HttpBackend;

Do we want to re-export s3 or allow users to bring their own (as we do with reqwest)? I tried really hard to make this TLS implementation agnostic, but rust-s3 doesn't build properly when it doesn't have a concrete credentials type.

nyurik

Do we want to re-export s3 or allow users to bring their own (as we do with reqwest)? I tried really hard to make this TLS implementation agnostic, but rust-s3 doesn't build properly when it doesn't have a concrete credentials type.

There is really no such thing as "bring your own" here. If we do not re-export a dependency, users must play the magic game of "match my dependencies with the dependencies of my dependency" - i.e. user must use the same version of reqwest lib as used by the API of the pmtiles crate. This is an antipattern because it will only work if Cargo ends up using identical version of reqwest for both. Otherwise, it might not compile. That's why sometimes people opt to use reqwest = "*" - which again is an antipattern, but they have to do it to match the API of a crate they want to use.

Instead, I think a much safer way is to either (1) ensure all pmtiles crate API only uses standard Rust types (or types declared in the pmtiles crate itself), or (2) re-export all 3rd party types together with all the other stuff related to those types - i.e. re-export reqwest and s3 sub-dependencies in their own namespaces like pmtiles::reqwest::* and pmtiles::s3::*.

Also, a few inline comments.

nyurik · 2024-01-31T19:47:58Z

src/error.rs

+
+#[cfg(any(feature = "s3-async-rustls", feature = "s3-async-native"))]
+#[derive(Debug, Error)]
+pub enum PmtS3Error {


why not flatten all errors - just add these to PmtError with a conditional #[cfg(...)] ? I am not sure there is much value in having multi-level errors here, is there?

You're right. Done.

nyurik · 2024-01-31T23:07:20Z

src/backend/mod.rs

+#[cfg(any(feature = "s3-async-native", feature = "s3-async-rustls"))]
+pub(crate) mod s3;
+
+#[cfg(feature = "http-async")]
+pub(crate) mod http;
+
+#[cfg(feature = "mmap-async-tokio")]
+pub(crate) mod mmap;


you don't really need to control the access here - enough to say pub mod s3 - that shouldn't make it public on the crate level, as long as you do not use pub mod backend in lib.rs.

That said, mod.rs offer a better way to compartmentalize things - you don't need to expose internals of the backend mod to the rest of the crate - I think it is cleaner to expose just the needed structs (up to you though) - you can also just remove the (crate) and keep sub-namespaces.

Suggested change

#[cfg(any(feature = "s3-async-native", feature = "s3-async-rustls"))]

pub(crate) mod s3;

#[cfg(feature = "http-async")]

pub(crate) mod http;

#[cfg(feature = "mmap-async-tokio")]

pub(crate) mod mmap;

#[cfg(any(feature = "s3-async-native", feature = "s3-async-rustls"))]

mod s3;

#[cfg(any(feature = "s3-async-native", feature = "s3-async-rustls"))]

pub use s3::S3Backend;

#[cfg(feature = "http-async")]

mod http;

#[cfg(feature = "http-async")]

pub use http::HttpBackend;

#[cfg(feature = "mmap-async-tokio")]

mod mmap;

#[cfg(feature = "mmap-async-tokio")]

pub use mmap::MmapBackend;

Nice catch, cleaned up.

lseelenbinder · 2024-02-01T19:55:14Z

There is really no such thing as "bring your own" here. If we do not re-export a dependency, users must play the magic game of "match my dependencies with the dependencies of my dependency" - i.e. user must use the same version of reqwest lib as used by the API of the pmtiles crate. This is an antipattern because it will only work if Cargo ends up using identical version of reqwest for both. Otherwise, it might not compile. That's why sometimes people opt to use reqwest = "*" - which again is an antipattern, but they have to do it to match the API of a crate they want to use.

Instead, I think a much safer way is to either (1) ensure all pmtiles crate API only uses standard Rust types (or types declared in the pmtiles crate itself), or (2) re-export all 3rd party types together with all the other stuff related to those types - i.e. re-export reqwest and s3 sub-dependencies in their own namespaces like pmtiles::reqwest::* and pmtiles::s3::*.

Fair enough. And users can always bring their own, if they want—just because we're nice and re-export doesn't mean users will use the exports. 😄

nyurik

thanks!!

lseelenbinder added 5 commits January 30, 2024 11:40

Add support for rust-s3 backend.

9b6f9d0

rustfmt

ed917c1

rustfmt

7b6b1bf

Add missing #[must_use]

43c5175

Fix up tests and feature matrix.

6345514

nyurik reviewed Jan 30, 2024

View reviewed changes

lseelenbinder added 2 commits January 31, 2024 12:34

Rename s3-async to s3-async-native.

edc9f9d

Use transparent for some errors.

d81df72

lseelenbinder added 2 commits January 31, 2024 12:37

Cleanup

1282e3b

Refactor backends.

7b022b1

lseelenbinder self-assigned this Jan 31, 2024

lseelenbinder marked this pull request as ready for review January 31, 2024 13:10

nyurik reviewed Jan 31, 2024

View reviewed changes

lseelenbinder added 3 commits February 1, 2024 20:42

Cleanup and simplify backend and main module visibility.

1759f83

Significantly simplify error code.

99b9041

Re-export s3 and reqwest crates.

8d87378

lseelenbinder added 2 commits February 1, 2024 20:59

Fix #[cfg()] typo.

629600d

Update README.

3bc3e0c

nyurik approved these changes Feb 2, 2024

View reviewed changes

nyurik mentioned this pull request Feb 2, 2024

Implement PMTiles S3 bucket support with authentication maplibre/martin#1171

Open

4 tasks

lseelenbinder merged commit 29be5e0 into main Feb 2, 2024
1 check passed

lseelenbinder deleted the add-s3-backend branch February 13, 2024 16:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for `rust-s3` backend. #30

Add support for `rust-s3` backend. #30

lseelenbinder commented Jan 30, 2024

nyurik left a comment •

edited

Loading

nyurik Jan 30, 2024

lseelenbinder Jan 31, 2024

nyurik Jan 30, 2024

nyurik Jan 30, 2024

lseelenbinder commented Jan 31, 2024

nyurik left a comment •

edited

Loading

nyurik Jan 31, 2024

lseelenbinder Feb 1, 2024

nyurik Jan 31, 2024

lseelenbinder Feb 1, 2024

lseelenbinder commented Feb 1, 2024

nyurik left a comment

Add support for rust-s3 backend. #30

Add support for rust-s3 backend. #30

Conversation

lseelenbinder commented Jan 30, 2024

nyurik left a comment • 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

lseelenbinder commented Jan 31, 2024

nyurik left a comment • 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

lseelenbinder commented Feb 1, 2024

nyurik left a comment

Choose a reason for hiding this comment

Add support for `rust-s3` backend. #30

Add support for `rust-s3` backend. #30

nyurik left a comment •

edited

Loading

nyurik left a comment •

edited

Loading